"Trivial" publishes for review requests

Review Request #7022 — Created March 7, 2015 and submitted

Information

Review Board
master
253d8e5...

Reviewers

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)

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

Col: 33 W292 no newline at end of file

reviewbotreviewbot

Col: 18 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

'PIPELINE_CSS' imported but unused

reviewbotreviewbot

'PIPELINE_JS' imported but unused

reviewbotreviewbot

Col: 33 W292 no newline at end of file

reviewbotreviewbot

Col: 18 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 15 E128 continuation line under-indented for visual indent

reviewbotreviewbot

We should hide this if siteconfig.get("mail_send_review_mail") is False.

daviddavid

Please un-do this line.

daviddavid

You could just add trivial=None in the argument list instead of this.

daviddavid

No parens after not. Should just be ... and not trivial.

chipx86chipx86

Space before the />.

chipx86chipx86

No comma after "not".

chipx86chipx86

Can you change this to use single quotes?

brenniebrennie

Can you change these to use single quotes?

brenniebrennie

Single quotes.

brenniebrennie

See next issue. This should be something like !this.$('#not-trivial').prop('checked') to make more sense semantically.

brenniebrennie

So the property is trivial and the id for the checkbox is #trivial. However, the label associated for the checkbox …

brenniebrennie

This should read something like "Determines if the review request's publish will not send an E-Mail."

brenniebrennie

Can we call this context varable send_email?

daviddavid

Because trivial is internally a boolean value, can you use options.trivial ? 1 : 0 ?

daviddavid

Because trivial is internally a boolean value, can you use options.trivial ? 1 : 0 ?

daviddavid

Please verify that trivial is false when e-mail is turned off (in case other people add handlers for the signal …

daviddavid

Can you test to make sure that there are no javascript errors if e-mail is turned off?

daviddavid

Col: 56 W291 trailing whitespace

reviewbotreviewbot

Col: 8 W291 trailing whitespace

reviewbotreviewbot

Col: 38 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 74 W291 trailing whitespace

reviewbotreviewbot

Col: 21 E231 missing whitespace after ','

reviewbotreviewbot

Col: 35 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 25 E231 missing whitespace after ','

reviewbotreviewbot

Col: 22 E225 missing whitespace around operator

reviewbotreviewbot

Col: 34 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 34 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 18 W291 trailing whitespace

reviewbotreviewbot

Col: 67 E225 missing whitespace around operator

reviewbotreviewbot

Col: 80 E501 line too long (99 > 79 characters)

reviewbotreviewbot

Col: 24 E228 missing whitespace around modulo operator

reviewbotreviewbot

Col: 65 E228 missing whitespace around modulo operator

reviewbotreviewbot

Col: 26 E225 missing whitespace around operator

reviewbotreviewbot

Col: 46 E231 missing whitespace after ','

reviewbotreviewbot

Col: 50 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 55 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 38 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 44 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 71 E228 missing whitespace around modulo operator

reviewbotreviewbot

Col: 30 E225 missing whitespace around operator

reviewbotreviewbot

Col: 68 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 80 E501 line too long (105 > 79 characters)

reviewbotreviewbot

Col: 28 E228 missing whitespace around modulo operator

reviewbotreviewbot

Col: 73 E225 missing whitespace around operator

reviewbotreviewbot

Col: 25 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 44 E231 missing whitespace after ','

reviewbotreviewbot

Col: 38 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

The "for" name no longer matches the ID of the input element.

daviddavid

Undo this change. A trailing comma in a JS object causes an error in some browsers.

brenniebrennie
reviewbot
  1. 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
    
    
  2. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. reviewboard/reviews/signals.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/settings.py (Diff revision 1)
     
     
     'django_reset' imported but unused
    
  5. reviewboard/settings.py (Diff revision 1)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  6. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_JS' imported but unused
    
  7. reviewboard/settings.py (Diff revision 1)
     
     
     'PIPELINE_CSS' imported but unused
    
  8. reviewboard/settings.py (Diff revision 1)
     
     
    Col: 33
     W292 no newline at end of file
    
  9. Col: 18
     E231 missing whitespace after ':'
    
  10. 
      
JE
reviewbot
  1. 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
    
    
  2. reviewboard/notifications/email.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. reviewboard/reviews/signals.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  4. reviewboard/settings.py (Diff revision 2)
     
     
     'django_reset' imported but unused
    
  5. reviewboard/settings.py (Diff revision 2)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  6. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_CSS' imported but unused
    
  7. reviewboard/settings.py (Diff revision 2)
     
     
     'PIPELINE_JS' imported but unused
    
  8. reviewboard/settings.py (Diff revision 2)
     
     
    Col: 33
     W292 no newline at end of file
    
  9. Col: 18
     E231 missing whitespace after ':'
    
  10. 
      
JE
reviewbot
  1. 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
    
    
  2. reviewboard/reviews/signals.py (Diff revision 3)
     
     
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  3. reviewboard/reviews/signals.py (Diff revision 3)
     
     
    Col: 1
     W191 indentation contains tabs
    
  4. reviewboard/reviews/signals.py (Diff revision 3)
     
     
    Col: 15
     E128 continuation line under-indented for visual indent
    
  5. 
      
JE
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Can you attach a screenshot?

  2. We should hide this if siteconfig.get("mail_send_review_mail") is False.

  3. Please un-do this line.

  4. You could just add trivial=None in the argument list instead of this.

  5. 
      
JE
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 5)
     
     

    No parens after not. Should just be ... and not trivial.

  3. Space before the />.

  4. No comma after "not".

  5. 
      
