Summary

Improve publishing options after submitting as another user.

Review Request #9861 — Created April 9, 2018 and submitted April 10, 2018, 2:44 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

653e022...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
F841 local variable 'draft' is assigned to but never used	reviewbot	April 9, 2018, 10:32 p.m.
F841 local variable 'draft' is assigned to but never used	reviewbot	April 9, 2018, 10:32 p.m.
Other wrapped strings in here don't have the parens (they're not necessary in this context)	david	April 9, 2018, 10:52 p.m.
There's an extra layer of indirection here. Why not just: if not review_request.public and not draft.changedesc_id: # ... publish_user = …	david	April 9, 2018, 10:56 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```
reviewboard/webapi/tests/test_review_request_draft.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'draft' is assigned to but never used
```

Change Summary:

Removed unused variables.

Commit:

f6b4cf762c6d2b8b448d17b7b72bdc102da11805

f3975936864a39b618903969e4a1672ad46f50c5

Diff:

Revision 2 (+175 -3)

Show changes

	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/webapi/resources/review_request_draft.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Other wrapped strings in here don't have the parens (they're not necessary in this context)
```

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

The issue has been dropped. Show all issues

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

chipx86 April 9, 2018, 10:53 p.m.

I was trying to keep the comment and its logic self-contained, since it was a bit more complicated.

chipx86 April 9, 2018, 10:53 p.m.

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

david April 9, 2018, 10:54 p.m.

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.

david April 9, 2018, 10:56 p.m.
```
Ohhh. I see. OK then.
```

Change Summary:


Removed parens around a wrapped string.
Expanded on the comment to reference the parameter from the request.

Commit:

f3975936864a39b618903969e4a1672ad46f50c5

653e0223873363611b81f84467a1361e4b18f50e

Diff:

Revision 3 (+176 -3)

Show changes

	reviewboard/webapi/resources/review_request_draft.py
	reviewboard/webapi/tests/test_review_request_draft.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (ec7dc5c)