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

Move contents of "Commits" tab in rbmozui to the main review request page

RESOLVED FIXED

Status

MozReview
General
P2
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Production

Details

MozReview Requests

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

Attachments

(17 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
38 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
39 bytes, text/x-review-board-request
smacleod
: review+
Details | Review
(Assignee)

Description

3 years ago
A whole separate page for the stuff under "commits" just adds another place to go, step to take. In retrospect, not valuable to keep them separate. Now that we've proven that we can build such an interface, let's put it in a better spot, under the "Description".

We can probably forgo the listing of the individual commits in the description, since it'll be redundant.
Product: bugzilla.mozilla.org → Developer Services

Updated

3 years ago
Priority: -- → P2
(Assignee)

Comment 1

3 years ago
Created attachment 8523388 [details]
MozReview Request: bz://1064111/mconley
(Assignee)

Comment 2

3 years ago
/r/663 - Bug 1064111 - [WIP] rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Pull down this commit:

hg pull review -r 944aef58cb635fc1ddb2ab94ac1370895f704df4
(Assignee)

Comment 3

3 years ago
/r/663 - Bug 1064111 - [WIP] rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Pull down this commit:

hg pull review -r 8704d3c5e24895c9d75b37dcfb012a1cc240db57
(Assignee)

Comment 4

3 years ago
/r/663 - Bug 1064111 - [WIP] rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Pull down this commit:

hg pull review -r 869a9973ab45fcf57abf000b4fa6609e02eba76d
(Assignee)

Updated

3 years ago
Attachment #8523388 - Flags: review?(smacleod)
(Assignee)

Comment 5

3 years ago
/r/663 - Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Pull down this commit:

hg pull review -r 33ce3c2f656afcfe62215fddeed4b70a65e9196e
(Assignee)

Comment 6

3 years ago
https://reviewboard.mozilla.org/r/661/#review417

I know this is kind of the opposite of those tiny atomic commits we love so dearly, but this is how it ended up I'm afraid. :/
(Assignee)

Comment 7

3 years ago
/r/663 - Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Pull down this commit:

hg pull review -r 33ce3c2f656afcfe62215fddeed4b70a65e9196e
(Assignee)

Comment 8

3 years ago
/r/663 - Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Pull down this commit:

hg pull review -r 33ce3c2f656afcfe62215fddeed4b70a65e9196e
Attachment #8523388 - Flags: review?(mstange)
https://reviewboard.mozilla.org/r/663/#review651

Man, how tested all the backend code is has really spoiled me!

I'm not going to lie, I think my eyes started to glaze over by the end of my
review, but I'm fine landing this with the assurance that you've at least
manually tested most of it. Especially since we'll be migrating to core
for this stuff in the future.

Thanks for taking care of this and sorry for the delay in review!

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 4)
> +

Why the blank line?

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 4)
> -        field = get_review_request_field('testing_done')
> -        if (field):
> +        testing_done_field = get_review_request_field('testing_done')
> +        if testing_done_field:

nit: blank between

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 4)
> +from rbmozui.fields import CommitsListField

this should come after Review Board imports.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +    can_record_change_entry = property(lambda self: True)

I think I'd rather have this as:

```python
@property
def can_record_change_entry(self):
    return True
```

I realize it's more verbose, but the current way just feels strange. If you feel strongly you can drop this, I'm fine with keeping the lambda though.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 4)
> +def get_child_review_requests(rr):

Hmm, we have this in rbbz if I'm not mistaken. I wonder if we should mark rbbz as a dependency for rbmozui so that we can import things.

We can push that off to another bug or something though.

::: pylib/rbmozui/rbmozui/static/js/commits.js
(Diff revision 4)
> +  // Takes a review request. If that review request has a draft, returns the
> +  // draft - otherwise, it returns the review request. This is used when
> +  // loading the reviewer lists for each child commit.
> +  var requestOrDraft = function(reviewRequest) {
> +    return reviewRequest.draft.id !== undefined ?
> +           reviewRequest.draft :
> +           reviewRequest;
> +  };

It'd be nice if we checked that the draft was owned by the current user too. For example, users with extra permissions are able to modify someone elses review request, or view their draft.

If doing this is complex in anyway though, just don't worry about it. This case should be pretty rare.
Attachment #8523388 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/661/#review739

Just fix the small nits from my review of the commit then feel free to land. Ship-it!
https://reviewboard.mozilla.org/r/661/#review743

There are some docs that will eventually need changed. https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/reviewboard.html

I'm find with that being a follow-up.
(Assignee)

Comment 12

3 years ago
https://reviewboard.mozilla.org/r/663/#review781

> Hmm, we have this in rbbz if I'm not mistaken. I wonder if we should mark rbbz as a dependency for rbmozui so that we can import things.
> 
> We can push that off to another bug or something though.

Yeah, I'll open a new bug to try to de-dupe some of this stuff.

