Add an extension point to extend the change message on post review
By implementing the new OnPostReview interface plugins can now extend
the change message that is being posted when the post review REST
endpoint is invoked.
E.g. this is useful if certain approvals have a special meaning (e.g.
custom logic that is implemented in Prolog submit rules, signal for
triggering an action like running CI etc.), as it allows the plugin to
tell users about this meaning in the change message. This makes the
effect of a given approval more transparent to the user.
We intend to use this extension point in the code-owners plugin where a
configured label counts as code owner approval. If an approval on this
label is given, we want to tell the user which files have been approved
by this vote (which depends on which of the files are owned by the
user).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I7742837f2b5d891a976dbc843d3d1e90e453c6eb
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 2f4c46c..c770733 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -1453,6 +1453,19 @@
[...]
----
+[post_review_extensions]
+== Post Review Extensions
+
+By implementing the `com.google.gerrit.server.restapi.change.OnPostReview`
+interface plugins can extend the change message that is being posted when the
+[post review](rest-api-changes.html#set-review) REST endpoint is invoked.
+
+This is useful if certain approvals have a special meaning (e.g. custom logic
+that is implemented in Prolog submit rules, signal for triggering an action
+like running CI etc.), as it allows the plugin to tell users about this meaning
+in the change message. This makes the effect of a given approval more
+transparent to the user.
+
[[ui_extension]]
== UI Extension
diff --git a/java/com/google/gerrit/acceptance/ExtensionRegistry.java b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
index 03644a6..5d01dcb 100644
--- a/java/com/google/gerrit/acceptance/ExtensionRegistry.java
+++ b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
@@ -43,6 +43,7 @@
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.git.validators.RefOperationValidationListener;
import com.google.gerrit.server.logging.PerformanceLogger;
+import com.google.gerrit.server.restapi.change.OnPostReview;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.gerrit.server.validators.AccountActivationValidationListener;
import com.google.gerrit.server.validators.ProjectCreationValidationListener;
@@ -83,6 +84,7 @@
private final DynamicMap<CapabilityDefinition> capabilityDefinitions;
private final DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions;
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
+ private final DynamicSet<OnPostReview> onPostReviews;
@Inject
ExtensionRegistry(
@@ -113,7 +115,8 @@
DynamicSet<WorkInProgressStateChangedListener> workInProgressStateChangedListeners,
DynamicMap<CapabilityDefinition> capabilityDefinitions,
DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions,
- DynamicMap<ProjectConfigEntry> pluginConfigEntries) {
+ DynamicMap<ProjectConfigEntry> pluginConfigEntries,
+ DynamicSet<OnPostReview> onPostReviews) {
this.accountIndexedListeners = accountIndexedListeners;
this.changeIndexedListeners = changeIndexedListeners;
this.groupIndexedListeners = groupIndexedListeners;
@@ -142,6 +145,7 @@
this.capabilityDefinitions = capabilityDefinitions;
this.pluginProjectPermissionDefinitions = pluginProjectPermissionDefinitions;
this.pluginConfigEntries = pluginConfigEntries;
+ this.onPostReviews = onPostReviews;
}
public Registration newRegistration() {
@@ -270,6 +274,10 @@
return add(pluginConfigEntries, pluginConfigEntry, exportName);
}
+ public Registration add(OnPostReview onPostReview) {
+ return add(onPostReviews, onPostReview);
+ }
+
private <T> Registration add(DynamicSet<T> dynamicSet, T extension) {
return add(dynamicSet, extension, "gerrit");
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 6a25afd..b1b2711 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -180,6 +180,7 @@
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ConflictsCacheImpl;
import com.google.gerrit.server.quota.QuotaEnforcer;
+import com.google.gerrit.server.restapi.change.OnPostReview;
import com.google.gerrit.server.restapi.change.SuggestReviewers;
import com.google.gerrit.server.restapi.group.GroupModule;
import com.google.gerrit.server.rules.DefaultSubmitRule;
@@ -413,6 +414,7 @@
DynamicSet.setOf(binder(), ExceptionHook.class);
DynamicSet.bind(binder(), ExceptionHook.class).to(ExceptionHookImpl.class);
DynamicSet.setOf(binder(), MailSoyTemplateProvider.class);
+ DynamicSet.setOf(binder(), OnPostReview.class);
DynamicMap.mapOf(binder(), MailFilter.class);
bind(MailFilter.class).annotatedWith(Exports.named("ListMailFilter")).to(ListMailFilter.class);
diff --git a/java/com/google/gerrit/server/restapi/change/OnPostReview.java b/java/com/google/gerrit/server/restapi/change/OnPostReview.java
new file mode 100644
index 0000000..b179d02
--- /dev/null
+++ b/java/com/google/gerrit/server/restapi/change/OnPostReview.java
@@ -0,0 +1,47 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.restapi.change;
+
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import java.util.Map;
+import java.util.Optional;
+
+/** Extension point that is invoked on post review. */
+@ExtensionPoint
+public interface OnPostReview {
+ /**
+ * Allows implementors to return a message that should be included into the change message that is
+ * posted on post review.
+ *
+ * @param user the user that posts the review
+ * @param changeNotes the change on which post review is performed
+ * @param patchSet the patch set on which post review is performed
+ * @param oldApprovals old approvals that changed as result of post review
+ * @param approvals all current approvals
+ * @return message that should be included into the change message that is posted on post review,
+ * {@link Optional#empty()} if the change message should not be extended
+ */
+ default Optional<String> getChangeMessageAddOn(
+ IdentifiedUser user,
+ ChangeNotes changeNotes,
+ PatchSet patchSet,
+ Map<String, Short> oldApprovals,
+ Map<String, Short> approvals) {
+ return Optional.empty();
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 604c87f..a66abaa 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -30,6 +30,7 @@
import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
import com.google.auto.value.AutoValue;
+import com.google.common.base.Joiner;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
@@ -177,6 +178,7 @@
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
private final PluginSetContext<CommentValidator> commentValidators;
+ private final PluginSetContext<OnPostReview> onPostReviews;
private final ReplyAttentionSetUpdates replyAttentionSetUpdates;
private final boolean strictLabels;
@@ -202,6 +204,7 @@
ProjectCache projectCache,
PermissionBackend permissionBackend,
PluginSetContext<CommentValidator> commentValidators,
+ PluginSetContext<OnPostReview> onPostReviews,
ReplyAttentionSetUpdates replyAttentionSetUpdates) {
this.updateFactory = updateFactory;
this.changeResourceFactory = changeResourceFactory;
@@ -222,6 +225,7 @@
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.commentValidators = commentValidators;
+ this.onPostReviews = onPostReviews;
this.replyAttentionSetUpdates = replyAttentionSetUpdates;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@@ -1413,6 +1417,23 @@
} else if (in.ready) {
buf.append("\n\n" + START_REVIEW_MESSAGE);
}
+
+ List<String> pluginMessages = new ArrayList<>();
+ onPostReviews.runEach(
+ onPostReview ->
+ onPostReview
+ .getChangeMessageAddOn(user, ctx.getNotes(), ps, oldApprovals, approvals)
+ .ifPresent(
+ pluginMessage ->
+ pluginMessages.add(
+ !pluginMessage.endsWith("\n")
+ ? pluginMessage + "\n"
+ : pluginMessage)));
+ if (!pluginMessages.isEmpty()) {
+ buf.append("\n\n");
+ buf.append(Joiner.on("\n").join(pluginMessages));
+ }
+
if (buf.length() == 0) {
return false;
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 7d73374..029d5a2 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -29,9 +29,16 @@
import com.google.common.collect.Iterables;
import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.ExtensionRegistry;
+import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -49,6 +56,9 @@
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.restapi.change.OnPostReview;
import com.google.gerrit.server.restapi.change.PostReview;
import com.google.gerrit.server.update.CommentsRejectedException;
import com.google.gerrit.testing.TestCommentHelper;
@@ -58,6 +68,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
@@ -69,6 +80,7 @@
@Inject private CommentValidator mockCommentValidator;
@Inject private TestCommentHelper testCommentHelper;
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ExtensionRegistry extensionRegistry;
private static final String COMMENT_TEXT = "The comment text";
private static final CommentForValidation FILE_COMMENT_FOR_VALIDATION =
@@ -474,6 +486,124 @@
assertThat(reviewer._accountId).isEqualTo(user.id().get());
}
+ @Test
+ public void extendChangeMessageFromPlugin() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ String testMessage = "hello from plugin";
+ TestOnPostReview testOnPostReview = new TestOnPostReview(testMessage);
+ try (Registration registration = extensionRegistry.newRegistration().add(testOnPostReview)) {
+ ReviewInput input = new ReviewInput().label("Code-Review", 1);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(r.getChangeId()).get().messages;
+ assertThat(Iterables.getLast(messages).message)
+ .isEqualTo(String.format("Patch Set 1: Code-Review+1\n\n%s\n", testMessage));
+ }
+ }
+
+ @Test
+ public void extendChangeMessageFromMultiplePlugins() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ String testMessage1 = "hello from plugin 1";
+ String testMessage2 = "message from plugin 2";
+ TestOnPostReview testOnPostReview1 = new TestOnPostReview(testMessage1);
+ TestOnPostReview testOnPostReview2 = new TestOnPostReview(testMessage2);
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(testOnPostReview1).add(testOnPostReview2)) {
+ ReviewInput input = new ReviewInput().label("Code-Review", 1);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(r.getChangeId()).get().messages;
+ assertThat(Iterables.getLast(messages).message)
+ .isEqualTo(
+ String.format(
+ "Patch Set 1: Code-Review+1\n\n%s\n\n%s\n", testMessage1, testMessage2));
+ }
+ }
+
+ @Test
+ public void onPostReviewExtensionThatDoesntExtendTheChangeMessage() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ TestOnPostReview testOnPostReview = new TestOnPostReview(/* message= */ null);
+ try (Registration registration = extensionRegistry.newRegistration().add(testOnPostReview)) {
+ ReviewInput input = new ReviewInput().label("Code-Review", 1);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(r.getChangeId()).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Code-Review+1");
+ }
+ }
+
+ @Test
+ public void onPostReviewCallbackGetsCorrectChangeAndPatchSet() throws Exception {
+ PushOneCommit.Result r = createChange();
+ amendChange(r.getChangeId());
+
+ TestOnPostReview testOnPostReview = new TestOnPostReview(/* message= */ null);
+ try (Registration registration = extensionRegistry.newRegistration().add(testOnPostReview)) {
+ ReviewInput input = new ReviewInput().label("Code-Review", 1);
+
+ // Vote on current patch set.
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ testOnPostReview.assertChangeAndPatchSet(r.getChange().getId(), 2);
+
+ // Vote on old patch set.
+ gApi.changes().id(r.getChangeId()).revision(1).review(input);
+ testOnPostReview.assertChangeAndPatchSet(r.getChange().getId(), 1);
+ }
+ }
+
+ @Test
+ public void onPostReviewCallbackGetsCorrectUser() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ TestOnPostReview testOnPostReview = new TestOnPostReview(/* message= */ null);
+ try (Registration registration = extensionRegistry.newRegistration().add(testOnPostReview)) {
+ ReviewInput input = new ReviewInput().label("Code-Review", 1);
+
+ // Vote from admin.
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ testOnPostReview.assertUser(admin);
+
+ // Vote from user.
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ testOnPostReview.assertUser(user);
+ }
+ }
+
+ @Test
+ public void onPostReviewCallbackGetsCorrectApprovals() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ TestOnPostReview testOnPostReview = new TestOnPostReview(/* message= */ null);
+ try (Registration registration = extensionRegistry.newRegistration().add(testOnPostReview)) {
+ // Add a new vote.
+ ReviewInput input = new ReviewInput().label("Code-Review", 1);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ testOnPostReview.assertApproval(
+ "Code-Review", /* expectedOldValue= */ 0, /* expectedNewValue= */ 1);
+
+ // Update an existing vote.
+ input = new ReviewInput().label("Code-Review", 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ testOnPostReview.assertApproval(
+ "Code-Review", /* expectedOldValue= */ 1, /* expectedNewValue= */ 2);
+
+ // Post without changing the vote.
+ input = new ReviewInput().label("Code-Review", 2);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ testOnPostReview.assertApproval(
+ "Code-Review", /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
+
+ // Delete the vote.
+ input = new ReviewInput().label("Code-Review", 0);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ testOnPostReview.assertApproval(
+ "Code-Review", /* expectedOldValue= */ 2, /* expectedNewValue= */ 0);
+ }
+ }
+
private List<RobotCommentInfo> getRobotComments(String changeId) throws RestApiException {
return gApi.changes().id(changeId).robotComments().values().stream()
.flatMap(Collection::stream)
@@ -495,4 +625,50 @@
.comparingElementsUsing(COMMENT_CORRESPONDENCE)
.containsExactly(commentsForValidation);
}
+
+ private static class TestOnPostReview implements OnPostReview {
+ private final Optional<String> message;
+
+ private Change.Id changeId;
+ private PatchSet.Id patchSetId;
+ private Account.Id accountId;
+ private Map<String, Short> oldApprovals;
+ private Map<String, Short> approvals;
+
+ TestOnPostReview(@Nullable String message) {
+ this.message = Optional.ofNullable(message);
+ }
+
+ @Override
+ public Optional<String> getChangeMessageAddOn(
+ IdentifiedUser user,
+ ChangeNotes changeNotes,
+ PatchSet patchSet,
+ Map<String, Short> oldApprovals,
+ Map<String, Short> approvals) {
+ this.changeId = changeNotes.getChangeId();
+ this.patchSetId = patchSet.id();
+ this.accountId = user.getAccountId();
+ this.oldApprovals = oldApprovals;
+ this.approvals = approvals;
+ return message;
+ }
+
+ public void assertChangeAndPatchSet(Change.Id expectedChangeId, int expectedPatchSetNum) {
+ assertThat(changeId).isEqualTo(expectedChangeId);
+ assertThat(patchSetId.get()).isEqualTo(expectedPatchSetNum);
+ }
+
+ public void assertUser(TestAccount expectedUser) {
+ assertThat(accountId).isEqualTo(expectedUser.id());
+ }
+
+ public void assertApproval(
+ String labelName, @Nullable Integer expectedOldValue, int expectedNewValue) {
+ assertThat(oldApprovals)
+ .containsExactly(
+ labelName, expectedOldValue != null ? expectedOldValue.shortValue() : null);
+ assertThat(approvals).containsExactly(labelName, (short) expectedNewValue);
+ }
+ }
}
