Improve publishing options after submitting as another user.

Review Request #9861 — Created April 9, 2018 and submitted

Information

Review Board
release-3.0.x
653e022...

Reviewers

Review Board 3.0 began tracking which users made changes to review
requests, so that special users could modify a review request without
making it appear as if the changes came from the original owner, and to
allow changing the owner of a review request. This caused a regression
in the behavior for RBTools's --submit-as, where the review request
would be created with the specified user but published with (and
notifications going out as) the logged in user.

This change restores the original behavior. The initial publish of a
review request now always comes from the owner of the review request,
rather than the logged in user. Since --submit-as only works for the
initial creation, this makes things work how they did in previous
releases.

A new publish_as_owner=1 option was also added when publishing drafts,
which forces this same logic on future updates. This will allow
automation scripts to update review requests fully on behalf of the
owner, which wasn't possible until now.

Unit tests were added for these new changes.

All unit tests pass.

Manually posted to my server using --submit-as=... --publish. Saw that
the notifications went out as the correct user.

Temporarily modified RBTools to set publish_as_owner. Posted with and
without it. Saw that the notifications were sent out as the correct user
in both cases.

Description From Last Updated

F841 local variable 'draft' is assigned to but never used

reviewbotreviewbot

F841 local variable 'draft' is assigned to but never used

reviewbotreviewbot

Other wrapped strings in here don't have the parens (they're not necessary in this context)

daviddavid

There's an extra layer of indirection here. Why not just: if not review_request.public and not draft.changedesc_id: # ... publish_user = …

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. 
      
  2. Other wrapped strings in here don't have the parens (they're not necessary in this context)

  3. reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)
     
     
     
     
     
     
     
     

    There's an extra layer of indirection here. Why not just:

    if not review_request.public and not draft.changedesc_id:
        # ...
        publish_user = review_request.owner
    else:
        # ...
        publish_useer = request.user
    
    1. I was trying to keep the comment and its logic self-contained, since it was a bit more complicated.

    2. Oh the thing missing from your snippet is that we're overwriting publish_as_owner, which is passed by the caller.

    3. In this case I don't think it's really that complex. The comment does a good job of explaining what's necessary. If we used the publish_as_owner variable for anything else I'd say the way you structured it is good, but as-is it requires reading two conditionals instead of just one.

    4. Ohhh. I see. OK then.

  4. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (ec7dc5c)
Loading...