> It'd be nice if we checked that the draft was owned by the current user too. For example, users with extra permissions are able to modify someone elses review request, or view their draft.
> 
> If doing this is complex in anyway though, just don't worry about it. This case should be pretty rare.

Currently I'm going on whether or not the review request is mutable by the user... I'm curious, why do we need the extra constraint that the user must also be the submitter in order to set reviewers / view the draft? Or am I misunderstanding?
(Assignee)

Comment 13

3 years ago
https://reviewboard.mozilla.org/r/663/#review801

> Yeah, I'll open a new bug to try to de-dupe some of this stuff.

Filed bug 1109721.
https://reviewboard.mozilla.org/r/663/#review803

> Currently I'm going on whether or not the review request is mutable by the user... I'm curious, why do we need the extra constraint that the user must also be the submitter in order to set reviewers / view the draft? Or am I misunderstanding?

We don't need the extra constraint, I was wrong. Carry on. :)
(Assignee)

Comment 15

3 years ago
https://reviewboard.mozilla.org/r/663/#review805

> We don't need the extra constraint, I was wrong. Carry on. :)

Cool, thanks. :)
(Assignee)

Updated

3 years ago
Attachment #8523388 - Flags: review?(smacleod)
Attachment #8523388 - Flags: review?(mstange)
Attachment #8523388 - Flags: review+
(Assignee)

Comment 16

3 years ago
/r/663 - Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Pull down this commit:

hg pull review -r 33ce3c2f656afcfe62215fddeed4b70a65e9196e
Comment on attachment 8523388 [details]
MozReview Request: bz://1064111/mconley

I guess when you remove mstange from the reviewers it cleared my r+ and made me r? again. Probably need to be smarter about checking the current flags when updating the attachment.
Attachment #8523388 - Flags: review?(smacleod) → review+
(Assignee)

Comment 18

3 years ago
Sorry - to be clear, I removed mstange's review? flag by myself, since Markus wasn't actually going to review the code. He gave me some feedback in person, which was basically "yes this is better".
(Assignee)

Comment 19

3 years ago
Landed as https://hg.mozilla.org/hgcustom/version-control-tools/rev/37c50bcdcd11
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

3 years ago
smacleod and I saw some pretty awful bugs on dev with this patch, so backed out.

I'll braindump what we need to do in a day or so when I get some cycles.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

3 years ago
Attachment #8523388 - Flags: review+ → review?(smacleod)
(Assignee)

Comment 21

3 years ago
/r/663 - Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?
/r/2347 - Fix erroneous fetch of child review requests.
/r/2349 - Checkpointing work
/r/2351 - Progress - stuff is showing up.
/r/2353 - Show the arrow on the current review request.
/r/2355 - Fix the commits forcing the description out of the main content area.
/r/2357 - Remove dead code

Pull down these commits:

hg pull review -r 0431ef0a780e96313ce72235441b308320f1519a
(Assignee)

Comment 22

3 years ago
/r/663 - Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?
/r/2347 - Fix erroneous fetch of child review requests.
/r/2349 - Checkpointing work
/r/2351 - Progress - stuff is showing up.
/r/2353 - Show the arrow on the current review request.
/r/2355 - Fix the commits forcing the description out of the main content area.
/r/2357 - Remove dead code
/r/2501 - Fix up some documentation
/r/2503 - Remove giant Reviews | Diffs header.
/r/2505 - Add (busted) editing and autocomplete popups
/r/2507 - Make the reviewers list update after it is edited.
/r/2509 - Add an epoch to the root review request.
/r/2511 - Fix up draft / review request id mixups
/r/2513 - Make the stack display a pre for theoretically better readability
/r/2515 - Make review request data reappear in Diff Viewer, but send reviewer to #0 when clicking Diff.
/r/2517 - Make pressing Enter in autocomplete work to properly.
/r/2519 - Fix setting epochs.

Pull down these commits:

hg pull review -r a9c8c0fc995aeae17cb474d2ffe6379f53d39988
(Assignee)

Comment 23

3 years ago
https://reviewboard.mozilla.org/r/661/#review1665

So I'm afraid I kind of screwed myself a bit by tackling this a bit at a time and not being too methodical or organized. What we've essentially got here is a stack of patches that, when squashed together, make a coherent patch. But the individual patches correct one another a bit, and so there's some contradiction along the way. Sorry about that. I considered folding some stuff together, but I worried about hitting too many conflicts.

All in all, the overall squashed change isn't soooo massive or different. A lot of it is just removals, or minor changes, or adding documentation. But some of it is pretty brand new.

Here's what this patch does:

