1161230 - Let a reviewer (or anyone) change the reviewers for a commit on review board

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

I was requested to review some code and wanted to be able to add an additional reviewer on one of the commits.  I know that the submitter can make this change, but it would be great if I could do this as well.

I would like to be able to either completely replace myself as the reviewer for a commit or to add another person in addition to me as a reviewer.

Comment 1

[Mark Côté [:mcote]]

This is on our short-term road map, so bumping priority.

Comment 4

[Dan Minor [:dminor]]

Attachment: MozReview Request: testing: create and push initial revision for review r?mdoglio

testing: create and push initial revision for review r?mdoglio

This creates a test revision and pushes it to the review repository.

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: allow review delegation (bug 1161230) r?smacleod

mozreview: allow review delegation (bug 1161230) r?smacleod

This allows a reviewer to edit the reviewers for a commit so that reviews can
be delegated to someone else.

A new endpoint is added to enable modifying reviewers. This gets around the
check for is_mutable_by when publishing which requires someone editing a
review to either be the original submitter or to have the
'reviews.can_edit_reviewrequest' permission, which grants too much power to be
enabled.

The modification is published as if by the original submitter. The othe
options would be to temporarily grant 'reviews.can_edit_reviewrequest' while
editing the reviewers or to attempt to monkey patch the relevant
'is_mutable_by' checks. Faking the publishing user seems the safest option,
but unfortunately means that the user may see a message indicating that the
review has been editing by themselves. We'll log the actual user who made
the request.

A side-effect of this change is that a draft is no longer created when editing
the reviewers. As the reviewers is the only editable field in a review request
and the editor provides 'ok' and 'cancel' buttons I see much value in
continuing to create a draft and have people press a separate button to
publish it.

Comment 6

[Mauro Doglio [:mdoglio]]

Comment on attachment 8674991 [details]
MozReview Request: testing: create and push initial revision for review r?mdoglio

https://reviewboard.mozilla.org/r/22301/#review19983

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8674992 [details]
MozReview Request: mozreview: allow review delegation (bug 1161230) r?smacleod

https://reviewboard.mozilla.org/r/22303/#review20347

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:23
(Diff revision 1)
> +       We require a separate resource to handle this so we can allow
> +       reviewers as well as the original submitter to modify the reviewers
> +       for a request.

Additional docstring lines shouldn't be indented to match the summary.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:38
(Diff revision 1)
> +                'description': 'The review request for which to trigger a Try '
> +                               'build',

I didn't know we were going to trigger try builds on reviewer change? ;)

Comment 8

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/22303/#review21187

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:61
(Diff revision 1)
> +        draft = ReviewRequestDraft.create(rr)
> +        draft.target_people.clear()

As we just discussed over vidyo, we'll probably want to avoid using the draft / publish mechanism here entirely. There are a number of edge cases which might cause a reviewer to publish the author's draft etc.

As a solution we should just directly modify the target people on the review request itself (If a draft exists too, we may need to copy our modifications to it as well... this might be strange though if the author has already made modifications... uhg). Additionally we could fake the change description an just inject one for the target people rather than having the publish generate it.

The other piece of manually injecting a reviewer update will be modifying the bugzilla flags to match. The code for this currently lives in a publish hook, so we'll need to do the same thing manually here now.

Comment 9

[Dan Minor [:dminor]]

I wonder if we should just disallow changing the reviewer if a draft exists.

Comment 10

[:glob ✱]

status update: i've been working on this for a while; there's some hurdles to overcome.

unfortunately review board's permissions model is very limited - only the submitter has permissions to make changes, and while an authentication extension can override the permissions checking, the level of granularity is extremely broad - "can edit" is at a review-request level, not per-field.

the current patch on this bug works around that buy telling review board that the user making the changes is the submitter; however this breaks the audit and permissions checks in bugzilla as the wrong user will be tagged as making the change.

when updating a review, all current UX interactions involve creating a draft, and then publishing or dismissing the draft.  i think it's important that the same workflow is used when the reviewers are updated, however the current patch immediately updates the review.  the problem here is a review request can only have one draft, because they can only be updated by the submitter.


i've tried and discarded many approaches.  my current thinking..

permissions issue:
- forbid updates if the submitter has a draft open
- create draft in submitter's name
- store the current user in the draft's extra_data
- review board will allow the change
- when updating bugzilla, check for the extra_data and that only the target_people field is being updated, and perform the bugzilla update logged in as the user in extra_data

draft issue:
- create a client-side draft in localstorage
- show the publish draft banner when it exists

Comment 11

[:glob ✱]

Attachment: mozreview: add reviewer delegation (bug 1161230)

Adds the ability for anyone with the required permissions in BMO to update the
target reviewers.  Due to limitations Review Board's limited permissions model
(only the submitter can make changes), this requires a localstorage hosted
draft, updating Review Board as the submitter, and providing a sudo-like
ability for updating Bugzilla.

Review commit: https://reviewboard.mozilla.org/r/38103/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38103/

Comment 12

[Steven MacLeod [:smacleod]]

Comment on attachment 8726601 [details]
mozreview: add reviewer delegation (bug 1161230)

https://reviewboard.mozilla.org/r/38103/#review36059

