Ensure reviewers are not added to attention set on submit

Originally fixed in If708dc98, but this only works for some submit
strategies. For other submit strategies, such as rebase always,
firstly the "rebase" is created which for some legacy reason, also
adds the person who performed the "submit as a reviewer. Then,
that person is added to the attention set. On the other hand,
with "merge if necessary" strategy, there is only one ChangeUpdate,
so the solution in the change above works.

The fix is simple: We can figure out that we're submitting a change
based on the legacy label, and then never add anyone to the attention
set, since all users should anyway be removed when submitting.

Change-Id: I0ce58a66eb3f9987050ea25125e91aa85e621883

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

@@ -59,6 +59,7 @@ import com.google.gerrit.entities.AttentionSetUpdate;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.Comment;
 import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.entities.LabelId;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.entities.RobotComment;
 import com.google.gerrit.entities.SubmissionId;
@@ -842,6 +843,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         // Skip adding robots to the attention set.
         continue;
       }
+
+      if (attentionSetUpdate.operation() == AttentionSetUpdate.Operation.ADD
+          && approvals.rowKeySet().contains(LabelId.legacySubmit().get())) {
+        // On submit, we sometimes can add the person who submitted the change as a reviewer, and in
+        // turn it will add that person to the attention set.
+        // This ensures we don't add users to the attention set on submit.
+        continue;
+      }
+
       addFooter(msg, FOOTER_ATTENTION, noteUtil.attentionSetUpdateToJson(attentionSetUpdate));
     }
   }

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

@@ -56,6 +56,7 @@ import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AttentionSetUpdate;
 import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.PatchSet;
@@ -63,7 +64,9 @@ import com.google.gerrit.entities.PatchSetApproval;
 import com.google.gerrit.entities.Permission;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.changes.AttentionSetInput;
 import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.changes.SubmitInput;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.api.projects.ConfigInput;
@@ -105,6 +108,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
 import org.eclipse.jgit.diff.DiffFormatter;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
@@ -1337,6 +1341,36 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void submitThatAddsUsersAsReviewersEnsuresTheyAreNotAddedToAttentionSet()
+      throws Exception {
+    PushOneCommit.Result r = createChange("refs/heads/master", "file1", "content");
+
+    // Someone else approves, because if admin reviews, they will be added to the reviewers (and the
+    // bug won't be reproduced).
+    requestScopeOperations.setApiUser(accountCreator.admin2().id());
+    change(r).current().review(ReviewInput.approve().addUserToAttentionSet(user.email(), "reason"));
+
+    requestScopeOperations.setApiUser(admin.id());
+
+    change(r).attention(admin.email()).remove(new AttentionSetInput("remove"));
+    change(r).current().submit();
+
+    AttentionSetUpdate attentionSet =
+        Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+
+    assertThat(attentionSet.account()).isEqualTo(admin.id());
+    assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+    assertThat(attentionSet.reason()).isEqualTo("remove");
+  }
+
+  private List<AttentionSetUpdate> getAttentionSetUpdatesForUser(
+      PushOneCommit.Result r, TestAccount account) {
+    return r.getChange().attentionSet().stream()
+        .filter(a -> a.account().get() == account.id().get())
+        .collect(Collectors.toList());
+  }
+
   private void assertSubmitter(PushOneCommit.Result change) throws Throwable {
     ChangeInfo info = get(change.getChangeId(), ListChangesOption.MESSAGES);
     assertThat(info.messages).isNotNull();

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

@@ -259,35 +259,6 @@ public class AttentionSetIT extends AbstractDaemonTest {
     assertThat(attentionSet.reason()).isEqualTo("Change was submitted");
   }
 
-  /**
-   * There is currently a bug that adds the person who submitted the change as reviewer, which in
-   * turn adds them to the attention set. This test ensures this doesn't happen.
-   */
-  @Test
-  public void submitDoesNotAddReviewersToAttentionSet() throws Exception {
-    PushOneCommit.Result r = createChange("refs/heads/master", "file1", "content");
-
-    // Someone else approves, because if admin reviews, they will be added to the reviewers (and the
-    // bug won't be reproduced).
-    requestScopeOperations.setApiUser(accountCreator.admin2().id());
-    change(r).current().review(ReviewInput.approve().addUserToAttentionSet(user.email(), "reason"));
-
-    requestScopeOperations.setApiUser(admin.id());
-
-    change(r).attention(admin.email()).remove(new AttentionSetInput("remove"));
-    change(r).current().submit();
-
-    // Attention set updates that relate to the admin (the person who replied) are filtered out.
-    AttentionSetUpdate attentionSet =
-        Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
-
-    assertThat(attentionSet.account()).isEqualTo(admin.id());
-    assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
-    assertThat(attentionSet.reason()).isEqualTo("remove");
-
-    change(r).addReviewer(user.email());
-  }
-
   @Test
   public void addedReviewersAreAddedToAttentionSetOnMergedChanges() throws Exception {
     PushOneCommit.Result r = createChange();