JE
JE
JE
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/notifications/email.py (Diff revision 6)
     
     

    Can you change this to use single quotes?

    1. The other methods of email.py all use double quotes for siteconfig.get() method.
    2. In the past we have used both double and single quotes, but we are moving towards using only single quotes in the code base (with some exceptions).

  3. reviewboard/reviews/signals.py (Diff revision 6)
     
     

    Can you change these to use single quotes?

    1. The other methods of signals.py all use double quotes.

  4. reviewboard/reviews/views.py (Diff revision 6)
     
     

    Single quotes.

    1. The other methods of views.py all use double quotes for siteconfig.get() method.

  5. See next issue. This should be something like !this.$('#not-trivial').prop('checked') to make more sense semantically.

  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-trivial so that the state of the check box is not the inverse of the state of the publish being trivial.

  7. This should read something like "Determines if the review request's publish will not send an E-Mail."

  8. 
      
JE
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 7)
     
     

    Can we call this context varable send_email?

  3. Because trivial is internally a boolean value, can you use options.trivial ? 1 : 0 ?

  4. Because trivial is internally a boolean value, can you use options.trivial ? 1 : 0 ?

  5. Please verify that trivial is false when e-mail is turned off (in case other people add handlers for the signal on the backend).

  6. 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?

  7. 
      
JE
reviewbot
  1. 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.
  2. solution_template.py (Diff revision 8)
     
     
    Col: 56
     W291 trailing whitespace
    
  3. solution_template.py (Diff revision 8)
     
     
    Col: 8
     W291 trailing whitespace
    
  4. solution_template.py (Diff revision 8)
     
     
    Col: 38
     E226 missing whitespace around arithmetic operator
    
  5. solution_template.py (Diff revision 8)
     
     
    Col: 74
     W291 trailing whitespace
    
  6. solution_template.py (Diff revision 8)
     
     
    Col: 21
     E231 missing whitespace after ','
    
  7. solution_template.py (Diff revision 8)
     
     
    Col: 35
     E226 missing whitespace around arithmetic operator
    
  8. solution_template.py (Diff revision 8)
     
     
    Col: 25
     E231 missing whitespace after ','
    
  9. solution_template.py (Diff revision 8)
     
     
    Col: 22
     E225 missing whitespace around operator
    
  10. solution_template.py (Diff revision 8)
     
     
    Col: 34
     E226 missing whitespace around arithmetic operator
    
  11. solution_template.py (Diff revision 8)
     
     
    Col: 34
     E226 missing whitespace around arithmetic operator
    
  12. solution_template.py (Diff revision 8)
     
     
    Col: 18
     W291 trailing whitespace
    
  13. solution_template.py (Diff revision 8)
     
     
    Col: 67
     E225 missing whitespace around operator
    
  14. solution_template.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (99 > 79 characters)
    
  15. solution_template.py (Diff revision 8)
     
     
    Col: 24
     E228 missing whitespace around modulo operator
    
  16. solution_template.py (Diff revision 8)
     
     
    Col: 65
     E228 missing whitespace around modulo operator
    
  17. solution_template.py (Diff revision 8)
     
     
    Col: 26
     E225 missing whitespace around operator
    
  18. solution_template.py (Diff revision 8)
     
     
    Col: 46
     E231 missing whitespace after ','
    
  19. solution_template.py (Diff revision 8)
     
     
    Col: 50
     E226 missing whitespace around arithmetic operator
    
  20. solution_template.py (Diff revision 8)
     
     
    Col: 55
     E226 missing whitespace around arithmetic operator
    
  21. solution_template.py (Diff revision 8)
     
     
    Col: 38
     E226 missing whitespace around arithmetic operator
    
  22. solution_template.py (Diff revision 8)
     
     
    Col: 44
     E226 missing whitespace around arithmetic operator
    
  23. solution_template.py (Diff revision 8)
     
     
    Col: 71
     E228 missing whitespace around modulo operator
    
  24. solution_template.py (Diff revision 8)
     
     
    Col: 30
     E225 missing whitespace around operator
    
  25. solution_template.py (Diff revision 8)
     
     
    Col: 68
     E226 missing whitespace around arithmetic operator
    
  26. solution_template.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (105 > 79 characters)
    
  27. solution_template.py (Diff revision 8)
     
     
    Col: 28
     E228 missing whitespace around modulo operator
    
  28. solution_template.py (Diff revision 8)
     
     
    Col: 73
     E225 missing whitespace around operator
    
  29. solution_template.py (Diff revision 8)
     
     
    Col: 25
     E226 missing whitespace around arithmetic operator
    
  30. solution_template.py (Diff revision 8)
     
     
    Col: 44
     E231 missing whitespace after ','
    
  31. solution_template.py (Diff revision 8)
     
     
    Col: 38
     E226 missing whitespace around arithmetic operator
    
  32. 
      
JE
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 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.

  2. 
      
david
  1. 
      
  2. The "for" name no longer matches the ID of the input element.

  3. 
      
JE
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Undo this change. A trailing comma in a JS object causes an error in some browsers.

  3. 
      
JE
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (051b134)
Loading...