This looks really good for the most part. I'd feel a lot more comfortable if you could get :mdog to check out the javascript changes as well, though (It'd be great to get a screenshot or two up as well).

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:45
(Diff revision 1)
> +    @webapi_response_errors(DOES_NOT_EXIST, INVALID_FORM_DATA,
> +                            REVIEW_UPDATE_ERROR, NOT_PARENT)

you're missing the login required and permission denied stuff here

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:61
(Diff revision 1)
> +            parent_rr = ReviewRequest.objects.get(pk=parent_request_id)

You need to be checking that the requesting user has access permission to all of the review requests you're touching here. We could have private review requests in the future and this resource would allow any user to update the reviewers on them.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:75
(Diff revision 1)
> +                    users.append(User.objects.get(username=username))

So my immediate reaction was you needed to do the user querying again here to catch any mirroring updates, but maybe we don't since you'll always do the verification first?

I'd prefer we do the query stuff, but I could be convinced otherwise.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:127
(Diff revision 1)
> +                finally:
> +                    self.clear_publish_as(parent_rr)

The transaction *should* take care of this... but I'm fine leaving it to be extra safe.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:137
(Diff revision 1)
> +    def set_publish_as(self, rr, user):
> +        commit_data = fetch_commit_data(rr)
> +        commit_data.draft_extra_data.update({
> +            PUBLISH_AS_KEY: user.id
> +        })
> +        commit_data.save(update_fields=['draft_extra_data'])
> +
> +    def clear_publish_as(self, rr):
> +        commit_data = fetch_commit_data(rr)
> +        if PUBLISH_AS_KEY in commit_data.draft_extra_data:
> +            del commit_data.draft_extra_data[PUBLISH_AS_KEY]
> +            commit_data.save(update_fields=['draft_extra_data'])

I feel like these should be in extra_data.py with all the other helper methods dealing with that data.

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:14
(Diff revision 1)
> +    """Resource to check the validity of the provided reviewer names.
> +    """

trailing `"""` on first line.

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:21
(Diff revision 1)
> +    @webapi_response_errors(INVALID_FORM_DATA,)

your resource should also specify some of the login ones, `LOGIN_REQUIRED` or something...

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:36
(Diff revision 1)
> +                User.objects.get(username=reviewer)

You need to emulate `reviewboard.webapi.resources.user:UserResource` here and call out to our auth backend's `query_users()` function or you won't recognize bugzilla accounts which haven't been mirrored yet.

I believe you might also miss the case where a user has changed their ircnick, which will only be mirrored on re-login, or when someone performs a user search (like with normal reviewer additions).

This are kind of edge cases but we handle them everywhere else, so it would be nice to stay consistent with regards to user mirroring here.

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:154
(Diff revision 1)
> +// don't show gravator for meta changes.
> +// rb is hardcoded to always use the submitter, but we allow non-submitters
> +// to update the target people.
> +
> +#reviews {
> +  .changedesc {
> +    .box-status {
> +      display: none;
> +    }
> +    .box-container {
> +      padding-left: 0;
> +    }
> +    .box::before, .box::after {
> +      display: none;
> +    }
> +  }
> +}

I like this change so much :), I wish upstream was like this.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:27
(Diff revision 1)
> -  if (!MozReview.isParent) {
> +  if (!RB.UserSession.instance.get("username")) {
> -    // At this time, there's no need to set up the editors for the reviewers if
> -    // we're not looking at the parent review request.
>      return;
>    }

is this enabling reviewer modification on any review requests, not just the parent? This is going to be problematic with the normal case...

If the author changes the reviewers on the child they won't be able to publish it there.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:163
(Diff revision 1)
> +    $("<div/>")
> +      .attr("id", "local-draft-banner")
> +      .addClass("banner")
> +      .addClass("box-inner")
> +      .append(

This seems really strange to me (the chained building of the html here) - doesn't RB have some builtin templating view thing you can use here instead of this?

It does kind of feel like this whole draft banner / local draft state would be better managed with a backbone model / view thing. Is there a reason you didn't go for that?

::: testing/vcttesting/reviewboard/mach_commands.py:799
(Diff revision 1)
> +                           '/modify-reviewers')

trailing slash - review board will return a redirect here to the trailing slash url, which will be auto followed, but the canonical urls have the trailing slash in RB.

::: testing/vcttesting/reviewboard/mach_commands.py:816
(Diff revision 1)
> +                           '/verify-reviewers')

trailing slash

Comment 13

[:glob ✱]

https://reviewboard.mozilla.org/r/38103/#review36059

> is this enabling reviewer modification on any review requests, not just the parent? This is going to be problematic with the normal case...
> 
> If the author changes the reviewers on the child they won't be able to publish it there.

yes, this allows for reviewer modification on any review request.  you can publish from the children too, because we don't use rb's publishing methods.

> This seems really strange to me (the chained building of the html here) - doesn't RB have some builtin templating view thing you can use here instead of this?
> 
> It does kind of feel like this whole draft banner / local draft state would be better managed with a backbone model / view thing. Is there a reason you didn't go for that?

i started with that, but it's dramatically more complicated for just a handful of elements with static content .. it would mostly be a large static string with the html wrapped in a needless template.

if you prefer i could do that here instead - parse a html string instead of building the elements directly (my default 'mode' is to skip the parsing step, which is why you see it done this way).

Comment 14

[:glob ✱]

