1034171 - Make publishing review requests mostly only possible from the squashed review request

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

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

Similar to bug 1034161, we want the squashed review request to be the main publishing point, _especially_ for publishes that change diffs.

Publishing a child review request should not be possible if we're changing:
1) Diffs
2) Dependencies

All other changes (like reviewers, groups, etc) are OK.

Comment 1

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

So one way to do this is to make it so that _no_ fields are editable in the web ui for review requests. If a user wants to update the description, for example, they need to update the commit message of their changesets. That's probably for the best anyhow, since subsequent pushes will overwrite anything manually entered into the description field.

I think we should approach this by making it so that _none_ of the review request fields are editable, _except_ through our special UI that we'll use to assign reviewers across all changesets, and allow batch publishing.

Comment 2

[Mark Côté [:mcote]]

Mind if I tack onto this bug the idea that (as I mentioned in bug 1041253) marking requests as submitted should only be possible from squashed requests, and that all dependent requests should be automatically marked as submitted at that point as well?  The aforementioned bug will take care of ensuring that the comment (which, as per standard Bugzilla workflow, should probably include an hgweb link to the pushed commit) will be mirrored over to Bugzilla.  It shouldn't resolve the bug, though, since that process varies across products.

Comment 3

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

Attachment: Review for review ID: bz://1034171

/r/79 - Bug 1034171: [WIP] Disable all editing in the review request viewer.

Comment 4

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

Comment on attachment 8459314 [details]
Review for review ID: bz://1034171

/r/79 - Bug 1034171: Disable all editing in the review request viewer.

Comment 5

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

Comment on attachment 8459314 [details]
Review for review ID: bz://1034171

/r/79 - Bug 1034171: Disable all editing in the review request viewer.

Comment 6

[Mark Côté [:mcote]]

I think the plan was to still allow people to set reviewers (the target people field) manually.  Even without the suggested reviewers from Bugzilla, autocomplete is pretty handy, and not easily available on the command line.  With this patch, it appears that it's impossible to submit a review unless you have a review group set, but even still no r? flags will be set on the bug.

Additionally, the restrictions on edit appear to apply to all review requests.  I was thinking that we would still let other teams use Review Board with the basic functionality (i.e. without the push workflow).  That said, I might be able to be persuaded otherwise if it makes things a lot more complicated, and if we plan to add git functionality to the push workflow before too long.

Comment 7

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

Comment on attachment 8459314 [details]
Review for review ID: bz://1034171

/r/79 - Bug 1034171: Disable all editing in the review request viewer.
/r/80 - Bug 1034171 - Follow-up: Only apply RBMozUI to pushed review requests.

Comment 8

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

Ok, I've got a second patch here that restricts all of the RBMozUI stuff to just review requests that have p2rb set to True in their extra_data (so that's just pushed review requests).

Comment 9

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

Good point about not getting r? flags though... hrm. I'll talk with you tomorrow about solutions for that.

Comment 10

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

Note that from a UI perspective, this is pretty ugly, and there are still a few things to do here:

  1. Make it possible to discard changes en-masse (this will be easier once bug 1034168 lands)
  2. Make it possible to mark review requests as submitted and discarded en-masse
  3. Deal with the problem of setting reviewers in Bugzilla - because this patch restricts setting reviewers to just be on the individual commits, we no longer r? in Bugzilla. Likely we should set the reviewers on the squashed review request to be the union of the reviewers on all individual commits.
  4. This needs to be prettied up like crazy, but I'll do that in some reskinning for down the road.

I'll also point out that I'm not a really savvy Backbone developer - this was really my first time writing anything beyond a simple patch to a Backbone app, so I might be doing horrific things in here. Feel free to point out where I've gone wrong.

Comment 11

[Mark Côté [:mcote]]

Cool, sorry I forgot to actually look at the new Commits view.  But yes, r?ing the union of commit reviewers seems like a good idea.

