Closed Bug 1135396 Opened 9 years ago Closed 9 years ago

Replace CommitListView with server-side rendered commit series table

Categories

(MozReview Graveyard :: General, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mconley, Assigned: mdoglio)

References

Details

Attachments

(2 files, 3 obsolete files)

We've iterated a bunch on how to display the commits for a pushed review request. We started with a separate tab for the Commits, and recently we moved away from that to a client-side rendering of the list of commits in the review request details area.

There are still some deficiencies with the list view. These include:

1) It's client-side rendered and results in much XHR traffic to build the list and show the reviewers
2) It's injected dynamically, which causes the details box to resize after the list has been loaded, which is jarring. This is particularly bad for review requests with many commits
3) There's no indication of what state a review request is in - whether or not it has been reviewed, has open issues, has been given "ship it's", etc.
4) It requires that the reviewee set the reviewers on each individual commit, which is painful for many commits.

I'm working on an alternative. Here's some UI I've been playing with that would replace the current tree view. It'd be rendered on the server-side:

http://jsbin.com/habedataki/3/edit?html,output

I'm not really thinking about colours just yet, just layout and workflow.

What I like about this is that it will show what reviewers have said about a particular commit. In the mock-up, you can see that on the first commit, glandium has opened 3 issues. On the second, he's opened a single issue, but also given it a ship-it.

One UI problem I haven't figured out yet is how to assign reviewers to individual commits. gps and I discussed this, and we thought it might be useful to be able to set the reviewers across all commits simultaneously - in my mock-up, that's what the "pencil" icon would do. It'd open up a text input not unlike what we currently have, and if any reviewers are set, all child commits would have their reviewers set to match.

Then, the user can tweak individual commits, adding and removing reviewers to individual commits as necessary.

It's an idea that might need more tweaking, and I could really use some UX input on this. But this is a start.
Blake, this kind of approaches what you scrawled down on the whiteboard a few weeks back. What are your thoughts on the reviewer-setting problem? Also, do you know if Markus Jaritz has a Bugzilla account? I can't find him on here.
Flags: needinfo?(bwinton)
Cc'ing some other people who might have some feedback or suggestions.
Cc-ing :maritz.  ;)
Assignee: nobody → mconley
This looks great!

I would really like the ability to edit reviewers on a per-commit basis. But I'm not sure how. Maybe a list of names with an "x" next to names to remove and a "+" that brings up a text box to type new reviewers (kinda like we have now)?

Also, I suspect we're moving to a model where separate commits have separate attachments and review flags in Bugzilla. We probably want a way to communicate and change the review flag state. This especially holds true for reviewers - they want a way to cancel r?.
Thanks for looking into this.

(In reply to Gregory Szorc [:gps] from comment #4)
> I would really like the ability to edit reviewers on a per-commit basis.

+1 on this

> Also, I suspect we're moving to a model where separate commits have separate
> attachments and review flags in Bugzilla. We probably want a way to
> communicate and change the review flag state. This especially holds true for
> reviewers - they want a way to cancel r?.

I thought there was a bug on supporting f? but I don't see it now…

Anyways, it would be good to keep feedback requests in mind too as it's something that is missing from mozreview at the moment. I think that for each reviewer, you should be able to see if they have reviewed the latest revision or not and whether they were flagged for review or feedback (maybe also ui-review). We don't have to solve multiple flag types in this bug but it's probably useful to keep it in mind for the design.
I think it is more likely that we invent a new Bugzilla flag type than try to work the feedback flag into MozReview. The varying interpretations as to what various transitions between the different flags mean is way too complicated for us to cargo cult forward. I want one unambiguous signal/state for each commit. Unfortunately, that may not be achievable when multiple reviewers are involved: we may have to concede to one signal/state per person per commit. But we should be able to represent that as an icon next to the reviewer. So I don't think the UX implications are too high.
I'm not sure whether this is useful feedback, but I think you can probably leave the determination of who should review each patch up to the list of reviewers for the entire review.  (Remember the first Palm Pilot, which solved the problem of handwriting recognition by using people's ability to adapt instead of trying to solve a hard technical problem by force.  ;)

If you totally need to implement that feature, how about having the pencil icon replace all the text fields, like so: http://jsbin.com/bahatotefi/4/edit?output ?
Flags: needinfo?(bwinton)
/r/4709 - rbmozui / mozreview : Move the Commits field from RBMozUI to MozReview, and rename it to Review Request Series. (Bug ???????) r=?
/r/4711 - Add Review / Comments buttons
/r/4713 - Add reviewers lists
/r/4715 - Move common.js to MozReview and point try.js at it.
/r/4717 - [WIP] Make reviewer lists editable.

Pull down these commits:

