Closed Bug 1169499 Opened 4 years ago Closed 4 years ago

Reviewers reset unless r? syntax is present

Categories

(MozReview Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gps, Assigned: dminor)

References

Details

Attachments

(1 file)

I just pushed a new review series to https://reviewboard.mozilla.org/r/7951/. I had reviewers set on all the old published review requests. After pushing new, rebased commits, reviewers have disappeared from the draft review requests (I'm not using r? syntax to define reviewers via commit messages). I reckon the clearing of reviewers is related to supporting the r? syntax.

If reviewers are specified in the review request, I don't think a new push w/o reviewers should clear the existing reviewers.
This is essentially the same problem as Bug 1164132 but I think it is worth solving separately as it is clearly undesirable behaviour that is easy to avoid.
Assignee: nobody → dminor
Priority: -- → P1
reviewboard: we should not reset reviewers if none are specified in commit summary (bug 1169499); r?gps

This is a symptom of a larger problem where specifying reviewers in the commit
summary causes any reviewers specified in the UI to be lost. Solving that
larger problem requires us to have a good means of identifying reviewers which
were specified in the UI and which were specified in the commit summary.

In the mean time, if no reviewers are specified in the commit summary we can
skip setting the reviewers on the draft, which will preserve reviewers that
were set in the UI if none are specified in the commit.
Attachment #8612805 - Flags: review?(gps)
Status: NEW → ASSIGNED
Comment on attachment 8612805 [details]
MozReview Request: reviewboard: we should not reset reviewers if none are specified in commit summary (bug 1169499); r?gps

https://reviewboard.mozilla.org/r/9635/#review8467

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:347
(Diff revision 1)
> +        else:
> +            draft = rr.get_or_create_draft(
> +                    summary=commit['message'].splitlines()[0],
> +                    description=commit['message'])

I liked your `props` dict approach from above to avoid code duplication. I think you should change this code while you are here.

::: hgext/reviewboard/tests/test-specify-reviewers.t:332
(Diff revision 1)
> -  $ hg commit --amend -m 'Bug 1 - Amended stuff; r?romulus, r?remus'
> +  $ hg commit --amend -m 'Bug 1 - Amended more stuff; r?romulus, r?remus'

I don't think you needed to change the commit message here. But it doesn't matter too much.
Attachment #8612805 - Flags: review?(gps) → review+
Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
(In reply to Dan Minor [:dminor] from comment #4)
> Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools

Well, obviously :)

The revision is: https://hg.mozilla.org/hgcustom/version-control-tools/rev/665dfb80447a
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.