opendev/gerrit
Code Issues Proposed changes

Downgrade notify for WIP reviewer deletion

Browse Source 
Previously, notify on deleting a reviewer defaulted to ALL. This
continues to be the case for non-WIP changes.

For WIP changes, we don't want to notify anyone unless this action
results in one or more approvals being removed from the change. If any
approvals are removed, the deleted reviewer and maybe the owner are the
only recipients (which may seem strange but that's existing behavior
when notify=OWNER for this particular notification). If no approvals are
removed, then no notification is sent.

Change-Id: I00821cddb2558fd35c17ae76c47567b8189be0db
This commit is contained in:
Logan Hanks
2017-05-30 13:49:04 -07:00
parent af6146c03b
commit 8760741834
6 changed files with 76 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -1096,9 +1096,9 @@ the error message is contained in the response body.
An email will be sent using the "abandon" template. The notify handling is ALL.
Notifications are suppressed on WIP changes that have never started review.
[options="header",cols="1,1"]
[options="header",cols="1,2"]
|=============================
|WIP State|notify=ALL
|WIP State |notify=ALL
|Ready for review|owner, reviewers, CCs, stars, ABANDONED_CHANGES watchers
|Work in progress|not sent
|Reviewable WIP |owner, reviewers, CCs, stars, ABANDONED_CHANGES watchers
@@ -3049,6 +3049,19 @@ request:
HTTP/1.1 204 No Content
----
.Notifications
An email will be sent using the "deleteReviewer" template. If deleting the
reviewer resulted in one or more approvals being removed, then the deleted
reviewer will also receive a notification (unless notify=NONE).
[options="header",cols="1,5"]
|=============================
|WIP State |Default Recipients
|Ready for review|notify=ALL: deleted reviewer (if voted), owner, reviewers, CCs, stars, ALL_COMMENTS watchers
|Work in progress|notify=NONE: deleted reviewer (if voted)
|=============================
[[list-votes]]
=== List Votes
--

57
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/DeleteReviewerSenderIT.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.acceptance.server.mail;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.server.account.WatchConfig.NotifyType.ALL_COMMENTS;
import com.google.gerrit.acceptance.AbstractNotificationTest;
@@ -175,31 +176,14 @@ public class DeleteReviewerSenderIT extends AbstractNotificationTest {
public void deleteReviewerFromReviewableWipChange() throws Exception {
StagedChange sc = stageReviewableWipChange();
removeReviewer(sc, extraReviewer);
assertThat(sender)
.sent("deleteReviewer", sc)
.notTo(sc.owner)
.to(extraReviewer)
.to(sc.reviewerByEmail) // TODO(logan): This should probably be CC.
.cc(extraCcer, sc.reviewer, sc.ccer)
.cc(sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS);
assertThat(sender).notSent();
}
@Test
public void deleteReviewerFromWipChange() throws Exception {
StagedChange sc = stageWipChange();
removeReviewer(sc, extraReviewer);
// TODO(logan): This should behave like notify=OWNER
assertThat(sender)
.sent("deleteReviewer", sc)
.notTo(sc.owner)
.to(extraReviewer)
.to(sc.reviewerByEmail) // TODO(logan): This should probably be CC.
.cc(extraCcer, sc.reviewer, sc.ccer)
.cc(sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS);
assertThat(sender).notSent();
}
@Test
@@ -217,6 +201,41 @@ public class DeleteReviewerSenderIT extends AbstractNotificationTest {
.bcc(ALL_COMMENTS);
}
@Test
public void deleteReviewerWithApprovalFromWipChange() throws Exception {
StagedChange sc = stageWipChange();
setApiUser(extraReviewer);
gApi.changes().id(sc.changeId).revision("current").review(ReviewInput.recommend());
sender.clear();
setApiUser(sc.owner);
removeReviewer(sc, extraReviewer);
assertThat(sender)
.sent("deleteReviewer", sc)
.to(extraReviewer)
.notTo(sc.owner, sc.ccer, sc.starrer, extraCcer)
.notTo(sc.reviewerByEmail, sc.ccerByEmail)
.notTo(ALL_COMMENTS);
}
@Test
public void deleteReviewerWithApprovalFromWipChangeNotifyOwner() throws Exception {
StagedChange sc = stageWipChange();
setApiUser(extraReviewer);
gApi.changes().id(sc.changeId).revision("current").review(ReviewInput.recommend());
sender.clear();
setApiUser(sc.owner);
removeReviewer(sc, extraReviewer, NotifyHandling.OWNER);
assertThat(sender).sent("deleteReviewer", sc).to(extraReviewer);
}
@Test
public void deleteReviewerByEmailFromWipChangeInNoteDb() throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageWipChange();
gApi.changes().id(sc.changeId).reviewer(sc.reviewerByEmail).remove();
assertThat(sender).notSent();
}
private interface Stager {
StagedChange stage(NotifyType... watches) throws Exception;
}

