If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

MozReview
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Production
Dependency tree / graph

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.

Updated

3 years ago
Blocks: 1021929
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 1

3 years ago
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

3 years ago
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8459314 [details]
Review for review ID: bz://1034171

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

Comment 4

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

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

Comment 5

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

/r/79 - Bug 1034171: Disable all editing in the review request viewer.
Attachment #8459314 - Flags: review?(smacleod)
Attachment #8459314 - Flags: review?(mcote)
Attachment #8459314 - Flags: review?(dkl)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
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).
(Assignee)

Comment 9

3 years ago
Good point about not getting r? flags though... hrm. I'll talk with you tomorrow about solutions for that.
(Assignee)

Comment 10

3 years ago
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.
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).
(Assignee)

Comment 12

3 years ago
I'll have another crack at this tomorrow.
(Assignee)

Comment 13

3 years ago
Created 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.
(Assignee)

Comment 14

3 years ago
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.
Attachment #8462708 - Flags: review?(smacleod)
Attachment #8462708 - Flags: review?(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.
(Assignee)

Comment 16

3 years ago
::: 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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(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!

Updated

3 years ago
Flags: needinfo?(mcote)

Updated

3 years ago
Attachment #8459314 - Attachment is obsolete: true
Attachment #8459314 - Flags: review?(smacleod)
Attachment #8459314 - Flags: review?(mcote)
Attachment #8459314 - Flags: review?(dkl)
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"?
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?
Ship It!
Blocks: 1047468
(Assignee)

Comment 21

3 years ago
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.
Ship It!
Ship It!
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"?
Already been reviews and approved to land.
(Assignee)

Comment 26

3 years ago
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.
::: 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.
::: pylib/rbbz/rbbz/extension.py
(Diff revision 1)
> +def gen_rrs_by_extra_data_key(request_request, key):

s/request_request/review_request
::: 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
::: 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.
(Assignee)

Comment 31

3 years ago
::: 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?
(Assignee)

Comment 32

3 years ago
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.
(Assignee)

Comment 33

3 years ago
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.
(Assignee)

Comment 34

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

smacleod was very much a co-author of this patch.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Attachment #8462708 - Flags: review?(mcote)
Attachment #8462708 - Flags: review?(smacleod)
Product: bugzilla.mozilla.org → Developer Services
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.