hg pull review -r 944ae34e7bb5f1233be01831724629b7cadf7a8d
Attached file MozReview Request: bz://1135396/gps (obsolete) —
/r/4745 - rbmozui / mozreview : Move the Commits field from RBMozUI to MozReview, and rename it to Review Request Series. (Bug ???????) r=?
/r/4747 - Add Review / Comments buttons
/r/4749 - Add reviewers lists
/r/4751 - Move common.js to MozReview and point try.js at it.
/r/4753 - [WIP] Make reviewer lists editable.
/r/4755 - mozreview: populate review state column
/r/4757 - mozreview: print link to commit
/r/4759 - mozreview: highlight current review request with alternate background
/r/4761 - mozreview: more styling on commits view

Pull down these commits:

hg pull review -r c2e8bafbe98005eb69e8a30e8d26750738855ba4
Comment on attachment 8573007 [details]
MozReview Request: bz://1135396/gps

/r/4745 - rbmozui / mozreview : Move the Commits field from RBMozUI to MozReview, and rename it to Review Request Series. (Bug ???????) r=?
/r/4747 - Add Review / Comments buttons
/r/4749 - Add reviewers lists
/r/4751 - Move common.js to MozReview and point try.js at it.
/r/4753 - [WIP] Make reviewer lists editable.
/r/4755 - mozreview: populate review state column
/r/4757 - mozreview: print link to commit
/r/4759 - mozreview: highlight current review request with alternate background
/r/4761 - mozreview: more styling on commits view

Pull down these commits:

hg pull review -r c2e8bafbe98005eb69e8a30e8d26750738855ba4
Comment on attachment 8572843 [details]
MozReview Request: bz://1135396/mconley

/r/4709 - rbmozui / mozreview : Move the Commits field from RBMozUI to MozReview, and rename it to Review Request Series. (Bug ???????) r=?
/r/5259 - Make sure MozReview extension shuts down properly.
/r/4711 - Add Review / Comments buttons
/r/4713 - Add reviewers lists
/r/4715 - Move common.js to MozReview and point try.js at it.
/r/4717 - [WIP] Make reviewer lists editable.
/r/5261 - Move great swarths of RBMozUI's commits.js into rrseries.js

Pull down these commits:

hg pull review -r 5d7bc6b0c858b42791959639a7aab21a97b6eec5
mconley, feel free to keep hacking as you are able to, but mdoglio will drive this to completion after autoland-to-try.
Assignee: mconley → mdoglio
Priority: -- → P1
Blocks: 1083870
Depends on: 1087740
Depends on: 1088356
Blocks: 1115584
Blocks: 1123142
Blocks: 1123337
Blocks: 1142978
/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r 0db3bdbd2e69ae91c56b263eea282861c386544d https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8592928 - Flags: review?(smacleod)
I published a rr to get some feedback on a couple of issues. It's not ready for a .review
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r 0db3bdbd2e69ae91c56b263eea282861c386544d https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)
/r/7619 - Mozreview: Fix autocomplete for reviewer list

Pull down these commits:

hg pull -r 8a5251de3777dcd8c6090e8506471af098f6b5b7 https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r bf8a161dcb920d1d17aa91c070eaa14becdfd922 https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r bf8a161dcb920d1d17aa91c070eaa14becdfd922 https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/7075/#review6417

I installed this locally and it looks good! Some visual nits:

1. Using a monospace font for the commit SHA-1 would make things look nicer.
2. I'm not sure if the SHA-1 fragment for the diff link is needed. Using the review ID or even omitting the column entirely (in favor of an icon?) seems acceptable.
3. On my computer, the column for reviewers is very small. The input box is only a few characters wide and difficult to use.
4. After publishing the review requests, the status column gets very wide. Content is 'The review request has not been marked "Ship It!"'. That seems excessive. This throws off rendering of the entire table.
Attachment #8572843 - Attachment is obsolete: true
Attachment #8573007 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/7075/#review6627

I updated the review request with fixes for 1 and 4.
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r dd5e49deb3c70c72570f288f99975df32c2ad251 https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/7077/#review6687

::: pylib/mozreview/mozreview/extension.py:133
(Diff revision 4)
> +        # use a custom method to calculate a review approval state

Start with capital, end with period.

::: pylib/mozreview/mozreview/fields.py:14
(Diff revision 4)
> -from mozreview.utils import is_parent, is_pushed
> +from mozreview.utils import is_parent, is_pushed, gen_child_rrs, get_parent
> -
>  from mozreview.autoland.models import AutolandRequest, AutolandEventLogEntry

Alphabetical order please (for the lines, and the imported names)

::: pylib/mozreview/mozreview/fields.py:63
(Diff revision 4)
> -
> +        current_rr = self.review_request_details

I'd rather just pass `self.review_request_details` to where it's used here. Giving it the name `curent_rr` might give the impression it's always the review request itself, where it might actually be the draft here.

::: pylib/mozreview/mozreview/fields.py:67
(Diff revision 4)
> +        parent_rr = parent_rr.get_draft() or parent_rr

To stick with the convention of `<name>_details` for things that may be a review request or a draft lets call this `parent_details` or `parent_rr_details`.

We also want to be passing the user object to get_draft here, as we only want to use the draft if the user should see it.

