Summary

Front-end for general comments

Review Request #6506 — Created Oct. 25, 2014 and submitted Dec. 4, 2014, 12:14 a.m.

Information

Owner

nicolexin

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

33ec3a2...

Reviewers

Groups

reviewboard, students

People

Description

Reviewboard currently support 3 types of comments: Diff commment, File attachment comments, Screenshot comments.
This is a new type of comments: General comments.

A general comment on a review request is used when a comment is not tied to specific lines of code or a special file attachment, and an issue is opened. Examples include suggestions for testing or pointing out errors in the change description.

The general comment backend model is built. (review request 6375)
The general comment web-api also done. (review request 6431)
This review request is for implemention of general comments at front-end.

Now the general comment can be displayed as a review box on the page
Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
A email is sent when a general comment is created and replied.

How to create a general comment:
1. On a review request page, click ‘Add Comment’ to the right of ‘Review’ button.
2. A green box will pop up. Write your general comment and tick the checkboxes if needed.
3. Click ‘OK’ to save the comment, then the green box will disappear and there will be a review draft banner on the top of the page.
4. If you wish to have multiple general comments in the same review, repeat step 1-3.
5. Once finished, click 'Publish' on the review draft banner to publish the review. (or you can discard or edit the review just like publishing other comments)

Testing Done

All js-tests passed.

Tests including:
1. reviewReplyEditor with general comments
2. General comment model
3. reviewReply with general comments
4. reviewDialogView with general comments

Files

Issues

Description	From	Last Updated
Indentation issue? Tabs?	chipx86	Nov. 1, 2014, 3:27 p.m.
No need for this, since we're not extending anything. Just assign directly without _.defaults().	chipx86	Nov. 1, 2014, 3:27 p.m.
Missing trailing comma.	chipx86	Nov. 1, 2014, 3:27 p.m.
Reviewables are just there to provide a way to review files. We won't need to provide this for general comments, …	chipx86	Nov. 16, 2014, 10:46 a.m.
By passing an ID, you're telling the comment dialog that this comment already exists on the server, which isn't true. …	chipx86	Nov. 15, 2014, 7:49 p.m.
It should probably return null, instead of false. When comparing in JavaScript, you want to use === instead of ==. …	chipx86	Nov. 16, 2014, 10:46 a.m.
Col: 13 E265 block comment should start with '# '	reviewbot	Nov. 16, 2014, 10:46 a.m.
Col: 13 E265 block comment should start with '# '	reviewbot	Nov. 16, 2014, 10:46 a.m.
It displays a comment dialog, not a review dialog.	chipx86	Nov. 20, 2014, 11:59 a.m.
This is the default, so no need to specify it.	chipx86	Nov. 20, 2014, 11:59 a.m.
This needs documentation above it.	chipx86	Nov. 20, 2014, 11:59 a.m.
This will happen automatically. No need to specify it.	chipx86	Nov. 20, 2014, 11:59 a.m.
This is the default. No need to specify it.	chipx86	Nov. 20, 2014, 11:59 a.m.
These are also all defaults. No need to override.	chipx86	Nov. 20, 2014, 11:59 a.m.
The trailing comma must be removed. In JavaScript, this is technically a syntax error, which will break IE and some …	chipx86	Nov. 20, 2014, 11:59 a.m.
This isn't the right place for this. Instead, the code that sets up the dialog for creating new general comments …	chipx86	Nov. 20, 2014, 11:59 a.m.
Doesn't look like this got fixed? Also, missing space before the {.	chipx86	Nov. 20, 2014, 11:59 a.m.
We should probably load these before other comments, so that they'll appear at the top of the review.	chipx86	Nov. 20, 2014, 11:59 a.m.
Should be aligned like the other ones.	chipx86	Nov. 20, 2014, 11:59 a.m.
For all the HTML you're introducing, make sure you're using 1-space indentation, and that it's aligned with respect to the …	chipx86	Nov. 20, 2014, 11:59 a.m.
Missing a var statement.	chipx86	Dec. 1, 2014, 6:18 p.m.
I'd rather not introduce a new event for this (which we do below). Instead, let's do what the other call …	chipx86	Dec. 1, 2014, 6:18 p.m.
This was marked as fixed in two of my prior reviews, but isn't fixed. This function doesn't return a boolean, …	chipx86	Dec. 1, 2014, 6:18 p.m.
Blank line between these. This should also be generalComment, or probably comment.	chipx86	Dec. 3, 2014, 3:26 p.m.
We only use one var statement. So above, do: var comment = ..., dlg;	chipx86	Dec. 3, 2014, 3:26 p.m.
This isn't needed anymore.	chipx86	Dec. 3, 2014, 3:26 p.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js

