From 8760741834448fc1249b31c92940816b75a2f492 Mon Sep 17 00:00:00 2001 From: Logan Hanks Date: Tue, 30 May 2017 13:49:04 -0700 Subject: [PATCH] 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 --- Documentation/rest-api-changes.txt | 17 +++++- .../server/mail/DeleteReviewerSenderIT.java | 57 ++++++++++++------- .../api/changes/DeleteReviewerInput.java | 2 +- .../gerrit/server/change/DeleteReviewer.java | 4 -- .../change/DeleteReviewerByEmailOp.java | 19 +++++-- .../server/change/DeleteReviewerOp.java | 8 +++ 6 files changed, 76 insertions(+), 31 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index a2feea3975..cb11f7c5a5 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -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 -- 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 b3fd774502..9838140cdd 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 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; } 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 34f550b4fc..5be5f3366c 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 @@ 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 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 6bee46d264..c2bcd69b21 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 @@ 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( 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 adfe3f5a27..341ad4a94c 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 @@ 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); } } } 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 a255f794e2..df4b43554a 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.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); }