Closed Bug 1064111 Opened 6 years ago Closed 6 years ago

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

Categories

(MozReview Graveyard :: General, defect, P2)

Production

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(17 files, 1 obsolete file)

39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
38 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
39 bytes, text/x-review-board-request
smacleod
: review+
Details
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
Priority: -- → P2
/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
/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
/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
Attachment #8523388 - Flags: review?(smacleod)
/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
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. :/
/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
/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/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?
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. :)
https://reviewboard.mozilla.org/r/663/#review805

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

Cool, thanks. :)
Attachment #8523388 - Flags: review?(smacleod)
Attachment #8523388 - Flags: review?(mstange)
Attachment #8523388 - Flags: review+
/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+
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".
Landed as https://hg.mozilla.org/hgcustom/version-control-tools/rev/37c50bcdcd11
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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 → ---
Attachment #8523388 - Flags: review+ → review?(smacleod)
/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
/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
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
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
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attachment #8523388 - Flags: review?(smacleod)
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+
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.