Add "Publish and Archive" to the review dialog.

Review Request #9220 - Created Sept. 24, 2017 and updated

Giulia Mattia
Review Board
release-3.0.x
653546b...
reviewboard, students

This change adds a shortcut allowing people who are publishing a review
to publish the review and archive the review request all in one shot.

Manually tested adding a comment and clicking "Publish and Archive" from
the banner. Once published, the review request is marked as archived.
Manually tested adding a comment, clicking "Edit Review" from the
banner, and clicking "Publish and Archive" from the review dialog view.
Once published, the review request is marked as archived.

Added 3 unit tests, which all pass (along with all other tests). One
scenario creates a new visit and archives the review request. The other
two scenarios update existing visits from visible to visible, and
visible to archived.

Added a javascript test to ensure that onPublishClicked is called with
the correct values.

  • 0
  • 0
  • 75
  • 0
  • 75
Description From Last Updated
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

Barret Rennie
  1. This looks great. You can probably remove the WIP tag :)

  2. reviewboard/accounts/managers.py (Diff revision 1)
     
     

    No space between . and "

  3. reviewboard/accounts/managers.py (Diff revision 1)
     
     

    Docstrings should be in the imperitive mood (i.e., they should read as instructions). e.g. "update" instead of "updates".

    No space around the docstring.

    Should be formatted as:

    """Single line summary.
    
    Multi line description (if applicable).
    
    Args:
        review_request (type of review_request):
            ...
    """
    

    If this returned something it would also need:

    """
    Returns:
        return_type:
        Description of returned value.
    """
    
    1. Would I need to account for Args: user and new_visibility in this case?

    2. Yes, you',ll have to document all of them.

  4. reviewboard/accounts/tests.py (Diff revision 1)
     
     

    "creates"

    For our testing docstrings, we wrap them at 79 but we
    don't really follow docstring convention, e.g. it is ok to do

    """Testing some_some_very_long_method
    does some thing
    """
    
  5. reviewboard/accounts/tests.py (Diff revision 1)
     
     

    Not necessary to pull this out.

  6. reviewboard/accounts/tests.py (Diff revision 1)
     
     
     
     
     

    We could also make this method return the visit object since we're using that in the tests. It would make them shorter and involve fewer queries.

    1. Just to clarify, do you mean the method test_update_visibility_create(), or ReviewRequestVisitManager.update_visibility() ?

    2. I mean update_visibility can return the visit to use it here. Commented in the wrong place :)

    3. Okay, so adding "return visit" to update_visibility() wouldn't affect any other parts of my code, but would shorten the testing?

    4. Yes.

  7. reviewboard/accounts/tests.py (Diff revision 1)
     
     

    You can do user=user here and it will look it up by primary key.

    Same below.

    1. Would it look something like this?
      visit = ReviewRequestVisit.objects.get(user=user)

    2. Exactly that, except you'll want to do review_request=review_request here as well.

    3. Ok, and not review_request=review_request.id (how it was before)?

    4. Yeah. Technically that will work but its not super idiomatic. Looking up an object by its id would be review_request_id=review_request.id. Doing review_request=review_request does that basically behind the scenes (looks it up by ID).

  8. reviewboard/accounts/tests.py (Diff revision 1)
     
     

    "updates"

  9. reviewboard/accounts/tests.py (Diff revision 1)
     
     

    Not necessary to pull this out.

  10. reviewboard/accounts/tests.py (Diff revision 1)
     
     
     

    Blank line between these.

  11. reviewboard/accounts/tests.py (Diff revision 1)
     
     

    "updates"

  12. reviewboard/accounts/tests.py (Diff revision 1)
     
     

    Not necessary to pull this out.

  13. The inner () should be {}.

    Space after :.

  14. Can you alphabetically sort this with the other imports?

  15. reviewboard/webapi/resources/base_review.py (Diff revision 1)
     
     
     

    Blank line between these.

  16. 
      
