Merge branch 'stable-3.4'
* stable-3.4:
Fix for java 9 and up issues with powerMock
Do not change existing Reviewer/CC
Fix owners-autoassign ReviewerState field type in project config
Change-Id: I11239c7223434d609c245a0b978b630e027c965b
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 6c908f5..489381c 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;
@@ -85,6 +86,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))) {
@@ -94,7 +100,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)) {
ReviewerInput addReviewerInput = new ReviewerInput();
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 9bf6934..164d121 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;
@@ -104,7 +108,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);
@@ -112,6 +116,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();
@@ -121,7 +152,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);
@@ -137,7 +169,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);
@@ -150,7 +183,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);
@@ -163,7 +196,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();
}
@@ -178,7 +211,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);
@@ -194,15 +227,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;
}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
index df2b891..573c82e 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -25,10 +25,12 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;
@RunWith(PowerMockRunner.class)
+@PowerMockIgnore("jdk.internal.reflect.*")
@PrepareForTest(JgitWrapper.class)
public class PathOwnersTest extends ClassicConfig {
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
index b68ec99..e911f1b 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/RegexTest.java
@@ -36,10 +36,12 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;
@RunWith(PowerMockRunner.class)
+@PowerMockIgnore("jdk.internal.reflect.*")
@PrepareForTest(JgitWrapper.class)
public class RegexTest extends Config {
