1205306 - Allow publishing entire commit series from any commit

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect, P1)

Description

[Armen [:armenzg]]

After pushing a second set of changes, I got a message in my original mozreview url saying "This review request is a draft. Be sure to publish when finished."
I did not realize that I needed to visit a new URL to publish the new set of changes.


armenzg: I can't figure out what to do in here: "This review request is a draft. Be sure to publish when finished. "
armenzg: I think I have to "Finish review" and then "Publish review"
jgraham: armenzg: There should be a publish button, but maybe on the actaul review rather than the parent review, or vice-versa
armenzg: jgraham, I've seen that the first time that I pushed the changes live
armenzg: not this second time with my recent changes
armenzg: that is frustrating
armenzg: I have https://reviewboard.mozilla.org/r/19037/ and https://reviewboard.mozilla.org/r/19035/
armenzg: the 2nd one has the button

Comment 1

[Mark Côté [:mcote]]

Sorry for taking so long to get back to you on this.

First, yes, you always have to publish review requests, reviews, and replies when they are created or updated, although review requests can be published at push time, which is the recommended method (it's just easier! :).

Second, if you don't publish a review request from the command line when pushing, at the moment you must publish from the parent review request.  This is why you are seeing the button on 19035 (the parent) but not on 19037 (the child).

There's a good argument for allowing publishing of the whole series on any commit, not just the parent.  We should probably do this at some point.  I'm going to morph this bug.

Btw it looks like you discarded that parent and then repushed, so you ended up with a new parent (20845) but the same child (19037).  I'm not sure if that's what you did as a result of your confusion, but I see you discarded everything, so I guess you started over at some point.  Sorry for the frustration!

Comment 2

[Armen [:armenzg]]

No worries! Thanks for looking into it.

Comment 3

[Mark Côté [:mcote]]

Bumping this because it's extra confusing since we've been deprecating the parent review requests.

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: allow publishing from any commit (bug 1205306) r?smacleod

This modifies the commit approval logic to allow triggering a publish of
all child reviews from any child review.

There's still cleanup to do here, I wanted to get feedback on the approach
before I spend more time on this.

Review commit: https://reviewboard.mozilla.org/r/29735/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29735/

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8704772 [details]
MozReview Request: mozreview: allow publishing from any commit (bug 1205306) r?smacleod

https://reviewboard.mozilla.org/r/29735/#review27311

::: pylib/rbbz/rbbz/extension.py:330
(Diff revision 1)
>          else:
> -            # This publish is not approved by the parent review request.
> +            # Trigger a publish on the parent. The parent will publish this
> +            # child later on.
> +            parent_rr = get_parent_rr(review_request)
> +            parent_rr.publish(user)
>              raise CommitPublishProhibited()

This is actually probably a pretty good way to handle all the publishing stuff, and should probably not break with Review Board upgrades etc, nice idea :D

I do have a few concerns, particularly with some edge cases, and we'll want to have tests which cover all of the possibilities.

A) How does the child still raising a `CommitPublishProhibited` manifest in the UI? I'm assuming the request to publish will end up returning an error message about the publishing not going through and the frontend won't reload or update? We might have to be a little more clever here and do all the publishing verification stuff the parent does, but from this handler, and then kick off the child publishes in order here. So basically don't rely on the publish handler from the parent taking over. We can use the same sort of signal permission mechanism I was using in order to bail out from the parent handler.

B) What happens if the user attempts to publish from a "discard on publish" publish handler? This is unlikely but definitely possible, we'll want to make sure we immediately reject in that case.

C) Same as B but for review requests which have never been published at all (they get created when a number of commits are pushed, but before publishing a new push with less commits overwrites them) - we'll need to reject the publish in this case as well.

I'm sure there are a couple other edge cases, it's probably a good idea to take a look at the ASCII Venn diagram in pushhooks.py for the different states the unpublished review requests can be in.

In summary, I really like this idea, we just need to be careful :)

Comment 7

[Dan Minor [:dminor]]

Not working on this one at the moment.

Comment 8

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.