Merge "Only add owners with permissions to attention-set" into stable-3.3
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 eef9321..e8f13d9 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
@@ -87,18 +87,14 @@
// TODO(davido): Switch back to using changes API again,
// when it supports batch mode for adding reviewers
ReviewInput in = new ReviewInput();
- Collection<Account.Id> reviewersAccounts =
- Optional.ofNullable(ownersForAttentionSet)
- .map(DynamicItem::get)
- .filter(Objects::nonNull)
- .map(owners -> owners.addToAttentionSet(changeInfo, reviewers))
- .orElse(reviewers);
in.reviewers = new ArrayList<>(reviewers.size());
+ Collection<Account.Id> validOwnersForAttentionSet = new ArrayList<>(reviewers.size());
for (Account.Id account : reviewers) {
if (isVisibleTo(changeInfo, account)) {
AddReviewerInput addReviewerInput = new AddReviewerInput();
addReviewerInput.reviewer = account.toString();
in.reviewers.add(addReviewerInput);
+ validOwnersForAttentionSet.add(account);
} else {
log.warn(
"Not adding account {} as reviewer to change {} because the associated ref is not visible",
@@ -107,6 +103,13 @@
}
}
+ Collection<Account.Id> reviewersAccounts =
+ Optional.ofNullable(ownersForAttentionSet)
+ .map(DynamicItem::get)
+ .filter(Objects::nonNull)
+ .map(owners -> owners.addToAttentionSet(changeInfo, validOwnersForAttentionSet))
+ .orElse(validOwnersForAttentionSet);
+
in.ignoreAutomaticAttentionSetRules = true;
in.addToAttentionSet =
reviewersAccounts.stream()
diff --git a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
index e016cc3..36795a6 100644
--- a/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
+++ b/owners-autoassign/src/test/java/com/googlesource/gerrit/owners/common/OwnersAutoassignWithAttentionSetIT.java
@@ -16,20 +16,30 @@
package com.googlesource.gerrit.owners.common;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static java.util.stream.Collectors.toList;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.AccessSection;
import com.google.gerrit.entities.Account.Id;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.Permission;
+import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.Scopes;
import com.googlesource.gerrit.owners.api.OwnersApiModule;
import com.googlesource.gerrit.owners.api.OwnersAttentionSet;
import java.util.Collection;
+import java.util.Collections;
import org.junit.Test;
@TestPlugin(
@@ -38,6 +48,8 @@
"com.googlesource.gerrit.owners.common.OwnersAutoassignWithAttentionSetIT$TestModule")
public class OwnersAutoassignWithAttentionSetIT extends LightweightPluginDaemonTest {
+ @Inject private ProjectOperations projectOperations;
+
@Override
public Module createModule() {
return new OwnersApiModule();
@@ -67,6 +79,35 @@
String ownerEmail1 = user.email();
String ownerEmail2 = accountCreator.user2().email();
+ setOwners(ownerEmail1, ownerEmail2);
+
+ ChangeInfo change = change(createChange()).get();
+ assertThat(change.reviewers.get(ReviewerState.REVIEWER)).hasSize(2);
+ assertThat(change.attentionSet).hasSize(1);
+ }
+
+ @Test
+ public void shouldAddToAttentionSetOneUserIfAnotherUserHasNoPermission() throws Exception {
+ TestAccount userWithAccessToProject = accountCreator.user();
+ TestAccount userWithNoAccessToProject = accountCreator.user2();
+
+ AccountGroup.UUID groupWithNoAccessToProject =
+ createGroup("groupWithNoAccessToProject", userWithNoAccessToProject);
+
+ setOwners(userWithAccessToProject.email(), userWithNoAccessToProject.email());
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref(AccessSection.ALL).group(groupWithNoAccessToProject))
+ .update();
+
+ ChangeInfo change = change(createChange()).get();
+ assertThat(change.attentionSet).isNotNull();
+ assertThat(change.attentionSet.keySet()).containsExactly(userWithAccessToProject.id().get());
+ }
+
+ private void setOwners(String ownerEmail1, String ownerEmail2) throws Exception {
pushFactory
.create(
admin.newIdent(),
@@ -83,9 +124,12 @@
+ "\n")
.to("refs/heads/master")
.assertOkStatus();
+ }
- ChangeInfo change = change(createChange()).get();
- assertThat(change.reviewers.get(ReviewerState.REVIEWER)).hasSize(2);
- assertThat(change.attentionSet).hasSize(1);
+ private AccountGroup.UUID createGroup(String name, TestAccount member) throws RestApiException {
+ GroupInput groupInput = new GroupInput();
+ groupInput.name = name(name);
+ groupInput.members = Collections.singletonList(String.valueOf(member.id().get()));
+ return AccountGroup.uuid(gApi.groups().create(groupInput).get().id);
}
}