::: pylib/mozreview/mozreview/fields.py:70
(Diff revision 4)
> +        children_rr = [child for child in gen_child_rrs(parent_rr, user)

Again, lets call this `child_rrs_details`, `children_details`, or `children_rr_details`.

::: pylib/mozreview/mozreview/fields.py:76
(Diff revision 4)
> -            'current_id': rr.id,
> +            'current_rr': current_rr,

I'd prefer the details naming - `'review_request_details': self.review_request_details`

::: pylib/mozreview/mozreview/fields.py:146
(Diff revision 4)
> -            return self._autoland_problem % ar.last_details
> +            return self._autoland_problem % ar.last_error_msg

Changes from Bug 1158162 slipping in?

::: pylib/mozreview/mozreview/hooks.py:1
(Diff revision 4)
> +from reviewboard.extensions.hooks import ReviewRequestApprovalHook

`from __future__ import unicode_literals`

::: pylib/mozreview/mozreview/hooks.py:6
(Diff revision 4)
> +        """This is mostly to change the wording of the default message
> +for pending reviews"""

The docstring summary line should always fit on a single line.

https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings

::: pylib/mozreview/mozreview/hooks.py:8
(Diff revision 4)
> +        if review_request.shipit_count == 0:
> +            return False, 'Pending review'
> +        elif review_request.issue_open_count > 0:
> +            return False, 'The review request has open issues.'
> +        else:
> +            return True

I'm not sure we should bother changing the message here if we're not actually changing the logic yet. I'd prefer the following though if we are:
```
if prev_approved:
    return True

if prev_failure == 'The review request has not been marked "Ship It!"':
    return prev_approved, 'Pending review'

return prev_approved, prev_failure
```

::: pylib/mozreview/mozreview/utils.py:2
(Diff revision 4)
> +import logging
> +from reviewboard.reviews.models.review_request import ReviewRequest

blank line between these.

::: pylib/mozreview/mozreview/utils.py:43
(Diff revision 4)
> +                                         repository=review_request.repository)

This will make a query to get the repository first, and then one for the review request we're looking for, correct?

something like `repository_id=review_request.repository_id` should work I believe and save us a DB query (I might have the syntax slightly off).

::: pylib/mozreview/mozreview/utils.py:75
(Diff revision 4)
> +                    draft = child.get_draft(user)
> +                    child = draft or child
> +                    yield child

`yield child.get_draft(user) or child`

::: pylib/mozreview/mozreview/autoland/models.py:55
(Diff revision 4)
> +    @property
> +    def last_error_msg(self):
> +        last_evt = self.event_log_entries.last()
> +        return last_evt.error_msg if last_evt else ""