Giulia Mattia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Giulia Mattia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Giulia Mattia
David Trowbridge
  1. The substance of the change looks good. Just a few formatting/documentation comments.

  2. reviewboard/accounts/managers.py (Diff revision 4)
     
     

    It was like this before, but can you fix up this docstring? There should be no space between the """ and the first line, the summary line should be in the imperative mood instead of passive ("Unarchive" vs. "Unarchives"), and we should document the review_request parameter.

    1. I'm unsure what type would go in the () for the review_request parameter.

    2. reviewboard.reviews.models.review_request.ReviewRequest

      This can be wrapped after any period to stay within the 80-column limit.

  3. reviewboard/accounts/managers.py (Diff revision 4)
     
     

    Same comments on this docstring--space, imperative mood, and "Args" documentation.

    1. Also unsure what type would go in the () for review_request, user and new_visibility.

    2. user is django.contrib.auth.models.User
      new_visibility is unicode (since it's one of the constants defined in the ReviewRequestVisit mode).

  4. We should include a trailing comma on this line too.

  5. Blank line between these two.

  6. Add a trailing comma here.

    1. I made the change but there is also no trailing comma in the alternative for publishToSubmitterOnly, which was already existing code. Is there a rule for when to use trailing commas?

    2. We should always have them in files that end in .es6.js. For ones that are just .js, it shouldn't have it on the last item in a list because some versions of IE choke. Probably what happened was we missed the existing one when converting this file to ES6.

    3. Thanks, that makes sense!

  7. 
      
Giulia Mattia
David Trowbridge
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 5)
     
     

    This should be reviewboard.reviews.models.review_request.ReviewRequest. The reviews.models.ReviewRequest is an alias but won't necessarily link to the right place. You can wrap this after any period to stay within the 80-column limit.

  3. reviewboard/accounts/managers.py (Diff revision 5)
     
     

    Same here.

  4. reviewboard/accounts/managers.py (Diff revision 5)
     
     
     
     
     

    See my reply to previous reviews.

  5. This line in too long. Can you do it as:

    expect(view._onPublishClicked).toHaveBeenCalledWith({
        publishAndArchive: true,
    });
    
  6. reviewboard/webapi/resources/base_review.py (Diff revision 5)
     
     
     
     
     
     

    This is getting big enough that we should probably put each parameter on its own line.

    1. Do you want me to separate the first line of parameters to be on their own lines (self, requst, review, public=None) ?

    2. Yeah, like so:

      def update_review(self,
                        request,
                        review,
                        public=None,
                        ...
      
    3. Ok thanks, is that just convention when parameter list is getting too long vs. what fits in each line?

    4. Yeah. Up to about 3 lines, it's still pretty readable wrapping with multiple parameters per line. Once it goes above that, it's usually best to split each one out.

  7. 
      
Giulia Mattia
Giulia Mattia
Review request changed
Giulia Mattia
Giulia Mattia
David Trowbridge
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 9)
     
     
     
     

    This should be aligned like:

    review_request (reviewboard.reviews.models.review_request.
                    ReviewRequest):
        The review request to update the visibility of.
    
  3. This isn't a .es6.js file, so there shouldn't be a comma after the last item in the object.

  4. 
      
Barret Rennie
  1. Pending these nits & David's, I'm happy with this change!

  2. reviewboard/accounts/managers.py (Diff revision 9)
     
     
     

    ReviewRequest should be aligned with reviewboard.

  3. reviewboard/accounts/managers.py (Diff revision 9)
     
     

    How about "The new visibility".

  4. 
      
Giulia Mattia
David Trowbridge
  1. Looking great!

  2. I think you can remove "WIP" from your summary now.

  3. Can you put the last item on the next line to keep in wrapping in 80 columns?

  4. 
      
Giulia Mattia
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
David Trowbridge
  1. Ship It!
  2. 
      
Loading...