Propagate the highest vote for each label.
This will allow us to not wipe out +2s with +1s in the case of a context
user being set.
Change-Id: I314facf0a1154897dc6f1f75dde76562226d8654
diff --git a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
index 7cc7074..f56d464 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
@@ -28,6 +28,7 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.extensions.common.CommitInfo;
+import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.MergeInput;
import com.google.gerrit.extensions.common.MergePatchSetInput;
import com.google.gerrit.extensions.common.RevisionInfo;
@@ -47,10 +48,12 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.OptionalInt;
import java.util.Set;
import java.util.TreeMap;
import java.util.UUID;
@@ -200,7 +203,11 @@
return;
}
- Map<String, ApprovalInfo> approvals = event.getApprovals();
+ Map<String, LabelInfo> labels =
+ gApi.changes()
+ .id(change._number)
+ .get(EnumSet.of(ListChangesOption.DETAILED_LABELS))
+ .labels;
for (String downstreamBranch : downstreamBranches) {
try {
@@ -209,8 +216,21 @@
for (Integer changeNumber : existingDownstream) {
ChangeInfo downstreamChange =
gApi.changes().id(changeNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
- for (Map.Entry<String, ApprovalInfo> label : approvals.entrySet()) {
- updateVote(downstreamChange, label.getKey(), label.getValue().value.shortValue());
+ for (Map.Entry<String, LabelInfo> labelEntry : labels.entrySet()) {
+ if (labelEntry.getValue().all.size() > 0) {
+ OptionalInt maxVote =
+ labelEntry
+ .getValue()
+ .all
+ .stream()
+ .filter(o -> o.value != null)
+ .mapToInt(i -> i.value)
+ .max();
+
+ if (maxVote.isPresent()) {
+ updateVote(downstreamChange, labelEntry.getKey(), (short) maxVote.getAsInt());
+ }
+ }
}
}
} catch (RestApiException | InvalidQueryParameterException e) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
index f9dca8a..552683d 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
@@ -24,6 +24,7 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.extensions.api.accounts.AccountApi;
import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -770,6 +771,7 @@
@Test
public void testContextUser() throws Exception {
+ // Branch flow for contextUser is master -> ds_one -> ds_two
Project.NameKey manifestNameKey = defaultSetup();
// Create initial change
PushOneCommit.Result initialResult = createChange("subject", "filename", "echo Hello");
@@ -834,6 +836,82 @@
approve(dsOneChangeInfo.id);
}
+ @Test
+ public void testContextUser_downstreamHighestVote() throws Exception {
+ // Branch flow for contextUser is master -> ds_one -> ds_two
+ Project.NameKey manifestNameKey = defaultSetup();
+ // Create initial change
+ PushOneCommit.Result initialResult = createChange("subject", "filename", "echo Hello");
+ // Project name is scoped by test, so we need to get it from our initial change
+ Project.NameKey projectNameKey = initialResult.getChange().project();
+ String projectName = projectNameKey.get();
+ createBranch(new Branch.NameKey(projectName, "ds_one"));
+ createBranch(new Branch.NameKey(projectName, "ds_two"));
+ initialResult.assertOkStatus();
+ merge(initialResult);
+
+ // Create normalUserGroup, containing current user, and contextUserGroup, containing contextUser
+ String normalUserGroup = createGroup("normalUserGroup");
+ gApi.groups().id(normalUserGroup).addMembers(user.get().getAccountId().toString());
+ AccountApi contextUserApi = gApi.accounts().create("randomContextUser");
+ String contextUserGroup = createGroup("contextUserGroup");
+ gApi.groups().id(contextUserGroup).addMembers(contextUserApi.get().name);
+
+ // Grant +2 to context user, since it doesn't have it by default
+ grantLabel(
+ "Code-Review",
+ -2,
+ 2,
+ projectNameKey,
+ "refs/heads/*",
+ false,
+ AccountGroup.UUID.parse(gApi.groups().id(contextUserGroup).get().id),
+ false);
+ pushContextUserConfig(manifestNameKey.get(), projectName, contextUserApi.get()._accountId);
+
+ // After we upload our config, we upload a new patchset to create the downstreams
+ PushOneCommit.Result result = createChange("subject", "filename2", "echo Hello", "sometopic");
+ result.assertOkStatus();
+ // Check that there are the correct number of changes in the topic
+ List<ChangeInfo> changesInTopic =
+ gApi.changes()
+ .query("topic: " + gApi.changes().id(result.getChangeId()).topic())
+ .withOptions(ListChangesOption.CURRENT_REVISION, ListChangesOption.CURRENT_COMMIT)
+ .get();
+ assertThat(changesInTopic).hasSize(3);
+
+ List<ChangeInfo> sortedChanges = sortedChanges(changesInTopic);
+
+ // Check that downstream is at Code-Review 0
+ ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
+ assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
+ ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
+
+ // Try to +1 master and see it succeed to +1 master and ds_one
+ ChangeInfo masterChangeInfo = sortedChanges.get(2);
+ assertThat(masterChangeInfo.branch).isEqualTo("master");
+ ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
+ recommend(masterChangeInfo.id);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(1);
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(1);
+
+ ChangeInfo dsTwoChangeInfo = sortedChanges.get(1);
+ assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
+ ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
+ assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(1);
+
+ // +2 ds_one and see that it overrides the +1 of the contextUser
+ approve(dsOneChangeInfo.id);
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(2);
+ assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(2);
+ // +0 ds_one and see that it goes back to the +1 of the contextUser
+ gApi.changes().id(dsOneChangeInfo.id).revision("current").review(ReviewInput.noScore());
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(1);
+ assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(1);
+ }
+
private Project.NameKey defaultSetup() throws Exception {
Project.NameKey manifestNameKey = createProject("platform/manifest");
setupTestRepo("default.xml", manifestNameKey, "master", "default.xml");
@@ -919,7 +997,7 @@
cfg.setString("global", null, "manifestProject", manifestName);
cfg.setInt("global", null, "contextUserId", contextUserId);
cfg.setString("automerger", "master:ds_one", "setProjects", project);
- cfg.setString("automerger", "master:ds_two", "setProjects", project);
+ cfg.setString("automerger", "ds_one:ds_two", "setProjects", project);
PushOneCommit push =
pushFactory.create(
db, admin.getIdent(), allProjectRepo, "Subject", "automerger.config", cfg.toText());
@@ -928,8 +1006,17 @@
}
private ApprovalInfo getVote(ChangeApi change, String label) throws RestApiException {
- List<ApprovalInfo> approvals = change.get(EnumSet.of(ListChangesOption.DETAILED_LABELS)).labels.get(label).all;
- return approvals.get(approvals.size() - 1);
+ List<ApprovalInfo> approvals =
+ change.get(EnumSet.of(ListChangesOption.DETAILED_LABELS)).labels.get(label).all;
+ ApprovalInfo maxApproval = null;
+ for (ApprovalInfo approval : approvals) {
+ if (maxApproval == null) {
+ maxApproval = approval;
+ } else if (approval.value != null && approval.value > maxApproval.value) {
+ maxApproval = approval;
+ }
+ }
+ return maxApproval;
}
private List<ChangeInfo> sortedChanges(List<ChangeInfo> changes) {
diff --git a/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config b/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config
index 77f79b9..bb2d262 100644
--- a/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config
+++ b/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config
@@ -1,14 +1,12 @@
[automerger "master:ds_one"]
addProjects = platform/added/project
ignoreProjects = whoo
-[automerger "master:ds_two"]
+[automerger "ds_one:ds_two"]
mergeAll = true
addProjects = platform/added/project
ignoreProjects = whoo
setProjects = platform/some/project
setProjects = platform/other/project
-[automerger "ds_two:ds_three"]
- setProjects = platform/some/project
[global]
alwaysBlankMerge = .*Import translations. DO NOT MERGE.*
alwaysBlankMerge = .*DO NOT MERGE ANYWHERE.*
