Downgrade notify for WIP reviewer deletion
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
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index a2feea3..cb11f7c 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -1096,9 +1096,9 @@
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 @@
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
--
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/DeleteReviewerSenderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/DeleteReviewerSenderIT.java
index b3fd774..9838140 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/DeleteReviewerSenderIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/DeleteReviewerSenderIT.java
@@ -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 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 @@
.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;
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DeleteReviewerInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DeleteReviewerInput.java
index 34f550b..5be5f33 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DeleteReviewerInput.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/DeleteReviewerInput.java
@@ -19,7 +19,7 @@
/** 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;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
index 6bee46d..c2bcd69 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
@@ -16,7 +16,6 @@
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 @@
if (input == null) {
input = new DeleteReviewerInput();
}
- if (input.notify == null) {
- input.notify = NotifyHandling.ALL;
- }
try (BatchUpdate bu =
updateFactory.create(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
index adfe3f5..341ad4a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
@@ -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 @@
private final DeleteReviewerInput input;
private ChangeMessage changeMessage;
- private Change.Id changeId;
+ private Change change;
@Inject
DeleteReviewerByEmailOp(
@@ -60,12 +61,12 @@
@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 @@
@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 @@
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);
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index a255f79..df4b435 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -18,6 +18,7 @@
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 @@
@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);
}
