-
-
reviewboard/notifications/email.py (Diff revision 1) Col: 80 E501 line too long (82 > 79 characters)
-
-
-
reviewboard/settings.py (Diff revision 1) 'from settings_local import *' used; unable to detect undefined names
-
-
-
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 1) Col: 18 E231 missing whitespace after ':'
"Trivial" publishes for review requests
Review Request #7022 — Created March 7, 2015 and submitted
Each time a review request is published, it will send an e-mail (assuming e-mails are turned on for the server as a whole). We want to add a checkbox with the label "Send E-Mail", in the green draft header. If the user un-checks this box before publishing, it will publish without sending an e-mail.
Tested with new diff, checked and unchecked for "Send E-Mail".
Tested with attachment only draft, checked and unchecked for "Send E-Mail".
| Description | From | Last Updated |
|---|---|---|
|
Col: 80 E501 line too long (82 > 79 characters) |
 reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
'django_reset' imported but unused |
reviewbot
|
|
|
'from settings_local import *' used; unable to detect undefined names |
reviewbot
|
|
|
'PIPELINE_JS' imported but unused |
reviewbot
|
|
|
'PIPELINE_CSS' imported but unused |
reviewbot
|
|
|
Col: 33 W292 no newline at end of file |
reviewbot
|
|
|
Col: 18 E231 missing whitespace after ':' |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
'django_reset' imported but unused |
reviewbot
|
|
|
'from settings_local import *' used; unable to detect undefined names |
reviewbot
|
|
|
'PIPELINE_CSS' imported but unused |
reviewbot
|
|
|
'PIPELINE_JS' imported but unused |
reviewbot
|
|
|
Col: 33 W292 no newline at end of file |
reviewbot
|
|
|
Col: 18 E231 missing whitespace after ':' |
reviewbot
|
|
|
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot
|
|
|
Col: 1 W191 indentation contains tabs |
reviewbot
|
|
|
Col: 15 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
We should hide this if siteconfig.get("mail_send_review_mail") is False. |
 david
|
|
|
Please un-do this line. |
david
|
|
|
You could just add trivial=None in the argument list instead of this. |
david
|
|
|
No parens after not. Should just be ... and not trivial. |
 chipx86
|
|
|
Space before the />. |
chipx86
|
|
|
No comma after "not". |
chipx86
|
|
|
Can you change this to use single quotes? |
 brennie
