Refine general comments support
Review Request #7764 — Created Nov. 9, 2015 and submitted
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_typesfields have been removed from the
BaseCommentResourcebecause 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_errorin theReviewGeneralCommentResourceand
ReviewReplyGeneralCommentResourcehave been corrected to
get_no_access_error, as the former method does not exist.
- Ran unit tests.
- Ran JS tests.
- Added comments from both the banner and the dialog.
- Deleted comments from the dialog.
| Description | From | Last Updated |
|---|---|---|
|
Shouldn't this pass in RB.UserSession.instance.get('commentsOpenAnIssue'));? |
 david
|
|
|
This should also indent 4 more spaces. |
david
|
|
|
Instead of using string concatenation, how about: $('<input type="button">').val(gettext('Cancel')), $('<input type="button">') .val(gettext('Discard')) .click(...) |
david
|
|
|
Why this change? |
david
|
|
|
No trailing commas in js (at least until we're totally ready to break IE8 with impunity). |
david
|
|
|
These should be in the form of <input type="button"/>, for consistency. |
 chipx86
|
|
|
Blank line before this. |
chipx86
|
|
|
It's not super clear from the change, so why is this special-cased? |
chipx86
|
|
|
Missing "Returns". |
chipx86
|
|
|
Let's leave the /> in. While not technically needed for HTML, it's something we do to help with readability a … |
chipx86
|
|
|
We don't have any other resources with these explicitly listed, but ideally, we should. We should have it for every … |
chipx86
|
|
|
You need to handle general_comments here, too. |
david
|
|
|
serailize? |
david
|
- Diff:
-
Revision 2 (+120 -37)
- 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.
-
-
- Change Summary:
-
Address David's issues. Remove add comment button from the draft banner.
- Diff:
-
Revision 3 (+87 -23)
-
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
- Change Summary:
-
Address David's issues
- Diff:
-
Revision 4 (+106 -38)
-
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
- Change Summary:
-
Address David's issues
- Diff:
-
Revision 5 (+105 -37)
-
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
-
-
-
-
-
-
Let's leave the
/>in. While not technically needed for HTML, it's something we do to help with readability a bit. -
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.
- Change Summary:
-
Addressed Christian's issues.
- Diff:
-
Revision 6 (+102 -20)
-
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)
-
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
- Diff:
-
Revision 8 (+128 -37)
-
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
- Diff:
-
Revision 9 (+128 -37)
-
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
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