Play better with automatic rotations.
If a rotation alias is added to REVIEWER, the rotation feature will
move it to CC. Moving it back to REVIEWER could trigger adding
another reviewer for each revision. Don't add a reviwer if already
in reviewers list.
Upgraded CopyrightPatternsTest to the latest version of Truth to fix
broken test.
Change-Id: Ifd31baad8e9031c1d4b50ad766f93d2c99349691
diff --git a/src/main/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApi.java b/src/main/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApi.java
index de97d60..ad047a6 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApi.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApi.java
@@ -31,6 +31,7 @@
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewResult;
+import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.CommentInfo;
@@ -328,8 +329,9 @@
.label(scannerConfig.reviewLabel, reviewRequired ? -1 : +2);
if (reviewRequired) {
- ri = addReviewers(ri, scannerConfig.ccs, ReviewerState.CC);
- ri = addReviewers(ri, scannerConfig.reviewers, ReviewerState.REVIEWER);
+ List<ReviewerInfo> reviewers = cApi.reviewers();
+ ri = addReviewers(reviewers, ri, scannerConfig.ccs, ReviewerState.CC);
+ ri = addReviewers(reviewers, ri, scannerConfig.reviewers, ReviewerState.REVIEWER);
}
ImmutableMap.Builder<String, List<CommentInput>> comments = ImmutableMap.builder();
if (reviewRequired) {
@@ -567,8 +569,15 @@
* {@code reviewers} as {@code type} CC or REVIEWER.
*/
@VisibleForTesting
- ReviewInput addReviewers(ReviewInput ri, Iterable<String> reviewers, ReviewerState type) {
+ ReviewInput addReviewers(
+ List<ReviewerInfo> existingReviewers,
+ ReviewInput ri,
+ Iterable<String> reviewers,
+ ReviewerState type) {
for (String reviewer : reviewers) {
+ if (containsReviewer(existingReviewers, reviewer)) {
+ continue;
+ }
ri = ri.reviewer(reviewer, type, true);
}
return ri;
@@ -590,6 +599,24 @@
return false;
}
+ /** Returns true if {@code priorReviewers} already includes {@code reviewer}. */
+ boolean containsReviewer(Iterable<? extends ReviewerInfo> priorReviewers, String reviewer) {
+ if (priorReviewers == null) {
+ return false;
+ }
+ for (ReviewerInfo prior : priorReviewers) {
+ if (Objects.equals(prior.username, reviewer) || Objects.equals(prior.name, reviewer)
+ || Objects.equals(prior.email, reviewer)
+ || Objects.equals(Integer.toString(prior._accountId), reviewer)) {
+ return true;
+ }
+ if (prior.secondaryEmails != null && prior.secondaryEmails.contains(reviewer)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
/**
* Puts the pieces together from a scanner finding to construct a coherent human-reable message.
*
diff --git a/src/test/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApiTest.java b/src/test/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApiTest.java
index efea843..7fd95d2 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApiTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/copyright/CopyrightReviewApiTest.java
@@ -33,6 +33,7 @@
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser;
@@ -90,36 +91,55 @@
@Test
public void testAddReviewers_addNone() throws Exception {
+ ImmutableList<ReviewerInfo> priorReviewers = ImmutableList.of();
ReviewInput ri = new ReviewInput();
- ri = reviewApi.addReviewers(ri, ImmutableList.of(), ReviewerState.CC);
+ ri = reviewApi.addReviewers(priorReviewers, ri, ImmutableList.of(), ReviewerState.CC);
assertThat(ri.reviewers).isNull();
- ri = reviewApi.addReviewers(ri, ImmutableList.of(), ReviewerState.REVIEWER);
+ ri = reviewApi.addReviewers(priorReviewers, ri, ImmutableList.of(), ReviewerState.REVIEWER);
assertThat(ri.reviewers).isNull();
}
@Test
public void testAddReviewers_addCC() throws Exception {
+ ImmutableList<ReviewerInfo> priorReviewers = ImmutableList.of();
ReviewInput ri = new ReviewInput();
- ri = reviewApi.addReviewers(ri, ImmutableList.of("someone"), ReviewerState.CC);
+ ri = reviewApi.addReviewers(priorReviewers, ri, ImmutableList.of("someone"), ReviewerState.CC);
assertThat(ri.reviewers).comparingElementsUsing(addressedTo()).containsExactly("CC:someone");
}
@Test
public void testAddReviewers_addReviewer() throws Exception {
+ ImmutableList<ReviewerInfo> priorReviewers = ImmutableList.of();
ReviewInput ri = new ReviewInput();
- ri = reviewApi.addReviewers(ri, ImmutableList.of("someone"), ReviewerState.REVIEWER);
+ ri = reviewApi.addReviewers(
+ priorReviewers, ri, ImmutableList.of("someone"), ReviewerState.REVIEWER);
assertThat(ri.reviewers)
.comparingElementsUsing(addressedTo())
.containsExactly("REVIEWER:someone");
}
@Test
+ public void testAddReviewers_addReviewerAlreadyCCd() throws Exception {
+ ImmutableList<ReviewerInfo> priorReviewers =
+ ImmutableList.of(ReviewerInfo.byEmail("someone", "someone@example.com"));
+ ReviewInput ri = new ReviewInput();
+ ri = reviewApi.addReviewers(
+ priorReviewers, ri, ImmutableList.of("someone"), ReviewerState.REVIEWER);
+ assertThat(ri.reviewers).isNull();
+ }
+
+ @Test
public void testAddReviewers_addMultiple() throws Exception {
+ ImmutableList<ReviewerInfo> priorReviewers = ImmutableList.of();
ReviewInput ri = new ReviewInput();
ri =
reviewApi.addReviewers(
- ri, ImmutableList.of("someone", "someone else"), ReviewerState.REVIEWER);
- ri = reviewApi.addReviewers(ri, ImmutableList.of("another", "and another"), ReviewerState.CC);
+ priorReviewers,
+ ri,
+ ImmutableList.of("someone", "someone else"),
+ ReviewerState.REVIEWER);
+ ri = reviewApi.addReviewers(
+ priorReviewers, ri, ImmutableList.of("another", "and another"), ReviewerState.CC);
assertThat(ri.reviewers)
.comparingElementsUsing(addressedTo())
.containsExactly(
diff --git a/src/test/java/com/googlesource/gerrit/plugins/copyright/lib/CopyrightPatternsTest.java b/src/test/java/com/googlesource/gerrit/plugins/copyright/lib/CopyrightPatternsTest.java
index 394eb76..e3fbc25 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/copyright/lib/CopyrightPatternsTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/copyright/lib/CopyrightPatternsTest.java
@@ -161,13 +161,13 @@
CopyrightPatterns.Rule rule = CopyrightPatterns.lookup.get(ruleName);
if (rule.exclusions != null) {
- assertThat(rules.excludePatterns).containsAllIn(rule.exclusions);
+ assertThat(rules.excludePatterns).containsAtLeastElementsIn(rule.exclusions);
}
if (rule.licenses != null) {
- assertThat(rules.excludePatterns).containsAllIn(rule.licenses);
+ assertThat(rules.excludePatterns).containsAtLeastElementsIn(rule.licenses);
}
if (rule.owners != null) {
- assertThat(rules.excludePatterns).containsAllIn(rule.owners);
+ assertThat(rules.excludePatterns).containsAtLeastElementsIn(rule.owners);
}
}
}