1. Fixes the draft / review request ID problem that plauged the first version
2. Makes it so that the diff viewer page shows the Commits as well. Unfortunately, I couldn't get it to look right without the rest of the review request data at the top that we've been hiding up until now. So what I've done is made the [Diff] links anchor to the first changed file, so that if somebody follows that link, in theory, they should get taken straight to the code. We should try that out on reviewboard-dev to make sure it feels right.
3. I've removed the massive and ridiculous Reviews | Diffs | Commits banner
4. Editing the reviewers on commits can only be done when viewing the root review request
5. Added a lot of documentation

I'm eager to hear what you think. Hopefully we can get this landed before the work week, and we can iterate on it and make it "good enough" so that we can start leaning into other stuff.
https://reviewboard.mozilla.org/r/661/#review1673

pylib/rbmozui/rbmozui/extension.py:5:1: F401 'settings' imported but unused
pylib/rbmozui/rbmozui/extension.py:6:1: F401 'include' imported but unused
pylib/rbmozui/rbmozui/extension.py:6:1: F401 'patterns' imported but unused
pylib/rbmozui/rbmozui/extension.py:6:1: F401 'url' imported but unused
pylib/rbmozui/rbmozui/extension.py:11:1: F401 'URLHook' imported but unused
pylib/rbmozui/rbmozui/fields.py:1:1: F401 'json' imported but unused
(Assignee)

Comment 25

3 years ago
Thanks - addressed this review feedback, as well as feedback from smacleod IRL / on an airplane.

Landed as https://hg.mozilla.org/hgcustom/version-control-tools/rev/4e5f1b647903
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Attachment #8523388 - Flags: review?(smacleod)
Attachment #8523388 - Flags: review+
I updated the docs in http://hg.mozilla.org/hgcustom/version-control-tools/rev/9472f1335f19
(Assignee)

Comment 27

2 years ago
Comment on attachment 8523388 [details]
MozReview Request: bz://1064111/mconley
Attachment #8523388 - Attachment is obsolete: true
Attachment #8618313 - Flags: review+
Attachment #8618314 - Flags: review+
Attachment #8618315 - Flags: review+
Attachment #8618316 - Flags: review+
Attachment #8618317 - Flags: review+
Attachment #8618318 - Flags: review+
Attachment #8618319 - Flags: review+
Attachment #8618320 - Flags: review+
Attachment #8618321 - Flags: review+
Attachment #8618322 - Flags: review+
Attachment #8618323 - Flags: review+
Attachment #8618324 - Flags: review+
Attachment #8618325 - Flags: review+
Attachment #8618326 - Flags: review+
Attachment #8618327 - Flags: review+
Attachment #8618328 - Flags: review+
Attachment #8618329 - Flags: review+
(Assignee)

Comment 28

2 years ago
Created attachment 8618313 [details]
MozReview Request: Fix erroneous fetch of child review requests.
(Assignee)

Comment 29

2 years ago
Created attachment 8618314 [details]
MozReview Request: Show the arrow on the current review request.
(Assignee)

Comment 30

2 years ago
Created attachment 8618315 [details]
MozReview Request: Fix up some documentation
(Assignee)

Comment 31

2 years ago
Created attachment 8618316 [details]
MozReview Request: Remove giant Reviews | Diffs header.
(Assignee)

Comment 32

2 years ago
Created attachment 8618317 [details]
MozReview Request: Add (busted) editing and autocomplete popups
(Assignee)

Comment 33

2 years ago
Created attachment 8618318 [details]
MozReview Request: Make the reviewers list update after it is edited.
(Assignee)

Comment 34

2 years ago
Created attachment 8618319 [details]
MozReview Request: Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?
(Assignee)

Comment 35

2 years ago
Created attachment 8618320 [details]
MozReview Request: Add an epoch to the root review request.
(Assignee)

Comment 36

2 years ago
Created attachment 8618321 [details]
MozReview Request: Fix up draft / review request id mixups
(Assignee)

Comment 37

2 years ago
Created attachment 8618322 [details]
MozReview Request: Fix the commits forcing the description out of the main content area.
(Assignee)

Comment 38

2 years ago
Created attachment 8618323 [details]
MozReview Request: Make the stack display a pre for theoretically better readability
(Assignee)

Comment 39

2 years ago
Created attachment 8618324 [details]
MozReview Request: Make review request data reappear in Diff Viewer, but send reviewer to #0 when clicking Diff.
(Assignee)

Comment 40

2 years ago
Created attachment 8618325 [details]
MozReview Request: Make pressing Enter in autocomplete work to properly.
(Assignee)

Comment 41

2 years ago
Created attachment 8618326 [details]
MozReview Request: Fix setting epochs.
(Assignee)

Comment 42

2 years ago
Created attachment 8618327 [details]
MozReview Request: Progress - stuff is showing up.
(Assignee)

Comment 43

2 years ago
Created attachment 8618328 [details]
MozReview Request: Checkpointing work
(Assignee)

Comment 44

2 years ago
Created attachment 8618329 [details]
MozReview Request: Remove dead code
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.