Merge changes from topic 'star-labels-part-2'

* changes:
  Support ignore label that suppresses notifications on update
  Include star labels into ChangeInfo

Documentation/dev-stars.txt

@@ -43,6 +43,24 @@ be updated from there.
 
 The default star is represented by the special star label 'star'.
 
+[[ignore-star]]
+== Ignore Star
+
+If the ignore star is set by a user, this user gets no email
+notifications for updates of that change, even if this user is a
+reviewer of the change or the change is matched by a project watch of
+the user.
+
+Since changes can only be ignored once they are created, users that
+watch a project will always get the email notifications for the change
+creation. Only then the change can be ignored.
+
+Users that are added as reviewer to a change that they have ignored
+will be notified about this, so that they know about the review
+request. They can the decide to remove the ignore star.
+
+The ignore star is represented by the special star label 'ignore'.
+
 [[query-stars]]
 == Query Stars
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -24,6 +24,7 @@ import static com.google.gerrit.gpg.testutil.TestKeys.validKeyWithExpiration;
 import static com.google.gerrit.gpg.testutil.TestKeys.validKeyWithSecondUserId;
 import static com.google.gerrit.gpg.testutil.TestKeys.validKeyWithoutExpiration;
 import static com.google.gerrit.server.StarredChangesUtil.DEFAULT_LABEL;
+import static com.google.gerrit.server.StarredChangesUtil.IGNORE_LABEL;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.base.Function;
@@ -36,6 +37,7 @@ import com.google.gerrit.acceptance.AccountCreator;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.extensions.api.accounts.EmailInput;
+import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.StarsInput;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
@@ -53,6 +55,7 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountExternalId;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.testutil.ConfigSuite;
+import com.google.gerrit.testutil.FakeEmailSender.Message;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
@@ -180,12 +183,14 @@ public class AccountIT extends AbstractDaemonTest {
         .starChange(triplet);
     ChangeInfo change = info(triplet);
     assertThat(change.starred).isTrue();
+    assertThat(change.stars).contains(DEFAULT_LABEL);
 
     gApi.accounts()
         .self()
         .unstarChange(triplet);
     change = info(triplet);
     assertThat(change.starred).isNull();
+    assertThat(change.stars).isNull();
   }
 
   @Test
@@ -199,6 +204,8 @@ public class AccountIT extends AbstractDaemonTest {
         new StarsInput(ImmutableSet.of(DEFAULT_LABEL, "red", "blue")));
     ChangeInfo change = info(triplet);
     assertThat(change.starred).isTrue();
+    assertThat(change.stars)
+        .containsExactly("blue", "red", DEFAULT_LABEL).inOrder();
     assertThat(gApi.accounts().self().getStars(triplet))
         .containsExactly("blue", "red", DEFAULT_LABEL).inOrder();
     List<ChangeInfo> starredChanges =
@@ -207,12 +214,15 @@ public class AccountIT extends AbstractDaemonTest {
     ChangeInfo starredChange = starredChanges.get(0);
     assertThat(starredChange._number).isEqualTo(r.getChange().getId().get());
     assertThat(starredChange.starred).isTrue();
+    assertThat(starredChange.stars)
+        .containsExactly("blue", "red", DEFAULT_LABEL).inOrder();
 
     gApi.accounts().self().setStars(triplet,
         new StarsInput(ImmutableSet.of("yellow"),
             ImmutableSet.of(DEFAULT_LABEL, "blue")));
     change = info(triplet);
     assertThat(change.starred).isNull();
+    assertThat(change.stars).containsExactly("red", "yellow").inOrder();
     assertThat(gApi.accounts().self().getStars(triplet)).containsExactly(
         "red", "yellow").inOrder();
     starredChanges = gApi.accounts().self().getStarredChanges();
@@ -220,6 +230,7 @@ public class AccountIT extends AbstractDaemonTest {
     starredChange = starredChanges.get(0);
     assertThat(starredChange._number).isEqualTo(r.getChange().getId().get());
     assertThat(starredChange.starred).isNull();
+    assertThat(starredChange.stars).containsExactly("red", "yellow").inOrder();
 
     setApiUser(user);
     exception.expect(AuthException.class);
@@ -239,6 +250,70 @@ public class AccountIT extends AbstractDaemonTest {
             "another invalid label")));
   }
 
