Merge changes from topic 'star-labels-part-2'
* changes:
Support ignore label that suppresses notifications on update
Include star labels into ChangeInfo
diff --git a/Documentation/dev-stars.txt b/Documentation/dev-stars.txt
index 1a98438..d9fd10c 100644
--- a/Documentation/dev-stars.txt
+++ b/Documentation/dev-stars.txt
@@ -43,6 +43,24 @@
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
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 53a47a3..e350cc9 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/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.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.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.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 @@
.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 @@
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 @@
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 @@
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);
@@ -240,6 +251,70 @@
}
@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";
List<AccountInfo> result = gApi.accounts()
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
index 34b19c2..f2d2634 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -36,6 +36,7 @@
public Timestamp updated;
public Timestamp submitted;
public Boolean starred;
+ public Collection<String> stars;
public Boolean reviewed;
public SubmitType submitType;
public Boolean mergeable;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java
index f53ecf8..62c14cb 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/AccountDashboardScreen.java
@@ -95,7 +95,7 @@
}
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) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java
index 77064fe..5e5e43c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -65,11 +65,8 @@
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 @@
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 @@
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 @@
if (labels.isEmpty()) {
deleteRef(repo, refName, oldObjectId);
} else {
+ checkMutuallyExclusiveLabels(labels);
updateLabels(repo, refName, oldObjectId, labels);
}
@@ -289,18 +295,6 @@
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 @@
}
}
+ 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;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index f93bb72..0c2366f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -89,6 +89,7 @@
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 @@
return info;
}
- @SuppressWarnings("deprecation")
private ChangeInfo toChangeInfo(ChangeData cd,
Optional<PatchSet.Id> limitToPsId) throws PatchListNotAvailableException,
GpgException, OrmException, IOException {
@@ -421,9 +421,17 @@
out.created = in.getCreatedOn();
out.updated = in.getLastUpdatedOn();
out._number = in.getId().get();
- out.starred = user.getStarredChanges().contains(in.getId())
- ? true
- : null;
+
+ 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;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index db889c2..182267e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -71,9 +71,16 @@
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();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
index ad543ea..0684d8f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
@@ -16,6 +16,7 @@
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.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 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 @@
}
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
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
index 4834efd..04085b6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java
@@ -486,7 +486,7 @@
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)) {