1273790 - existing r- attachment flags in bugzilla are reset to r? when a reviewer is added via review board

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[:glob ✱]

existing r- attachment flags in bugzilla are reset to r? when a reviewer is added via review board.

str:
1. author publishes a review requesting review from userA
   attachment flags: userA r?
2. userA leaves r-
   attachment flags: userA r-
3. author uses review board web ui to add userB as a reviewer

expected:
   review board ui : userA r-, userB r?
   attachment state: userA r-, userB r?

actual:
   review board ui : userA r-, userB r?
   attachment flags: userA r?, userB r?

Comment 1

[:glob ✱]

Attachment: MozReview Request: mozreview: don't reset r- flags when reviewers are added (bug 1273790); r?smacleod

When a reviewer is added via the Review Board web UI, prevent existing r-
attachment flags in Bugzilla from being reset back to r?.

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

Comment 2

[:glob ✱]

that patch doesn't reset r- to r? when a new revision is pushed.

Comment 3

[:glob ✱]

Attachment: MozReview Request: mozreview: don't reset r- flags when reviewers are added (bug 1273790); r?smacleod

When a review request is updated we need to handle prior r-'s correctly.
If the update contains changes to the diff, we need to reset r-'s to r?,
otherwise r-'s need to be carried forward.

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

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 8754398 [details]
MozReview Request: mozreview: don't reset r- flags when reviewers are added (bug 1273790); r?smacleod

https://reviewboard.mozilla.org/r/53950/#review50666

::: pylib/mozreview/mozreview/bugzilla/attachments.py:74
(Diff revision 1)
>                  user_email_cache[bum.bugzilla_user_id] = email
>  
>              reviewers[email] = False
>  
>          for review in gen_latest_reviews(review_request):
> -            # The last review given by this reviewer had a ship-it, so we
> +            # If the last review given by this reviewer had a ship-it, so we

This change makes the sentence a little non-sensical.

::: pylib/mozreview/mozreview/bugzilla/attachments.py:79
(Diff revision 1)
> -            if review.ship_it:
> +            # If the last review was r-, we only carry that forward if the
> +            # diff wasn't touched by this change.

TBH, I think it makes sense to not touch the flags at all unless there is a diff change. Or at least, if the only change is to the reviewers, don't touch the flags. It really doesn't make sense to re-request review if someone has cancelled their flag just because another reviewer was added.

::: pylib/mozreview/mozreview/signal_handlers.py:319
(Diff revision 1)
> +                diff_fieldset = get_review_request_field('diff')
> +                field = diff_fieldset(review_request)
> +                old_value = field.load_value(review_request)
> +                new_value = field.load_value(review_request_draft)
> +                reset_r_minus = field.has_value_changed(old_value, new_value)

I would have thought this would be as easy as checking `if review_request_draft.diffset:`, since I'd expect the diffset to be `None` when there is no new diff.

I took a quick peek at the creation of drafts in the RB codebase and it really looks like it should be null on the model... If this isn't the case I'm quite confused.

::: pylib/mozreview/mozreview/signal_handlers.py:323
(Diff revision 1)
> +                # contains changes to the diff.
> +                diff_fieldset = get_review_request_field('diff')
> +                field = diff_fieldset(review_request)
> +                old_value = field.load_value(review_request)
> +                new_value = field.load_value(review_request_draft)
> +                reset_r_minus = field.has_value_changed(old_value, new_value)

We're only basing this on whether the parent request has a changed diff, so this will reset all `r-` even for commits that haven't been updated yet.

We'll want to make the diffset check per commit, and change our logic based on that.

Comment 5

[:glob ✱]

Comment on attachment 8754398 [details]
MozReview Request: mozreview: don't reset r- flags when reviewers are added (bug 1273790); r?smacleod

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

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8754398 [details]
MozReview Request: mozreview: don't reset r- flags when reviewers are added (bug 1273790); r?smacleod

https://reviewboard.mozilla.org/r/53950/#review51832

::: pylib/mozreview/mozreview/bugzilla/attachments.py:11
(Diff revision 2)
>  
>  from mozreview.bugzilla.client import (
>      BugzillaAttachmentUpdates,
>  )
> +from mozreview.extra_data import (
> +    REVIEW_FLAG_KEY

trailing comma

::: pylib/mozreview/mozreview/bugzilla/attachments.py:56
(Diff revision 2)
>      # could be avoided if Bugzilla accepted a numeric userid in the
>      # requestee parameter when modifying an attachment.
>      user_email_cache = {}
>  
>      for review_request_draft, review_request in children_to_post:
> -        reviewers = {}
> +        carry_forward = {}

way clearer... good call :)

::: pylib/mozreview/mozreview/bugzilla/attachments.py:81
(Diff revision 2)
> -            # their flag on bugzilla, we may be setting it back to r+, but
> -            # we will consider the manual flag change on bugzilla user
> -            # error for now.
> -            if review.ship_it:
> -                reviewers[review.user.email] = True
> +            # is a change to meta data only), then carry forward all flags.
> +            # If the diffset was updated, carry forward just r+'s.  All other
> +            # flags should be reset to r?.
> +            if review_request_draft.diffset:
> +                review_flag = review.extra_data.get(REVIEW_FLAG_KEY)
> +                carry_forward[review.user.email] = review_flag == 'r+'

We should still use ship-it here, or at least fallback to it if the flag doesn't exist since these updates could happen with reviews created before we had the fields.

::: pylib/mozreview/mozreview/bugzilla/client.py:74
(Diff revision 2)
>      def create_or_update_attachment(self, review_request_id, summary,
> -                                    comment, url, reviewers):
> +                                    comment, url, carry_forward):
>          """Creates or updates an attachment containing a review-request URL.
>  
> -        The reviewers argument should be a dictionary mapping reviewer email
> -        to a boolean indicating if that reviewer has given an r+ on the
> +        The carry_forward argument should be a dictionary mapping reviewer
> +        email to a boolean indicating if that reviewer has review on

nit: "review flag"

::: pylib/mozreview/mozreview/bugzilla/client.py:121
(Diff revision 2)
> -                elif 'requestee' not in f or f['requestee'] not in reviewers:
> +                elif 'requestee' not in f or \
> +                        f['requestee'] not in carry_forward:

using `\\` frownd upon in PEP8:
```
elif ('requestee' not in f or
      f['requestee'] not in carry_forward):
```

::: pylib/mozreview/mozreview/bugzilla/client.py:143
(Diff revision 2)
>          # reviewer is not accepting review requests, this will block
>          # publishing, with no way for the author to fix it.  So we'll just
>          # ignore manually removed r+s.
>          # This is sorted so behavior is deterministic (this mucks with test
>          # output otherwise).
> -        for reviewer, rplus in sorted(reviewers.iteritems()):
> +        for reviewer, rplus in sorted(carry_forward.iteritems()):

I'm not sure "rplus" makes sense here anymore, even though r+ is the only case it can get here and be true.

::: pylib/mozreview/mozreview/bugzilla/client.py:424
(Diff revision 2)
>          review_flags = [f for f in flag_list if f['name'] == 'review']
>  
>          for f in review_flags:
>              # Bugzilla attachments have a requestee only if the status is `?`.
>              # In the other cases requestee == setter.
> -            if ((reviewer == f.get('requestee') and f['status'] == '?')  or
> +            if ((reviewer == f.get('requestee') and f['status'] == '?') or

I stared at this for too long to realize you just removed a space haha

Comment 8

[:glob ✱]

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