+  @Test
+  public void starWithDefaultAndIgnoreLabel() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String triplet = project.get() + "~master~" + r.getChangeId();
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("The labels " + DEFAULT_LABEL
+        + " and " + IGNORE_LABEL + " are mutually exclusive."
+        + " Only one of them can be set.");
+    gApi.accounts().self().setStars(triplet,
+        new StarsInput(ImmutableSet.of(DEFAULT_LABEL, "blue", IGNORE_LABEL)));
+  }
+
+  @Test
+  public void ignoreChange() throws Exception {
+    PushOneCommit.Result r = createChange();
+
+    AddReviewerInput in = new AddReviewerInput();
+    in.reviewer = user.email;
+    gApi.changes()
+        .id(r.getChangeId())
+        .addReviewer(in);
+
+    TestAccount user2 = accounts.user2();
+    in = new AddReviewerInput();
+    in.reviewer = user2.email;
+    gApi.changes()
+        .id(r.getChangeId())
+        .addReviewer(in);
+
+    setApiUser(user);
+    gApi.accounts().self().setStars(r.getChangeId(),
+        new StarsInput(ImmutableSet.of(IGNORE_LABEL)));
+
+    sender.clear();
+    setApiUser(admin);
+    gApi.changes()
+        .id(r.getChangeId())
+        .abandon();
+    List<Message> messages = sender.getMessages();
+    assertThat(messages).hasSize(1);
+    assertThat(messages.get(0).rcpt()).containsExactly(user2.emailAddress);
+  }
+
+  @Test
+  public void addReviewerToIgnoredChange() throws Exception {
+    PushOneCommit.Result r = createChange();
+
+    setApiUser(user);
+    gApi.accounts().self().setStars(r.getChangeId(),
+        new StarsInput(ImmutableSet.of(IGNORE_LABEL)));
+
+    sender.clear();
+    setApiUser(admin);
+
+    AddReviewerInput in = new AddReviewerInput();
+    in.reviewer = user.email;
+    gApi.changes()
+        .id(r.getChangeId())
+        .addReviewer(in);
+    List<Message> messages = sender.getMessages();
+    assertThat(messages).hasSize(1);
+    assertThat(messages.get(0).rcpt()).containsExactly(user.emailAddress);
+  }
+
   @Test
   public void suggestAccounts() throws Exception {
     String adminUsername = "admin";

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java

@@ -36,6 +36,7 @@ public class ChangeInfo {
   public Timestamp updated;
   public Timestamp submitted;
   public Boolean starred;
+  public Collection<String> stars;
   public Boolean reviewed;
   public SubmitType submitType;
   public Boolean mergeable;

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java

@@ -95,7 +95,7 @@ public class AccountDashboardScreen extends Screen implements ChangeListScreen {
   }
 
   private static String queryIncoming(String who) {
-    return "is:open reviewer:" + who + " -owner:" + who;
+    return "is:open reviewer:" + who + " -owner:" + who + " -star:ignore";
   }
 
   private static String queryClosed(String who) {

gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java

@@ -65,11 +65,8 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
-import java.util.Collection;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
@@ -116,6 +113,13 @@ public class StarredChangesUtil {
               Joiner.on(", ").join(invalidLabels)));
     }
 
+    static IllegalLabelException mutuallyExclusiveLabels(String label1,
+        String label2) {
+      return new IllegalLabelException(
+          String.format("The labels %s and %s are mutually exclusive."
+              + " Only one of them can be set.", label1, label2));
+    }
+
     IllegalLabelException(String message) {
       super(message);
     }
@@ -125,6 +129,7 @@ public class StarredChangesUtil {
       LoggerFactory.getLogger(StarredChangesUtil.class);
 
   public static final String DEFAULT_LABEL = "star";
+  public static final String IGNORE_LABEL = "ignore";
   public static final ImmutableSortedSet<String> DEFAULT_LABELS =
       ImmutableSortedSet.of(DEFAULT_LABEL);
 
@@ -180,6 +185,7 @@ public class StarredChangesUtil {
       if (labels.isEmpty()) {
         deleteRef(repo, refName, oldObjectId);
       } else {
+        checkMutuallyExclusiveLabels(labels);
         updateLabels(repo, refName, oldObjectId, labels);
       }
 
@@ -289,18 +295,6 @@ public class StarredChangesUtil {
     return changeData.get(0).stars();
   }
 
-  public Set<Account.Id> byChangeFromIndex(Change.Id changeId, String label)
-      throws OrmException, NoSuchChangeException {
-    Set<Account.Id> accounts = new HashSet<>();
-    for (Map.Entry<Account.Id, Collection<String>> e : byChangeFromIndex(
-        changeId).asMap().entrySet()) {
-      if (e.getValue().contains(label)) {
-        accounts.add(e.getKey());
-      }
-    }
-    return accounts;
-  }
-
   @Deprecated
   public ResultSet<Change.Id> queryFromIndex(final Account.Id accountId) {
     try {
@@ -380,6 +374,13 @@ public class StarredChangesUtil {
     }
   }
 
+  private static void checkMutuallyExclusiveLabels(Set<String> labels) {
+    if (labels.containsAll(ImmutableSet.of(DEFAULT_LABEL, IGNORE_LABEL))) {
+      throw IllegalLabelException.mutuallyExclusiveLabels(DEFAULT_LABEL,
+          IGNORE_LABEL);
+    }
+  }
+
   private static void validateLabels(Set<String> labels) {
     if (labels == null) {
       return;

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -89,6 +89,7 @@ import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GpgException;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.StarredChangesUtil;
 import com.google.gerrit.server.WebLinks;
 import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.api.accounts.AccountInfoComparator;
@@ -377,7 +378,6 @@ public class ChangeJson {
     return info;
   }
 
-  @SuppressWarnings("deprecation")
   private ChangeInfo toChangeInfo(ChangeData cd,
       Optional<PatchSet.Id> limitToPsId) throws PatchListNotAvailableException,
       GpgException, OrmException, IOException {
@@ -421,9 +421,17 @@ public class ChangeJson {
     out.created = in.getCreatedOn();
     out.updated = in.getLastUpdatedOn();
     out._number = in.getId().get();
-    out.starred = user.getStarredChanges().contains(in.getId())
+
+    if (user.isIdentifiedUser()) {
+      Collection<String> stars = cd.stars().get(user.getAccountId());
+      out.starred = stars.contains(StarredChangesUtil.DEFAULT_LABEL)
           ? true
           : null;
+      if (!stars.isEmpty()) {
+        out.stars = stars;
+      }
+    }
+
     if (in.getStatus().isOpen() && has(REVIEWED) && user.isIdentifiedUser()) {
       Account.Id accountId = user.getAccountId();
       out.reviewed = cd.reviewedBy().contains(accountId) ? true : null;

gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java

@@ -71,9 +71,16 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions<ChangeData> {
   static final Schema<ChangeData> V29 =
       schema(V28, ChangeField.HASHTAG_CASE_AWARE);
 
+  @Deprecated
   static final Schema<ChangeData> V30 =
       schema(V29, ChangeField.STAR, ChangeField.STARBY);
 
+  @SuppressWarnings("deprecation")
+  static final Schema<ChangeData> V31 = new Schema.Builder<ChangeData>()
+      .add(V30)
+      .remove(ChangeField.STARREDBY)
+      .build();
+
   public static final String NAME = "changes";
   public static final ChangeSchemaDefinitions INSTANCE =
       new ChangeSchemaDefinitions();

gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.mail;
 
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 
+import com.google.common.collect.Multimap;
 import com.google.gerrit.common.errors.EmailException;
 import com.google.gerrit.extensions.api.changes.ReviewInput.NotifyHandling;
 import com.google.gerrit.reviewdb.client.Account;
@@ -29,6 +30,7 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.mail.ProjectWatch.Watchers;
 import com.google.gerrit.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListEntry;
@@ -49,9 +51,11 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.text.MessageFormat;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashSet;
+import java.util.Map;
 import java.util.Set;
 import java.util.TreeSet;
 
@@ -304,11 +308,22 @@ public abstract class ChangeEmail extends NotificationEmail {
     }
 
     try {
-      // BCC anyone who has starred this change.
+      // BCC anyone who has starred this change
+      // and remove anyone who has ignored this change.
       //
-      for (Account.Id accountId : args.starredChangesUtil.byChangeFromIndex(
-          change.getId(), StarredChangesUtil.DEFAULT_LABEL)) {
-        super.add(RecipientType.BCC, accountId);
+      Multimap<Account.Id, String> stars =
+          args.starredChangesUtil.byChangeFromIndex(change.getId());
+      for (Map.Entry<Account.Id, Collection<String>> e :
+          stars.asMap().entrySet()) {
+        if (e.getValue().contains(StarredChangesUtil.DEFAULT_LABEL)) {
+          super.add(RecipientType.BCC, e.getKey());
+        }
+        if (e.getValue().contains(StarredChangesUtil.IGNORE_LABEL)) {
+          AccountState accountState = args.accountCache.get(e.getKey());
+          if (accountState != null) {
+            removeUser(accountState.getAccount());
+          }
+        }
       }
     } catch (OrmException | NoSuchChangeException err) {
       // Just don't BCC everyone. Better to send a partial message to those

gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java

@@ -486,7 +486,7 @@ public abstract class OutgoingEmail {
     return r.toString();
   }
 
-  private void removeUser(Account user) {
+  protected void removeUser(Account user) {
     String fromEmail = user.getPreferredEmail();
     for (Iterator<Address> j = smtpRcptTo.iterator(); j.hasNext();) {
       if (j.next().email.equals(fromEmail)) {