Groups:

+	students

We just made some changes to some files that may result in some merge difficulties.

When you get a chance, I'd like to see a screenshot of an e-mail with general comments.

reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js (Diff revision 1)
Show all issues
```
Indentation issue? Tabs?
```

reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js (Diff revision 1)

Show all issues

No need for this, since we're not extending anything. Just assign directly without _.defaults().

reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js (Diff revision 1)
Show all issues
```
Missing trailing comma.
```

Description:

		The general comment model is built. (review request 6375)
		The general comment web-api also done. (review request 6431)
		This review request is for implemention of general comments at front-end.

		WIP:
~		- front-end model built
	~	- Now the general comment can be displayed as a review box on the page
	+	- Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good
	+	- A email is sent when a general comment is replied
	+
	+	Note:
	+	Now a general comment is created by adding at admin page, therefore an email is only sent when it has been commented.
	+
	+

		Todo:
~		- work on templates and modify model as required.
	~	- Find a way to create a general comment at front-end
	+	- Refine the email
		- add tests
		- testing

Commit:

-	982c5140fc77ff812cf65f7a8eeb6a39de56efbc
+	477dcb1a2bfe51d058bc2e4dc022e50ce5615dbb

Diff:

Revision 2 (+326 -4)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/generalCommentBlockModel.js
	reviewboard/static/rb/js/models/generalReviewableModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
	9 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/models/generalCommentBlockModel.js
    reviewboard/static/rb/js/models/generalReviewableModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/models/generalCommentBlockModel.js
    reviewboard/static/rb/js/models/generalReviewableModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js

Description:

		The general comment model is built. (review request 6375)
		The general comment web-api also done. (review request 6431)
		This review request is for implemention of general comments at front-end.

		WIP:
		- Now the general comment can be displayed as a review box on the page
		- Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good
		- A email is sent when a general comment is replied

		Note:
		Now a general comment is created by adding at admin page, therefore an email is only sent when it has been commented.



		Todo:
		- Find a way to create a general comment at front-end
		- Refine the email
~		- add tests
	~	- modifying tests
		- testing

Testing Done:

~		No testing done yet.
	~	Before pulling from origin master (Nov 8): passing all js-tests
	+	After pulling from origin master: failing 7 tests
	+
	+	Will look into this next week.

Commit:

-	477dcb1a2bfe51d058bc2e4dc022e50ce5615dbb
+	e4d1737772eff20c8a81a109beb4c8af786f11f7

Diff:

Revision 3 (+355 -5)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/generalCommentBlockModel.js
	reviewboard/static/rb/js/models/generalReviewableModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/models/generalCommentBlockModel.js
    reviewboard/static/rb/js/models/generalReviewableModel.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/models/generalCommentBlockModel.js
    reviewboard/static/rb/js/models/generalReviewableModel.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 3)

Show all issues

By passing an ID, you're telling the comment dialog that this comment already exists on the server, which isn't true. You shouldn't pass an ID.
Also note how the data you're passing doesn't match the function signature for createGeneralComment.

reviewboard/static/rb/js/models/generalReviewableModel.js (Diff revision 3)

Show all issues

Reviewables are just there to provide a way to review files. We won't need to provide this for general comments, since "General" isn't a type of reviewable thing. General comments are independent of any files.

Likewise, you won't need GeneralCommentBlock.

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 3)

Show all issues

It should probably return null, instead of false.
When comparing in JavaScript, you want to use === instead of ==. Seems weird, but that's how it works. === is a string comparison, whereas == attempts to cast things.
Also, blank line between this conditional and the next return statement.

Description:

		The general comment model is built. (review request 6375)
		The general comment web-api also done. (review request 6431)
		This review request is for implemention of general comments at front-end.

		WIP:
		- Now the general comment can be displayed as a review box on the page
~		- Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good
~		- A email is sent when a general comment is replied
	~	- Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
	~	- A email is sent when a general comment is created

~		Note:
~		Now a general comment is created by adding at admin page, therefore an email is only sent when it has been commented.
	~	Problems:
	~	- a general comment cannot be replied (Bad request 400 error: see status report.)
	+	- failing ReviewDialogViewTest



		Todo:
~		- Find a way to create a general comment at front-end
~		- Refine the email
	~	- modifying ReviewDialogViewTest ordering
	~	- fix reply to a general commenr
-		- modifying tests
-		- testing

Commit:

-	e4d1737772eff20c8a81a109beb4c8af786f11f7
+	87574d945ab334ebb61c58ccc216ee59d4937d77

Diff:

Revision 4 (+407 -5)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/generalCommentBlockModel.js
	reviewboard/static/rb/js/models/generalReviewableModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	15 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/generalReviewableView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/js/models/generalCommentBlockModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/models/generalReviewableModel.js
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/generalReviewableView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/js/models/generalCommentBlockModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/models/generalReviewableModel.js
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js

reviewboard/staticbundles.py (Diff revision 4)

Show all issues

Col: 13
 E265 block comment should start with '# '

reviewboard/staticbundles.py (Diff revision 4)

Show all issues

Col: 13
 E265 block comment should start with '# '

Description:

		The general comment model is built. (review request 6375)
		The general comment web-api also done. (review request 6431)
		This review request is for implemention of general comments at front-end.

		WIP:
		- Now the general comment can be displayed as a review box on the page
		- Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
~		- A email is sent when a general comment is created
	~	- A email is sent when a general comment is created
	+	- All js-test passed

		Problems:
~		- a general comment cannot be replied (Bad request 400 error: see status report.)
	~	- a general comment cannot be replied (Bad request 400 error: see status report.)
-		- failing ReviewDialogViewTest



		Todo:
-		- modifying ReviewDialogViewTest ordering
		- fix reply to a general commenr

Testing Done:

~		Before pulling from origin master (Nov 8): passing all js-tests
	~	All js-tests passed.
-		After pulling from origin master: failing 7 tests
-
-		Will look into this next week.

Commit:

-	87574d945ab334ebb61c58ccc216ee59d4937d77
+	0512a960383504359ec065ea12ee1bba04dd6eba

Diff:

Revision 5 (+360 -6)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	12 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Summary:

-	[WIP]Front-end for general comments
+	Front-end for general comments

Description:

		The general comment model is built. (review request 6375)
		The general comment web-api also done. (review request 6431)
		This review request is for implemention of general comments at front-end.

~		WIP:
~		- Now the general comment can be displayed as a review box on the page
~		- Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
~		- A email is sent when a general comment is created
	~	Now the general comment can be displayed as a review box on the page
	~	Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
	~	A email is sent when a general comment is created and replied
	~	All js-test passed
-		- All js-test passed
-
-		Problems:
-		- a general comment cannot be replied (Bad request 400 error: see status report.)
-
-
-
-		Todo:
-		- fix reply to a general commenr