https://reviewboard.mozilla.org/r/38103/#review36059

> So my immediate reaction was you needed to do the user querying again here to catch any mirroring updates, but maybe we don't since you'll always do the verification first?
> 
> I'd prefer we do the query stuff, but I could be convinced otherwise.

yup - i'll mirror the query logic from verify_reviewer here (or extract it out into a util class, or something).

Comment 15

[:glob ✱]

https://reviewboard.mozilla.org/r/38103/#review36059

> You need to emulate `reviewboard.webapi.resources.user:UserResource` here and call out to our auth backend's `query_users()` function or you won't recognize bugzilla accounts which haven't been mirrored yet.
> 
> I believe you might also miss the case where a user has changed their ircnick, which will only be mirrored on re-login, or when someone performs a user search (like with normal reviewer additions).
> 
> This are kind of edge cases but we handle them everywhere else, so it would be nice to stay consistent with regards to user mirroring here.

notes from our discussion: query_users() is close, but we don't want to perform user matching, nor logout when there aren't any matches.  i'll add a get_user() method to bugzilla/models.py that either returns the user object or throws User.DoesNotExist

> yes, this allows for reviewer modification on any review request.  you can publish from the children too, because we don't use rb's publishing methods.

notes from our discussion: we need to handle updates made by the submitter differently from non-submitter changes.  target-people changes shouldn't be blocked if a draft exists, and updating the reviewers should create/update a real review board draft.

Comment 16

[:glob ✱]

Comment on attachment 8726601 [details]
mozreview: add reviewer delegation (bug 1161230)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38103/diff/1-2/

Comment 17

[Steven MacLeod [:smacleod]]

Comment on attachment 8726601 [details]
mozreview: add reviewer delegation (bug 1161230)

https://reviewboard.mozilla.org/r/38103/#review41531

Ah, sorry to bomb you with so many issues - most of them should be nits and pretty easy to handle (hopefully).

::: pylib/mozreview/mozreview/errors.py:86
(Diff revision 2)
>  AUTOLAND_REVIEW_NOT_APPROVED = WebAPIError(
>      1008,
>      "Unable to continue as the review has not been approved.",
>      http_status=405)  # 405 Method Not Allowed
> +
> +REVIEW_UPDATE_ERROR = WebAPIError(

This uses the term `REVIEW` but has nothing to do with reviews themselves, only review requests (I realize the `AUTOLAND_REVIEW_NOT_APPROVED` error above has the same problem, but I'd argue that's also a mistake).

It might make more sense to call this something like `REVIEWER_UPDATE_ERROR`, or less preferably, `REVIEW_REQUEST_UPDATE_ERROR`.

::: pylib/mozreview/mozreview/errors.py:89
(Diff revision 2)
>      http_status=405)  # 405 Method Not Allowed
> +
> +REVIEW_UPDATE_ERROR = WebAPIError(
> +    1009,
> +    "Failed to update the review request.",
> +    http_status=405)  # 405 Method Not Allowed

We're definitely abusing this error code for other types of errors as part of this change (e.g. when it should have been a 500 or something).

We should probably either use a more general error code here, or preferably, update the sites using this error to use a built-in, or something more appropriate.

::: pylib/mozreview/mozreview/extension.py:77
(Diff revision 2)
> +from mozreview.resources.inflate_draft import (
> +    inflate_draft_resource,
> +)

FYI: This is the import style (4 space indented multi-line) that I mentioned in the other comments.

::: pylib/mozreview/mozreview/extra_data.py:210
(Diff revision 2)
>              parent_rr_draft.changedesc = ChangeDescription.objects.create()
>  
>          parent_rr_draft.save()
> +
> +
> +def set_publish_as(parent_rr, user):

All of the other functions relying on `commit_data` allow a `commit_data` keyword argument so that if you've already fetched the data in the caller you can avoid the repeated lookups - please add that here.

::: pylib/mozreview/mozreview/extra_data.py:223
(Diff revision 2)
> +        PUBLISH_AS_KEY: user.id
> +    })
> +    commit_data.save(update_fields=['draft_extra_data'])
> +
> +
> +def clear_publish_as(parent_rr):

All of the other functions relying on `commit_data` allow a `commit_data` keyword argument so that if you've already fetched the data in the caller you can avoid the repeated lookups - please add that here.

::: pylib/mozreview/mozreview/resources/inflate_draft.py:6
(Diff revision 2)
> +from djblets.webapi.decorators import (webapi_login_required,
> +                                       webapi_request_fields,
> +                                       webapi_response_errors)
> +from djblets.webapi.errors import (DOES_NOT_EXIST, INVALID_FORM_DATA,
> +                                   NOT_LOGGED_IN, PERMISSION_DENIED)
> +from reviewboard.reviews.models import (ReviewRequest, ReviewRequestDraft)
> +from reviewboard.site.urlresolvers import local_site_reverse
> +from reviewboard.webapi.resources import WebAPIResource
> +
> +from mozreview.errors import (NOT_PARENT, REVIEW_UPDATE_ERROR)
> +from mozreview.extra_data import (is_parent, gen_child_rrs)

Please switch these imports to the multiline 4 space intendation type we've started using elsewhere.

::: pylib/mozreview/mozreview/resources/inflate_draft.py:19
(Diff revision 2)
> +class InflateDraftResource(WebAPIResource):
> +    """Inflate a parent review request by creating drafts on all children"""

