1257976 - When a carryforward r+ is missing, do not try to create a replacement r?

Status: RESOLVED FIXED

(MozReview Graveyard :: Integration: Bugzilla, defect)

Description

[Mark Côté [:mcote]]

Whether intentional or not, currently MozReview will try to r? the reviewer if the review should be carried forward and the corresponding r+ is missing on the BMO attachment (presumably due to manually editing the attachment details).  This isn't wrong per se, since it can be seen as a workaround due to the fact that the author can't recreate the reviewer's r+ themselves (unless it's the trivial case of them being the same person).  However, in the case that the reviewer is not accepting reviews, this can prevent publishing a commit set because the author has no way to undo the reviewer's "ship it".

To avoid the extra delay of querying BMO to see if the reviewer is currently accepting review requests, we should just ignore the case where a carryforward r+ is missing.

Note that when bug 1195661 is fixed to allow purposeful re-requesting of previously granted reviews, we *should* try to post an r? in all cases, including this one, since the user will want to know if the reviewer isn't accepting requests.  This isn't a problem since they can just switch to another reviewer and submit again.

Comment 1

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: If a carryforward r+ is missing in BMO, do not create an r? (bug 1257976). r?smacleod

If someone has manually removed an r+ from BMO, we should not try to
replace it with an r? because it will prevent the commit from being
published if the reviewer is not accepting review requests, and there's
no way for the author to workaround this since they can't invalidate
the reviewer's "ship it".

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

Comment 2

[Steven MacLeod [:smacleod]]

Comment on attachment 8732376 [details]
MozReview Request: mozreview: If a carryforward r+ is missing in BMO, do not create an r? (bug 1257976). r?smacleod

https://reviewboard.mozilla.org/r/41149/#review37729

Did you run the tests? I'd expect this to change at least one, if it doesn't we need to add test coverage for this case.

::: pylib/mozreview/mozreview/bugzilla/client.py:142
(Diff revision 1)
> -        for r in sorted(reviewers.keys()):
> +        for reviewer in [reviewer for (reviewer, rplus)
> +                         in sorted(reviewers.iteritems()) if not rplus]:

`for reviewer, rplus in sorted(reviewers.iteritems()) if not rplus:`

Comment 3

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/41149/#review37729

I did, and no failures except the current one in test-auth.t.  I can add a test.

Comment 4

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/41149/#review37729

> `for reviewer, rplus in sorted(reviewers.iteritems()) if not rplus:`

Bah thanks, I knew what I was doing was too complicated heh.

Comment 5

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/41149/#review37729

> Bah thanks, I knew what I was doing was too complicated heh.

Er wait, that doesn't work.  This would, though:

    for reviewer, _ in [reviewer, rplus in sorted(reviewers.iteritems()) if not rplus]:

But it's still rather redundant...

Comment 6

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/41149/#review37729

> Er wait, that doesn't work.  This would, though:
> 
>     for reviewer, _ in [reviewer, rplus in sorted(reviewers.iteritems()) if not rplus]:
> 
> But it's still rather redundant...

Bah, no, that doesn't work either.

Comment 7

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/41149/#review37729

> Bah, no, that doesn't work either.

I'm pretty sure what I sent you should work, assuming your previous solution worked. What are you seeing instead?

Comment 8

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/41149/#review37729

> I'm pretty sure what I sent you should work, assuming your previous solution worked. What are you seeing instead?

I just tried this in the terminal to see if it would work:

>>> d = {'a': True, 'b': False, 'c': False, 'd': True}
>>> for k, v in sorted(d.iteritems()) if not v:
  File "<stdin>", line 1
    for k, v in sorted(d.iteritems()) if not v:
                                              ^
SyntaxError: invalid syntax

Comment 9

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/41149/#review37729

> I just tried this in the terminal to see if it would work:
> 
> >>> d = {'a': True, 'b': False, 'c': False, 'd': True}
> >>> for k, v in sorted(d.iteritems()) if not v:
>   File "<stdin>", line 1
>     for k, v in sorted(d.iteritems()) if not v:
>                                               ^
> SyntaxError: invalid syntax

bah, totally forgot the conditional only works in comprehensions not in a straight for loop. woops my bad :P

In that case, I think I'd prefer you just drop the rplus conditional into the loop:
```
for reviewer, rplus in sorted(reviewers.iteritems()):
    if rplus:
        continue
    
    flags.append({
        'name': 'review',
        'status': '?',
        'requestee': reviewer,
        'new': True
    })
```
or
```
for reviewer, rplus in sorted(reviewers.iteritems()):
    if not rplus:
        flags.append({
            'name': 'review',
            'status': '?',
            'requestee': reviewer,
            'new': True
        })
```

Comment 10

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/41149/#review37729

> bah, totally forgot the conditional only works in comprehensions not in a straight for loop. woops my bad :P
> 
> In that case, I think I'd prefer you just drop the rplus conditional into the loop:
> ```
> for reviewer, rplus in sorted(reviewers.iteritems()):
>     if rplus:
>         continue
>     
>     flags.append({
>         'name': 'review',
>         'status': '?',
>         'requestee': reviewer,
>         'new': True
>     })
> ```
> or
> ```
> for reviewer, rplus in sorted(reviewers.iteritems()):
>     if not rplus:
>         flags.append({
>             'name': 'review',
>             'status': '?',
>             'requestee': reviewer,
>             'new': True
>         })
> ```

Yeah, that's how I had it originally until I tried to get clever. :)  I'll go back to that.

Comment 11

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/41149/#review37729

As discussed, since Bugsy needs to be extended to deal with attachments, I filed this as a separate bug, which I'll get to as soon as possible: https://bugzilla.mozilla.org/show_bug.cgi?id=1258777

Comment 12

[Mark Côté [:mcote]]

Comment on attachment 8732376 [details]
MozReview Request: mozreview: If a carryforward r+ is missing in BMO, do not create an r? (bug 1257976). r?smacleod

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

Comment 13

[Steven MacLeod [:smacleod]]

Comment on attachment 8732376 [details]
MozReview Request: mozreview: If a carryforward r+ is missing in BMO, do not create an r? (bug 1257976). r?smacleod

https://reviewboard.mozilla.org/r/41149/#review38559

Comment 14

[Mark Côté [:mcote]]

This has landed and been deployed.

http://hg.mozilla.org/hgcustom/version-control-tools/rev/986b289049ac