|
|
|
Can you change these to use single quotes? |
brennie
|
|
|
Single quotes. |
brennie
|
|
|
See next issue. This should be something like !this.$('#not-trivial').prop('checked') to make more sense semantically. |
brennie
|
|
|
So the property is trivial and the id for the checkbox is #trivial. However, the label associated for the checkbox … |
brennie
|
|
|
This should read something like "Determines if the review request's publish will not send an E-Mail." |
brennie
|
|
|
Can we call this context varable send_email? |
david
|
|
|
Because trivial is internally a boolean value, can you use options.trivial ? 1 : 0 ? |
david
|
|
|
Because trivial is internally a boolean value, can you use options.trivial ? 1 : 0 ? |
david
|
|
|
Please verify that trivial is false when e-mail is turned off (in case other people add handlers for the signal … |
david
|
|
|
Can you test to make sure that there are no javascript errors if e-mail is turned off? |
david
|
|
|
Col: 56 W291 trailing whitespace |
reviewbot
|
|
|
Col: 8 W291 trailing whitespace |
reviewbot
|
|
|
Col: 38 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 74 W291 trailing whitespace |
reviewbot
|
|
|
Col: 21 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 35 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 25 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 22 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 34 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 34 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 18 W291 trailing whitespace |
reviewbot
|
|
|
Col: 67 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 80 E501 line too long (99 > 79 characters) |
reviewbot
|
|
|
Col: 24 E228 missing whitespace around modulo operator |
reviewbot
|
|
|
Col: 65 E228 missing whitespace around modulo operator |
reviewbot
|
|
|
Col: 26 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 46 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 50 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 55 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 38 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 44 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 71 E228 missing whitespace around modulo operator |
reviewbot
|
|
|
Col: 30 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 68 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 80 E501 line too long (105 > 79 characters) |
reviewbot
|
|
|
Col: 28 E228 missing whitespace around modulo operator |
reviewbot
|
|
|
Col: 73 E225 missing whitespace around operator |
reviewbot
|
|
|
Col: 25 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
Col: 44 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 38 E226 missing whitespace around arithmetic operator |
reviewbot
|
|
|
The "for" name no longer matches the ID of the input element. |
david
|
|
|
Undo this change. A trailing comma in a JS object causes an error in some browsers. |
brennie
|
JE
Summary: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 2 (+27 -10) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/settings.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/settings.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js -
reviewboard/notifications/email.py (Diff revision 2) Col: 80 E501 line too long (82 > 79 characters)
-
-
-
reviewboard/settings.py (Diff revision 2) 'from settings_local import *' used; unable to detect undefined names
-
-
-
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 2) Col: 18 E231 missing whitespace after ':'
JE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+25 -11) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js -
reviewboard/reviews/signals.py (Diff revision 3) Col: 1 E101 indentation contains mixed spaces and tabs
-
-
reviewboard/reviews/signals.py (Diff revision 3) Col: 15 E128 continuation line under-indented for visual indent
JE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+25 -11) |
-
Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
Can you attach a screenshot?
-
reviewboard/templates/reviews/review_header.html (Diff revision 4) We should hide this if
siteconfig.get("mail_send_review_mail")is False. -
-
reviewboard/webapi/resources/review_request_draft.py (Diff revision 4) You could just add
trivial=Nonein the argument list instead of this.
JE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+29 -12) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
reviewboard/notifications/email.py (Diff revision 5) No parens after
not. Should just be... and not trivial. -
-
JE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+29 -12) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
-
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 6) See next issue. This should be something like
!this.$('#not-trivial').prop('checked')to make more sense semantically. -
reviewboard/templates/reviews/review_header.html (Diff revision 6) So the property is trivial and the id for the checkbox is
#trivial. However, the label associated for the checkbox is "Send E-Mail."Semantically, I believe that the ID for the field should be
#not-trivialso that the state of the check box is not the inverse of the state of the publish being trivial. -
reviewboard/webapi/resources/review_request_draft.py (Diff revision 6) This should read something like "Determines if the review request's publish will not send an E-Mail."
JE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+29 -12) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
-
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 7) Because trivial is internally a boolean value, can you use
options.trivial ? 1 : 0? -
reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js (Diff revision 7) Because trivial is internally a boolean value, can you use
options.trivial ? 1 : 0? -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 7) Please verify that
trivialisfalsewhen e-mail is turned off (in case other people add handlers for the signal on the backend). -
reviewboard/templates/reviews/review_header.html (Diff revision 7) Can you test to make sure that there are no javascript errors if e-mail is turned off?
-
Tool: Pyflakes Processed Files: solution_template.py Tool: PEP8 Style Checker Processed Files: solution_template.py WARNING: Number of comments exceeded maximum, showing 30 of 38. -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
JE
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
Hey Jessica,
There are a bunch of open issues in old reviews. Can you go through and mark them as fixed/dropped as appropriate? That way new reviewers will know what to look at.
-
-
reviewboard/templates/reviews/review_header.html (Diff revision 9) The "for" name no longer matches the ID of the input element.
JE
Commit: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+29 -13)
|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/views/reviewRequestEditorView.js
-
-
reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 10) Undo this change. A trailing comma in a JS object causes an error in some browsers.
Tool: PEP8 Style Checker Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/settings.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_header.html Tool: Pyflakes Processed Files: reviewboard/notifications/email.py reviewboard/webapi/resources/review_request_draft.py reviewboard/settings.py reviewboard/reviews/signals.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/templates/reviews/review_header.html