I'm not really sold on the "inflate" term used, I didn't find it intuitive what the purpose was without looking at this docstring. Maybe something like `EnsureDraftsResource`. Either way, expanding on this docstring would be good as well.

I'd also really like a reason documented here for why this is needed at all. I haven't gone through the entire change, but bouncing around to where this resource is used it wasn't obvioius to me why we were doing it.

::: pylib/mozreview/mozreview/resources/inflate_draft.py:38
(Diff revision 2)
> +        try:
> +            parent_rr = ReviewRequest.objects.get(pk=parent_request_id)
> +        except ReviewRequest.DoesNotExist:
> +            return DOES_NOT_EXIST
> +        if not is_parent(parent_rr):
> +            return NOT_PARENT
> +        if not (parent_rr.is_accessible_by(request.user)
> +                or parent_rr.is_mutable_by(request.user)):
> +            return PERMISSION_DENIED

please follow indented blocks with a blank line if they are unrelated:
```
if not ...:
    pass

if not ...:
    pass
````
vs
```
if not ...:
    pass
else if not ...:
    pass
```

::: pylib/mozreview/mozreview/resources/inflate_draft.py:42
(Diff revision 2)
> +        if not is_parent(parent_rr):
> +            return NOT_PARENT
> +        if not (parent_rr.is_accessible_by(request.user)
> +                or parent_rr.is_mutable_by(request.user)):
> +            return PERMISSION_DENIED

These two cases should be swapped (we're leaking parent status of private review requests). Also, if they don't have access permission, should we instead be returning DOES_NOT_EXIST for that case? Can you check what RB core does here.

::: pylib/mozreview/mozreview/resources/inflate_draft.py:51
(Diff revision 2)
> +            return PERMISSION_DENIED
> +
> +        try:
> +            with transaction.atomic():
> +                for child_rr in gen_child_rrs(parent_rr):
> +                    if not bool(child_rr.get_draft()):

It's not generally pythonic to use bool like this, `None` which is returned when there is no draft is a Falsey value, so it should be fine to just check `if not child_rr.get_draft()`, or if you need a distinction between `None` and some other Falsey value, `if child_rr.get_draft() is None`

::: pylib/mozreview/mozreview/resources/inflate_draft.py:53
(Diff revision 2)
> +        try:
> +            with transaction.atomic():
> +                for child_rr in gen_child_rrs(parent_rr):
> +                    if not bool(child_rr.get_draft()):
> +                        ReviewRequestDraft.create(child_rr)
> +                if not bool(parent_rr.get_draft()):

See other comment about unpythonic `bool` use

::: pylib/mozreview/mozreview/resources/inflate_draft.py:56
(Diff revision 2)
> +        except Exception as e:
> +                logging.error("failed to inflate %s: %s"
> +                              % (parent_rr.id, str(e)))
> +                return REVIEW_UPDATE_ERROR.with_message(str(e))

Can we narrow this exception handler down at all? Do we know what's going to possibly cause us to hit this, and if so is it possible to write a test to excercise this error path?

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:9
(Diff revision 2)
> +from djblets.webapi.decorators import (webapi_login_required,
> +                                       webapi_request_fields,
> +                                       webapi_response_errors)
> +from djblets.webapi.errors import (DOES_NOT_EXIST, INVALID_FORM_DATA,
> +                                   NOT_LOGGED_IN, PERMISSION_DENIED)
> +from reviewboard.reviews.errors import PublishError
> +from reviewboard.reviews.models import (ReviewRequest, ReviewRequestDraft)
> +from reviewboard.site.urlresolvers import local_site_reverse
> +from reviewboard.webapi.resources import WebAPIResource
> +
> +from mozreview.bugzilla.client import Bugzilla
> +from mozreview.errors import (NOT_PARENT, REVIEW_UPDATE_ERROR)
> +from mozreview.extra_data import (is_parent,
> +                                  gen_child_rrs,
> +                                  set_publish_as,
> +                                  clear_publish_as,
> +                                  update_parent_rr_reviewers)
> +from mozreview.models import get_bugzilla_api_key

Please switch these imports to the multiline 4 space intendation type we've started using elsewhere.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:64
(Diff revision 2)
> +        try:
> +            parent_rr = ReviewRequest.objects.get(pk=parent_request_id)
> +        except ReviewRequest.DoesNotExist:
> +            return DOES_NOT_EXIST
> +        if not is_parent(parent_rr):
> +            return NOT_PARENT
> +        if not (parent_rr.is_accessible_by(request.user)
> +                or parent_rr.is_mutable_by(request.user)):
> +            return PERMISSION_DENIED

blanks between unrelated indented blocks

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:68
(Diff revision 2)
> +        if not is_parent(parent_rr):
> +            return NOT_PARENT
> +        if not (parent_rr.is_accessible_by(request.user)
> +                or parent_rr.is_mutable_by(request.user)):
> +            return PERMISSION_DENIED

These two cases should be swapped (we're leaking parent status of private review requests). Also, if they don't have access permission, should we instead be returning DOES_NOT_EXIST for that case? Can you check what RB core does here.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:74
(Diff revision 2)
> +            return NOT_PARENT
> +        if not (parent_rr.is_accessible_by(request.user)
> +                or parent_rr.is_mutable_by(request.user)):
> +            return PERMISSION_DENIED
> +
> +        # validate and expand the new reviewer list

Comments like this should start with a capital and end with a period.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:86
(Diff revision 2)
> +            child_reviewers[child_rrid] = users
> +        if invalid_users:

blank line

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:89
(Diff revision 2)
> +                return INVALID_FORM_DATA.with_message(
> +                    "The reviewer '%s' was not found" % invalid_users[0]
> +                )

When using invalid form data you want this form:
```
return INVALID_FORM_DATA, {
    'fields': {
        '<field-name>': ['<error-str>'],
    }
}
```

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:93
(Diff revision 2)
> +                return INVALID_FORM_DATA.with_message(
> +                    "The reviewers '%s' were not found"
> +                    % "', '".join(invalid_users)
> +                )

When using invalid form data you want this form:
```
return INVALID_FORM_DATA, {
    'fields': {
        '<field-name>': ['<error-str>'],
    }
}
```

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:98
(Diff revision 2)
> +                return INVALID_FORM_DATA.with_message(
> +                    "The reviewers '%s' were not found"
> +                    % "', '".join(invalid_users)
> +                )
> +
> +        # review board only supports the submitter updating a review

capitalize Review Board

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:99
(Diff revision 2)
> +                    "The reviewers '%s' were not found"
> +                    % "', '".join(invalid_users)
> +                )
> +
> +        # review board only supports the submitter updating a review
> +        # request.  in order for this to work, we publish these changes

Capitalize beginning of sentences.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:100
(Diff revision 2)
> +                    % "', '".join(invalid_users)
> +                )
> +
> +        # review board only supports the submitter updating a review
> +        # request.  in order for this to work, we publish these changes
> +        # in review board under the review submitter's account, and

Review Board

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:103
(Diff revision 2)
> +        # review board only supports the submitter updating a review
> +        # request.  in order for this to work, we publish these changes
> +        # in review board under the review submitter's account, and
> +        # set an extra_data field which instructs our bugzilla
> +        # connector to use this request's user when adjusting flags.
> +

Throw a blank comment line here to connec thte paragraphs.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:104
(Diff revision 2)
> +        # request.  in order for this to work, we publish these changes
> +        # in review board under the review submitter's account, and
> +        # set an extra_data field which instructs our bugzilla
> +        # connector to use this request's user when adjusting flags.
> +
> +        # updating the review request requires creating a draft and

Capital

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:112
(Diff revision 2)
> +
> +        try:
> +            with transaction.atomic():
> +
> +                # check for existing drafts
> +                has_draft = bool(parent_rr.get_draft())

`has_draft = parent_rr.get_draft() is not None`

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:119
(Diff revision 2)
> +                    for child_rr in gen_child_rrs(parent_rr):
> +                        has_draft = bool(child_rr.get_draft())
> +                        if has_draft:
> +                            break
> +                if has_draft:
> +                    return REVIEW_UPDATE_ERROR.with_message(

`405 Method Not Allowed` does seem apropriate here.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:121
(Diff revision 2)
> +                        if has_draft:
> +                            break
> +                if has_draft:
> +                    return REVIEW_UPDATE_ERROR.with_message(
> +                        "Unable to update reviewers as the review request has "
> +                        "pending changes (a draft exists)")

I think I'd prefer something like "(The author has a draft)", so we don't confuse the delegator thinking they have a draft themselves.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:123
(Diff revision 2)
> +                if has_draft:
> +                    return REVIEW_UPDATE_ERROR.with_message(
> +                        "Unable to update reviewers as the review request has "
> +                        "pending changes (a draft exists)")
> +
> +                # update reviewers

Scrap this comment.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:131
(Diff revision 2)
> +                                    % child_rr.id)
> +                            draft = ReviewRequestDraft.create(child_rr)

blank line

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:135
(Diff revision 2)
> +                                draft.target_people.add(user)
> +                    set_publish_as(parent_rr, request.user)

blank line

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:146
(Diff revision 2)
> +                    clear_publish_as(parent_rr)
> +
> +        except PublishError as e:
> +                logging.error("failed to update reviewers on %s: %s"
> +                              % (parent_rr.id, str(e)))
> +                return REVIEW_UPDATE_ERROR.with_message(str(e))

This doesn't feel like it should be a 405.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:70
(Diff revision 2)
> +    @webapi_request_fields(
> +        optional={
> +            'includeDrafts': {
> +                'type': bool,
> +                'description': 'Include drafts',
> +            }
> +        }
> +    )

It seems strange that we're making this an option, we should probably just add it on to every request (or are you worried about performance?)

::: pylib/mozreview/mozreview/resources/review_request_summary.py:112
(Diff revision 2)
>          # However, the Bugzilla extension uses the existing, nonstandard
>          # return value, so we have to wait until it is fetching review
>          # requests by bug before fixing this.
>  
> -        return 200, self._summarize_families(request, families)[0]
> +        if 'includeDrafts' in request.GET:
> +            incDrafts = bool(request.GET.get('includeDrafts'))

unpythonic bool.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:242
(Diff revision 2)
> +        When 'includeDrafts' is true, a 'has_draft' boolean key is returned
> +        indicating if a draft exists for that review request.

ya, the one off field like this is strange to me, lets just tie it into the example and always include it.

::: pylib/mozreview/mozreview/resources/review_request_summary.py:274
(Diff revision 2)
>  
>          if commit:
>              d['commit'] = commit
>  
> +        if includeDrafts:
> +            d['has_draft'] = bool(review_request.get_draft())

`d['has_draft'] = review_request.get_draft() is not None`

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:3
(Diff revision 2)
> +from django.contrib.auth.models import User
> +from django.utils import six
> +from djblets.webapi.decorators import (webapi_login_required,
> +                                       webapi_request_fields,
> +                                       webapi_response_errors)
> +from djblets.webapi.errors import (INVALID_FORM_DATA, NOT_LOGGED_IN)
> +from mozreview.bugzilla.client import Bugzilla
> +from mozreview.models import get_bugzilla_api_key
> +from reviewboard.site.urlresolvers import local_site_reverse
> +from reviewboard.webapi.resources import WebAPIResource

import style

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:15
(Diff revision 2)
> +from mozreview.models import get_bugzilla_api_key
> +from reviewboard.site.urlresolvers import local_site_reverse
> +from reviewboard.webapi.resources import WebAPIResource
> +
> +
> +class VerifyReviewerResource(WebAPIResource):

I wonder if this should verify the users are accepting review flags too? is that even possible? hmmm, just a thought.

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:16
(Diff revision 2)
> +from reviewboard.site.urlresolvers import local_site_reverse
> +from reviewboard.webapi.resources import WebAPIResource
> +
> +
> +class VerifyReviewerResource(WebAPIResource):
> +    """Resource to check the validity of the provided reviewer names."""

"Resource to check the validity of provided reviewer names."

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:33
(Diff revision 2)
> +        new_reviewers = filter(None,
> +                               map(lambda u: u.strip(), reviewers.split(',')))

list comprehensions are the preferred way of handling stuff like this. The following would be equivalent (but does perform the strip twice):
`new_reviewers = [u.strip() for u in reviewers.split(',') if u.strip()]`

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:40
(Diff revision 2)
> +                invalid_reviewers.append(reviewer)
> +        if invalid_reviewers:

blank line

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:43
(Diff revision 2)
> +                return INVALID_FORM_DATA.with_message(
> +                    "The reviewer '%s' was not found" % invalid_reviewers[0]
> +                )

When using invalid form data you want this form:
```
return INVALID_FORM_DATA, {
    'fields': {
        '<field-name>': ['<error-str>'],
    }
}
```

::: pylib/mozreview/mozreview/resources/verify_reviewer.py:47
(Diff revision 2)
> +                return INVALID_FORM_DATA.with_message(
> +                    "The reviewers '%s' were not found"
> +                    % "', '".join(invalid_reviewers)
> +                )

When using invalid form data you want this form:
```
return INVALID_FORM_DATA, {
    'fields': {
        '<field-name>': ['<error-str>'],
    }
}
```

::: pylib/mozreview/mozreview/signal_handlers.py:7
(Diff revision 2)
> +from django.contrib.auth.models import User
> +

no space between django imports, multiline 4 space indent import please.

::: pylib/mozreview/mozreview/signal_handlers.py:292
(Diff revision 2)
> +        # before we can publish.  Because we only allow editing of the
> +        # target_people (reviewers), that's all we check here.

This is untrue, a number of fields on the child review request could be updated (such as the description, or summary, where we stick commit message fragments). This is the generic publish handler so it would apply to more than just the delgation case.

I have no idea how this isn't breaking some tests, but I'm guessing we just don't have on for when you change the message of a commit and repush...

::: pylib/mozreview/mozreview/static/mozreview/css/review.less
(Diff revision 2)
> -  cursor: pointer;
> -

I apparently paired with mconley on the super early changeset that introduced this... I have 0 recollection of why we did it, heh. weird...

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:152
(Diff revision 2)
>    padding-left: 10px;
>    padding-right: 10px;
>    font-size: 10pt;
>  }
> +
> +// don't show gravator for meta changes.

yay!

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:229
(Diff revision 2)
> +    if (!MozReview.isParent) {
> +      // Unfortunately we cannot publish from children, so provide a link
> +      // to the parent instead.
> +      var parent_rrid = $("#mozreview-parent-request").data("id");
> +      $("#draft-banner").append(
> +          $('<a href="../' + parent_rrid + '/">Publish or Discard from the "Review Summary" view</a>'));
> +    }
> +  }

I am *really* happy we will have this now.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:295
(Diff revision 2)
> +            // inflate the draft to encompass the parent and all children
> +            inflateNativeDraft();

Hmmm, okay, now I'm going to guess why we're doing this - you've enabled editing the reviewers on all of the children for the author as well? and this is to ensure the current request they're on gets a draft banner which links to where they should publish?

If so, I can probably get on board with that. I'm a little too fried right now to know if there is a better solution (that doesn't involve setting up our own global draft banner).

If this is the case, please change this comment to indicate why we're doing this, not what the function does, and update the functions documentation to indicate the purpose.

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:296
(Diff revision 2)
> -              // Manually record a draft exists so the banner will be displayed.
> +            // Manually record a draft exists so the banner will be displayed.
> -              var view = RB.PageManager.getPage().reviewRequestEditorView;
> +            var view = RB.PageManager.getPage().reviewRequestEditorView;
> -              view.model.set("hasDraft", true);
> +            view.model.set("hasDraft", true);
> +            MozReview.reviewEditor.trigger("saved");
> +            // inflate the draft to encompass the parent and all children
> +            inflateNativeDraft();

I still don't understand why we're doing this?

::: pylib/mozreview/mozreview/templates/mozreview/user-data.html:4
(Diff revision 2)
>  {% load mozreview %}
> -<div id="scm-level" data-scm-level="{{ request.mozreview_profile|scm_level }}"></div>
> +<div id="user_data"
> +  data-scm-level="{{ request.mozreview_profile|scm_level }}"
> +  data-debug="{{ request.mozreview_profile }}"

Accidental leftover?

::: pylib/mozreview/mozreview/templates/mozreview/user-data.html:6
(Diff revision 2)
>  {% load mozreview %}
> -<div id="scm-level" data-scm-level="{{ request.mozreview_profile|scm_level }}"></div>
> +<div id="user_data"
> +  data-scm-level="{{ request.mozreview_profile|scm_level }}"
> +  data-debug="{{ request.mozreview_profile }}"
> +  {% if request.user and review_request %}
> +    {% if review_request.submitter.username == request.user.username %}

We should compare primary keys of the user objects rather than the username (I doubt it'll make a difference, but I wouldn't want some strange string equality thing to bite us)

::: pylib/mozreview/mozreview/templatetags/mozreview.py:45
(Diff revision 2)
>  def scm_level(mozreview_profile):
>      if mozreview_profile is None:
>          return ''
>      elif mozreview_profile.has_scm_ldap_group('scm_level_3'):
>          return '3'
> +    elif mozreview_profile.has_scm_ldap_group('scm_level_2'):
> +        return '2'
>      elif mozreview_profile.has_scm_ldap_group('scm_level_1'):
>          return '1'
> +    else:
> +        return ''

This made me chuckle thinking back to you describing the return values from this, heh :)

Comment 18

[:glob ✱]

https://reviewboard.mozilla.org/r/38103/#review41531

> When using invalid form data you want this form:
> ```
> return INVALID_FORM_DATA, {
>     'fields': {
>         '<field-name>': ['<error-str>'],
>     }
> }
> ```

this changes errors thrown during testing back to their generic message.  getting the actual error out of the payload is cumbersome as it uses the fieldname as a key.  in cases where there's a single param i think it makes more sense to avoid this complexity and return the detailed message as the default output.

Comment 19

[Mauro Doglio [:mdoglio]]

Sorry for holding on reviewing ths patch so long. It's probably a good idea for someone else to review the frontend part.

Comment 20

[:glob ✱]

Attachment: testing: Add indicator of review drafts to summary (bug 1161230)

Review commit: https://reviewboard.mozilla.org/r/57518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57518/

Comment 21

[:glob ✱]

Comment on attachment 8726601 [details]
mozreview: add reviewer delegation (bug 1161230)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38103/diff/2-3/

Comment 22

[Māris Fogels [:mars] (please needinfo)]

https://reviewboard.mozilla.org/r/38103/#review55698

Comment 24

[Steven MacLeod [:smacleod]]

Comment on attachment 8759517 [details]
testing: Add indicator of review drafts to summary (bug 1161230)

https://reviewboard.mozilla.org/r/57518/#review55964

(Personally, I still think the commit message should be "mozreview:" rather than "testing:", as this is pretty strictly just about MozReview)

Comment 25

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/38103/#review55968

Just publishing this comment early before tackling the whole review as it can be taken care of independently, and this is quite a lot of code to review.

::: hgext/reviewboard/tests/test-review-request-delegation.t:53
(Diff revision 3)
> +  
> +  review id:  bz://1/mynick
> +  review url: http://$DOCKER_HOSTNAME:$HGPORT1/r/1 (draft)
> +  (published review request 1)
> +
> +Change the reviewer while logged in as reviewer1

We should really update the docs for this change, it's quite the new feature.

Comment 26

[Steven MacLeod [:smacleod]]

Comment on attachment 8726601 [details]
mozreview: add reviewer delegation (bug 1161230)

https://reviewboard.mozilla.org/r/38103/#review57264

Woo, we made it :D

::: pylib/mozreview/mozreview/errors.py:86
(Diff revision 3)
> +REVIEW_REQUEST_UPDATE_NOT_ALLOWED = WebAPIError(
> +    1009,
> +    "Failed to update the review request.",
> +    http_status=405)  # 405 Method not Allowed

This seems a little like we're abusing the meaning of HTTP 405...

::: pylib/mozreview/mozreview/resources/ensure_drafts.py:38
(Diff revision 3)
> +    """Ensures drafts are created on each child review request for the given
> +    parent.

1 line summaries only. maybe "Ensure drafts exist for each child request"?

::: pylib/mozreview/mozreview/resources/ensure_drafts.py:41
(Diff revision 3)
> +    This causes Review Board to show the draft banner on the parent and all
> +    children when any child is updated.

I had already forgotten that we only use this resource when we're dealing with the submitter of the review request themselves. Which took me tracing back up through where we make use of it to find the check for being a submitter.

I think it'd be valuable to list either here, or somewhere in the JS function which makes use of the resource, the higher level purpose in use with the submitter, rather than just what it does mechanically.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:68
(Diff revision 3)
> +    We require a separate resource to handle this so we can allow
> +    anyone with permissions in bugzilla to modify the request.
> +
> +    The reviewers JSON is in the form of:
> +        {
> +            $child_rrid: [ 'reviewer1', 'reviewer2', ... ],

I thought this was python, not perl? ;)

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:107
(Diff revision 3)
> +            return PERMISSION_DENIED
> +
> +        if not is_parent(parent_rr):
> +            return NOT_PARENT
> +
> +        # Validate and expand the new reviewer list

Period.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:146
(Diff revision 3)
> +        # publishing it, so we have to be careful to not overwrite
> +        # existing drafts.
> +
> +        try:
> +            with transaction.atomic():
> +

nuke the blank

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:147
(Diff revision 3)
> +                has_draft = parent_rr.get_draft() is not None
> +                if not has_draft:
> +                    for child_rr in gen_child_rrs(parent_rr):
> +                        has_draft = bool(child_rr.get_draft())
> +                        if has_draft:
> +                            break
> +                if has_draft:
> +                    return REVIEW_REQUEST_UPDATE_NOT_ALLOWED.with_message(
> +                        "Unable to update reviewers as the review request has "
> +                        "pending changes (the patch author has a draft)")

I'm not a big fan of flag variables, may I suggest adding `import itertools` and then using:

```Python
for rr in itertools.chain([parent_rr], gen_child_rrs(parent_rr)):
    if rr.get_draft() is not None:
        return REVIEW_REQUEST_UPDATE_NOT_ALLOWED.with_message(
            "Unable to update reviewers as the review request has "
            "pending changes (the patch author has a draft)")
