Merge changes I68ee52d4,I2d6f183e

* changes:
  Remove RemoveFromAttentionSetInput
  Review without changing the attention set

java/com/google/gerrit/entities/AttentionSetUpdate.java

@@ -24,9 +24,7 @@ import java.time.Instant;
  * in reverse chronological order. Since each update contains all required information and
  * invalidates all previous state, only the most recent record is relevant for each user.
  *
- * <p>See {@link AttentionSetInput} and {@link
- * com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput} for the representation in
- * the API.
+ * <p>See {@link AttentionSetInput} for the representation in the API.
  */
 @AutoValue
 public abstract class AttentionSetUpdate {

java/com/google/gerrit/extensions/api/changes/AttentionSetApi.java

@@ -20,7 +20,7 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 /** API for managing the attention set of a change. */
 public interface AttentionSetApi {
 
-  void remove(RemoveFromAttentionSetInput input) throws RestApiException;
+  void remove(AttentionSetInput input) throws RestApiException;
 
   /**
    * A default implementation which allows source compatibility when adding new methods to the
@@ -28,7 +28,7 @@ public interface AttentionSetApi {
    */
   class NotImplemented implements AttentionSetApi {
     @Override
-    public void remove(RemoveFromAttentionSetInput input) throws RestApiException {
+    public void remove(AttentionSetInput input) throws RestApiException {
       throw new NotImplementedException();
     }
   }

java/com/google/gerrit/extensions/api/changes/AttentionSetInput.java

@@ -14,20 +14,25 @@
 
 package com.google.gerrit.extensions.api.changes;
 
+import com.google.gerrit.extensions.restapi.DefaultInput;
+
 /**
  * Input at API level to add a user to the attention set.
  *
- * @see RemoveFromAttentionSetInput
  * @see com.google.gerrit.extensions.common.AttentionSetEntry
  */
 public class AttentionSetInput {
   public String user;
-  public String reason;
+  @DefaultInput public String reason;
 
   public AttentionSetInput(String user, String reason) {
     this.user = user;
     this.reason = reason;
   }
 
+  public AttentionSetInput(String reason) {
+    this.reason = reason;
+  }
+
   public AttentionSetInput() {}
 }

java/com/google/gerrit/extensions/api/changes/RemoveFromAttentionSetInput.java

@@ -1,33 +0,0 @@
-// 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.extensions.api.changes;
-
-import com.google.gerrit.extensions.restapi.DefaultInput;
-
-/**
- * Input at API level to remove a user from the attention set.
- *
- * @see AttentionSetInput
- * @see com.google.gerrit.extensions.common.AttentionSetEntry
- */
-public class RemoveFromAttentionSetInput {
-  @DefaultInput public String reason;
-
-  public RemoveFromAttentionSetInput(String reason) {
-    this.reason = reason;
-  }
-
-  public RemoveFromAttentionSetInput() {}
-}

java/com/google/gerrit/extensions/api/changes/ReviewInput.java

@@ -81,6 +81,14 @@ public class ReviewInput {
   /** Users that should be removed from the attention set of this change. */
   public List<AttentionSetInput> removeFromAttentionSet;
 
+  /**
+   * Users in the attention set will only be added and removed based on {@link #addToAttentionSet}
+   * and {@link #removeFromAttentionSet}. Normally, they are also added and removed when some events
+   * occur. E.g, adding/removing reviewers, marking a change ready for review or work in progress,
+   * and replying on changes.
+   */
+  public boolean ignoreDefaultAttentionSetRules;
+
   public enum DraftHandling {
     /** Leave pending drafts alone. */
     KEEP,
@@ -167,6 +175,11 @@ public class ReviewInput {
     return this;
   }
 
+  public ReviewInput blockDefaultAttentionSetRules() {
+    ignoreDefaultAttentionSetRules = true;
+    return this;
+  }
+
   public ReviewInput setWorkInProgress(boolean workInProgress) {
     this.workInProgress = workInProgress;
     ready = !workInProgress;

java/com/google/gerrit/server/api/changes/AttentionSetApiImpl.java

@@ -17,7 +17,7 @@ package com.google.gerrit.server.api.changes;
 import static com.google.gerrit.server.api.ApiUtil.asRestApiException;
 
 import com.google.gerrit.extensions.api.changes.AttentionSetApi;
-import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput;
+import com.google.gerrit.extensions.api.changes.AttentionSetInput;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.server.change.AttentionSetEntryResource;
 import com.google.gerrit.server.restapi.change.RemoveFromAttentionSet;
@@ -41,7 +41,7 @@ public class AttentionSetApiImpl implements AttentionSetApi {
   }
 
   @Override
-  public void remove(RemoveFromAttentionSetInput input) throws RestApiException {
+  public void remove(AttentionSetInput input) throws RestApiException {
     try {
       removeFromAttentionSet.apply(attentionSetEntryResource, input);
     } catch (Exception e) {

java/com/google/gerrit/server/change/AttentionSetUnchangedOp.java

@@ -0,0 +1,34 @@
+// 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.change;
+
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.ChangeContext;
+
+/**
+ * Ensures that the attention set will not be changed, thus blocks {@link RemoveFromAttentionSetOp}
+ * and {@link AddToAttentionSetOp}.
+ */
+public class AttentionSetUnchangedOp implements BatchUpdateOp {
+
+  @Override
+  public boolean updateChange(ChangeContext ctx) throws RestApiException {
+    ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
+    update.ignoreDefaultAttentionSetRules();
+    return true;
+  }
+}

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

@@ -135,6 +135,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   private String topic;
   private String commit;
   private Map<Account.Id, AttentionSetUpdate> plannedAttentionSetUpdates;
+  private boolean ignoreDefaultAttentionSetRules;
   private Optional<Account.Id> assignee;
   private Set<String> hashtags;
   private String changeMessage;
@@ -408,7 +409,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         .forEach(u -> plannedAttentionSetUpdates.putIfAbsent(u.account(), u));
   }
 
+  /**
+   * If we need to ignore default attention set rules, no need to add any new updates in this class.
+   */
   public void addToPlannedAttentionSetUpdates(AttentionSetUpdate update) {
+    if (ignoreDefaultAttentionSetRules) {
+      return;
+    }
     addToPlannedAttentionSetUpdates(ImmutableSet.of(update));
   }
 
@@ -822,6 +829,14 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     }
   }
 
+  /**
+   * When set, default attention set rules are ignored (E.g, adding reviewers -> adds to attention
+   * set, etc).
+   */
+  public void ignoreDefaultAttentionSetRules() {
+    ignoreDefaultAttentionSetRules = true;
+  }
+
   private void addPatchSetFooter(StringBuilder sb, int ps) {
     addFooter(sb, FOOTER_PATCH_SET).append(ps);
     if (psState != null) {

java/com/google/gerrit/server/restapi/change/PostReviewAttentionSet.java

@@ -27,6 +27,7 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.AddToAttentionSetOp;
+import com.google.gerrit.server.change.AttentionSetUnchangedOp;
 import com.google.gerrit.server.change.RemoveFromAttentionSetOp;
 import com.google.gerrit.server.change.ReviewerAdder;
 import com.google.gerrit.server.change.RevisionResource;
@@ -81,6 +82,14 @@ public class PostReviewAttentionSet {
       throws BadRequestException, IOException, PermissionBackendException,
           UnprocessableEntityException, ConfigInvalidException {
     processManualUpdates(bu, revision, input);
+    if (input.ignoreDefaultAttentionSetRules) {
+
+      // If We ignore default attention set rules it means we need to pass this information to
+      // ChangeUpdate. Also, we should stop all other attention set update that are part of
+      // this method (that happen in PostReview.
+      bu.addOp(revision.getChange().getId(), new AttentionSetUnchangedOp());
+      return;
+    }
     processRules(bu, revision, input, reviewerResults);
   }
 

java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java

@@ -15,11 +15,13 @@
 package com.google.gerrit.server.restapi.change;
 
 import com.google.common.base.Strings;
-import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.extensions.api.changes.AttentionSetInput;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.AttentionSetEntryResource;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.RemoveFromAttentionSetOp;
@@ -33,20 +35,24 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 
 /** Removes a single user from the attention set. */
 public class RemoveFromAttentionSet
-    implements RestModifyView<AttentionSetEntryResource, RemoveFromAttentionSetInput> {
+    implements RestModifyView<AttentionSetEntryResource, AttentionSetInput> {
   private final BatchUpdate.Factory updateFactory;
   private final RemoveFromAttentionSetOp.Factory opFactory;
+  private final AccountResolver accountResolver;
 
   @Inject
   RemoveFromAttentionSet(
-      BatchUpdate.Factory updateFactory, RemoveFromAttentionSetOp.Factory opFactory) {
+      BatchUpdate.Factory updateFactory,
+      RemoveFromAttentionSetOp.Factory opFactory,
+      AccountResolver accountResolver) {
     this.updateFactory = updateFactory;
     this.opFactory = opFactory;
+    this.accountResolver = accountResolver;
   }
 
   @Override
   public Response<Object> apply(
-      AttentionSetEntryResource attentionResource, RemoveFromAttentionSetInput input)
+      AttentionSetEntryResource attentionResource, AttentionSetInput input)
       throws RestApiException, PermissionBackendException, IOException, ConfigInvalidException,
           UpdateException {
     if (input == null) {
@@ -56,6 +62,20 @@ public class RemoveFromAttentionSet
     if (input.reason.isEmpty()) {
       throw new BadRequestException("missing field: reason");
     }
+    input.user = Strings.nullToEmpty(input.user).trim();
+    if (!input.user.isEmpty()) {
+      Account.Id attentionUserId = null;
+      try {
+        attentionUserId = accountResolver.resolve(input.user).asUnique().account().id();
+      } catch (AccountResolver.UnresolvableAccountException ex) {
+        throw new BadRequestException(
+            "The user specified in the input body couldn't be found.", ex);
+      }
+      if (attentionUserId.get() != attentionResource.getAccountId().get()) {
+        throw new BadRequestException(
+            "The field \"user\" must be empty, or must match the user specified in the URL.");
+      }
+    }
     ChangeResource changeResource = attentionResource.getChangeResource();
     try (BatchUpdate bu =
         updateFactory.create(

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

@@ -32,7 +32,6 @@ import com.google.gerrit.entities.AttentionSetUpdate;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.AttentionSetInput;
 import com.google.gerrit.extensions.api.changes.HashtagsInput;
-import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -134,7 +133,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
     PushOneCommit.Result r = createChange();
     change(r).addToAttentionSet(new AttentionSetInput(user.email(), "added"));
     fakeClock.advance(Duration.ofSeconds(42));
-    change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("removed"));
+    change(r).attention(user.id().toString()).remove(new AttentionSetInput("removed"));
     AttentionSetUpdate expectedAttentionSetUpdate =
         AttentionSetUpdate.createFromRead(
             fakeClock.now(), user.id(), AttentionSetUpdate.Operation.REMOVE, "removed");
@@ -142,12 +141,35 @@ public class AttentionSetIT extends AbstractDaemonTest {
 
     // Second removal is ignored.
     fakeClock.advance(Duration.ofSeconds(42));
-    change(r)
-        .attention(user.id().toString())
-        .remove(new RemoveFromAttentionSetInput("removed again"));
+    change(r).attention(user.id().toString()).remove(new AttentionSetInput("removed again"));
     assertThat(r.getChange().attentionSet()).containsExactly(expectedAttentionSetUpdate);
   }
 
+  @Test
+  public void removeUserWithInvalidUserInput() throws Exception {
+    PushOneCommit.Result r = createChange();
+    BadRequestException exception =
+        assertThrows(
+            BadRequestException.class,
+            () ->
+                change(r)
+                    .attention(user.id().toString())
+                    .remove(new AttentionSetInput("invalid user", "reason")));
+    assertThat(exception.getMessage())
+        .isEqualTo("The user specified in the input body couldn't be found.");
+
+    exception =
+        assertThrows(
+            BadRequestException.class,
+            () ->
+                change(r)
+                    .attention(user.id().toString())
+                    .remove(new AttentionSetInput(admin.email(), "reason")));
+    assertThat(exception.getMessage())
+        .isEqualTo(
+            "The field \"user\" must be empty, or must match the user specified in the URL.");
+  }
+
   @Test
   public void changeMessageWhenAddedAndRemovedExplicitly() throws Exception {
     PushOneCommit.Result r = createChange();
@@ -155,7 +177,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
     assertThat(Iterables.getLast(r.getChange().messages()).getMessage())
         .contains("Added to attention set");
 
-    change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("foo"));
+    change(r).attention(user.id().toString()).remove(new AttentionSetInput("foo"));
     assertThat(Iterables.getLast(r.getChange().messages()).getMessage())
         .contains("Removed from attention set");
   }
@@ -163,7 +185,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
   @Test
   public void removeUnrelatedUser() throws Exception {
     PushOneCommit.Result r = createChange();
-    change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("foo"));
+    change(r).attention(user.id().toString()).remove(new AttentionSetInput("foo"));
     assertThat(r.getChange().attentionSet()).isEmpty();
   }
 
@@ -315,8 +337,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
     change(r).addReviewer(user.id().toString());
     change(r)
         .attention(user.id().toString())
-        .remove(
-            new RemoveFromAttentionSetInput("removed and not re-added when re-adding as reviewer"));
+        .remove(new AttentionSetInput("removed and not re-added when re-adding as reviewer"));
 
     change(r).addReviewer(user.id().toString());
 
@@ -408,7 +429,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
   public void reviewersAreNotAddedForNoReasonBecauseOfAnUpdate() throws Exception {
     PushOneCommit.Result r = createChange();
     change(r).addToAttentionSet(new AttentionSetInput(user.email(), "user"));
-    change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("removed"));
+    change(r).attention(user.id().toString()).remove(new AttentionSetInput("removed"));
 
     HashtagsInput hashtagsInput = new HashtagsInput();
     hashtagsInput.add = ImmutableSet.of("tag");
@@ -571,7 +592,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
     ReviewInput reviewInput =
         ReviewInput.create().setWorkInProgress(true).addUserToAttentionSet(user.email(), "reason");
 
-    change(r).attention(user.email()).remove(new RemoveFromAttentionSetInput("removal"));
+    change(r).attention(user.email()).remove(new AttentionSetInput("removal"));
     change(r).current().review(reviewInput);
 
     // Attention set updates that relate to the admin (the person who replied) are filtered out.
@@ -715,7 +736,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
     TestAccount user2 = accountCreator.user2();
     requestScopeOperations.setApiUser(user2.id());
 
-    change(r).attention(user.email()).remove(new RemoveFromAttentionSetInput("reason"));
+    change(r).attention(user.email()).remove(new AttentionSetInput("reason"));
     ReviewInput reviewInput = new ReviewInput();
     change(r).current().review(reviewInput);
 
@@ -743,7 +764,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
     change(r).addReviewer(user.email());
     change(r)
         .attention(user.email())
-        .remove(new RemoveFromAttentionSetInput("Reviewer is not in attention-set"));
+        .remove(new AttentionSetInput("Reviewer is not in attention-set"));
 
     TestAccount cc = accountCreator.admin2();
     AddReviewerInput input = new AddReviewerInput();
@@ -809,7 +830,7 @@ public class AttentionSetIT extends AbstractDaemonTest {
     change(r).addReviewer(input);
 
     requestScopeOperations.setApiUser(user.id());
-    change(r).attention(reviewer.email()).remove(new RemoveFromAttentionSetInput("reason"));
+    change(r).attention(reviewer.email()).remove(new AttentionSetInput("reason"));
 
     ReviewInput reviewInput = new ReviewInput();
     change(r).current().review(reviewInput);
@@ -848,6 +869,45 @@ public class AttentionSetIT extends AbstractDaemonTest {
     assertThat(attentionSet.reason()).isEqualTo("removed on reply");
   }
 
+  @Test
+  public void attentionSetUnchangedWithIgnoreDefaultAttentionSetRules() throws Exception {
+    PushOneCommit.Result r = createChange();
+    change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "reason"));
+    change(r)
+        .current()
+        .review(
+            ReviewInput.create()
+                .reviewer(admin.email(), ReviewerState.CC, false)
+                .blockDefaultAttentionSetRules());
+
+    // admin is still in the attention set, although replies remove from attention set, and removing
+    // from reviewer also should remove from attention set.
+    AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
+    assertThat(attentionSet.account()).isEqualTo(admin.id());
+    assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.ADD);
+    assertThat(attentionSet.reason()).isEqualTo("reason");
+  }
+
+  @Test
+  public void attentionSetStillChangesWithIgnoreDefaultAttentionSetRulesWithInputList()
+      throws Exception {
+    PushOneCommit.Result r = createChange();
+    change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "reason"));
+    change(r)
+        .current()
+        .review(
+            ReviewInput.create()
+                .removeUserFromAttentionSet(admin.email(), "removed")
+                .blockDefaultAttentionSetRules());
+
+    // Admin is still removed although we block default attention set rules, since we remove
+    // the admin manually.
+    AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
+    assertThat(attentionSet.account()).isEqualTo(admin.id());
+    assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+    assertThat(attentionSet.reason()).isEqualTo("removed");
+  }
+
   private List<AttentionSetUpdate> getAttentionSetUpdatesForUser(
       PushOneCommit.Result r, TestAccount account) {
     return r.getChange().attentionSet().stream()