brennie got a fish trophy!

Summary

Allow for trivial review reply publishes

Review Request #7447 — Created June 23, 2015 and submitted June 25, 2015, 2:45 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

f638708...

Reviewers

Groups

reviewboard

People

Description

Trivial review replies can be published, that is, published without

sending an e-mail notification.
This patch also fixes up some of the semantics in the review request

publishing. The not-trivial id has been changed to a send-email

class as it makes a bit more semantic sense.
NB: This is heavily based on Jessica Qian's work in

https://reviews.reviewboard.org/r/7125/

Testing Done

Ran unit tests. Ran JS tests.

Tested both review reply and review request publishing with and without
the "Send E-Mail" box checked. It works as expected.

Tested that the "Send E-Mail" checkboxes do not appear when e-mails are
disabled for the server.

Issues

Description	From	Last Updated
This should default to False rather than None	david	June 24, 2015, 5:20 p.m.
See my other comment about this in the review request banner.	david	June 24, 2015, 5:19 p.m.
Doesn't this change make it so you can't click the text to change the checkbox anymore?	david	June 24, 2015, 5:19 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/reviews/models/review.py
    reviewboard/webapi/resources/review_reply.py
    reviewboard/reviews/signals.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/reviews/models/review.py
    reviewboard/webapi/resources/review_reply.py
    reviewboard/reviews/signals.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js

Description:: Trivial review replies can be published, that is, published without
sending an e-mail notification.

This patch also fixes up some of the semantics in the review request
~ publishing. The not-trivial id has been changed to a end-email
~ publishing. The not-trivial id has been changed to a send-email
class as it makes a bit more semantic sense.

NB: This is heavily based on Jessica Qian's work in
https://reviews.reviewboard.org/r/7125/
Testing Done:: ~
Ran unit tests.
~
Ran unit tests. Ran JS tests.

Tested both review reply and review request publishing with and without
the "Send E-Mail" box checked. It works as expected.

Tested that the "Send E-Mail" checkboxes do not appear when e-mails are
disabled for the server.

Commit:

a9101464acb271fb9a87fe1f7f807ae219b59715

e81c9f7f54597bf73926203a22b5ad4875e3d002

Diff:

Revision 2 (+147 -22)

Show changes

	reviewboard/notifications/email.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review.py
	reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	reviewboard/static/rb/js/resources/models/reviewReplyModel.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	reviewboard/static/rb/js/views/reviewBoxView.js
	8 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js

Change Summary:

Remove blank line.

Commit:

e81c9f7f54597bf73926203a22b5ad4875e3d002

9efbc9e0c79dc9c03c9886f55b2c193ee6bb4a79

Diff:

Revision 3 (+146 -22)

Show changes

	reviewboard/notifications/email.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review.py
	reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	reviewboard/static/rb/js/resources/models/reviewReplyModel.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	reviewboard/static/rb/js/views/reviewBoxView.js
	8 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js

Change Summary:

Remove debugging statement

Commit:

9efbc9e0c79dc9c03c9886f55b2c193ee6bb4a79

5d27618c35b8271e7e51149c687b5b2432b9f199

Diff:

Revision 4 (+144 -22)

Show changes

	reviewboard/notifications/email.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review.py
	reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	reviewboard/static/rb/js/resources/models/reviewReplyModel.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	reviewboard/static/rb/js/views/reviewBoxView.js
	8 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js

reviewboard/reviews/models/review.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should default to False rather than None
```

reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js (Diff revision 4)

The issue has been dropped. Show all issues

See my other comment about this in the review request banner.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)

The issue has been dropped. Show all issues

Doesn't this change make it so you can't click the text to change the checkbox anymore?

brennie June 24, 2015, 5:19 p.m.

If you nest a an <input> inside a <label>, all text inside the <label> will focus/toggle the <input>.

Change Summary:

Address David's issues.

Commit:

5d27618c35b8271e7e51149c687b5b2432b9f199

f6387080f115de44e4a86f616747554120e01f3c

Diff:

Revision 5 (+144 -22)

Show changes

	reviewboard/notifications/email.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/review.py
	reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
	reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
	reviewboard/static/rb/js/resources/models/reviewReplyModel.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	reviewboard/static/rb/js/views/reviewBoxView.js
	8 more

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/tests/test_review_reply.py
    reviewboard/webapi/resources/review_request_draft.py
    reviewboard/reviews/models/review.py
    reviewboard/notifications/email.py
    reviewboard/reviews/signals.py
    reviewboard/webapi/resources/review_reply.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.js
    reviewboard/static/rb/js/resources/models/reviewReplyModel.js
    reviewboard/static/rb/js/views/reviewBoxListView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js
    reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js
    reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
    reviewboard/static/rb/js/views/reviewBoxView.js
    reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (88d8d33)

		Trivial review replies can be published, that is, published without
		sending an e-mail notification.

		This patch also fixes up some of the semantics in the review request
~		publishing. The `not-trivial` id has been changed to a `end-email`
	~	publishing. The `not-trivial` id has been changed to a `send-email`
		class as it makes a bit more semantic sense.

		NB: This is heavily based on Jessica Qian's work in
		https://reviews.reviewboard.org/r/7125/

~		Ran unit tests.
	~	Ran unit tests. Ran JS tests.

		Tested both review reply and review request publishing with and without
		the "Send E-Mail" box checked. It works as expected.

		Tested that the "Send E-Mail" checkboxes do not appear when e-mails are
		disabled for the server.