Also, this is probably a good place to put the URL to the commit head for easy pulling (bug 1039522).

Comment 12

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

I'll have another crack at this tomorrow.

Comment 13

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

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

/r/88 - Bug 1034171: Disable all editing in the review request viewer.
/r/89 - Bug 1034171 - Follow-up: Only apply RBMozUI to pushed review requests.
/r/90 - Bug 1034171 - Follow-up: Set the reviewers on the squashed review request to be the union of the child reviewers.

Comment 14

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

Comment on attachment 8462708 [details]
Review for review ID: bz://1034171/mconley

/r/88 - Bug 1034171: Disable all editing in the review request viewer.
/r/89 - Bug 1034171 - Follow-up: Only apply RBMozUI to pushed review requests.
/r/90 - Bug 1034171 - Follow-up: Set the reviewers on the squashed review request to be the union of the child reviewers.

Comment 15

[Mark Côté [:mcote]]

Not sure if there's something wrong with my local setup, but, although the reviews are being published, the target people field isn't being set and hence there are no r? flags on the Bugzilla attachment.

Comment 16

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

::: pylib/rbmozui/rbmozui/static/js/commits.js
(Diff revision 1)
> +                    location.reload();

Interesting... could you comment out this line and retry? I'd be curious to see what the logging on line 376 says.

Comment 17

[Mark Côté [:mcote]]

::: pylib/rbmozui/rbmozui/static/js/commits.js
(Diff revision 1)
> +                    location.reload();

> Interesting... could you comment out this line and retry? I'd be curious to see what the logging on line 376 says.
Oook nevermind.  Appears to work now... something weird with old review requests lying around, maybe.  The only thing I noticed is that there isn't a clear "loading" indication.  When I tested just now, I actually thought it didn't work, but a few seconds later it refreshed and all was well.  Aside from that, seems great!

Comment 18

[Steven MacLeod [:smacleod]]

To my unexperienced with backbone eyes, this looks fine. I'll admin, my review wasn't *super* in depth, but I didn't see any glaring issues.

::: pylib/rbmozui/rbmozui/views.py
(Diff revision 1)
> +          'squashedID': review_request.id,

Why the change of convention here? why not "squashed_id"?

Comment 19

[Steven MacLeod [:smacleod]]

This change makes it so that the big "Reviews | Diff | Commits" tabs only display on pushed requests, if I'm not mistaken? While the commits tab doesn't make sense to show on non pushed requests, the other tabs still apply. Do you think we should still show them on all review requets?

Comment 20

[Steven MacLeod [:smacleod]]

Ship It!

Comment 21

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

Comment on attachment 8462708 [details]
Review for review ID: bz://1034171/mconley

/r/88 - reviewboard: Bug 1035502 - Copy some p2rb extra data from the draft on publish. r=mconley
/r/89 - Bug 1034171: Disable all editing in the review request viewer.
/r/90 - Bug 1034171 - Follow-up: Only apply RBMozUI to pushed review requests.
/r/98 - Bug 1034171 - Follow-up: Set the reviewers on the squashed review request to be the union of the child reviewers.

Comment 22

[Steven MacLeod [:smacleod]]

Ship It!

Comment 23

[Steven MacLeod [:smacleod]]

Ship It!

Comment 24

[Steven MacLeod [:smacleod]]

To my unexperienced with backbone eyes, this looks fine. I'll admin, my review wasn't super in depth, but I didn't see any glaring issues.

::: pylib/rbmozui/rbmozui/views.py
(Diff revision 2)
> +          'squashedID': review_request.id,

Why the change of convention here? why not "squashed_id"?

Comment 25

[Steven MacLeod [:smacleod]]

Already been reviews and approved to land.

Comment 26

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

Comment on attachment 8462708 [details]
Review for review ID: bz://1034171/mconley

