1138687 - Modifying reviewers appears to be broken with latest update

Status: RESOLVED WORKSFORME

(MozReview Graveyard :: General, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1) On reviewboard-dev, open a push-based review request, and set some reviewers.
2) Publish the review request.
3) Remove one of the reviewers.
4) Attempt to publish

ER:

The reviewer should be removed.

AR:

The reviewer apparently is not removed from the review request.


Something has broken RBMozUI pretty badly here, and we need to sort it out ASAP. Also, clearly we need some regression testing, since this isn't the first time a problem like this has slipped through.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

What's particularly frustrating is that I cannot seem to reproduce this with my local instance of MozReview to test. :/

Comment 2

[Gregory Szorc [:gps]]

Is it possible the dev deployment is busted? Can you reproduce on production?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Yes - it was on production that I first noticed this. Both appear to be similarly busted.

Can anybody confirm this weirdness on reviewboard-dev so I know it's not just me?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Not sure if it's related, but I get the following in my console when I load a review request page now:

17:01:49.547 ReferenceError: RBMozUI is not defined       try.min.b3b12353aba7.js:1:1668

I do not see that error with my local development instance, with or without the Try Autoland UI enabled.

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/1c27abbab8e0

Comment 6

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1138687/gps

/r/4511 - rbmozui: track down busted reviewers interface (bug 1138687)

Pull down this commit:

hg pull review -r 856f1ccc1ab69c78693c74fc21ed4df3991f13b3

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8571660 [details]
MozReview Request: bz://1138687/gps

/r/4511 - rbmozui: track down busted reviewers interface (bug 1138687)

Pull down this commit:

hg pull review -r 856f1ccc1ab69c78693c74fc21ed4df3991f13b3

Comment 8

[Gregory Szorc [:gps]]

Ignore the production review request I've created (for now).

Inspecting the HTTP traffic through the Firefox dev tools, it appears the bug is occurring during the publish phase. The response from the POST and PUT requests adjusting the draft all return a response with the proper target_people array. However, the response for the PUT to mark the draft as published appears to lose data.

I suspect we can test for this without a Selenium test...

Comment 9

[Gregory Szorc [:gps]]

I /think/ the problem is that when we modify the child review request, we're not creating a draft on the parent review request. When we go to publish the parent review request, the server is like "I don't know about a draft" and it refuses to comply.

The error I'm getting in the Selenium test is completely different from what I see when modifying a reviewer.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8571660 [details]
MozReview Request: bz://1138687/gps

/r/4511 - testing: modify reviewers on appropriate object
/r/4513 - reviewboard: add test demonstrating failure to change reviewers (bug 1138687)

Pull down these commits:

hg pull review -r eb1fc0372344b9b5421bb568b709005690b51712

Comment 11

[Gregory Szorc [:gps]]

Now you can start reviewing things.

I locally downgraded to Review Board 2.0.12 and the test starts working. So something changed upstream.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8571660 [details]
MozReview Request: bz://1138687/gps

/r/4511 - testing: modify reviewers on appropriate object
/r/4513 - reviewboard: add test demonstrating failure to change reviewers (bug 1138687)
/r/4515 - testing: add target_people to dumpreview output

Pull down these commits:

hg pull review -r cd864627c816a7bcf93c6085874fcaaf4b78f98d

Comment 13

[Gregory Szorc [:gps]]

I think this is worse than initially indicated.

I just pushed a new series to the review in this bug which added a new commit. I set the reviewer and hit publish and my new commit disappeared from the commits list! I managed to get it back by doing a shift refresh. But the reviewer wasn't set.

I've very tempted to downgrade production back to 2.0.12.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to Gregory Szorc [:gps] from comment #13)
> I think this is worse than initially indicated.
> 
> I just pushed a new series to the review in this bug which added a new
> commit. I set the reviewer and hit publish and my new commit disappeared
> from the commits list! I managed to get it back by doing a shift refresh.
> But the reviewer wasn't set.
> 
> I've very tempted to downgrade production back to 2.0.12.

I don't believe there were any database evolutions with 2.0.13, so a downgrade to 2.0.12 shouldn't be too painful.

I agree that a downgrade is probably our safest bet to get us working properly again. Then we can do some real investigation, and come up with ways of preventing this sort of thing in the future.

Comment 15

[Gregory Szorc [:gps]]

I downgraded prod to 2.0.12.

I think I'll spend a good chunk of time Tuesday writing selenium tests so we can start testing web site interaction (which currently requires a bunch of manual testing).

Comment 16

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4511/#review3959

Ship It!

Comment 17

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4513/#review3963

::: hgext/reviewboard/tests/test-reviewer-modification.t
(Diff revision 1)
> +  $ rbmanage dumpreview $HGPORT1 1

Ya there is supposed to be a draft here. Actually, I think I'm remembering that this draft creation on the parent takes place in js land of our extensions. (We increment an epoch or something in extra data).

mconley, is that what's going wrong here? (if so maybe we should be managing the epoch server side)

Comment 18

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4515/#review3961

::: testing/vcttesting/reviewboard/mach_commands.py
(Diff revision 1)
> +    d['target_people'] = [p.get().username for p in rr.target_people]

I wonder if we should be returning `target_groups` as well.

Comment 19

[Steven MacLeod [:smacleod]]

Comment on attachment 8571660 [details]
MozReview Request: bz://1138687/gps

https://reviewboard.mozilla.org/r/4509/#review3965

Ship It!

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(In reply to Steven MacLeod [:smacleod] from comment #17)
> https://reviewboard.mozilla.org/r/4513/#review3963
> 
> ::: hgext/reviewboard/tests/test-reviewer-modification.t
> (Diff revision 1)
> > +  $ rbmanage dumpreview $HGPORT1 1
> 
> Ya there is supposed to be a draft here. Actually, I think I'm remembering
> that this draft creation on the parent takes place in js land of our
> extensions. (We increment an epoch or something in extra data).
> 
> mconley, is that what's going wrong here? (if so maybe we should be managing
> the epoch server side)

Yes - that's how it works; in commits.js, when we update the reviewers on any child commit, we set the reviewers on the parent commit to be the union of all reviewers across all child commits, *and* we bump a number inside extra_data. That way, if the union doesn't end up changing, Review Board doesn't complain about the parent review request not having any changes.

The test here doesn't capture that. To address this, I'm re-jigging the view into a server-rendered table, and I'm adding a ManyToMany Django signal hook to watch for reviewers being changed on a child commit - that way, I can bump the epoch server side.

Comment 21

[Mark Côté [:mcote]]

We have too many P1s, so I'm spreading out the priorities.  P3 -> P4, P2 -> P3, and some portion of P1s will become P2.

Comment 23

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: add test demonstrating failure to change reviewers (bug 1138687)

Comment 24

[Gregory Szorc [:gps]]

Attachment: MozReview Request: testing: add target_people to dumpreview output

Comment 25

[Gregory Szorc [:gps]]

Attachment: MozReview Request: testing: modify reviewers on appropriate object

Comment 26

[Gregory Szorc [:gps]]

Without checking, I'm going to say this was fixed by the commit list rewrite that landed a few weeks ago.