Testing Done:

		All js-tests passed.
	+
	+	Tests including:
	+	1. reviewReplyEditor with general comments
	+	2. General comment model
	+	3. reviewReply with general comments
	+	4. reviewDialogView with general comments

Commit:

-	0512a960383504359ec065ea12ee1bba04dd6eba
+	8c3aeb7b1883e7c936447222a62177fe1bd3aa40

Diff:

Revision 6 (+360 -6)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	12 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Can you also show screenshots for the review dialog and comment dialog?

One more piece of feedback, unrelated to code. Each review request's summary, description, and testing done should be fully self-descriptive, allowing a reviewer or someone down the road to read through it and know exactly what you're doing, why, and how, without having to dive into the code. We'll also be using these for the commit message. This also massively helps when we later are looking for the source of regressions, or are drafting release notes.

You can browse through any of my commits in the repository for examples of how these should look.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 6)
Show all issues
```
It displays a comment dialog, not a review dialog.
```
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 6)
Show all issues
```
This is the default, so no need to specify it.
```
reviewboard/static/rb/js/resources/models/generalCommentModel.js (Diff revision 6)
Show all issues
```
This needs documentation above it.
```
reviewboard/static/rb/js/resources/models/generalCommentModel.js (Diff revision 6)
Show all issues
```
This will happen automatically. No need to specify it.
```
reviewboard/static/rb/js/resources/models/generalCommentModel.js (Diff revision 6)
Show all issues
```
This is the default. No need to specify it.
```

reviewboard/static/rb/js/resources/models/generalCommentModel.js (Diff revision 6)

Show all issues

These are also all defaults. No need to override.

reviewboard/static/rb/js/resources/models/reviewModel.js (Diff revision 6)

Show all issues

The trailing comma must be removed. In JavaScript, this is technically a syntax error, which will break IE and some other browsers.

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 6)

Show all issues

This isn't the right place for this. Instead, the code that sets up the dialog for creating new general comments on the review request page should call this when the comment has been saved.

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 6)
Show all issues
```
Doesn't look like this got fixed?
Also, missing space before the {.
```

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 6)

Show all issues

We should probably load these before other comments, so that they'll appear at the top of the review.

reviewboard/templates/notifications/review_email.html (Diff revision 6)
Show all issues
```
Should be aligned like the other ones.
```

reviewboard/templates/reviews/boxes/review.html (Diff revision 6)

Show all issues

For all the HTML you're introducing, make sure you're using 1-space indentation, and that it's aligned with respect to the other HTML.

Also make sure that the nesting looks correct. For instance, here, it looks like the <div> is nested inside the <a>, but it's not. The </div> also isn't aligning with the <div>.

This will all be a bit easier to see when you've made sure they're aligned with other HTML and using the proper indentation size.

Description:

~		The general comment model is built. (review request 6375)
	~	Reviewboard currently support 3 types of comments: Diff commment, File attachment comments, Screenshot comments.
	+	This is a new type of comments: General comments.
	+
	+	A general comment on a review request is used when a comment is not tied to specific lines of code or a special file attachment, and an issue is opened. Examples include suggestions for testing or pointing out errors in the change description.
	+
	+	The general comment backend model is built. (review request 6375)
		The general comment web-api also done. (review request 6431)
		This review request is for implemention of general comments at front-end.

		Now the general comment can be displayed as a review box on the page
		Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
~		A email is sent when a general comment is created and replied
	~	A email is sent when a general comment is created and replied.
-		All js-test passed

Commit:

-	8c3aeb7b1883e7c936447222a62177fe1bd3aa40
+	752f22088cffaa0768cdb85507f6bc0cb0996c00

Diff:

Revision 7 (+344 -11)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	12 more

File Captions:

Screen Shot 2014-11-16 at 7.52.55 PM.png:
- With one reply
+ Email with one reply
Screen Shot 2014-11-16 at 7.53.21 PM.png:
- With multiple replies
+ Email with multiple replies

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 7)
Show all issues
```
Missing a var statement.
```

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 7)

