Don't suggest service users as reviewers
Service users usually decide based on their own criteria which changes
they are acting on and don't react on review invitations sent by email.
Add a new setting suggest.skipServiceUsers to gate this behavior.
Bug: Issue 14117
Change-Id: If0202fffda8ce173f2317d61c4f5387e18868736
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index c362710..9d7e93b 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -5140,6 +5140,13 @@
+
By default 50.
+[[suggest.skipServiceUsers]]suggest.skipServiceUsers::
++
+If link:access-control.html#service_users[service users] should be skipped when
+suggesting reviewers.
++
+By default true.
+
[[tracing]]
=== Section tracing
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
index 2a55e41..38be27e 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
+++ b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
@@ -51,6 +51,7 @@
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupMembers;
+import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.change.ReviewerAdder;
import com.google.gerrit.server.index.account.AccountField;
import com.google.gerrit.server.index.account.AccountIndexCollection;
@@ -130,6 +131,7 @@
private final IndexConfig indexConfig;
private final AccountControl.Factory accountControlFactory;
private final Provider<CurrentUser> self;
+ private final ServiceUserClassifier serviceUserClassifier;
@Inject
ReviewersUtil(
@@ -143,7 +145,8 @@
AccountIndexCollection accountIndexes,
IndexConfig indexConfig,
AccountControl.Factory accountControlFactory,
- Provider<CurrentUser> self) {
+ Provider<CurrentUser> self,
+ ServiceUserClassifier serviceUserClassifier) {
this.accountLoaderFactory = accountLoaderFactory;
this.accountQueryBuilder = accountQueryBuilder;
this.accountIndexRewriter = accountIndexRewriter;
@@ -155,6 +158,7 @@
this.indexConfig = indexConfig;
this.accountControlFactory = accountControlFactory;
this.self = self;
+ this.serviceUserClassifier = serviceUserClassifier;
}
public interface VisibilityControl {
@@ -200,13 +204,17 @@
reviewerState, changeNotes, suggestReviewers, projectState, candidateList);
logger.atFine().log("Sorted recommendations: %s", sortedRecommendations);
- // Filter accounts by visibility and enforce limit
+ // Filter accounts by visibility, skip service users and enforce limit
List<Account.Id> filteredRecommendations = new ArrayList<>();
try (Timer0.Context ctx = metrics.filterVisibility.start()) {
for (Account.Id reviewer : sortedRecommendations) {
if (filteredRecommendations.size() >= limit) {
break;
}
+ if (suggestReviewers.isSkipServiceUsers()
+ && serviceUserClassifier.isServiceUser(reviewer)) {
+ continue;
+ }
// Check if change is visible to reviewer and if the current user can see reviewer
if (visibilityControl.isVisibleTo(reviewer)
&& accountControlFactory.get().canSee(reviewer)) {
diff --git a/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java b/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java
index e071c89..71ff493 100644
--- a/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java
@@ -31,6 +31,8 @@
private static final int DEFAULT_MAX_SUGGESTED = 10;
+ private static final boolean DEFAULT_SKIP_SERVICE_USERS = true;
+
protected final ReviewersUtil reviewersUtil;
private final boolean suggestAccounts;
@@ -39,6 +41,7 @@
protected int limit;
protected String query;
protected final int maxSuggestedReviewers;
+ protected boolean skipServiceUsers;
@Option(
name = "--limit",
@@ -78,6 +81,10 @@
return maxAllowedWithoutConfirmation;
}
+ public boolean isSkipServiceUsers() {
+ return skipServiceUsers;
+ }
+
@Inject
public SuggestReviewers(
AccountVisibility av, @GerritServerConfig Config cfg, ReviewersUtil reviewersUtil) {
@@ -100,6 +107,9 @@
ReviewerAdder.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
logger.atFine().log("AccountVisibility: %s", av.name());
+
+ this.skipServiceUsers =
+ cfg.getBoolean("suggest", "skipServiceUsers", DEFAULT_SKIP_SERVICE_USERS);
}
public static GerritConfigListener configListener() {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index 888878f..ffde622 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -450,6 +450,23 @@
}
@Test
+ public void suggestNoServiceAccounts() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+ String changeIdReviewed = createChangeFromApi();
+ String changeId = createChangeFromApi();
+
+ String name = name("foo");
+ TestAccount foo = accountCreator.create(name);
+ reviewChange(changeIdReviewed, foo);
+
+ assertReviewers(suggestReviewers(changeId, name), ImmutableList.of(foo), ImmutableList.of());
+
+ groupOperations.group(serviceUsersUUID()).forUpdate().addMember(foo.id()).update();
+
+ assertReviewers(suggestReviewers(changeId, name), ImmutableList.of(), ImmutableList.of());
+ }
+
+ @Test
public void suggestNoExistingReviewers() throws Exception {
requestScopeOperations.setApiUser(user.id());
String changeId = createChangeFromApi();
@@ -608,6 +625,13 @@
return user(name, fullName, name);
}
+ private AccountGroup.UUID serviceUsersUUID() {
+ return groupCache
+ .get(AccountGroup.nameKey("Service Users"))
+ .orElseThrow(() -> new IllegalStateException("unable to find 'Service Users'"))
+ .getGroupUUID();
+ }
+
private void reviewChange(String changeId, TestAccount reviewer) throws RestApiException {
gApi.changes().id(changeId).addReviewer(reviewer.id().toString());
}
