Interim: Don't add members of Non-Interactive Users to attention set

The attention set was designed to help Gerrit users focus on changes
that require them to take action. A set of default rules helps to
bring changes to a user's attention, for example, when a reviewer
has posted comments.

For CI results, these default rules aren't as useful. For example:
one might not care if one of the CI checks that have to run
come back as SUCCESS, but only if it fails.

To allow for a base distinction, this change allows to classify a
user as a 'bot' by checking for a membership in the existing
"Non-Interactive Users" group. That is just one out of many
possible solutions and we consider it to be interim. Other
potential solutions include:
 1) Distinguishing robot and human labels
 2) Adding an isRobot bit to an account
 3) Using capabilities or feature flags
 4) Using group membership (this change)

This change unblocks attention set while allowing for more time
to define a long-term strategy given these options.

Change-Id: Ib56272bb5e6487f8718a70e11461482b520a8056

java/com/google/gerrit/pgm/util/BatchProgramModule.java

@@ -34,6 +34,7 @@ import com.google.gerrit.server.account.CapabilityCollection;
 import com.google.gerrit.server.account.FakeRealm;
 import com.google.gerrit.server.account.GroupCacheImpl;
 import com.google.gerrit.server.account.GroupIncludeCacheImpl;
+import com.google.gerrit.server.account.NonInteractiveUserGroupRobotClassifier;
 import com.google.gerrit.server.account.Realm;
 import com.google.gerrit.server.account.externalids.ExternalIdModule;
 import com.google.gerrit.server.cache.CacheRemovalListener;
@@ -164,6 +165,7 @@ public class BatchProgramModule extends FactoryModule {
     install(SectionSortCache.module());
     install(ChangeKindCacheImpl.module());
     install(MergeabilityCacheImpl.module());
+    install(NonInteractiveUserGroupRobotClassifier.module());
     install(TagCache.module());
     install(PureRevertCache.module());
     factory(CapabilityCollection.Factory.class);

java/com/google/gerrit/server/account/NonInteractiveUserGroupRobotClassifier.java

@@ -0,0 +1,63 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.account;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.server.group.InternalGroup;
+import com.google.inject.AbstractModule;
+import com.google.inject.Module;
+import com.google.inject.Scopes;
+import com.google.inject.Singleton;
+import java.util.Optional;
+import javax.inject.Inject;
+
+/**
+ * An implementation of {@link RobotClassifier} that will consider a user to be a robot if they are
+ * a member in the {@code Non-Interactive Users} group.
+ */
+@Singleton
+public class NonInteractiveUserGroupRobotClassifier implements RobotClassifier {
+  public static Module module() {
+    return new AbstractModule() {
+      @Override
+      protected void configure() {
+        bind(RobotClassifier.class)
+            .to(NonInteractiveUserGroupRobotClassifier.class)
+            .in(Scopes.SINGLETON);
+      }
+    };
+  }
+
+  private final GroupCache groupCache;
+
+  @Inject
+  NonInteractiveUserGroupRobotClassifier(GroupCache groupCache) {
+    this.groupCache = groupCache;
+  }
+
+  @Override
+  public boolean isRobot(Account.Id user) {
+    // TODO(hiesel, brohlfs, paiking): This is just an interim solution until we have figured out a
+    // long-term solution.
+    // Discussion is at: https://gerrit-review.googlesource.com/c/gerrit/+/274854
+    Optional<InternalGroup> maybeGroup =
+        groupCache.get(AccountGroup.nameKey("Non-Interactive Users"));
+    if (maybeGroup.isPresent()) {
+      return maybeGroup.get().getMembers().stream().anyMatch(member -> user.equals(member));
+    }
+    return false;
+  }
+}

java/com/google/gerrit/server/account/RobotClassifier.java

@@ -0,0 +1,30 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.account;
+
+import com.google.gerrit.entities.Account;
+
+public interface RobotClassifier {
+  /** Returns {@code true} if the given user is considered a {@code robot} user. */
+  boolean isRobot(Account.Id user);
+
+  /** An instance that can be used for testing and will consider no user to be a robot. */
+  class NoOp implements RobotClassifier {
+    @Override
+    public boolean isRobot(Account.Id user) {
+      return false;
+    }
+  }
+}

java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -95,6 +95,7 @@ import com.google.gerrit.server.account.EmailExpander;
 import com.google.gerrit.server.account.GroupCacheImpl;
 import com.google.gerrit.server.account.GroupControl;
 import com.google.gerrit.server.account.GroupIncludeCacheImpl;
+import com.google.gerrit.server.account.NonInteractiveUserGroupRobotClassifier;
 import com.google.gerrit.server.account.VersionedAuthorizedKeys;
 import com.google.gerrit.server.account.externalids.ExternalIdModule;
 import com.google.gerrit.server.auth.AuthBackend;
@@ -239,6 +240,7 @@ public class GerritGlobalModule extends FactoryModule {
     install(GroupCacheImpl.module());
     install(GroupIncludeCacheImpl.module());
     install(MergeabilityCacheImpl.module());
+    install(NonInteractiveUserGroupRobotClassifier.module());
     install(PatchListCacheImpl.module());
     install(ProjectCacheImpl.module());
     install(SectionSortCache.module());

java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -66,6 +66,7 @@ import com.google.gerrit.entities.SubmitRecord;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.account.RobotClassifier;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.util.AttentionSetUtil;
 import com.google.gerrit.server.util.LabelVote;
@@ -119,6 +120,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   private final ChangeDraftUpdate.Factory draftUpdateFactory;
   private final RobotCommentUpdate.Factory robotCommentUpdateFactory;
   private final DeleteCommentRewriter.Factory deleteCommentRewriterFactory;
+  private final RobotClassifier robotClassifier;
 
   private final Table<String, Account.Id, Optional<Short>> approvals;
   private final Map<Account.Id, ReviewerStateInternal> reviewers = new LinkedHashMap<>();
@@ -164,6 +166,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       RobotCommentUpdate.Factory robotCommentUpdateFactory,
       DeleteCommentRewriter.Factory deleteCommentRewriterFactory,
       ProjectCache projectCache,
+      RobotClassifier robotClassifier,
       @Assisted ChangeNotes notes,
       @Assisted CurrentUser user,
       @Assisted Date when,
@@ -174,6 +177,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         draftUpdateFactory,
         robotCommentUpdateFactory,
         deleteCommentRewriterFactory,
+        robotClassifier,
         notes,
         user,
         when,
@@ -197,6 +201,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       ChangeDraftUpdate.Factory draftUpdateFactory,
       RobotCommentUpdate.Factory robotCommentUpdateFactory,
       DeleteCommentRewriter.Factory deleteCommentRewriterFactory,
+      RobotClassifier robotClassifier,
       @Assisted ChangeNotes notes,
       @Assisted CurrentUser user,
       @Assisted Date when,
@@ -207,6 +212,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.draftUpdateFactory = draftUpdateFactory;
     this.robotCommentUpdateFactory = robotCommentUpdateFactory;
     this.deleteCommentRewriterFactory = deleteCommentRewriterFactory;
+    this.robotClassifier = robotClassifier;
     this.approvals = approvals(labelNameComparator);
   }
 
@@ -385,7 +391,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
    * must first create the removal, and the addition will not take effect.
    */
   public void addToPlannedAttentionSetUpdates(Set<AttentionSetUpdate> updates) {
-    if (updates == null || updates.isEmpty()) {
+    if (updates == null || updates.isEmpty() || robotClassifier.isRobot(accountId)) {
+      // No updates to do. Robots don't change attention set.
       return;
     }
     checkArgument(
@@ -771,7 +778,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         || getNotes().getChange().isWorkInProgress()
         || status == Change.Status.MERGED) {
       // Attention set shouldn't change here for changes that are work in progress or are about to
-      // be submitted.
+      // be submitted or when the caller is a robot.
       return;
     }
     Set<Account.Id> currentReviewers =
@@ -829,6 +836,12 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         // Skip users that are not in the attention set: no need to remove them.
         continue;
       }
+
+      if (attentionSetUpdate.operation() == AttentionSetUpdate.Operation.ADD
+          && robotClassifier.isRobot(attentionSetUpdate.account())) {
+        // Skip adding robots to the attention set.
+        continue;
+      }
       addFooter(msg, FOOTER_ATTENTION, noteUtil.attentionSetUpdateToJson(attentionSetUpdate));
     }
   }

javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java

@@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.AccountCreator;
 import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -52,6 +53,7 @@ import org.junit.Test;
 public class AttentionSetIT extends AbstractDaemonTest {
 
   @Inject private RequestScopeOperations requestScopeOperations;
+  @Inject protected AccountCreator accountCreator;
 
   /** Simulates a fake clock. Uses second granularity. */
   private static class FakeClock implements LongSupplier {
@@ -922,6 +924,32 @@ public class AttentionSetIT extends AbstractDaemonTest {
     assertThat(attentionSet.reason()).isEqualTo("removed");
   }
 
+  @Test
+  public void robotsNotAddedToAttentionSet() throws Exception {
+    TestAccount robot =
+        accountCreator.create(
+            "robot1", "robot1@example.com", "Ro Bot", "Ro", "Non-Interactive Users");
+    PushOneCommit.Result r = createChange();
+    // TODO(paiking): Adding robots explicitly should throw an exception and only implicit additions
+    // should fail silently.
+    change(r).addToAttentionSet(new AttentionSetInput(robot.email(), "reason"));
+    change(r).addReviewer(robot.email());
+
+    assertThat(r.getChange().attentionSet()).isEmpty();
+  }
+
+  @Test
+  public void robotReviewDoesNotChangeAttentionSet() throws Exception {
+    TestAccount robot =
+        accountCreator.create(
+            "robot2", "robot2@example.com", "Ro Bot", "Ro", "Non-Interactive Users");
+    PushOneCommit.Result r = createChange();
+    requestScopeOperations.setApiUser(robot.id());
+    change(r).addReviewer(user.id().toString());
+
+    assertThat(r.getChange().attentionSet()).isEmpty();
+  }
+
   private List<AttentionSetUpdate> getAttentionSetUpdatesForUser(
       PushOneCommit.Result r, TestAccount account) {
     return r.getChange().attentionSet().stream()

javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java

@@ -38,6 +38,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.FakeRealm;
 import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.Realm;
+import com.google.gerrit.server.account.RobotClassifier;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.config.AnonymousCowardName;
@@ -165,6 +166,7 @@ public abstract class AbstractChangeNotesTest {
                 bind(ExecutorService.class)
                     .annotatedWith(FanOutExecutor.class)
                     .toInstance(assertableFanOutExecutor);
+                bind(RobotClassifier.class).to(RobotClassifier.NoOp.class);
               }
             });