Show all issues

I'd rather not introduce a new event for this (which we do below). Instead, let's do what the other call sites do, which is create the comment and listen to that comment's 'saved' event instead.

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 7)

Show all issues

This was marked as fixed in two of my prior reviews, but isn't fixed. This function doesn't return a boolean, but rather must return a string or null.

Commit:

-	752f22088cffaa0768cdb85507f6bc0cb0996c00
+	301ddf7a8143c4b9164fcbaaa885d349878aeb9b

Diff:

Revision 8 (+346 -11)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	12 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Unless I missed it, it might be useful to upload a view (or just describe in words in the description) of how to actually create a GeneralComment. For example, how did you get the green display box to submit the issue/general comment?

```
Last 3 changes! Then it's going in :)
```

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 8)

Show all issues

Blank line between these.

This should also be generalComment, or probably comment.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 8)

Show all issues

We only use one var statement. So above, do:

var comment = ...,
    dlg;

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 8)
Show all issues
```
This isn't needed anymore.
```

Description:

		Reviewboard currently support 3 types of comments: Diff commment, File attachment comments, Screenshot comments.
		This is a new type of comments: General comments.

		A general comment on a review request is used when a comment is not tied to specific lines of code or a special file attachment, and an issue is opened. Examples include suggestions for testing or pointing out errors in the change description.

		The general comment backend model is built. (review request 6375)
		The general comment web-api also done. (review request 6431)
		This review request is for implemention of general comments at front-end.

		Now the general comment can be displayed as a review box on the page
		Issue bar is working (e.g. fix/drop/reopen the issue) and the issue summary seems good.
		A email is sent when a general comment is created and replied.
	+
	+
	+
	+	How to create a general comment:
	+	1. On a review request page, click ‘Add Comment’ to the right of ‘Review’ button.
	+	2. A green box will pop up. Write your general comment and tick the checkboxes if needed.
	+	3. Click ‘OK’ to save the comment, then the green box will disappear and there will be a review draft banner on the top of the page.
	+	4. If you wish to have multiple general comments in the same review, repeat step 1-3.
	+	5. Once finished, click 'Publish' on the review draft banner to publish the review. (or you can discard or edit the review just like publishing other comments)

Commit:

-	301ddf7a8143c4b9164fcbaaa885d349878aeb9b
+	33ec3a2c5403d97ad47637865b1ff43358f3b16e

Diff:

Revision 9 (+347 -11)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/commentIssueManagerModel.js
	reviewboard/static/rb/js/models/reviewReplyEditorModel.js
	reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/generalCommentModel.js
	reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	11 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/commentIssueManagerModel.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/notifications/reply_email.html
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/templates/notifications/reply_email.txt
    reviewboard/static/rb/js/resources/models/generalCommentModel.js
    reviewboard/static/rb/js/resources/models/tests/reviewReplyModelTests.js
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/templates/notifications/review_email.html
    reviewboard/templates/reviews/review_request_actions_primary.html
    reviewboard/static/rb/js/resources/models/tests/generalCommentModelTests.js
    reviewboard/static/rb/js/resources/models/generalCommentReplyModel.js
    reviewboard/static/rb/js/resources/models/reviewModel.js
    reviewboard/static/rb/js/models/reviewReplyEditorModel.js
    reviewboard/templates/notifications/review_email.txt
    reviewboard/static/rb/js/models/tests/reviewReplyEditorModelTests.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/nicole_xin/general-comments (60a48e6)

Front-end for general comments

Information

Reviewers

Groups:

Description:

Commit:

Diff:

Description:

Testing Done:

Commit:

Diff:

Description:

Commit:

Diff:

Description:

Testing Done:

Commit:

Diff:

Summary:

Description:

Testing Done:

Commit:

Diff:

Added Files:

Description:

Commit:

Diff:

File Captions:

Added Files:

Commit:

Diff:

Description:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: