Merge "PostReview: Send batch event when multiple reviewers are added at once"
diff --git a/java/com/google/gerrit/acceptance/ExtensionRegistry.java b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
index 85c4c13..1e5598e 100644
--- a/java/com/google/gerrit/acceptance/ExtensionRegistry.java
+++ b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
@@ -25,6 +25,8 @@
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.events.GroupIndexedListener;
import com.google.gerrit.extensions.events.ProjectIndexedListener;
+import com.google.gerrit.extensions.events.ReviewerAddedListener;
+import com.google.gerrit.extensions.events.ReviewerDeletedListener;
import com.google.gerrit.extensions.events.RevisionCreatedListener;
import com.google.gerrit.extensions.events.TopicEditedListener;
import com.google.gerrit.extensions.events.WorkInProgressStateChangedListener;
@@ -91,6 +93,8 @@
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
private final DynamicSet<PluginPushOption> pluginPushOptions;
private final DynamicSet<OnPostReview> onPostReviews;
+ private final DynamicSet<ReviewerAddedListener> reviewerAddedListeners;
+ private final DynamicSet<ReviewerDeletedListener> reviewerDeletedListeners;
@Inject
ExtensionRegistry(
@@ -125,7 +129,9 @@
DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions,
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
DynamicSet<PluginPushOption> pluginPushOption,
- DynamicSet<OnPostReview> onPostReviews) {
+ DynamicSet<OnPostReview> onPostReviews,
+ DynamicSet<ReviewerAddedListener> reviewerAddedListeners,
+ DynamicSet<ReviewerDeletedListener> reviewerDeletedListeners) {
this.accountIndexedListeners = accountIndexedListeners;
this.changeIndexedListeners = changeIndexedListeners;
this.groupIndexedListeners = groupIndexedListeners;
@@ -158,6 +164,8 @@
this.pluginConfigEntries = pluginConfigEntries;
this.pluginPushOptions = pluginPushOption;
this.onPostReviews = onPostReviews;
+ this.reviewerAddedListeners = reviewerAddedListeners;
+ this.reviewerDeletedListeners = reviewerDeletedListeners;
}
public Registration newRegistration() {
@@ -302,6 +310,14 @@
return add(onPostReviews, onPostReview);
}
+ public Registration add(ReviewerAddedListener reviewerAddedListener) {
+ return add(reviewerAddedListeners, reviewerAddedListener);
+ }
+
+ public Registration add(ReviewerDeletedListener reviewerDeletedListener) {
+ return add(reviewerDeletedListeners, reviewerDeletedListener);
+ }
+
private <T> Registration add(DynamicSet<T> dynamicSet, T extension) {
return add(dynamicSet, extension, "gerrit");
}
diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java
index a333ce5..cbbd01a 100644
--- a/java/com/google/gerrit/server/change/AddReviewersOp.java
+++ b/java/com/google/gerrit/server/change/AddReviewersOp.java
@@ -219,8 +219,13 @@
.map(r -> accountCache.get(r.accountId()))
.flatMap(Streams::stream)
.collect(toList());
- reviewerAdded.fire(
- ctx.getChangeData(change), patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
+ eventSender =
+ () ->
+ reviewerAdded.fire(
+ ctx.getChangeData(change), patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
+ if (sendEvent) {
+ sendEvent();
+ }
}
}
}
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index a26f107..1e40429 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -202,16 +202,23 @@
"Cannot email update for change %s", currChange.getId());
}
}
- reviewerDeleted.fire(
- ctx.getChangeData(currChange),
- patchSet,
- accountCache.get(reviewer.id()).orElse(AccountState.forAccount(reviewer)),
- ctx.getAccount(),
- mailMessage,
- newApprovals,
- oldApprovals,
- notify.handling(),
- ctx.getWhen());
+
+ NotifyHandling notifyHandling = notify.handling();
+ eventSender =
+ () ->
+ reviewerDeleted.fire(
+ ctx.getChangeData(currChange),
+ patchSet,
+ accountCache.get(reviewer.id()).orElse(AccountState.forAccount(reviewer)),
+ ctx.getAccount(),
+ mailMessage,
+ newApprovals,
+ oldApprovals,
+ notifyHandling,
+ ctx.getWhen());
+ if (sendEvent) {
+ sendEvent();
+ }
}
private Iterable<PatchSetApproval> approvals(ChangeContext ctx, Account.Id accountId) {
diff --git a/java/com/google/gerrit/server/change/ReviewerOp.java b/java/com/google/gerrit/server/change/ReviewerOp.java
index 716ac5e..12227c2 100644
--- a/java/com/google/gerrit/server/change/ReviewerOp.java
+++ b/java/com/google/gerrit/server/change/ReviewerOp.java
@@ -32,6 +32,8 @@
public class ReviewerOp implements BatchUpdateOp {
protected boolean sendEmail = true;
+ protected boolean sendEvent = true;
+ protected Runnable eventSender = () -> {};
protected PatchSet patchSet;
protected Result opResult;
@@ -42,6 +44,14 @@
this.sendEmail = false;
}
+ public void suppressEvent() {
+ this.sendEvent = false;
+ }
+
+ public void sendEvent() {
+ eventSender.run();
+ }
+
void setPatchSet(PatchSet patchSet) {
this.patchSet = requireNonNull(patchSet);
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 5002a82..5c252f4 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -92,7 +92,9 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.PublishCommentUtil;
import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.EmailReviewComments;
@@ -105,6 +107,7 @@
import com.google.gerrit.server.change.WorkInProgressOp;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.extensions.events.CommentAdded;
+import com.google.gerrit.server.extensions.events.ReviewerAdded;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -187,6 +190,7 @@
private final BatchUpdate.Factory updateFactory;
private final ChangeResource.Factory changeResourceFactory;
private final ChangeData.Factory changeDataFactory;
+ private final AccountCache accountCache;
private final ApprovalsUtil approvalsUtil;
private final ChangeMessagesUtil cmUtil;
private final CommentsUtil commentsUtil;
@@ -206,6 +210,7 @@
private final PluginSetContext<CommentValidator> commentValidators;
private final PluginSetContext<OnPostReview> onPostReviews;
private final ReplyAttentionSetUpdates replyAttentionSetUpdates;
+ private final ReviewerAdded reviewerAdded;
private final boolean strictLabels;
private final boolean publishPatchSetLevelComment;
@@ -214,6 +219,7 @@
BatchUpdate.Factory updateFactory,
ChangeResource.Factory changeResourceFactory,
ChangeData.Factory changeDataFactory,
+ AccountCache accountCache,
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
CommentsUtil commentsUtil,
@@ -233,10 +239,12 @@
PermissionBackend permissionBackend,
PluginSetContext<CommentValidator> commentValidators,
PluginSetContext<OnPostReview> onPostReviews,
- ReplyAttentionSetUpdates replyAttentionSetUpdates) {
+ ReplyAttentionSetUpdates replyAttentionSetUpdates,
+ ReviewerAdded reviewerAdded) {
this.updateFactory = updateFactory;
this.changeResourceFactory = changeResourceFactory;
this.changeDataFactory = changeDataFactory;
+ this.accountCache = accountCache;
this.commentsUtil = commentsUtil;
this.publishCommentUtil = publishCommentUtil;
this.psUtil = psUtil;
@@ -256,6 +264,7 @@
this.commentValidators = commentValidators;
this.onPostReviews = onPostReviews;
this.replyAttentionSetUpdates = replyAttentionSetUpdates;
+ this.reviewerAdded = reviewerAdded;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
this.publishPatchSetLevelComment =
gerritConfig.getBoolean("event", "comment-added", "publishPatchSetLevelComment", true);
@@ -371,6 +380,7 @@
logger.atFine().log("adding reviewer additions");
for (ReviewerModification reviewerResult : reviewerResults) {
reviewerResult.op.suppressEmail(); // Send a single batch email below.
+ reviewerResult.op.suppressEvent(); // Send events below, if possible as batch.
bu.addOp(revision.getChange().getId(), reviewerResult.op);
if (!ccOrReviewer && reviewerResult.reviewers.contains(account)) {
logger.atFine().log("calling user is explicitly added as reviewer or CC");
@@ -386,6 +396,7 @@
ReviewerModification selfAddition =
reviewerModifier.ccCurrentUser(revision.getUser(), revision);
selfAddition.op.suppressEmail();
+ selfAddition.op.suppressEvent();
bu.addOp(revision.getChange().getId(), selfAddition.op);
}
@@ -433,8 +444,10 @@
reviewerResult.gatherResults(cd);
}
- // Sending from AddReviewersOp was suppressed so we can send a single batch email here.
+ // Sending emails and events from ReviewersOps was suppressed so we can send a single batch
+ // email/event here.
batchEmailReviewers(revision.getUser(), revision.getChange(), reviewerResults, notify);
+ batchReviewerEvents(revision.getUser(), cd, revision.getPatchSet(), reviewerResults, ts);
}
return Response.ok(output);
@@ -512,6 +525,35 @@
}
}
+ private void batchReviewerEvents(
+ CurrentUser user,
+ ChangeData cd,
+ PatchSet patchSet,
+ List<ReviewerModification> reviewerModifications,
+ Timestamp when) {
+ List<AccountState> newlyAddedReviewers = new ArrayList<>();
+
+ // There are no events for CCs and reviewers added/deleted by email.
+ for (ReviewerModification modification : reviewerModifications) {
+ Result reviewerAdditionResult = modification.op.getResult();
+ if (modification.state() == ReviewerState.REVIEWER) {
+ newlyAddedReviewers.addAll(
+ reviewerAdditionResult.addedReviewers().stream()
+ .map(psa -> psa.accountId())
+ .map(accountId -> accountCache.get(accountId))
+ .flatMap(Streams::stream)
+ .collect(toList()));
+ } else if (modification.state() == ReviewerState.REMOVED) {
+ // There is no batch event for reviewer removals, hence fire the event for each
+ // modification that deleted a reviewer immediately.
+ modification.op.sendEvent();
+ }
+ }
+
+ // Fire a batch event for all newly added reviewers.
+ reviewerAdded.fire(cd, patchSet, newlyAddedReviewers, user.asIdentifiedUser().state(), when);
+ }
+
private RevisionResource onBehalfOf(RevisionResource rev, LabelTypes labelTypes, ReviewInput in)
throws BadRequestException, AuthException, UnprocessableEntityException,
PermissionBackendException, IOException, ConfigInvalidException {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index b79be80..96bc65d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -27,6 +27,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -56,6 +57,8 @@
import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.events.CommentAddedListener;
+import com.google.gerrit.extensions.events.ReviewerAddedListener;
+import com.google.gerrit.extensions.events.ReviewerDeletedListener;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -74,6 +77,7 @@
import com.google.inject.Inject;
import com.google.inject.Module;
import java.sql.Timestamp;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
@@ -699,6 +703,61 @@
}
@Test
+ public void addingReviewers() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ TestAccount user2 = accountCreator.user2();
+
+ TestReviewerAddedListener testReviewerAddedListener = new TestReviewerAddedListener();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(testReviewerAddedListener)) {
+ // add user and user2
+ ReviewResult reviewResult =
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(ReviewInput.create().reviewer(user.email()).reviewer(user2.email()));
+
+ assertThat(
+ reviewResult.reviewers.values().stream()
+ .filter(a -> a.reviewers != null)
+ .map(a -> Iterables.getOnlyElement(a.reviewers).name)
+ .collect(toImmutableSet()))
+ .containsExactly(user.fullName(), user2.fullName());
+ }
+
+ assertThat(
+ gApi.changes().id(r.getChangeId()).reviewers().stream()
+ .map(a -> a.name)
+ .collect(toImmutableSet()))
+ .containsExactly(user.fullName(), user2.fullName());
+
+ // Ensure only one batch email was sent for this operation
+ FakeEmailSender.Message message = Iterables.getOnlyElement(sender.getMessages());
+ assertThat(message.body())
+ .containsMatch(
+ Pattern.quote("Hello ")
+ + "("
+ + Pattern.quote(String.format("%s, %s", user.fullName(), user2.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s, %s", user2.fullName(), user.fullName()))
+ + ")");
+ assertThat(message.htmlBody())
+ .containsMatch(
+ "("
+ + Pattern.quote(String.format("%s and %s", user.fullName(), user2.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s and %s", user2.fullName(), user.fullName()))
+ + ")"
+ + Pattern.quote(" to <strong>review</strong> this change"));
+
+ // Ensure that a batch event has been sent:
+ // * 1 batch event for adding user and user2 as reviewers
+ assertThat(testReviewerAddedListener.receivedEvents).hasSize(1);
+ assertThat(testReviewerAddedListener.getReviewerIds()).containsExactly(user.id(), user2.id());
+ }
+
+ @Test
public void deletingReviewers() throws Exception {
PushOneCommit.Result r = createChange();
@@ -712,21 +771,25 @@
sender.clear();
- // remove user and user2
- ReviewResult reviewResult =
- gApi.changes()
- .id(r.getChangeId())
- .current()
- .review(
- ReviewInput.create()
- .reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true)
- .reviewer(user2.email(), ReviewerState.REMOVED, /* confirmed= */ true));
+ TestReviewerDeletedListener testReviewerDeletedListener = new TestReviewerDeletedListener();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(testReviewerDeletedListener)) {
+ // remove user and user2
+ ReviewResult reviewResult =
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true)
+ .reviewer(user2.email(), ReviewerState.REMOVED, /* confirmed= */ true));
- assertThat(
- reviewResult.reviewers.values().stream()
- .map(a -> a.removed.name)
- .collect(toImmutableSet()))
- .containsExactly(user.fullName(), user2.fullName());
+ assertThat(
+ reviewResult.reviewers.values().stream()
+ .map(a -> a.removed.name)
+ .collect(toImmutableSet()))
+ .containsExactly(user.fullName(), user2.fullName());
+ }
assertThat(gApi.changes().id(r.getChangeId()).reviewers()).isEmpty();
@@ -748,6 +811,12 @@
+ "|"
+ Pattern.quote(String.format("%s and %s", user2.fullName(), user.fullName()))
+ ")");
+
+ // Ensure that events have been sent:
+ // * 2 events for removing user and user2 as reviewers (one event per removed reviewer, batch
+ // event not available for reviewer removal)
+ assertThat(testReviewerDeletedListener.receivedEvents).hasSize(2);
+ assertThat(testReviewerDeletedListener.getReviewerIds()).containsExactly(user.id(), user2.id());
}
@Test
@@ -766,30 +835,38 @@
sender.clear();
- // remove user and user2 while adding user3 and user4
- ReviewResult reviewResult =
- gApi.changes()
- .id(r.getChangeId())
- .current()
- .review(
- ReviewInput.create()
- .reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true)
- .reviewer(user2.email(), ReviewerState.REMOVED, /* confirmed= */ true)
- .reviewer(user3.email())
- .reviewer(user4.email()));
+ TestReviewerAddedListener testReviewerAddedListener = new TestReviewerAddedListener();
+ TestReviewerDeletedListener testReviewerDeletedListener = new TestReviewerDeletedListener();
+ try (Registration registration =
+ extensionRegistry
+ .newRegistration()
+ .add(testReviewerAddedListener)
+ .add(testReviewerDeletedListener)) {
+ // remove user and user2 while adding user3 and user4
+ ReviewResult reviewResult =
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true)
+ .reviewer(user2.email(), ReviewerState.REMOVED, /* confirmed= */ true)
+ .reviewer(user3.email())
+ .reviewer(user4.email()));
- assertThat(
- reviewResult.reviewers.values().stream()
- .filter(a -> a.removed != null)
- .map(a -> a.removed.name)
- .collect(toImmutableSet()))
- .containsExactly(user.fullName(), user2.fullName());
- assertThat(
- reviewResult.reviewers.values().stream()
- .filter(a -> a.reviewers != null)
- .map(a -> Iterables.getOnlyElement(a.reviewers).name)
- .collect(toImmutableSet()))
- .containsExactly(user3.fullName(), user4.fullName());
+ assertThat(
+ reviewResult.reviewers.values().stream()
+ .filter(a -> a.removed != null)
+ .map(a -> a.removed.name)
+ .collect(toImmutableSet()))
+ .containsExactly(user.fullName(), user2.fullName());
+ assertThat(
+ reviewResult.reviewers.values().stream()
+ .filter(a -> a.reviewers != null)
+ .map(a -> Iterables.getOnlyElement(a.reviewers).name)
+ .collect(toImmutableSet()))
+ .containsExactly(user3.fullName(), user4.fullName());
+ }
assertThat(
gApi.changes().id(r.getChangeId()).reviewers().stream()
@@ -832,6 +909,15 @@
+ "|"
+ Pattern.quote(String.format("%s and %s", user2.fullName(), user.fullName()))
+ ")");
+
+ // Ensure that events have been sent:
+ // * 1 batch event for adding user3 and user4 as reviewers
+ // * 2 events for removing user and user2 as reviewers (one event per removed reviewer, batch
+ // event not available for reviewer removal)
+ assertThat(testReviewerAddedListener.receivedEvents).hasSize(1);
+ assertThat(testReviewerAddedListener.getReviewerIds()).containsExactly(user3.id(), user4.id());
+ assertThat(testReviewerDeletedListener.receivedEvents).hasSize(2);
+ assertThat(testReviewerDeletedListener.getReviewerIds()).containsExactly(user.id(), user2.id());
}
@Test
@@ -964,4 +1050,36 @@
return Optional.empty();
}
}
+
+ private static class TestReviewerAddedListener implements ReviewerAddedListener {
+ List<ReviewerAddedListener.Event> receivedEvents = new ArrayList<>();
+
+ @Override
+ public void onReviewersAdded(ReviewerAddedListener.Event event) {
+ receivedEvents.add(event);
+ }
+
+ public ImmutableSet<Account.Id> getReviewerIds() {
+ return receivedEvents.stream()
+ .flatMap(e -> e.getReviewers().stream())
+ .map(accountInfo -> Account.id(accountInfo._accountId))
+ .collect(toImmutableSet());
+ }
+ }
+
+ private static class TestReviewerDeletedListener implements ReviewerDeletedListener {
+ List<ReviewerDeletedListener.Event> receivedEvents = new ArrayList<>();
+
+ @Override
+ public void onReviewerDeleted(ReviewerDeletedListener.Event event) {
+ receivedEvents.add(event);
+ }
+
+ public ImmutableSet<Account.Id> getReviewerIds() {
+ return receivedEvents.stream()
+ .map(ReviewerDeletedListener.Event::getReviewer)
+ .map(accountInfo -> Account.id(accountInfo._accountId))
+ .collect(toImmutableSet());
+ }
+ }
}
