Summary

Refine general comments support

Review Request #7764 — Created Nov. 9, 2015 and submitted Nov. 24, 2015, 9:43 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This patch adds the finishing touches to support for general comments
-- that is, comments unattached from a file attachment, diff, etc. --
on reviews.

A button has been added to the review dialog to add a general comment.
These comments can be deleted with the delete comment icon that is
present with all other comments. JavaScript unit tests have been added
to test the deletion functionality.

A button has been added to the draft review banner to add a general
comment. This comment can only be deleted from within the review
dialog.

The include_text_types fields have been removed from the
BaseCommentResource because it requires special parsing. They were
added, but their type was incorrect. The field expects a
comma-separated list of the text types, not a single text type. This
broke the ability to comment has so has been reverted. A unit test has
been added to ensure this behaviour does not regress.

Calls to _no_access_error in the ReviewGeneralCommentResource and
ReviewReplyGeneralCommentResource have been corrected to
get_no_access_error, as the former method does not exist.

Testing Done


Ran unit tests.
Ran JS tests.
Added comments from both the banner and the dialog.
Deleted comments from the dialog.

Files

Issues

Description	From	Last Updated
Shouldn't this pass in RB.UserSession.instance.get('commentsOpenAnIssue'));?	david	Nov. 12, 2015, 6:16 p.m.
This should also indent 4 more spaces.	david	Nov. 12, 2015, 6:18 p.m.
Instead of using string concatenation, how about: $('<input type="button">').val(gettext('Cancel')), $('<input type="button">') .val(gettext('Discard')) .click(...)	david	Nov. 16, 2015, 10:10 a.m.
Why this change?	david	Nov. 16, 2015, 10:10 a.m.
No trailing commas in js (at least until we're totally ready to break IE8 with impunity).	david	Nov. 16, 2015, 4:41 p.m.
These should be in the form of <input type="button"/>, for consistency.	chipx86	Nov. 19, 2015, 2:47 a.m.
Blank line before this.	chipx86	Nov. 19, 2015, 2:46 a.m.
It's not super clear from the change, so why is this special-cased?	chipx86	Nov. 19, 2015, 2:50 a.m.
Missing "Returns".	chipx86	Nov. 19, 2015, 2:47 a.m.
Let's leave the /> in. While not technically needed for HTML, it's something we do to help with readability a …	chipx86	Nov. 19, 2015, 2:52 a.m.
We don't have any other resources with these explicitly listed, but ideally, we should. We should have it for every …	chipx86	Nov. 19, 2015, 2:52 a.m.
You need to handle general_comments here, too.	david	Nov. 23, 2015, 5:40 p.m.
serailize?	david	Nov. 24, 2015, 12:58 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Diff:

Revision 2 (+120 -37)

Show changes

	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
	reviewboard/templates/reviews/reviewable_base.html
	reviewboard/webapi/mixins.py
	4 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js

Given that we have "Add Comment" in the review request actions, I don't think we also need it in the draft banner.

The text type changes scare me--please have Christian give that a thorough review.

brennie Nov. 12, 2015, 4:38 p.m.

The text type changes are reverting stuff that Yanija did.

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Shouldn't this pass in RB.UserSession.instance.get('commentsOpenAnIssue'));?
```
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This should also indent 4 more spaces.
```

Change Summary:

Address David's issues. Remove add comment button from the draft banner.

Diff:

Revision 3 (+87 -23)

Show changes

	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
	reviewboard/templates/reviews/reviewable_base.html

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js

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

The issue has been resolved. Show all issues

Instead of using string concatenation, how about:

$('<input type="button">').val(gettext('Cancel')),
$('<input type="button">')
    .val(gettext('Discard'))
    .click(...)

reviewboard/templates/reviews/reviewable_base.html (Diff revision 3)

The issue has been dropped. Show all issues

Why this change?

brennie Nov. 13, 2015, 10:39 p.m.

<input> tags don't need to be self-closing. I originally had added a button here for adding a comment.

I can undo this if you wish.

Change Summary:

Address David's issues

Diff:

Revision 4 (+106 -38)

Show changes

	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
	reviewboard/templates/reviews/reviewable_base.html
	reviewboard/webapi/mixins.py
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js

reviewboard/static/rb/js/views/draftReviewBannerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
No trailing commas in js (at least until we're totally ready to break IE8 with impunity).
```

Change Summary:

Address David's issues

Diff:

Revision 5 (+105 -37)

Show changes

	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
	reviewboard/templates/reviews/reviewable_base.html
	reviewboard/webapi/mixins.py
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/templates/reviews/reviewable_base.html
    reviewboard/static/rb/js/resources/models/reviewModel.js

Looks good, but I still want Christian to look at the text types stuff.

reviewboard/static/rb/js/views/draftReviewBannerView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
These should be in the form of <input type="button"/>, for consistency.
```
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Blank line before this.
```
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
It's not super clear from the change, so why is this special-cased?
```
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Missing "Returns".
```

reviewboard/templates/reviews/reviewable_base.html (Diff revision 5)

The issue has been resolved. Show all issues

Let's leave the /> in. While not technically needed for HTML, it's something we do to help with readability a bit.

reviewboard/webapi/resources/base_comment.py (Diff revision 5)

The issue has been dropped. Show all issues

We don't have any other resources with these explicitly listed, but ideally, we should. We should have it for every resource that supports this (by using MarkdownFieldsMixin).

I don't mind removing it here for now, but we should absolutely have a task to track adding these (and related options) in.

brennie Nov. 19, 2015, 2:45 a.m.

We can include this, but have to have 'type;: six.text_type or come up with some other way of allowing comma-separated values.

Inlcuding this as is results in ALL comments becoming unsubmittable.

Change Summary:

Addressed Christian's issues.

Diff:

Revision 6 (+102 -20)

Show changes

	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js

Diff:

Revision 7 (+122 -36)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
	reviewboard/webapi/mixins.py
	4 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/notifications/webhooks.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/notifications/webhooks.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js

reviewboard/notifications/webhooks.py (Diff revision 7)
The issue has been resolved. Show all issues
```
You need to handle general_comments here, too.
```

Diff:

Revision 8 (+128 -37)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
	reviewboard/webapi/mixins.py
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/notifications/webhooks.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/notifications/webhooks.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js

reviewboard/notifications/webhooks.py (Diff revision 8)
The issue has been resolved. Show all issues
```
serailize?
```

Diff:

Revision 9 (+128 -37)

Show changes

	reviewboard/notifications/webhooks.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/reviewModel.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
	reviewboard/webapi/mixins.py
	4 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/notifications/webhooks.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/base_comment.py
    reviewboard/webapi/tests/mixins_comment.py
    reviewboard/webapi/mixins.py
    reviewboard/notifications/webhooks.py
    reviewboard/webapi/resources/review_general_comment.py
    reviewboard/webapi/resources/review_reply_general_comment.py

Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/resources/models/reviewModel.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/nicole_xin/general-comments (7a1c248)