```

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:150
(Diff revision 3)
> +            with transaction.atomic():
> +
> +                has_draft = parent_rr.get_draft() is not None
> +                if not has_draft:
> +                    for child_rr in gen_child_rrs(parent_rr):
> +                        has_draft = bool(child_rr.get_draft())

`has_draft = child_rr.get_draft() is not None` - using bool is usually unpythonic.

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:152
(Diff revision 3)
> +                            break
> +                if has_draft:

Please put blanks between the end of an indented block and the next line *especially* when dealing with conditional blocks that aren't linked (`if` and `if` vs `if` and `else` etc.)

::: pylib/mozreview/mozreview/resources/modify_reviewer.py:182
(Diff revision 3)
> +                    clear_publish_as(parent_rr)
> +
> +        except PublishError as e:
> +                logging.error("failed to update reviewers on %s: %s"
> +                              % (parent_rr.id, str(e)))
> +                return REVIEW_REQUEST_UPDATE_ERROR.with_message(str(e))

Review Board has a built-in error for this case[1], so we can scrap the `REVIEW_REQUEST_UPDATE_ERROR`.


[1] https://github.com/reviewboard/reviewboard/blob/c2e033e507786a9ab666b8d32279870f197052ad/reviewboard/webapi/errors.py#L135-L138

::: pylib/mozreview/mozreview/resources/review_request_summary.py:285
(Diff revision 3)
>                  json.loads(fetch_commit_data(family['parent']).get_for(
>                      family['parent'], COMMITS_KEY))
>              ]
>              summaries.append({
>                  'parent': self._summarize_review_request(
> -                    request, family['parent']),
> +                    request, family['parent'], None),

When a parameter is optional, please specify the argument using the parameter name, eg `commit=None`.

But, in this case you're just repeating the default argument, so please leave it off.

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:160
(Diff revision 3)
>  #id_shipit,
>  label[for="id_shipit"] {
>    display: none;
>  }
> +
> +// don't show gravator for meta changes.

Capitalization. s/gravator/gravatar

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:160
(Diff revision 3)
> +// don't show gravator for meta changes.
> +// rb is hardcoded to always use the submitter, but we allow non-submitters
> +// to update the target people.

The line breaks are really strange here, I think you should have the second line start on the first.

Also, s/rb/RB

Comment 27

[:glob ✱]

https://reviewboard.mozilla.org/r/38103/#review55698

Comment 28

[:glob ✱]

https://reviewboard.mozilla.org/r/38103/#review55968

> We should really update the docs for this change, it's quite the new feature.

dropping; this is being tracked with the documentation flag on the bug

Comment 29

[:glob ✱]

https://reviewboard.mozilla.org/r/38103/#review57264

> This seems a little like we're abusing the meaning of HTTP 405...

it's deceptive due to the default error message; i'll change it to indicate that it is actually a not-allowed error, not any other sort of failure.
where it's used, the error is replaced with one that's more specific.

> `has_draft = child_rr.get_draft() is not None` - using bool is usually unpythonic.

urgh, i though i fixed all those; sorry.

Comment 30

[:glob ✱]

Comment on attachment 8759517 [details]
testing: Add indicator of review drafts to summary (bug 1161230)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57518/diff/1-2/

Comment 31

[:glob ✱]

Comment on attachment 8726601 [details]
mozreview: add reviewer delegation (bug 1161230)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38103/diff/3-4/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/c28006880d97
testing: Add indicator of review drafts to summary r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/e16074ff71c8
mozreview: add reviewer delegation r=smacleod

Comment 33

[:glob ✱]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/c168c24d81af