/r/88 - reviewboard: Bug 1035502 - Copy some p2rb extra data from the draft on publish. r=mconley
/r/89 - Bug 1034171: Disable all editing in the review request viewer.
/r/90 - Bug 1034171 - Follow-up: Only apply RBMozUI to pushed review requests.
/r/98 - Bug 1034171 - Follow-up: Set the reviewers on the squashed review request to be the union of the child reviewers.
/r/99 - Bug 1034168: rbbz / reviewboard - When discarding squashed review request drafts should rollback changes on children.

Comment 27

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +        SignalHook(self, pre_delete, on_pre_delete)

I think we should start having single purpose, or flow, handlers that are named based on what they do. We can have multiple handlers on a signal for the different use cases.

So in this case, we'd want something like "handle_draft_pre_delete", and then the checks at the top indicate it's a draft.

Just thinking we don't want to end up where we have these giant multipurpose signal handlers when they could just be separate signals.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +def on_pre_delete(sender, instance, using, **kwargs):

The explanation for why we do all these things is in the hg server push side stuff, but maybe we should add a docstring here with a little explanation.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +    if not review_request_draft:
> +        return

Please document why we only allow this on the squashed.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +        # items that are in unpublished_ids, so we'll remove anything we publish

s/unpublished_ids/unpublished_rids

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> -def generate_child_review_requests(squashed_review_request):
> +def gen_child_rrs(review_request):

Lets add a small docstring that explains what happens when an rid isn't found (we skip it), etc.

Also worth mentioning that it's perfectly legal to call this on the draft.

Comment 28

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +def gen_rrs_by_extra_data_key(request_request, key):

s/request_request/review_request

Comment 29

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +        for child in gen_rrs_by_rrids(unpublished_rids):

s/gen_rrs_by_rrids/gen_rrs_by_rids

Comment 30

[Steven MacLeod [:smacleod]]

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> -                logging.error('Could not reopen or publish child review '
> +                    unpublished_rids.remove(child.id)

The child.id here will be an int, but the list contains strings, we'll need to convert to a string before calling remove or this whole thing will fail.

Comment 31

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

::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> -                logging.error('Could not reopen or publish child review '
> +                    unpublished_rids.remove(child.id)

> The child.id here will be an int, but the list contains strings, we'll need to convert to a string before calling remove or this whole thing will fail.
Do we know why the list contains strings? Why aren't they ints instead?

Comment 32

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

Comment on attachment 8462708 [details]
Review for review ID: bz://1034171/mconley

/r/88 - reviewboard: Bug 1035502 - Copy some p2rb extra data from the draft on publish. r=mconley
/r/89 - Bug 1034171: Disable all editing in the review request viewer.
/r/90 - Bug 1034171 - Follow-up: Only apply RBMozUI to pushed review requests.
/r/98 - Bug 1034171 - Follow-up: Set the reviewers on the squashed review request to be the union of the child reviewers.
/r/99 - Bug 1034168: rbbz / reviewboard - When discarding squashed review request drafts should rollback changes on children.
/r/100 - rbbz/rbmozui: Update tests to pass after changes from bug 1034168 and bug 1035502 land.

Comment 33

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

Comment on attachment 8462708 [details]
Review for review ID: bz://1034171/mconley

/r/88 - reviewboard: Bug 1035502 - Copy some p2rb extra data from the draft on publish. r=mconley
/r/89 - Bug 1034171: Disable all editing in the review request viewer.
/r/90 - Bug 1034171 - Follow-up: Only apply RBMozUI to pushed review requests.
/r/98 - Bug 1034171 - Follow-up: Set the reviewers on the squashed review request to be the union of the child reviewers.
/r/99 - Bug 1034168: rbbz / reviewboard - When discarding squashed review request drafts should rollback changes on children.
/r/100 - rbbz/rbmozui: Update tests to pass after changes from bug 1034168 and bug 1035502 land.

Comment 34

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

Fixed in https://hg.mozilla.org/hgcustom/version-control-tools/rev/89bff437e356

smacleod was very much a co-author of this patch.