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

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P2)

Description

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

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.

Comment 1

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

Attachment: MozReview Request: bz://1064111/mconley

Comment 2

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

/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

Comment 3

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

/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

Comment 4

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

/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

Comment 5

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

/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 6

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

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. :/

Comment 7

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

/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 8

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

/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 9

[Steven MacLeod [:smacleod]]

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.

Comment 10

[Steven MacLeod [:smacleod]]

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!

Comment 11

[Gregory Szorc [:gps]]

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.

Comment 12

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

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?

Comment 13

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

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.

Comment 14

[Steven MacLeod [:smacleod]]

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. :)

Comment 15

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

https://reviewboard.mozilla.org/r/663/#review805

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

Cool, thanks. :)

Comment 16

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

/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 17

[Steven MacLeod [:smacleod]]

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.

Comment 18

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

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".

Comment 19

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

Landed as https://hg.mozilla.org/hgcustom/version-control-tools/rev/37c50bcdcd11

Comment 20

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

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.

Comment 21

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

/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

Comment 22

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

/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

Comment 23

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

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.

Comment 24

[Gregory Szorc [:gps]]

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

Comment 25

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

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

Comment 26

[Matthew N. [:MattN]]

I updated the docs in http://hg.mozilla.org/hgcustom/version-control-tools/rev/9472f1335f19

Comment 28

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

Attachment: MozReview Request: Fix erroneous fetch of child review requests.

Comment 29

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

Attachment: MozReview Request: Show the arrow on the current review request.

Comment 30

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

Attachment: MozReview Request: Fix up some documentation

Comment 31

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

Attachment: MozReview Request: Remove giant Reviews | Diffs header.

Comment 32

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

Attachment: MozReview Request: Add (busted) editing and autocomplete popups

Comment 33

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

Attachment: MozReview Request: Make the reviewers list update after it is edited.

Comment 34

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

Attachment: MozReview Request: Bug 1064111 - rbmozui: Move contents of "Commits" tab to the main review request page. r=?

Comment 35

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

Attachment: MozReview Request: Add an epoch to the root review request.

Comment 36

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

Attachment: MozReview Request: Fix up draft / review request id mixups

Comment 37

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

Attachment: MozReview Request: Fix the commits forcing the description out of the main content area.

Comment 38

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

Attachment: MozReview Request: Make the stack display a pre for theoretically better readability

Comment 39

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

Attachment: MozReview Request: Make review request data reappear in Diff Viewer, but send reviewer to #0 when clicking Diff.

Comment 40

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

Attachment: MozReview Request: Make pressing Enter in autocomplete work to properly.

Comment 41

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

Attachment: MozReview Request: Fix setting epochs.

Comment 42

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

Attachment: MozReview Request: Progress - stuff is showing up.

Comment 43

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

Attachment: MozReview Request: Checkpointing work

Comment 44

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

Attachment: MozReview Request: Remove dead code