opendev/gerrit
Code Issues Proposed changes

Downgrade default notify for WIP reviews

Browse Source 
Sometimes reviews are posted to a change while it's in the WIP state.
The owner may be preparing guidance for their upcoming reviewers, or a
CI bot may be reporting about tests. In general, for the purpose of
filtering messages, we distinguish between bot and human actions on the
basis of whether a tag is provided. A non-null tag is assumed to
indicate automation.

For reviews posted by humans, the default notify level depends on
whether hasReviewStarted is true for the change. If a new WIP change is
created and pending reviewers are added, those reviewers should never
receive an email about the change until the owner starts review.
Therefore, when WIP is true, hasReviewStarted is false, and tag is
null, the default notify level is downgraded from ALL to OWNER. If the
change has started review, the default notify level continues to be ALL
(only if tag is null).

For reviews posted by bots, the default notify level is downgraded for
all WIP changes, even after they've entered review. For example, a CI
bot may post failing test results for a series of intermediate patch
sets. This is just noise to reviewers, but useful to the owner, so when
WIP is true and tag is non-null, the default notify level is downgraded
to OWNER.

Change-Id: I93395e36470090377d2120bb8136e7c11670d224
This commit is contained in:
Logan Hanks 2017-06-12 14:21:47 -07:00
parent fc05596270
commit ea3e3b704a
3 changed files with 91 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
Documentation/rest-api-changes.txt
View File

@ -3769,25 +3769,26 @@ Use the notify property of the top-level link:#review-input[ReviewInput] instead
For the purposes of this table, *everyone* means *owner, reviewers, CCs, stars, and ALL_COMMENTS
watchers*.
[options="header",cols="1,1,1,1,1,1,1"]
[options="header",cols="2,1,1,2,2"]
|=============================
|WIP State |Review Started|Tag Given|Default|notify=ALL|notify=OWNER_REVIEWERS|notify=OWNER
|Ready for review|N/A|N/A|everyone|everyone|owner, reviewers, CCs|owner
|Work in progress|no |no |not sent|everyone|owner, reviewers, CCs|owner
|Work in progress|no |yes|owner |everyone|owner, reviewers, CCs|owner
|Work in progress|yes|no |everyone|everyone|owner, reviewers, CCs|owner
|Work in progress|yes|yes|owner |everyone|owner, reviewers, CCs|owner
|WIP State |Review Started|Tag Given|Default |notify=ALL
|Ready for review|N/A |N/A |everyone|everyone
|Work in progress|no |no |not sent|everyone
|Work in progress|no |yes |owner |everyone
|Work in progress|yes |no |everyone|everyone
|Work in progress|yes |yes |owner |everyone
|=============================
If reviewers are added, then a second email will be sent using the "newchange"
template. The notification logic for this email is the same as for
link:#add-reviewer[Add Reviewer].
[options="header",cols="1,1,1,1,1"]
[options="header",cols="1,1,1"]
|=============================
|WIP State |Default|notify=ALL|notify=OWNER_REVIEWERS|notify=OWNER
|Ready for review|owner, reviewers, CCs|owner, reviewers, CCs|owner, reviewers, CCs|owner
|Work in progress|not sent|owner, reviewers, CCs|owner, reviewers, CCs|owner
|WIP State |Default |notify=ALL
|Ready for review|owner, reviewers, CCs|owner, reviewers, CCs
|Work in progress|not sent |owner, reviewers, CCs
|=============================

61
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CommentSenderIT.java
View File

@ -182,11 +182,59 @@ public class CommentSenderIT extends AbstractNotificationTest {
.bcc(ALL_COMMENTS);
}
@Test
public void commentOnWipChangeByBot() throws Exception {
StagedChange sc = stageWipChange(ALL_COMMENTS);
TestAccount bot = sc.testAccount("bot");
review(bot, sc.changeId, ENABLED, null, "tag");
assertThat(sender)
.sent("comment", sc)
.to(sc.owner)
.notTo(sc.reviewer, sc.ccer)
.notTo(sc.reviewerByEmail, sc.ccerByEmail)
.notTo(sc.starrer)
.notTo(ALL_COMMENTS);
}
@Test
public void commentOnReviewableWipChangeByBot() throws Exception {
StagedChange sc = stageReviewableWipChange(ALL_COMMENTS);
TestAccount bot = sc.testAccount("bot");
review(bot, sc.changeId, ENABLED, null, "tag");
assertThat(sender)
.sent("comment", sc)
.to(sc.owner)
.notTo(sc.reviewer, sc.ccer)
.notTo(sc.reviewerByEmail, sc.ccerByEmail)
.notTo(sc.starrer)
.notTo(ALL_COMMENTS);
}
@Test
public void commentOnReviewableWipChangeByBotNotifyAll() throws Exception {
StagedChange sc = stageWipChange(ALL_COMMENTS);
TestAccount bot = sc.testAccount("bot");
review(bot, sc.changeId, ENABLED, ALL, "tag");
assertThat(sender)
.sent("comment", sc)
.to(sc.owner)
.cc(sc.reviewer, sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS);
}
@Test
public void commentOnReviewableWipChangeByOwner() throws Exception {
StagedChange sc = stageReviewableWipChange(ALL_COMMENTS);
review(sc.owner, sc.changeId, ENABLED);
assertThat(sender).notSent();
assertThat(sender)
.sent("comment", sc)
.notTo(sc.owner)
.cc(sc.reviewer, sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS);
}
private void review(TestAccount account, String changeId, EmailStrategy strategy)
@ -197,9 +245,20 @@ public class CommentSenderIT extends AbstractNotificationTest {
private void review(
TestAccount account, String changeId, EmailStrategy strategy, @Nullable NotifyHandling notify)
throws Exception {
review(account, changeId, strategy, notify, null);
}
private void review(
TestAccount account,
String changeId,
EmailStrategy strategy,
@Nullable NotifyHandling notify,
@Nullable String tag)
throws Exception {
setEmailStrategy(account, strategy);
ReviewInput in = ReviewInput.recommend();
in.notify = notify;
in.tag = tag;
gApi.changes().id(changeId).revision("current").review(in);
}
}

22
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@ -225,9 +225,9 @@ public class PostReview
checkRobotComments(revision, input.robotComments);
}
NotifyHandling reviewerNotify = input.notify;
if (input.notify == null) {
input.notify =
revision.getChange().isWorkInProgress() ? NotifyHandling.OWNER : NotifyHandling.ALL;
input.notify = defaultNotify(revision.getChange(), input);
}
ListMultimap<RecipientType, Account.Id> accountsToNotify =
@ -321,12 +321,28 @@ public class PostReview
reviewerResult.gatherResults();
}
emailReviewers(revision.getChange(), reviewerResults, input.notify, accountsToNotify);
emailReviewers(revision.getChange(), reviewerResults, reviewerNotify, accountsToNotify);
}
return Response.ok(output);
}
private NotifyHandling defaultNotify(Change c, ReviewInput in) {
if (!c.isWorkInProgress()) {
return NotifyHandling.ALL;
}
if (in.tag != null) {
// Treat the presence of tag as indicator of a "bot".
return NotifyHandling.OWNER;
}
if (!c.hasReviewStarted()) {
// If review hasn't started we want to minimize recipients, even on
// "human" comments.
return NotifyHandling.OWNER;
}
return NotifyHandling.ALL;
}
private void emailReviewers(
Change change,
List<PostReviewers.Addition> reviewerAdditions,