Add "Publish and Archive" to the review dialog.

Review Request #9220 — Created Sept. 24, 2017 and submitted

Information

Review Board
release-4.0.x
9275765...

Reviewers

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.

Description From Last Updated

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

daviddavid

No space between . and "

brenniebrennie

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

brenniebrennie

"creates" For our testing docstrings, we wrap them at 79 but we don't really follow docstring convention, e.g. it is …

brenniebrennie

E501 line too long (84 > 79 characters)

reviewbotreviewbot

Not necessary to pull this out.

brenniebrennie

E501 line too long (86 > 79 characters)

reviewbotreviewbot

We could also make this method return the visit object since we're using that in the tests. It would make …

brenniebrennie

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

brenniebrennie

"updates"

brenniebrennie

E501 line too long (100 > 79 characters)

reviewbotreviewbot

Not necessary to pull this out.

brenniebrennie

Blank line between these.

brenniebrennie

E501 line too long (106 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

"updates"

brenniebrennie

E501 line too long (100 > 79 characters)

reviewbotreviewbot

Not necessary to pull this out.

brenniebrennie

E501 line too long (106 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

Trailing comma

brenniebrennie

Trailing comma

brenniebrennie

Trailing comma

brenniebrennie

The inner () should be {}. Space after :.

brenniebrennie

Col: 34 Expected ')' to match '(' from line 114 and instead saw ':'.

reviewbotreviewbot

Col: 35 Expected ')' and instead saw 'true'.

reviewbotreviewbot

Col: 39 Missing semicolon.

reviewbotreviewbot

Col: 13 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 13 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 14 Missing semicolon.

reviewbotreviewbot

Col: 14 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 14 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

Can you alphabetically sort this with the other imports?

brenniebrennie

E501 line too long (81 > 79 characters)

reviewbotreviewbot

Blank line between these.

brenniebrennie

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (100 > 79 characters)

reviewbotreviewbot

E501 line too long (106 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (100 > 79 characters)

reviewbotreviewbot

E501 line too long (106 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

E501 line too long (122 > 79 characters)

reviewbotreviewbot

E501 line too long (106 > 79 characters)

reviewbotreviewbot

E501 line too long (122 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

It was like this before, but can you fix up this docstring? There should be no space between the """ …

daviddavid

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

daviddavid

We should include a trailing comma on this line too.

daviddavid

Blank line between these two.

daviddavid

Add a trailing comma here.

daviddavid

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 …

daviddavid

Same here.

daviddavid

See my reply to previous reviews.

daviddavid

This line in too long. Can you do it as: expect(view._onPublishClicked).toHaveBeenCalledWith({ publishAndArchive: true, });

daviddavid

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

daviddavid

W291 trailing whitespace

reviewbotreviewbot

ReviewRequest should be aligned with reviewboard.

brenniebrennie

This should be aligned like: review_request (reviewboard.reviews.models.review_request. ReviewRequest): The review request to update the visibility of.

daviddavid

How about "The new visibility".

brenniebrennie

This isn't a .es6.js file, so there shouldn't be a comma after the last item in the object.

daviddavid

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

daviddavid

Can you change to "Unarchive a review request"

chipx86chipx86

The "Returns the visit" needs to be removed from the summary. Description also needs to be fleshed out to describe …

chipx86chipx86

Should ideally list the visibility constants.

chipx86chipx86

Can you swap these so they're in alphabetical order?

chipx86chipx86

Here, too.

chipx86chipx86

And here.

chipx86chipx86

Indentation is kinda wonky. Can you change this to: attrs: [ ... ],

chipx86chipx86

4.0?

chipx86chipx86

Looks like this is the wrong way now.

chipx86chipx86
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

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

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

    No space between . and "

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

    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)
     
     
    Show all issues

    "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)
     
     
    Show all issues

    Not necessary to pull this out.

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    "updates"

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

    Not necessary to pull this out.

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

    Blank line between these.

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

    "updates"

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

    Not necessary to pull this out.

  13. Show all issues

    Trailing comma

  14. Show all issues

    Trailing comma

  15. Show all issues

    Trailing comma

  16. Show all issues

    The inner () should be {}.

    Space after :.

  17. Show all issues

    Can you alphabetically sort this with the other imports?

  18. reviewboard/webapi/resources/base_review.py (Diff revision 1)
     
     
     
    Show all issues

    Blank line between these.

  19. 
      
giuliacm
Review request changed
Commit:
0d3d440a6186d1c24a207a3bc894654f3c8bb51b
be07e3c00c4e71080e8738353cf4abd7968fe2df

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

giuliacm
Review request changed
Commit:
be07e3c00c4e71080e8738353cf4abd7968fe2df
f4adfb8c6ab4cc1e071ca4368576e3132931af77

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

giuliacm
david
  1. The substance of the change looks good. Just a few formatting/documentation comments.

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

    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)
     
     
    Show all issues

    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. Show all issues

    We should include a trailing comma on this line too.

  5. Show all issues

    Blank line between these two.

  6. Show all issues

    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. 
      
giuliacm
david
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 5)
     
     
    Show all issues

    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)
     
     
    Show all issues

    Same here.

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

    See my reply to previous reviews.

  5. Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    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. 
      
giuliacm
giuliacm
Review request changed
giuliacm
giuliacm
david
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 9)
     
     
     
     
    Show all issues

    This should be aligned like:

    review_request (reviewboard.reviews.models.review_request.
                    ReviewRequest):
        The review request to update the visibility of.
    
  3. Show all issues

    This isn't a .es6.js file, so there shouldn't be a comma after the last item in the object.

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

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

    ReviewRequest should be aligned with reviewboard.

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

    How about "The new visibility".

  4. 
      
giuliacm
david
  1. Looking great!

  2. Show all issues

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

  3. Show all issues

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

  4. 
      
giuliacm
david
  1. Ship It!
  2. 
      
david
david
chipx86
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 12)
     
     
    Show all issues

    Can you change to "Unarchive a review request"

  3. reviewboard/accounts/managers.py (Diff revision 12)
     
     
    Show all issues

    The "Returns the visit" needs to be removed from the summary.

    Description also needs to be fleshed out to describe what updating the visibility means.

  4. reviewboard/accounts/managers.py (Diff revision 12)
     
     
     
    Show all issues

    Should ideally list the visibility constants.

  5. Show all issues

    Can you swap these so they're in alphabetical order?

  6. Show all issues

    Here, too.

  7. Show all issues

    And here.

  8. Show all issues

    Indentation is kinda wonky. Can you change this to:

    attrs: [
        ...
    ],
    
  9. Show all issues

    4.0?

  10. 
      
david
chipx86
  1. 
      
  2. Show all issues

    Looks like this is the wrong way now.

  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (29aa306)