Fix submission of changes using the botUser identity
Since Change-Id: I39fe14d7e the submission isn't done anymore
using the botUser, which is exactly against the main purpose of this
plugin.
Restore the proper identity of the change submitter as botUser and add
validation tests to make sure that this fundamental feature does not
break anymore by accident and unnoticed.
Bug: Issue 10173
Change-Id: I064468385548c1696898158bd8153165684d5be2
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
index ee8801f..31a11fa 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
@@ -6,13 +6,12 @@
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.Emails;
-import com.google.gerrit.server.change.ChangesCollection;
+import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.GetRelated;
import com.google.gerrit.server.change.GetRelated.ChangeAndCommit;
import com.google.gerrit.server.change.GetRelated.RelatedInfo;
@@ -40,8 +39,6 @@
@Inject ChangeData.Factory changeDataFactory;
- @Inject private ChangesCollection collection;
-
@Inject AutomergeConfig config;
@Inject Provider<ReviewDb> db;
@@ -60,6 +57,8 @@
@Inject SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
+ @Inject ChangeResource.Factory changeResourceFactory;
+
/**
* Check if the current patchset of the specified change has dependent unmerged changes.
*
@@ -167,10 +166,13 @@
permissionBackend.user(getBotUser()).change(notes).database(db).check(READ);
ChangeData changeData =
changeDataFactory.create(db.get(), new Project.NameKey(project), changeId);
+
RevisionResource r =
- new RevisionResource(collection.parse(changeId), changeData.currentPatchSet());
+ new RevisionResource(
+ changeResourceFactory.create(changeData.notes(), getBotUser()),
+ changeData.currentPatchSet());
return r;
- } catch (ResourceNotFoundException | AuthException | PermissionBackendException e) {
+ } catch (AuthException | PermissionBackendException e) {
throw new NoSuchChangeException(changeId);
}
}
diff --git a/src/test/java/com/criteo/gerrit/plugins/automerge/AutomaticMergerTest.java b/src/test/java/com/criteo/gerrit/plugins/automerge/AutomaticMergerTest.java
new file mode 100644
index 0000000..b8ee99f
--- /dev/null
+++ b/src/test/java/com/criteo/gerrit/plugins/automerge/AutomaticMergerTest.java
@@ -0,0 +1,97 @@
+package com.criteo.gerrit.plugins.automerge;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.GerritConfig;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.Changes;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.account.externalids.ExternalIds;
+import com.google.inject.Inject;
+import java.util.Arrays;
+import java.util.List;
+import org.junit.Before;
+import org.junit.Test;
+
+@NoHttpd
+@TestPlugin(
+ name = "autosubmitter",
+ sysModule = "com.criteo.gerrit.plugins.automerge.AutomergeModule")
+public class AutomaticMergerTest extends LightweightPluginDaemonTest {
+ private static final String BOT_USERS = "Bot Users";
+ private static final String DEVELOPERS = "Developers";
+ private TestAccount botUser;
+ private TestAccount regularUser;
+ @Inject ExternalIds extIds;
+
+ @Before
+ public void setup() throws Exception {
+ gApi.groups().create(BOT_USERS);
+ gApi.groups().create(DEVELOPERS);
+ botUser = accountCreator.create("botuser", "botuser@mycompany.com", "Bot User", BOT_USERS);
+ regularUser =
+ accountCreator.create("developer", "developer@mycompany.com", "Developer", DEVELOPERS);
+ grant(project, "refs/*", "submit", false, groupUUID(BOT_USERS));
+ grantLabel("Code-Review", -2, 2, project, "refs/*", false, groupUUID(DEVELOPERS), false);
+ }
+
+ @Test
+ @GerritConfig(name = "automerge.botEmail", value = "botuser@mycompany.com")
+ public void changeReviewedShouldNotBeAutomaticallyMergedIfNotApproved() throws Exception {
+ int changeNum = createChangeNum(user);
+
+ assertThat(changesApi().id(changeNum).get().status).isEqualTo(ChangeStatus.NEW);
+ }
+
+ @Test
+ @GerritConfig(name = "automerge.botEmail", value = "botuser@mycompany.com")
+ public void changeReviewedShouldBeAutomaticallyMergedOnceApproved() throws Exception {
+ int changeNum = createChangeNum(user);
+ changesApi().id(changeNum).current().review(ReviewInput.approve());
+
+ assertThat(changesApi().id(changeNum).get().status).isEqualTo(ChangeStatus.MERGED);
+ }
+
+ @Test
+ @GerritConfig(name = "automerge.botEmail", value = "botuser@mycompany.com")
+ public void changeShouldBeAutomaticallyMergedByBotUser() throws Exception {
+ int changeNum = createChangeNum(user);
+ ChangeApi changeApi = changesApi().id(changeNum);
+ changeApi.current().review(ReviewInput.approve());
+
+ ChangeInfo changeInfo = changeApi.get();
+ assertThat(changeInfo.submitter).isNotNull();
+ assertThat(changeInfo.submitter._accountId).isEqualTo(new Integer(botUser.id.get()));
+ assertThat(changeInfo.submitter.email).isEqualTo(botUser.email);
+ }
+
+ private AccountGroup.UUID groupUUID(String name) {
+ return groupCache.get(new AccountGroup.NameKey(name)).get().getGroupUUID();
+ }
+
+ private Changes changesApi() {
+ setApiUser(regularUser);
+ return gApi.changes();
+ }
+
+ private int createChangeNum(TestAccount user) throws Exception {
+ List<String> msgs =
+ Arrays.asList(createChangeAsUser("refs/for/master", user).getMessage().split("\n"));
+ String changeUrl = msgs.get(msgs.size() - 1).trim().split(" ")[0];
+ return Integer.parseInt(changeUrl.substring(changeUrl.lastIndexOf('/') + 1));
+ }
+
+ protected PushOneCommit.Result createChangeAsUser(String ref, TestAccount user) throws Exception {
+ PushOneCommit.Result result = pushFactory.create(db, user.getIdent(), testRepo).to(ref);
+ result.assertOkStatus();
+ return result;
+ }
+}