2
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DeleteReviewerInput.java
View File

@@ -19,7 +19,7 @@ import java.util.Map;
/** Input passed to {@code DELETE /changes/[id]/reviewers/[id]}. */
public class DeleteReviewerInput {
/** Who to send email notifications to after the reviewer is deleted. */
public NotifyHandling notify = NotifyHandling.ALL;
public NotifyHandling notify = null;
public Map<RecipientType, NotifyInfo> notifyDetails;
}

4
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
View File

@@ -16,7 +16,6 @@ package com.google.gerrit.server.change;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -56,9 +55,6 @@ public class DeleteReviewer
if (input == null) {
input = new DeleteReviewerInput();
}
if (input.notify == null) {
input.notify = NotifyHandling.ALL;
}
try (BatchUpdate bu =
updateFactory.create(

19
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.change;
import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -44,7 +45,7 @@ public class DeleteReviewerByEmailOp implements BatchUpdateOp {
private final DeleteReviewerInput input;
private ChangeMessage changeMessage;
private Change.Id changeId;
private Change change;
@Inject
DeleteReviewerByEmailOp(
@@ -60,12 +61,12 @@ public class DeleteReviewerByEmailOp implements BatchUpdateOp {
@Override
public boolean updateChange(ChangeContext ctx) throws OrmException {
changeId = ctx.getChange().getId();
change = ctx.getChange();
PatchSet.Id psId = ctx.getChange().currentPatchSetId();
String msg = "Removed reviewer " + reviewer;
changeMessage =
new ChangeMessage(
new ChangeMessage.Key(changeId, ChangeUtil.messageUuid()),
new ChangeMessage.Key(change.getId(), ChangeUtil.messageUuid()),
ctx.getAccountId(),
ctx.getWhen(),
psId);
@@ -78,11 +79,19 @@ public class DeleteReviewerByEmailOp implements BatchUpdateOp {
@Override
public void postUpdate(Context ctx) {
if (input.notify == null) {
if (change.isWorkInProgress()) {
input.notify = NotifyHandling.NONE;
} else {
input.notify = NotifyHandling.ALL;
}
}
if (!NotifyUtil.shouldNotify(input.notify, input.notifyDetails)) {
return;
}
try {
DeleteReviewerSender cm = deleteReviewerSenderFactory.create(ctx.getProject(), changeId);
DeleteReviewerSender cm =
deleteReviewerSenderFactory.create(ctx.getProject(), change.getId());
cm.setFrom(ctx.getAccountId());
cm.addReviewersByEmail(Collections.singleton(reviewer));
cm.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn());
@@ -90,7 +99,7 @@ public class DeleteReviewerByEmailOp implements BatchUpdateOp {
cm.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails));
cm.send();
} catch (Exception err) {
log.error("Cannot email update for change " + changeId, err);
log.error("Cannot email update for change " + change.getId(), err);
}
}
}

8
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
View File

@@ -18,6 +18,7 @@ import com.google.common.collect.Iterables;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Account;
@@ -165,6 +166,13 @@ public class DeleteReviewerOp implements BatchUpdateOp {
@Override
public void postUpdate(Context ctx) {
if (input.notify == null) {
if (currChange.isWorkInProgress()) {
input.notify = oldApprovals.isEmpty() ? NotifyHandling.NONE : NotifyHandling.OWNER;
} else {
input.notify = NotifyHandling.ALL;
}
}
if (NotifyUtil.shouldNotify(input.notify, input.notifyDetails)) {
emailReviewers(ctx.getProject(), currChange, changeMessage);
}