CodeOwnersOnPostReview: Fix handling of unchanged code owner approval
The detection of unchanged approvals in CodeOwnersOnPostReview didn't
work. If a label is unchanged its value in the oldApprovals map is null
(a key for the label name is present, but its value is null), but so far
we wrongly checked that no key is present.
Due to this we wrongly extended the change message if the exact same
code owner approval was applied again. In this case the change message
said that by reapplying the vote the files were now code-owner approved
by the user, which wrongly implied that they were not code-owner
approved by the user before.
If a vote is reapplied Gerrit core omits it from the change message:
a) Code-Review+1 is voted when Code-Review+1 is already present:
no change message is added
b) Code-Review+1 and Other+1 are voted when Code-Review+1 is already
present:
the change message says that Other+1 was added, but doesn't mention
Code-Review+1
c) Code-Review+1 is voted and a comment is added when Code-Review+1 is
already present:
the change message says that a comment was added, but doesn't mention
Code-Review+1
If the reapplying of the label is not mentioned by Gerrit core, we
shouldn't extend the change message in this case.
I considered that the change message should say that the files are still
approved (as we do when an approval is upgraded, e.g. from Code-Review+1
to Code-Review+2), but doing this is:
a) confusing if at the same time Gerrit doesn't even mentioned that this
vote was applied
b) not possible with the current extension point, as it doesn't allow us
to distinguish between the user applying the same code-owner approval
again and the user applying an unrelated approval and not touching
the existing code-owner approval (in both cases oldApprovals would
contain null for the code owner approval, and approvals would contain
its current value).
This increases the test coverage for CodeOwnersOnPostReview from 91.2%
to 92.2%.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I9ac2e188139710e769b79f04fba924fcb45aaec5
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnPostReview.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnPostReview.java
index c018d61..75f5c84 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnPostReview.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnPostReview.java
@@ -86,8 +86,11 @@
RequiredApproval requiredApproval =
codeOwnersPluginConfiguration.getRequiredApproval(changeNotes.getProjectName());
- if (!oldApprovals.containsKey(requiredApproval.labelType().getName())) {
- // if oldApprovals doesn't contain the label, the label was not changed
+ if (oldApprovals.get(requiredApproval.labelType().getName()) == null) {
+ // If oldApprovals doesn't contain the label or if the labels value in it is null, the label
+ // was not changed.
+ // This either means that the user only voted on unrelated labels, or that the user applied
+ // the exact same code owner approval again, that was already present.
return Optional.empty();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnPostReviewIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnPostReviewIT.java
index 29a47e5..34c20bb 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnPostReviewIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnPostReviewIT.java
@@ -15,15 +15,20 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.LabelDefinitionInput;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
+import com.google.inject.Inject;
import java.util.Collection;
import java.util.HashMap;
import org.junit.Test;
@@ -32,6 +37,9 @@
* Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.CodeOwnersOnPostReview}.
*/
public class CodeOwnersOnPostReviewIT extends AbstractCodeOwnersIT {
+ @Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ProjectOperations projectOperations;
+
@Test
@GerritConfig(name = "plugin.code-owners.disabled", value = "true")
public void changeMessageNotExtendedIfCodeOwnersFuctionalityIsDisabled() throws Exception {
@@ -79,6 +87,162 @@
}
@Test
+ public void changeMessageNotExtended_sameCodeOwnerApprovalAppliedAgain() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ int messageCount = gApi.changes().id(changeId).get().messages.size();
+
+ // Apply the Code-Review+1 approval again
+ recommend(changeId);
+
+ // Check that no new change message was added.
+ // Gerrit code omits the change message if no vote was changed.
+ assertThat(gApi.changes().id(changeId).get().messages.size()).isEqualTo(messageCount);
+ }
+
+ @Test
+ public void changeMessageNotExtended_sameCodeOwnerApprovalAppliedAgainTogetherWithOtherLabel()
+ throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.values = ImmutableMap.of("+1", "Other", " 0", "Approved");
+ gApi.projects().name(project.get()).label("Other").create(input).get();
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Other")
+ .range(0, 1)
+ .ref("refs/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ // Apply the Code-Review+1 approval again and add an unrelated vote.
+ ReviewInput reviewInput = ReviewInput.recommend();
+ reviewInput.labels.put("Other", (short) 1);
+ gApi.changes().id(changeId).current().review(reviewInput);
+
+ // The message is unchanged, since reapplying the same code owner approval is ignored by Gerrit
+ // core (the change message only mentions the new vote, but not the reapplied vote).
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Other+1");
+ }
+
+ @Test
+ public void changeMessageNotExtended_sameCodeOwnerApprovalAppliedAgainTogetherWithComment()
+ throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.values = ImmutableMap.of("+1", "Other", " 0", "Approved");
+ gApi.projects().name(project.get()).label("Other").create(input).get();
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Other")
+ .range(0, 1)
+ .ref("refs/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ // Apply the Code-Review+1 approval again and add a comment.
+ ReviewInput.CommentInput commentInput = new ReviewInput.CommentInput();
+ commentInput.line = 1;
+ commentInput.message = "some comment";
+ commentInput.path = path;
+ ReviewInput reviewInput = ReviewInput.recommend();
+ reviewInput.comments = reviewInput.comments = new HashMap<>();
+ reviewInput.comments.put(commentInput.path, Lists.newArrayList(commentInput));
+ gApi.changes().id(changeId).current().review(reviewInput);
+
+ // The message is unchanged, since reapplying the same code owner approval is ignored by Gerrit
+ // core (the change message only mentions the comment, but not the reapplied vote).
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1:\n\n" + "(1 comment)");
+ }
+
+ @Test
+ public void changeMessageNotExtended_sameCodeOwnerApprovalAppliedByOtherCodeOwner()
+ throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message)
+ .isEqualTo(
+ String.format(
+ "Patch Set 1: Code-Review+1\n\n"
+ + "By voting Code-Review+1 the following files are now code-owner approved by"
+ + " %s:\n"
+ + "* %s\n",
+ admin.fullName(), path));
+
+ // Apply the Code-Review+1 by another code owner
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message)
+ .isEqualTo(
+ String.format(
+ "Patch Set 1: Code-Review+1\n\n"
+ + "By voting Code-Review+1 the following files are now code-owner approved by"
+ + " %s:\n"
+ + "* %s\n",
+ user.fullName(), path));
+ }
+
+ @Test
public void changeMessageListsPathsThatAreStillApproved() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
