Merge branch 'stable-3.3' into stable-3.4
* stable-3.3:
Do not change existing Reviewer/CC
Fix owners-autoassign ReviewerState field type in project config
Change-Id: I2dbb51ca54b3159e16ac1721e0fe4c8b5cab70f1
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignConfigModule.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignConfigModule.java
index 5536ebc..dfa812d 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignConfigModule.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/AutoassignConfigModule.java
@@ -40,7 +40,7 @@
new ProjectConfigEntry(
"Auto-assign field",
ReviewerState.REVIEWER.name(),
- ProjectConfigEntryType.STRING,
+ ProjectConfigEntryType.LIST,
Arrays.asList(ReviewerState.CC.name(), ReviewerState.REVIEWER.name()),
true,
"Change field to use for the assigned accounts"));
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
index fe62307..ea1975c 100644
--- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
+++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/ReviewerManager.java
@@ -44,6 +44,7 @@
import java.util.Collections;
import java.util.Objects;
import java.util.Optional;
+import java.util.Set;
import java.util.stream.Collectors;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -91,6 +92,11 @@
throws ReviewerManagerException, NoSuchProjectException {
try {
ChangeInfo changeInfo = cApi.get();
+ Set<Integer> currentReviewers =
+ changeInfo.reviewers.values().stream()
+ .flatMap(Collection::stream)
+ .map(ri -> ri._accountId)
+ .collect(Collectors.toSet());
ReviewerState reviewerState = cfg.autoassignedReviewerState(projectNameKey);
try (ManualRequestContext ctx =
requestContext.openAs(Account.id(changeInfo.owner._accountId))) {
@@ -100,7 +106,7 @@
in.reviewers = new ArrayList<>(accountsIds.size());
Collection<Account.Id> validOwnersForAttentionSet = new ArrayList<>(accountsIds.size());
for (Account.Id account : accountsIds) {
- if (isVisibleTo(changeInfo, account)) {
+ if (!currentReviewers.contains(account.get()) && isVisibleTo(changeInfo, account)) {
AddReviewerInput addReviewerInput = new AddReviewerInput();
addReviewerInput.reviewer = account.toString();
addReviewerInput.state = reviewerState;
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
index cabe670..4aa6ff4 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/AbstractAutoassignIT.java
@@ -19,9 +19,13 @@
import static com.googlesource.gerrit.owners.common.AutoassignConfigModule.PROJECT_CONFIG_AUTOASSIGN_FIELD;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.common.RawInputUtil;
+import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.ChangeEditApi;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.project.ProjectConfig;
@@ -97,7 +101,7 @@
addOwnersToRepo("", ownerEmail, NOT_INHERITED);
- Collection<AccountInfo> reviewers = getAutoassignedAccounts(change(createChange()));
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(change(createChange()).get());
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
@@ -105,6 +109,33 @@
}
@Test
+ public void shouldNotReAutoassignUserInPath() throws Exception {
+ String ownerEmail = user.email();
+
+ addOwnersToRepo("", ownerEmail, NOT_INHERITED);
+
+ ChangeApi changeApi = change(createChange());
+ ChangeInfo changeInfo = changeApi.get();
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeInfo);
+ assertThat(reviewers).hasSize(1);
+
+ // Switch user from CC to Reviewer or the other way around
+ AddReviewerInput switchReviewerInput = new AddReviewerInput();
+ switchReviewerInput.reviewer = ownerEmail;
+ switchReviewerInput.state =
+ assignedUserState == ReviewerState.REVIEWER ? ReviewerState.CC : ReviewerState.REVIEWER;
+ changeApi.addReviewer(switchReviewerInput);
+
+ ChangeEditApi changeEdit = changeApi.edit();
+ changeEdit.create();
+ changeEdit.modifyFile("foo", RawInputUtil.create("foo content"));
+ changeEdit.publish();
+
+ // It should not re-assign any user
+ assertThat(getAutoassignedAccounts(changeInfo)).isNull();
+ }
+
+ @Test
public void shouldAutoassignUserInPathWithInheritance() throws Exception {
String childOwnersEmail = accountCreator.user2().email();
String parentOwnersEmail = user.email();
@@ -114,7 +145,8 @@
addOwnersToRepo(childpath, childOwnersEmail, INHERITED);
Collection<AccountInfo> reviewers =
- getAutoassignedAccounts(change(createChange("test change", childpath + "foo.txt", "foo")));
+ getAutoassignedAccounts(
+ change(createChange("test change", childpath + "foo.txt", "foo")).get());
assertThat(reviewers).isNotNull();
assertThat(reviewersEmail(reviewers)).containsExactly(parentOwnersEmail, childOwnersEmail);
@@ -130,7 +162,8 @@
addOwnersToRepo(childpath, childOwnersEmail, NOT_INHERITED);
Collection<AccountInfo> reviewers =
- getAutoassignedAccounts(change(createChange("test change", childpath + "foo.txt", "foo")));
+ getAutoassignedAccounts(
+ change(createChange("test change", childpath + "foo.txt", "foo")).get());
assertThat(reviewers).isNotNull();
assertThat(reviewersEmail(reviewers)).containsExactly(childOwnersEmail);
@@ -143,7 +176,7 @@
addOwnersToRepo("", "suffix", ".java", ownerEmail, NOT_INHERITED);
Collection<AccountInfo> reviewers =
- getAutoassignedAccounts(change(createChange("test change", "foo.java", "foo")));
+ getAutoassignedAccounts(change(createChange("test change", "foo.java", "foo")).get());
assertThat(reviewers).isNotNull();
assertThat(reviewersEmail(reviewers)).containsExactly(ownerEmail);
@@ -156,7 +189,7 @@
addOwnersToRepo("", "suffix", ".java", ownerEmail, NOT_INHERITED);
ChangeApi changeApi = change(createChange("test change", "foo.bar", "foo"));
- Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi);
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
assertThat(reviewers).isNull();
}
@@ -171,7 +204,7 @@
addOwnersToRepo(childpath, "suffix", ".java", childOwnersEmail, INHERITED);
ChangeApi changeApi = change(createChange("test change", childpath + "foo.java", "foo"));
- Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi);
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
assertThat(reviewers).isNotNull();
assertThat(reviewersEmail(reviewers)).containsExactly(parentOwnersEmail, childOwnersEmail);
@@ -187,15 +220,16 @@
addOwnersToRepo(childpath, "suffix", ".java", childOwnersEmail, NOT_INHERITED);
ChangeApi changeApi = change(createChange("test change", childpath + "foo.java", "foo"));
- Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi);
+ Collection<AccountInfo> reviewers = getAutoassignedAccounts(changeApi.get());
assertThat(reviewers).isNotNull();
assertThat(reviewersEmail(reviewers)).containsExactly(childOwnersEmail);
}
- private Collection<AccountInfo> getAutoassignedAccounts(ChangeApi changeApi)
+ private Collection<AccountInfo> getAutoassignedAccounts(ChangeInfo changeInfo)
throws RestApiException {
- Collection<AccountInfo> reviewers = changeApi.get().reviewers.get(assignedUserState);
+ Collection<AccountInfo> reviewers =
+ gApi.changes().id(changeInfo._number).get().reviewers.get(assignedUserState);
return reviewers;
}
