Prevent users from publishing empty (no modifications made) review request drafts.

Review Request #6322 — Created Sept. 14, 2014 and submitted

mloyzer
Review Board
master
3452
70eab4a...
reviewboard, students

When a user submits an empty draft for a review request a warning is issued to let them know they did not make any modifications.

The general flow is:
1. In the review_request_draft model determine if there were any changes to any of the fields, if not then raise a 'Not Modified' exception
2. The review_request_draft controller/resource will catch the 'Not Modified' exception and return an appropriate error message to the Client JS to display a meaningful message to the user.
3. Client JS displays error message and the 'Publish Changes' and 'Discard Draft' buttons remain disabled.
4. Once the user updates (edits and confirms by clicking 'OK') a field (Description, Testing Done) then the 'Publish Changes' and 'Discard Draft' buttons are re-enabled.

Tried "not-modifying" the Description, and Testing Done fields to determine the output.

Wrote test case to publish an empty draft (test case passes).

Postive Test Cases:
Added a file (successful publish).
Remove a file (successful publish).
Changing the description field (successful publish).
Add comments (successful publish).
Add branch/target people/bug number.

  • 0
  • 0
  • 30
  • 0
  • 30
Description From Last Updated
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. 
      
chipx86
  1. Some high-level comments:

    • Code should never inject functions into other objects (the one being put into reviewRequest).
    • You should use get() and set() instead of accessing attributes directly.
    • Make sure to keep to the < 80 character line length limit.

    As discussed in IRC, there's a different way this particular change should probably be tackled. This is really an issue that needs to be handled on the backend, rather than the frontend, in the publishing code.

  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
        reviewboard/reviews/models/review_request_draft.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. reviewboard/changedescs/models.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/reviews/errors.py (Diff revision 2)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/reviews/errors.py (Diff revision 2)
     
     
    Col: 1
     W391 blank line at end of file
    
  5. Col: 1
     W293 blank line contains whitespace
    
  6. 
      
  1. Not sure about the front end, but it seems like the behaviour on the back end could be added to unit tests.

  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. reviewboard/changedescs/models.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/reviews/errors.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/reviews/errors.py (Diff revision 3)
     
     
    Col: 1
     W391 blank line at end of file
    
  5. Col: 1
     W293 blank line contains whitespace
    
  6. Col: 1
     W293 blank line contains whitespace
    
  7.  local variable 'draft' is assigned to but never used
    
  8. Col: 1
     W293 blank line contains whitespace
    
  9. 
      
chipx86
  1. Some general comments:

    1) The summary is vague. It doesn't tell me what field we're talking about or why it matters. Should have something a little more precise yet still concise.
    2) Make sure you check your diff for things like trailing whitespace or unnecessary changes (like extra blank lines in sectinos of code that wasn't otherwise modified).

  2. reviewboard/changedescs/models.py (Diff revision 3)
     
     

    New functions and classes should include docstrings. They're always in the format of:

    """Single-line summary."""
    

    or:

    """Single-line summary.
    
    Multi-line comment.
    """
    

    Same with anything else you've introduced.

  3. The blank line should remain.

  4. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. Col: 1
     W293 blank line contains whitespace
    
  3.  local variable 'draft' is assigned to but never used
    
  4. Col: 1
     W293 blank line contains whitespace
    
  5. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. reviewboard/changedescs/models.py (Diff revision 6)
     
     
    Col: 69
     W291 trailing whitespace
    
  3. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/changedescs/models.py (Diff revision 7)
     
     
     
     
     

    This can just be return bool(self.fields_changed)

  3. Please revert the change to this line (should have two blank lines between the imports and the class).

  4. Can you move this up above the other if changedesc: block to avoid saving the object?

  5. It looks like you added a tab character here. Please remove it.

    1. I didn't see this in my code, i tried just playing with the whitespace...hopefully it will be resolved

  6. Can you swap these? (N before P)

  7. Can you put NOTHING_TO_PUBLISH before PUBLISH_ERROR?

  8. You're not using the exception instance, so this should just be except NotModifiedError:

  9. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. 
      
david
  1. 
      
  2. There's still an extra tab character at the beginning of this line.

  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. 
      
david
  1. The code looks pretty good. I have just a couple comments/questions:

    • Can you change the summary to more accurately reflect what's going on? Something like "Prevent users from publishing empty review request drafts".
    • Can you test the positive cases? I'd like to be sure that adding/removing a file attachment or uploading a new diff without changing any of the "fields" still works.
  2. 
      
david
  1. You've got some spelling errors in your description and testing done:

    • review_request_drat -> review_request_draft
    • re-enabled -> re-enables (to match tense)
    • Postivie -> Positive
    • successfull -> successful
  2. 
      
  1. 
      
  2. reviewboard/reviews/errors.py (Diff revision 9)
     
     

    What is the difference between this and:

    class NotModifiedError(Exception):
        pass
    
  3. 
      
mike_conley
  1. I've tested this, and it works as advertised. Great stuff!

    I do think, however, that the alert dialog to display error messages is pretty awful. That's not caused by this patch - it pre-existed - but I do think that's something we should get rid of at some point. I filed issue 3578 to switch from alert to something else.

    Anyhow, this looks fine to me.

  2. reviewboard/changedescs/models.py (Diff revision 9)
     
     
     
     
     
     

    This documentation should go below the function header - see the record_field_change function, for example.

  3. Oh good, yes - this has burned us before when things have gone wrong on the server side, and the user was just flat-out stuck in this half-published state.

  4. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/changedescs/models.py (Diff revision 10)
     
     
     
     
     
     

    This msut be inside the function, not on the outside.

  3. reviewboard/reviews/errors.py (Diff revision 10)
     
     

    Needs a docstring.

  4. Models aren't allowed to access elements in the page. Only ReviewRequestEditorView is allowed to deal with the state of these buttons.

  5. reviewboard/webapi/errors.py (Diff revision 10)
     
     
     

    How about "... review request without any modifications." ?

  6. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
    
    
  2. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request_draft.py
        reviewboard/webapi/errors.py
        reviewboard/reviews/models/review_request_draft.py
        reviewboard/webapi/tests/test_review_request_draft.py
        reviewboard/reviews/errors.py
        reviewboard/changedescs/models.py
    
    
  2. 
      
  1. Ship It!

  2. 
      
david
  1. I'm going to make a trivial change (putting back the button re-enabling, but doing so in the view instead of the model) and then push this. Thanks for your solid work!

  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (f29c57f)
Loading...