1047465 - Properly handle closing review requests as discarded and related operations

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Steven MacLeod [:smacleod]]

We need to figure out what to do in the case where we close a squashed review request as discarded. This could happen in a couple of use cases:
 - WONTFIX Bug
 - I messed up and pushed the wrong thing, the push is invalid
 - I'm not working on it anymore? (maybe same as WONTFIX)
 - This code is the wrong approach to the bug, I want to start fresh
 - etc.

It could be argued that in some of these use cases the previous review requests should be revived and then updated (So that we continue from the old history). In these cases we'd like to resurrect the old review requests and continue where we left off.

In the case where we'd like to start fresh, with a new set of review requests, we're going to get a collision on the reviewid we store in the commit-id field. Since this must be unique, we'd need to clear commit-id field when closing the other set of review requests (OR, detect it's closed when pushing and then clear it and create new requests).

It could be argued that the user could just modify their ircnick configuration so that they're using a different reviewid, but that seems super hacky.

I propose we clear the commit-id field on close, and on re-open attempt to reinstate it (pulling it from extra_data.p2rb.identifier) if there is no active review requests for that identifier. These would be manual operations from the Web UI, so if you wanted to continue with old work you'd need to reopen it, and then push (pushing won't automatically reopen).

Comment 1

[Mark Côté [:mcote]]

Yeah, I agree with your suggested approach.  Probably something we should fix before launch.

Comment 2

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

I _think_ we fixed this in the latest big world-changing landing... we definitely clear the commit ID on the squashed review request on discard.

However, for closing as submitted, and re-opening, we're not doing anything when we perform these on the squashed review request. I have a patch to fix that. With tests! :D

Comment 3

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

Attachment: Review for review ID: bz://1047465/mconley

/r/119 - rbbz: Bug 1047465 - Close and re-open child review requests if squashed review request is closed or re-opened.
/r/120 - rbbz: Bug 1047465 - tests for submitting and discarding squashed review requests.

Comment 4

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> -            child.close(ReviewRequest.DISCARDED,
> +        child.close(ReviewRequest.DISCARDED,
> -                        user=user,
> +                    user=user,
> -                        description=NEVER_USED_DESCRIPTION)
> +                    description=NEVER_USED_DESCRIPTION)

This is closing the children as DISCARDED even when the squashed is closed as submitted.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> -    if (is_review_request_squashed(review_request) and
> -            type == ReviewRequest.DISCARDED):
> -        # At the point of discarding, it's possible that if this review
> +    if not is_review_request_squashed(review_request):
> +        return
> +

I'd prefer if we did one of two things with this signal:

a) Make it into two signal handlers `on_review_request_closed_discarded` and `on_review_request_closed_submitted`. Each of these will handle the `review_request_closed` signal and bail if the conditions don't meet the case they are in. Then the handler is kept simple only having to take care of a single case.

b) Break out the specific case handling functionality into helper functions. In the single signal handler check the bare minimum conditions to decide which case we're in, and then call the correct function to handle it.

I think either of these would move us toward more maintainable code especially if we start adding more signal handlers etc. (Along the same lines of when I wanted us to name the draft deletion handler for what it does).

Comment 5

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> -            child.close(ReviewRequest.DISCARDED,
> +        child.close(ReviewRequest.DISCARDED,
> -                        user=user,
> +                    user=user,
> -                        description=NEVER_USED_DESCRIPTION)
> +                    description=NEVER_USED_DESCRIPTION)

> This is closing the children as DISCARDED even when the squashed is closed as submitted.
nevermind this comment, total brain fart on my part.

Comment 6

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +    review_request.commit = review_request.extra_data['p2rb.identifier']
> +    review_request.save()

If someone has already created a new set of requests under that commit id, this could fail. We need to guard for that and show the user a descriptive error message.

Comment 7

[Steven MacLeod [:smacleod]]

For the most part these tests look great, there is just some funkiness going on with the reopening. I'm really glad you wrote these though because it made that apparent :D

::: hgext/reviewboard/tests/test-review-request-closed-discarded.t
(Diff revision 1)
> +  Draft: 1
> +    Bugs: 123
> +    Commit ID: None
> +    Summary: Review for review ID: bz://123/mynick
> +    Description:
> +      /r/2 - Bug 123 - Foo 1
> +      /r/3 - Bug 123 - Foo 2
> +      
> +      Pull down these commits:
> +      
> +      hg pull review -r 9d24f6cb513e7a5b4e19b684e863304b47dfe4c9
> +      
> +    Extra:
> +      p2rb: True
> +      p2rb.commits: [["bb41178fa30c323500834d0368774ef4ed412d7b", "2"], ["9d24f6cb513e7a5b4e19b684e863304b47dfe4c9", "3"]]
> +      p2rb.discard_on_publish_rids: []
> +      p2rb.identifier: bz://123/mynick
> +      p2rb.is_squashed: True
> +      p2rb.unpublished_rids: []

Hmmm, why does this squashed request, and all of the child requests, have drafts after reopening? I thought the reopen just changes the status and didn't draft anything? We really need to investigate this.

Also, you'll notice that the commit_id is set on the actual review_request object, but not the draft here. When this is published the `None` commit_id from the draft overwrited the published value you re-instated, so things aren't working as expected.

::: hgext/reviewboard/tests/test-review-request-closed-discarded.t
(Diff revision 1)
> +    Commit ID: None

As mentioned above, after the reopen and publish this is set back to `None`

::: hgext/reviewboard/tests/test-review-request-closed-submitted.t
(Diff revision 1)
> +  $ rbmanage ../rbserver reopen $HGPORT1 1
> +
> +Squashed review request with ID 1 should be re-opened...
> +
> +  $ rbmanage ../rbserver dumpreview $HGPORT1 1
> +  Review: 1
> +    Status: pending
> +    Public: True
> +    Bugs: 123
> +    Commit ID: bz://123/mynick
> +    Summary: Review for review ID: bz://123/mynick
> +    Description:
> +      /r/2 - Bug 123 - Foo 1
> +      /r/3 - Bug 123 - Foo 2
> +      
> +      Pull down these commits:
> +      
> +      hg pull review -r 9d24f6cb513e7a5b4e19b684e863304b47dfe4c9
> +      
> +    Extra:
> +      p2rb: True
> +      p2rb.commits: [["bb41178fa30c323500834d0368774ef4ed412d7b", "2"], ["9d24f6cb513e7a5b4e19b684e863304b47dfe4c9", "3"]]
> +      p2rb.discard_on_publish_rids: []
> +      p2rb.identifier: bz://123/mynick
> +      p2rb.is_squashed: True
> +      p2rb.unpublished_rids: []

Ah, so the reopen didn't create drafts on everything. This is strange that reopening a discarded squashed is making drafts, but reopening a submitted isn't.

Comment 8

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

> Hmmm, why does this squashed request, and all of the child requests, have drafts after reopening? I thought the reopen just changes the status and didn't draft anything? We really need to investigate this.
> 
> Also, you'll notice that the commit_id is set on the actual review_request object, but not the draft here. When this is published the `None` commit_id from the draft overwrited the published value you re-instated, so things aren't working as expected.

[Yeah, the creation go drafts when re-opening discarded review requests seems to be by design.](https://github.com/reviewboard/reviewboard/blob/8800048438f0773948788a9a106ba62653ee08f3/reviewboard/reviews/models/review_request.py#L672)

I've added code to the other patch in this push that sets the commit id on the draft if a draft exists. Thanks for noticing that!

> Ah, so the reopen didn't create drafts on everything. This is strange that reopening a discarded squashed is making drafts, but reopening a submitted isn't.

I think this is OK? See my other comment about this being by design.

Comment 9

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

I can't seem to push my updated patches - I'm getting:

pushing to ssh://[e-mail]/hg/reviews/version-control-tools/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 9 changesets with 9 changes to 10 files (+1 heads)
submitting 4 changesets for review
remote: ** unknown exception encountered, please report by visiting
remote: ** http://mercurial.selenic.com/wiki/BugTracker
remote: ** Python 2.6.6 (r266:84292, Nov 21 2013, 10:50:32) [GCC 4.4.7 20120313 (Red Hat 4.4.7-4)]
remote: ** Mercurial Distributed SCM (version 3.0.1)
remote: ** Extensions loaded: rbserver
remote: Traceback (most recent call last):
remote:   File "/usr/bin/hg-ssh", line 86, in <module>
remote:     main()
remote:   File "/usr/bin/hg-ssh", line 71, in main
remote:     dispatch.dispatch(dispatch.request(cmd))
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/dispatch.py", line 69, in dispatch
remote:     ret = _runcatch(req)
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/dispatch.py", line 138, in _runcatch
remote:     return _dispatch(req)
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/dispatch.py", line 819, in _dispatch
remote:     cmdpats, cmdoptions)
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/dispatch.py", line 599, in runcommand
remote:     ret = _runcommand(ui, options, cmd, d)
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/dispatch.py", line 910, in _runcommand
remote:     return checkargs()
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/dispatch.py", line 881, in checkargs
remote:     return cmdfunc()
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/dispatch.py", line 816, in <lambda>
remote:     d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/util.py", line 518, in check
remote:     return func(*args, **kwargs)
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/commands.py", line 5161, in serve
remote:     s.serve_forever()
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/sshserver.py", line 94, in serve_forever
remote:     while self.serve_one():
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/sshserver.py", line 112, in serve_one
remote:     rsp = wireproto.dispatch(self.repo, self, cmd)
remote:   File "/usr/lib64/python2.6/site-packages/mercurial/wireproto.py", line 483, in dispatch
remote:     return func(repo, proto, *args)
remote:   File "/data/version-control-tools/hgext/reviewboard/hgrb/proto.py", line 150, in reviewboard
remote:     cookie=bzcookie)
remote:   File "/data/version-control-tools/hgext/reviewboard/hgrb/proto.py", line 14, in post_reviews
remote:     return pr(*args, **kwargs)
remote:   File "/data/version-control-tools/pylib/reviewboardmods/reviewboardmods/pushhooks.py", line 171, in post_reviews
remote:     squashed_rr.extra_data['p2rb.discard_on_publish_rids']))
remote:   File "/usr/lib/python2.6/site-packages/RBTools-0.6.1-py2.6.egg/rbtools/api/resource.py", line 196, in __getitem__
remote:     raise KeyError
remote: KeyError
abort: unexpected response: empty string


So I think we need to modify the extra_data for the squashed request to have that key. Did you have a script around to make such extra_data manipulations easy, smacleod?

Comment 10

[Steven MacLeod [:smacleod]]

Spoke with mconley about this in IRC.

Just for the record, I usually write these scripts ad hoc, here is an example that messes with extra data: 

> from rbtools.api.client import RBClient
> 
> client = RBClient("https://reviewboard-dev.allizom.org/",
>                   username="[e-mail]",
>                   password="<redacted>")
> 
> 
> root = client.get_root()
> 
> r = root.get_review_request(review_request_id=91)
> r.update(data={
>         "extra_data.p2rb.identifier": "bz://1035502/smacleod",
>     })
> 
> r.get_draft().update(**{
>         "commit_id": "bz://1035502/smacleod",
>         "extra_data.p2rb.identifier": "bz://1035502/smacleod",
>     })
> 
> r = root.get_review_request(review_request_id=92)
> r.update(data={
>         "extra_data.p2rb.identifier": "bz://1035502/smacleod",
>     })
> 
> r.get_draft().update(**{
>         "extra_data.p2rb.identifier": "bz://1035502/smacleod",
>     })

Comment 11

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

Comment on attachment 8474186 [details]
Review for review ID: bz://1047465/mconley

/r/119 - rbbz: Bug 1047465 - Close and re-open child review requests if squashed review request is closed or re-opened.
/r/120 - rbbz: Bug 1047465 - tests for submitting and discarding squashed review requests.

Pull down these commits:

hg pull review -r 5e9ca45e6a9d02847ed7b154e7793ae0cc37255f

Comment 12

[Steven MacLeod [:smacleod]]

Looks good to me. Just a few nitpicks to fix and you have my r+

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +def close_squashed_review_request(user, review_request, type,

Please rename this so it isn't "type". Even though Review Board does, we really shouldn't be using this built-in name as a parameter.

How about "status", that's what this parameter becomes eventually anyways.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +def close_squashed_review_request(user, review_request, type,
> +                                  child_close_description):
> +    """Closes all child review requests for a squashed review request."""

Lets rename this `close_child_review_requests` since it doesn't actually close the squashed review request itself.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +    commit_id = review_request.extra_data['p2rb.identifier']

I'd prefer if we call `commit_id` `reviewid` or `identifier` instead. It's not technically the commit_id until we set it and what it actually represents is what we've been calling the "identifier" or "reviewid".

We're only piggy-backing on the commit_id field because of the ability to search it through the API.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 2)
> +    # If the review request has a draft, we have to set the commit_id there as
> +    # well, otherwise it'll get overwritten on publish.
> +    draft = review_request.get_draft(user)
> +    if draft:
> +        draft.commit = commit_id
> +        draft.save()

I don't like that we have to do this. I'm not really sure *why* Review Board has to create drafts in this case but I'm going to look into it.

We'll have to live with these draft creations for now I guess.

Comment 13

[Steven MacLeod [:smacleod]]

Ship It!

Comment 14

[Steven MacLeod [:smacleod]]

Ship It!

Comment 15

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

Landed patch:

https://hg.mozilla.org/hgcustom/version-control-tools/rev/121749233e0d

with tests:

https://hg.mozilla.org/hgcustom/version-control-tools/rev/6bc1ab4e0cff

Comment 16

[Gregory Szorc [:gps]]

I love the testing improvements you made!