Changes from Bug 1158162 slipping in?

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:26
(Diff revision 4)
> -$(document).ready(function() {
> +$(window).load(function() {

Why the switch to window load?
Attachment #8592928 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/7075/#review6817

::: pylib/mozreview/mozreview/fields.py:78
(Diff revision 4)
> +            'children_rr': children_rr

Nit - add trailing comma

::: pylib/mozreview/mozreview/hooks.py:6
(Diff revision 4)
> +        """This is mostly to change the wording of the default message
> +for pending reviews"""

Nit - This alignment seems off.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:18
(Diff revision 4)
> +@register.filter()

Nit - newline above.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:14
(Diff revision 4)
> +# TODO - mention that reviewer_list was mostly copied from Review Boar

Busted comment? I guess this was probably mine. I'm not entirely sure we need to credit Review Board for such a small code fragment. Maybe we do. Dunno.

::: pylib/mozreview/mozreview/autoland/models.py:55
(Diff revision 4)
> +    @property
> +    def last_error_msg(self):
> +        last_evt = self.event_log_entries.last()
> +        return last_evt.error_msg if last_evt else ""

This last_error_msg stuff seems unrelated... did some other work get mixed in here?

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:26
(Diff revision 4)
> -$(document).ready(function() {
> +$(window).load(function() {

If I understand correctly, we have the common.js run on the load event of the document so that it is guaranteed to run first. Window load event runs next because the load event bubbles up to it.

I think we can probably do something a bit more sensical - have common.js run on document.ready, and then when it's done, have it fire a custom event that the rest of the MozReview scripts can listen for - that way, they know that common.js has initialized.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js
(Diff revision 4)
> -        deferEventSetup: true

Pretty sure we need deferEventSetup so that we can fire `$(reviewerList).inlineEditor("setupEvents");` - otherwise, I believe there are keyboard shorcuts that will not work... I think.

Worth seeing if I'm off my rocker.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:78
(Diff revision 4)
> +      var reviewerListEditors = reviewerList

Nit: The rest of this file uses 2-space indentation, so this block (basically, everything in this conditional) is misaligned.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:266
(Diff revision 4)
> -      // We've changed the layout of the page by fleshing out the commit view,
> +  var updateParentReviewers = function() {

So this is going to have the same problem that we were trying to solve with the epoch - namely, if we change the reviewers in the child commits, but it doesn't result in a change in the population of reviewers on the parent review request, Review Board will consider this an "empty change" on the parent review request, and fail to publish.

I think we should bump the epoch via a ManyToMany signal handler for target_people on ReviewRequests, server-side: if we notice that target_people has changed on a child commit, bump an epoch in the extra_data in a draft on the parent review request while also updating the target_people of it.

If you want to split that out to another bug, that's fine - but until then, we should put the client-side epoch stuff back.
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r 75870657475d8c080ab6280c7459c2e7a51b87cf https://reviewboard-hg.mozilla.org/version-control-tools/
Attachment #8592928 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/7077/#review7235

> This will make a query to get the repository first, and then one for the review request we're looking for, correct?
> 
> something like `repository_id=review_request.repository_id` should work I believe and save us a DB query (I might have the syntax slightly off).

No, the orm will not perform a lookup to get the repository id. The following expressions are in fact identically translated with a single query filtered by the foreign key:
repository=review_request.repository.id
repository=review_request.repository_id
repository=review_request.repository
repository_id=review_request.repository
and so on

> Why the switch to window load?

I think this is a leftover from a previous attempt. I see mconley has open a few issues on this too, I'll followup with him on this.
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r 1b6860cd5fd9a122e8713abb7603d66997395259 https://reviewboard-hg.mozilla.org/version-control-tools/
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r 8f975901cc2d57b7e043760fe959e37b374bfc72 https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/7075/#review7327

> So this is going to have the same problem that we were trying to solve with the epoch - namely, if we change the reviewers in the child commits, but it doesn't result in a change in the population of reviewers on the parent review request, Review Board will consider this an "empty change" on the parent review request, and fail to publish.
> 
> I think we should bump the epoch via a ManyToMany signal handler for target_people on ReviewRequests, server-side: if we notice that target_people has changed on a child commit, bump an epoch in the extra_data in a draft on the parent review request while also updating the target_people of it.
> 
> If you want to split that out to another bug, that's fine - but until then, we should put the client-side epoch stuff back.

I thought the CombinedReviewersField was taking care of this.
No longer blocks: 1115584
https://reviewboard.mozilla.org/r/7075/#review7917

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js
(Diff revision 4)
> -        deferEventSetup: true

If I defer the event setup and I trigger the setupEvents myself the autocomplete stops working. On the other hand I don't see any strange behaviour if I don't pass deferEventSetup. Do you know which shortcut I should check?
https://reviewboard.mozilla.org/r/7075/#review7919

> If I understand correctly, we have the common.js run on the load event of the document so that it is guaranteed to run first. Window load event runs next because the load event bubbles up to it.
> 
> I think we can probably do something a bit more sensical - have common.js run on document.ready, and then when it's done, have it fire a custom event that the rest of the MozReview scripts can listen for - that way, they know that common.js has initialized.

Now common.js fires a mozreview_ready event that commits.js, reviews.js and try.js listen to.
https://reviewboard.mozilla.org/r/7077/#review7855

Please expand your commit message to provide a more indepth explanation of what this commit does and why.

::: pylib/mozreview/mozreview/fields.py:61
(Diff revision 7)
>      def as_html(self):
> -        rr = ensure_review_request(self.review_request_details)
> -
> -        root_rr = get_root(rr)
> +        user = self.request.user
> +        review_request_details = self.review_request_details
> +        # get the parent for the current review request
> +        parent_details = get_parent(
> +            review_request_details.get_review_request())
> +        # use a parent draft when available
> +        parent_details = parent_details.get_draft(user) or parent_details
> +        # in case of a half published review series some children
> +        # could be not visible to some users
> +        children_details = [child for child in gen_child_rrs(parent_details, user)
> +                       if child.is_accessible_by(user)]
>  
>          template = get_template('mozreview/commits.html')
> +
>          return template.render(Context({
> -            'current_id': rr.id,
> -            'root_rr': root_rr,
> +            'review_request_details': review_request_details,
> +            'parent_details': parent_details,
> +            'children_details': children_details,
>          }))

I think a lot of the code in here is self explanatory without the comments. Additionally I don't believe the current comment about the children_details is quite correct.

There are a few little style nits I have as well with spacing / indentation / naming, so it's easier to just call them out together - here is how I'd write this out:

```
    def as_html():
        user = self.request.user
        parent = get_parent(self.review_request_details.get_review_request())
        parent_details = parent.get_draft() or parent

        # If a user can view the parent draft they should also have
        # permission to view every child. We check if the child is
        # accessible anyways in case it has been restricted for other
        # reasons.
        children_details = [
            child for child in gen_child_rrs(parent_details, user)
            if child.is_accessible_by(user)]

        return get_template('mozreview/commits.html').render(Context({
            'review_request_details': self.review_request_details,
            'parent_details': parent_details,
            'children_details': children_details,
        }))
```

::: pylib/mozreview/mozreview/utils.py:21
(Diff revision 7)
> +def get_parent(review_request):

This should be consolidated with functions found in `pylib/mozreview/mozreview/extra_data.py`

::: pylib/mozreview/mozreview/utils.py:53
(Diff revision 7)
> +def gen_child_rrs(review_request, user=None):

This should be consolidated with functions found in `pylib/mozreview/mozreview/extra_data.py`

::: pylib/mozreview/mozreview/utils.py:81
(Diff revision 7)
> +def get_rr_for_id(rid):

This should be consolidated with functions found in `pylib/mozreview/mozreview/extra_data.py`

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:1
(Diff revision 7)
> -ul.current-arrow > li {
> -  label {
> -    font-size: 100% !important;
> -    margin-right: 5px;
> +/*
> + * We need to copy a few definitions from rb because there's a bug
> + * that prevents to import them
> + */

Have you posted to the RB mailing list about our problem with STATIC_ROOT yet?

Also, if we're going to have this comment we really should link to a filed bug, or explain the bug. Wouldn't want to forget why this happened.

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:41
(Diff revision 7)
> +   & td, & th {

Can you put each selector on its own line please.

Also, if you're just doing "& td" I don't believe you actually need the &.

::: pylib/mozreview/mozreview/static/mozreview/js/common.js:4
(Diff revision 7)
> -  // The back-end should have already supplied us with the squashed / root review
> +  // The back-end should have already supplied us with the parent review

Thanks for making the change from root to parent :), we should probably keep moving in that direction.

::: pylib/mozreview/mozreview/static/mozreview/js/common.js:28
(Diff revision 7)
> +  var pageReviewRequest = RB.PageManager.getPage().reviewRequest;
>    var pageEditor = RB.PageManager.getPage().reviewRequestEditor;
> +  var pageView = RB.PageManager.getPage().reviewRequestEditorView;

Should getPage() be broken out into its own `var page` or something?
https://reviewboard.mozilla.org/r/7075/#review7935

> I thought the CombinedReviewersField was taking care of this.

CombinedReviewersField just deals with adding the epoch field itself, we still need to add in something on the server that detects changes in child reviewers and bumps the epoch if needed. As Mike said, we definitely want to fix this or keep the JS - I'm really leaning towards fixing this properly now though since our tests are having to emulate the behaviour hackily.
Attachment #8592928 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/7075/#review7951

> If I defer the event setup and I trigger the setupEvents myself the autocomplete stops working. On the other hand I don't see any strange behaviour if I don't pass deferEventSetup. Do you know which shortcut I should check?

Are you certain that you're calling setupEvents yourself after setting deferEventSetup to true?
Attachment #8592928 - Flags: review?(smacleod)
Comment on attachment 8592928 [details]
MozReview Request: bz://1135396/mdoglio

/r/7077 - mozreview: server-side rendering of the commit list (Bug 1135396)

Pull down this commit:

hg pull -r 013ec53f32ee8af14ab6e2de3b343e8827d1188c https://reviewboard-hg.mozilla.org/version-control-tools/
https://reviewboard.mozilla.org/r/7077/#review8059

> I think this is a leftover from a previous attempt. I see mconley has open a few issues on this too, I'll followup with him on this.

In the last version I listen to an event fired once common.js gets executed, as per mconley's suggestion
https://reviewboard.mozilla.org/r/7077/#review8067

> Have you posted to the RB mailing list about our problem with STATIC_ROOT yet?
> 
> Also, if we're going to have this comment we really should link to a filed bug, or explain the bug. Wouldn't want to forget why this happened.

I did it, this is the thread https://groups.google.com/forum/#!topic/reviewboard-dev/81jTaw4hXkY
mozreview: server-side rendering of the commit list (Bug 1135396)
Attachment #8592928 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/7077/#review8767

I haven't fully finished my review but I'm calling it a night so I'll post what I have so far.

::: pylib/mozreview/mozreview/extra_data.py:18
(Diff revision 9)
>  UNPUBLISHED_RRIDS_KEY = MOZREVIEW_KEY + '.unpublished_rids'
> +REVIEWER_EPOCH_KEY = MOZREVIEW_KEY + '.reviewer_epoch'

Alphabetical order.

::: pylib/mozreview/mozreview/extra_data.py:71
(Diff revision 9)
>          if child:
> +            if user:
> +                if child.is_accessible_by(user):
> +                    yield child.get_draft(user) or child
> +            else:
> -            yield child
> +                yield child

This logic is a little strange and subtle. More verbosely what we're doing is:
```
if not child:
    # We couldn't retrieve the child
    continue # Case A

if user:
    if not child.is_accessible_by(user):
        # The user cannot access the child
        continue # Case B
    else:
        # The user can access the child
        draft = child.get_draft(user)
        
        if draft:
            # The user can also access a draft
            yield draft # Case C
        else:
            # The user cannot access any existing drafts
            yield child # Case D
else:
    # We were not given a user to check against, so
    # only ever yeild the child itself.
    yield child # Case E
```

The docstring doesn't indicate at all that if you provide a user you'll start getting drafts if they exist. It also might make sense to have a way to provide a user to get the permission checking, but not to return drafts even if they exist for the user - I'm unsure on this point (this would mean adding a flag or something so that "Case C" never gets triggered).

Either way, I think we should modify the docstring to be much more specific about how this function behaves. If we want to keep the current behaviour I'd prefer to break the nesting down a bit like so:

```  
if not child
    continue

if user is None:
    yield child

if child.is_accessible_by(user):
    yield child.get_draft(user) or child
````

::: pylib/mozreview/mozreview/extra_data.py:105
(Diff revision 9)
> +    reviewer_list = User.objects.filter(directed_drafts__in=child_rr_ids)

hmm, I'm worried this is going to fail in the case where a review request wasn't updated, but has distinct reviewers. In that case there won't be a draft for the review request and it's reviewers won't be present in this query. Actually, I'm thinking we'll use it's *non draft* review request id in this query, which could pull in reviewers from a completely unrelated draft (I don't believe the pk for drafts has to match review requests).

This logic needs some serious testing and possibly some modifications. We should make sure to test cases where not all review requests are updated.

::: pylib/mozreview/mozreview/extra_data.py:38
(Diff revision 9)
>  def get_parent_rr(review_request):

Mind tossing a docstring on this while you're here?

::: pylib/mozreview/mozreview/extension.py:161
(Diff revision 9)
> +            logging.debug("reviewers list changed on %s" % draft_rr)
> +            logging.debug("kwargs: %s" % kwargs)

I think this is a little *too* verbose to land, even for debug. Lets remove these statements before landing.

::: pylib/mozreview/mozreview/extension.py:163
(Diff revision 9)
> +            parent_rr_draft = parent_rr.get_draft()
> +            if parent_rr_draft:

One of the main reasons for this epoch stuff is to make sure the parent *has* a draft and that it's not empty. So we definitely want to create a draft if it doesn't exist, and update the reviewers.

This is most likely why you're having issues on second pushes to already published review requests.

So, an alternative to all this would be listening for the model creation/modification signal on review request drafts. Whenever a child has a draft created or modified, we check and make sure the parent request also has a draft (if not, create it) and then run the reviewer updating logic.

::: pylib/mozreview/mozreview/extra_data.py:108
(Diff revision 9)
> +    parent_rr_draft.extra_data[REVIEWER_EPOCH_KEY] = int(time.time())

Thinking through all the different reviewer modification cases and how they reflect drafts is giving me a headache - but I definitely think we have some bugs with child drafts ending up empty - still having a draft on the parent when we shouldn't, etc. What we should really be doing is somehow tracking what has actually changed on the children / parent and then discarding drafts if we detect it has become empty.

I don't think we should try and handle all the bugs right now in this request though, that's just bloating the scope. What will make it easier to fix things in a follow up though is instead of an epoch just having a custom field for "reviewer_set" or something, which would be a json feld which maps child review request id to list of reviewer ids for that child request.

This field would then properly get tracked in the changedesc of the parent and we could rely on checking the changedesc to see if a publish actually needs to happen or not.

This means we don't actually need to bump the epoch, we would just always set the parents "reviewer_set" to be the new value.
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
https://reviewboard.mozilla.org/r/7077/#review8799

> This logic is a little strange and subtle. More verbosely what we're doing is:
> ```
> if not child:
>     # We couldn't retrieve the child
>     continue # Case A
> 
> if user:
>     if not child.is_accessible_by(user):
>         # The user cannot access the child
>         continue # Case B
>     else:
>         # The user can access the child
>         draft = child.get_draft(user)
>         
>         if draft:
>             # The user can also access a draft
>             yield draft # Case C
>         else:
>             # The user cannot access any existing drafts
>             yield child # Case D
> else:
>     # We were not given a user to check against, so
>     # only ever yeild the child itself.
>     yield child # Case E
> ```
> 
> The docstring doesn't indicate at all that if you provide a user you'll start getting drafts if they exist. It also might make sense to have a way to provide a user to get the permission checking, but not to return drafts even if they exist for the user - I'm unsure on this point (this would mean adding a flag or something so that "Case C" never gets triggered).
> 
> Either way, I think we should modify the docstring to be much more specific about how this function behaves. If we want to keep the current behaviour I'd prefer to break the nesting down a bit like so:
> 
> ```  
> if not child
>     continue
> 
> if user is None:
>     yield child
> 
> if child.is_accessible_by(user):
>     yield child.get_draft(user) or child
> ````

I kept the current behaviour and used your suggestion for the nesting.

> hmm, I'm worried this is going to fail in the case where a review request wasn't updated, but has distinct reviewers. In that case there won't be a draft for the review request and it's reviewers won't be present in this query. Actually, I'm thinking we'll use it's *non draft* review request id in this query, which could pull in reviewers from a completely unrelated draft (I don't believe the pk for drafts has to match review requests).
> 
> This logic needs some serious testing and possibly some modifications. We should make sure to test cases where not all review requests are updated.

Can we have a chat about this? In one of my previous attempts to update the reviewer list I was creating a draft for the parent when this was not available, but that ended in some strange behaviour of the ui. I was using ReviewRequestDraft.create to do so but for some reason it didn't work.

> Thinking through all the different reviewer modification cases and how they reflect drafts is giving me a headache - but I definitely think we have some bugs with child drafts ending up empty - still having a draft on the parent when we shouldn't, etc. What we should really be doing is somehow tracking what has actually changed on the children / parent and then discarding drafts if we detect it has become empty.
> 
> I don't think we should try and handle all the bugs right now in this request though, that's just bloating the scope. What will make it easier to fix things in a follow up though is instead of an epoch just having a custom field for "reviewer_set" or something, which would be a json feld which maps child review request id to list of reviewer ids for that child request.
> 
> This field would then properly get tracked in the changedesc of the parent and we could rely on checking the changedesc to see if a publish actually needs to happen or not.
> 
> This means we don't actually need to bump the epoch, we would just always set the parents "reviewer_set" to be the new value.

Chatting about this would be great as well
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
Attachment #8592928 - Attachment is obsolete: true
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
Attachment #8613566 - Flags: review?(smacleod)
Attachment #8613566 - Flags: review?(smacleod)
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

https://reviewboard.mozilla.org/r/7077/#review9401

This is awesome, we're really close to landing this - I just have a few small issues, mostly style nits, and then I think we'll probably be ready after one more pass. Great job on this stuff :)

::: pylib/mozreview/mozreview/extension.py:158
(Diff revision 12)
> +        if is_pushed(instance):
> +            rr = instance.get_review_request()
> +            if not is_parent(rr):
> +                parent_rr = get_parent_rr(rr)
> +                parent_rr_draft = parent_rr.get_draft()
> +                if parent_rr_draft is None:
> +                    parent_rr_draft = ReviewRequestDraft.create(parent_rr)
> +                update_parent_rr_reviewers(parent_rr_draft)

```
rr = instance.get_review_request()

if not is_pushed(instance) or is_parent(rr):
    return

...
```

::: pylib/mozreview/mozreview/extra_data.py:112
(Diff revision 12)
> +def update_parent_rr_reviewers(parent_rr_draft):

Might be worth optimizing this to take the child draft we know needs updating and pass it in. Lets just follow up on that though, please add a TODO about it.

::: pylib/mozreview/mozreview/extra_data.py:127
(Diff revision 12)
> +        reviewers = actual_child.target_people.values_list('id', flat=True)\

Breaking lines with `\\` is frowned upon (except in some very specific cases). Generally it is preferred to wrap the code in parenthesis which will allow implicit continuation to the next line.
```
    reviewers = (actual_child.target_people.values_list('id', flat=True).
                 order_by('id'))
```

I think I'd prefer something like this though:
```
    reviewers = actual_child.target_people.values_list(
        'id', flat=True).order_by('id')
```

::: pylib/mozreview/mozreview/extra_data.py:129
(Diff revision 12)
> +        reviewers_map_after[str(child.id)] = list(reviewers)
> +    if reviewers_map_after != reviewers_map_before:

style nit: can you add a blank between these.

::: pylib/mozreview/mozreview/extra_data.py:131
(Diff revision 12)
> +        total_reviewers = sum(reviewers_map_after.values(), [])

Intereseting! I've never seen `sum` used that way before.

Since total_reviewers is only used as a set on the next line, lets just move the set call up to wrap this.

::: pylib/mozreview/mozreview/extra_data.py:134
(Diff revision 12)
> +        if parent_rr_draft.changedesc is None:
> +            parent_rr_draft.changedesc = ChangeDescription.objects.create()

Can you toss blank lines before / after indented blocks like this? I find it can really make the code easier to read giving blocks a bit of space.

::: pylib/mozreview/mozreview/fields.py:40
(Diff revision 12)
> +    def record_change_entry(self, changedesc, old_value, new_value):
> +        super(CombinedReviewersField, self).record_change_entry(
> +            changedesc, old_value, new_value)

If we're just calling the super method we shouldn't need to define this at all?

::: pylib/mozreview/mozreview/fields.py:27
(Diff revision 12)
> -    field_id = "p2rb.reviewer_epoch"
> +    field_id = "p2rb.reviewer_map"

Doing this can break review requests that are in a draft state when we push this change. If something had a draft with only the changed epoch this is going to break (Well, maybe it was anyway, due to the bugs with extra data fields we were seeing?)

TBH, I'm not sure if we should care or not. It's probably extremely unlikely we'll hit this edge case when deploying. We should just make sure we do it at an inactive time though.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:159
(Diff revision 12)
> +

remove this blank line.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:195
(Diff revision 12)
> -      $(reviewerList).inlineEditor("field")
> +

remove blank.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:14
(Diff revision 12)
> +

remove this blank.

::: pylib/mozreview/mozreview/extra_data.py:134
(Diff revision 12)
> +        if parent_rr_draft.changedesc is None:
> +            parent_rr_draft.changedesc = ChangeDescription.objects.create()

If this is all it took to make things work, I'm pretty convinced the bugs were were seeing are RB core bugs - from my reading of the code though I don't see why this would fix it all.

Could you make sure you've run through all the cases we tested over vidyo and that this fixes them?

Particularly, I'd like to see an automated test which modifies the revewiers on a published child (But keeps the overall set of reviewers across all commits the same) and then attempts to publish the parent (which should be allowed to publish, with only our extra data field changed).
https://reviewboard.mozilla.org/r/7077/#review8937

> One of the main reasons for this epoch stuff is to make sure the parent *has* a draft and that it's not empty. So we definitely want to create a draft if it doesn't exist, and update the reviewers.
> 
> This is most likely why you're having issues on second pushes to already published review requests.
> 
> So, an alternative to all this would be listening for the model creation/modification signal on review request drafts. Whenever a child has a draft created or modified, we check and make sure the parent request also has a draft (if not, create it) and then run the reviewer updating logic.

I fixed this using the post_save signal because the extra_data neeeded fro handling the push_based flow are not there at creation time.
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
https://reviewboard.mozilla.org/r/7077/#review9409

> If we're just calling the super method we shouldn't need to define this at all?

yeah, this is a leftover of my experiments :p

> If this is all it took to make things work, I'm pretty convinced the bugs were were seeing are RB core bugs - from my reading of the code though I don't see why this would fix it all.
> 
> Could you make sure you've run through all the cases we tested over vidyo and that this fixes them?
> 
> Particularly, I'd like to see an automated test which modifies the revewiers on a published child (But keeps the overall set of reviewers across all commits the same) and then attempts to publish the parent (which should be allowed to publish, with only our extra data field changed).

The only reason I can think of is that this condition is False when we create the parent draft https://github.com/reviewboard/reviewboard/blob/cd421cf065a82d030469d576376e97a474b5cbcd/reviewboard/reviews/models/review_request_draft.py#L140 which means review_request.public is False. I'm not sure why that should be True though.
I'll write a test as you suggested.
Attachment #8613566 - Flags: review?(smacleod)
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
https://reviewboard.mozilla.org/r/7077/#review9757

> The only reason I can think of is that this condition is False when we create the parent draft https://github.com/reviewboard/reviewboard/blob/cd421cf065a82d030469d576376e97a474b5cbcd/reviewboard/reviews/models/review_request_draft.py#L140 which means review_request.public is False. I'm not sure why that should be True though.
> I'll write a test as you suggested.

I modified AutocompleteTest.test_reviewer_autocomplete to cover this use case
Attachment #8613566 - Flags: review?(smacleod) → review+
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

https://reviewboard.mozilla.org/r/7077/#review9869

Woooo! lets get these nits fixed up and this sucker deployed! :D

::: pylib/mozreview/mozreview/extra_data.py:78
(Diff revision 14)
> -        if child:
> +        if not child:
> +            continue
> +
> +        if user is None:
>              yield child
> +            continue
> +
> +        if child.is_accessible_by(user):
> +            yield child.get_draft(user) or child

Super nit (I know, I told you to write it this way heh.): lets just use elif for the `user is None` and `child.is_accessible_by(user)` so we can get rid of that continue in the middle.

::: pylib/mozreview/mozreview/fields.py:69
(Diff revision 14)
> -        return template.render(Context({
> -            'current_id': rr.id,
> +        logging.info("looking for %s draft" % parent)
> +        logging.info("draft for %s: %s" % (parent.id, parent.get_draft(user)))

I don't think we want these logging statements to land (definitely not at the info level).

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:111
(Diff revision 14)
> +

Get rid of this blank.
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

https://reviewboard.mozilla.org/r/7077/#review9881

Actually, going to need to revoke my ship-it for this. We'll need all the tests updated to take the changes into account before we can land this.
Attachment #8613566 - Flags: review+
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
Attachment #8613566 - Flags: review?(smacleod)
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

https://reviewboard.mozilla.org/r/7077/#review9997

Looks good - will review again when the fixed tests are posted.

::: pylib/mozreview/mozreview/extra_data.py:80
(Diff revision 15)
> +

These blanks aren't needed when it's an `elif` or `else`.

::: pylib/mozreview/mozreview/extra_data.py:83
(Diff revision 15)
>  

Nuke this blank too.
Attachment #8613566 - Flags: review?(smacleod) → review+
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

https://reviewboard.mozilla.org/r/7077/#review9999

Oops, didn't mean to ship-it! (Guess my subconcious really wants this to land! heh)
Attachment #8613566 - Flags: review+
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

mozreview: server-side rendering of the commit list (Bug 1135396)
Attachment #8613566 - Flags: review?(smacleod)
Comment on attachment 8613566 [details]
MozReview Request: mozreview: server-side rendering of the commit list (Bug 1135396)

https://reviewboard.mozilla.org/r/7077/#review10103

Yay! time to land this thing :)
Attachment #8613566 - Flags: review?(smacleod) → review+
Landed on https://hg.mozilla.org/hgcustom/version-control-tools/rev/7f42763f612b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This is deployed to prod \o/
Depends on: 1176972
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: