Closed Bug 1286740 Opened 9 years ago Closed 8 years ago

Changing reviewer in MozReview changes the requester on bugzilla

Categories

(MozReview Graveyard :: Integration: Bugzilla, defect)

Product:
MozReview Graveyard
Component:
Integration: Bugzilla
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: xidorn, Unassigned)

Details

Attachments

(1 file)

mozreview: Reuse flags when publishing review requests (bug 1286740)
59 bytes, text/x-review-board-request
glob
: review+
imadueme
: review+
Details
If I change the reviewer to someone, the requester of that review request becomes me, not the original requester. e.g. in bug 1277876, bsmedberg requested review from me, and I redirected it to bz. Now it shows "xidorn: review? bz" rather than "bsmedberg: review? bz". Similiarly, in bug 1286089, gps redirected my review request to glandium, and now it shows "gps: review? glandium", and I received a "review canceled" flag mail. I don't think this is something we expect. I think we want to keep the requester even if the review request is redirected.
Yeah this is a bit complicated. In Bugzilla, you can change the requestee of a r? even if you aren't the requester. However, you can't *create* an r? with the requester as someone else. So in the simple case that you describe, this would be straightforward. However let's say you also added two other reviewers, "foo" and "bar". In that case you'd have to re-use the existing r? and then add two new ones, which would end up as (using your first example), "bsmedberg: review? bz", "xidorn: review? foo", "xidorn: review? bar". The complicated case is probably pretty rare, but we'd need to account for it, which will be a bit tricky to get right. Definitely something we can do though.
What would happen if you change requestee directly on Bugzilla? IIRC that wouldn't change the requester, would that?
A similar scenario (I think) just happened to me: After r+'ing a patch, the author decided one of my nits needed a big change, so I thought I would remind myself to re-review the patch by setting it back to 'r?'. But this made me the requester as well. Of course comment 1 shows it may not be obvious to "fix". Maybe there could be an option (in both bugzilla and mozreview) to set the requester to the patch author?
Oh, and now MozReview doesn't remove the existing review request at all.
Still happening. Also, Bugzilla retains the original review request! (I think that's what Xidorn said in comment 4?) E.g.: After a push, we have [r?me] in mozreview, and 'author:r?me' in Bugzilla. In mozreview, I edit the reviewer from 'me' to 'them', OK, Publish. Now we have [r?them] in mozreview as expected, but in bugzilla the original request 'author:r?me' is still there, and there's an added request 'me:r?them'! Instead I would have expected the bugzilla request to change to 'author:r?them' (i.e., no more 'me' anywhere). If I really wanted to stay as reviewer, I would expect to be able to edit the mozreview reviewer field to 'me, them', in which case both requests would exist in Bugzilla, but both with the same original author, not me as requester.
This just bit me (again) today. The big problem with mozreviews current setup is that when the request is granted the original patch author is not notified properly and hence the changes can slip through the cracks and not get landed. Piling that responsibility on the (incorrectly-set!) reviewer is not reasonable. Can we at least fix the simple case? The complicated cases are hard for even humans to do right, but the simple case is overwhelmingly the most common one.
Flags: needinfo?(mcote)
Yeah I can take a look at this.
Assignee: nobody → mcote
Status: NEW → ASSIGNED
Flags: needinfo?(mcote)
(In reply to Mark Côté [:mcote] from comment #8) > Created attachment 8857288 [details] > mozreview: Reuse flags when publishing review requests (bug 1286740) Heh, overly greedy reviewer-string stripping here.
Comment on attachment 8857288 [details] mozreview: Reuse flags when publishing review requests (bug 1286740) https://reviewboard.mozilla.org/r/129242/#review131812 this approach looks good to me
Attachment #8857288 - Flags: review?(glob)
Attachment #8857288 - Flags: review?(glob)
Nearly good to go; imadueme is going to take it over the finish line.
Assignee: mcote → imadueme
Actually the tests all pass. Let's get this reviewed and landed.
Assignee: imadueme → mcote
Comment on attachment 8857288 [details] mozreview: Reuse flags when publishing review requests (bug 1286740) https://reviewboard.mozilla.org/r/129242/#review138012 lgtm
Attachment #8857288 - Flags: review?(glob) → review+
Comment on attachment 8857288 [details] mozreview: Reuse flags when publishing review requests (bug 1286740) https://reviewboard.mozilla.org/r/129242/#review138140 - Finally fixed my vct testing environment and was able to run the integration tests and python tests. All passed. - Manually tested clearing/changing review flags in my development RB and Bugzilla instances. The attachements where created as expected. - Code style looks good
Attachment #8857288 - Flags: review?(imadueme) → review+
Pushed by mcote@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/5ff43906ca3e mozreview: Reuse r? flags when publishing review requests ; r=glob,r=imadueme
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: