Allow publishing entire commit series from any commit



3 years ago
a year ago


(Reporter: armenzg, Unassigned)


(Blocks: 1 bug)


(Whiteboard: [ux][parents])

User Story

It's not entirely obvious that, if you don't publish from the command line when pushing, you have to publish from the parent review request.  Since we are de-emphasizing the parent, it would make sense for a Publish button to be on all child commits whenever any child (or parent) has changed.


(1 obsolete attachment)

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 and
armenzg: the 2nd one has the button

Comment 1

3 years ago
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!
User Story: (updated)
Priority: -- → P2
Summary: After pushing more changes to MozReview, the previous URL was useless → Allow publishing entire commit series from any commit
No worries! Thanks for looking into it.

Comment 3

3 years ago
Bumping this because it's extra confusing since we've been deprecating the parent review requests.
Priority: P2 → P1
Whiteboard: [ux][parents]


3 years ago
Blocks: 1212358


3 years ago
Duplicate of this bug: 1231663


3 years ago
Assignee: nobody → dminor


3 years ago
Blocks: 1232701

Comment 5

2 years ago
Created attachment 8704772 [details]
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:
See other reviews:
Attachment #8704772 - Flags: review?(smacleod)
Comment on attachment 8704772 [details]
MozReview Request: mozreview: allow publishing from any commit (bug 1205306) r?smacleod

::: pylib/rbbz/rbbz/
(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 for the different states the unpublished review requests can be in.

In summary, I really like this idea, we just need to be careful :)
Attachment #8704772 - Flags: review?(smacleod)


2 years ago
Product: Developer Services → MozReview

Comment 7

2 years ago
Not working on this one at the moment.
Assignee: dminor → nobody


2 years ago
Attachment #8704772 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.