From e2d854a9bd52cfdb5798a0bfa418c84478483245 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 9 Mar 2017 08:58:03 +0100 Subject: [PATCH] Fix ignoring change when project is watched Users that ignored a change of a project that they watch still received email notifications for the ignored change. Change-Id: Ie6c295f7315b99e8370619ecdd33330f69ac0c0a Signed-off-by: Edwin Kempin --- .../server/project/ProjectWatchIT.java | 37 +++++++++++++++++ .../server/mail/send/AbandonedSender.java | 1 + .../gerrit/server/mail/send/ChangeEmail.java | 40 +++++++++---------- .../server/mail/send/CommentSender.java | 1 + .../mail/send/DeleteReviewerSender.java | 1 + .../server/mail/send/DeleteVoteSender.java | 1 + .../gerrit/server/mail/send/MergedSender.java | 1 + .../mail/send/ReplacePatchSetSender.java | 1 + .../server/mail/send/RestoredSender.java | 1 + .../server/mail/send/RevertedSender.java | 1 + 10 files changed, 65 insertions(+), 20 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java index 77d3c551a4..ab543e8b83 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java @@ -15,13 +15,17 @@ package com.google.gerrit.acceptance.server.project; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.server.StarredChangesUtil.IGNORE_LABEL; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.common.data.Permission; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.api.changes.StarsInput; import com.google.gerrit.extensions.common.GroupInfo; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; @@ -443,4 +447,37 @@ public class ProjectWatchIT extends AbstractDaemonTest { assertThat(m.body()).contains("Change subject: TRIGGER\n"); assertThat(m.body()).contains("Gerrit-PatchSet: 1\n"); } + + @Test + public void watchProjectNoNotificationForIgnoredChange() throws Exception { + // watch project + String watchedProject = createProject("watchedProject").get(); + setApiUser(user); + watch(watchedProject, null); + + // push a change to watched project + setApiUser(admin); + TestRepository watchedRepo = + cloneProject(new Project.NameKey(watchedProject), admin); + PushOneCommit.Result r = + pushFactory + .create(db, admin.getIdent(), watchedRepo, "ignored change", "a", "a1") + .to("refs/for/master"); + r.assertOkStatus(); + + // ignore the change + setApiUser(user); + gApi.accounts().self().setStars(r.getChangeId(), new StarsInput(ImmutableSet.of(IGNORE_LABEL))); + + sender.clear(); + + // post a comment -> should not trigger email notification since user ignored the change + setApiUser(admin); + ReviewInput in = new ReviewInput(); + in.message = "comment"; + gApi.changes().id(r.getChangeId()).current().review(in); + + // assert email notification + assertThat(sender.getMessages()).isEmpty(); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/AbandonedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/AbandonedSender.java index 17aee723e9..ec62833472 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/AbandonedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/AbandonedSender.java @@ -43,6 +43,7 @@ public class AbandonedSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); includeWatchers(NotifyType.ABANDONED_CHANGES); + removeUsersThatIgnoredTheChange(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 461029a999..bc09488e1d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -69,6 +69,7 @@ public abstract class ChangeEmail extends NotificationEmail { protected final Change change; protected final ChangeData changeData; + protected ListMultimap stars; protected PatchSet patchSet; protected PatchSetInfo patchSetInfo; protected String changeMessage; @@ -164,6 +165,12 @@ public abstract class ChangeEmail extends NotificationEmail { } authors = getAuthors(); + try { + stars = args.starredChangesUtil.byChangeFromIndex(change.getId()); + } catch (OrmException e) { + throw new EmailException("Failed to load stars for change " + change.getChangeId(), e); + } + super.init(); if (timestamp != null) { setHeader("Date", new Date(timestamp.getTime())); @@ -309,28 +316,21 @@ public abstract class ChangeEmail extends NotificationEmail { return; } - try { - // BCC anyone who has starred this change - // and remove anyone who has ignored this change. - // - ListMultimap stars = - args.starredChangesUtil.byChangeFromIndex(change.getId()); - for (Map.Entry> 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()); - } + for (Map.Entry> e : stars.asMap().entrySet()) { + if (e.getValue().contains(StarredChangesUtil.DEFAULT_LABEL)) { + super.add(RecipientType.BCC, e.getKey()); + } + } + } + + protected void removeUsersThatIgnoredTheChange() { + for (Map.Entry> e : stars.asMap().entrySet()) { + if (e.getValue().contains(StarredChangesUtil.IGNORE_LABEL)) { + AccountState accountState = args.accountCache.get(e.getKey()); + if (accountState != null) { + removeUser(accountState.getAccount()); } } - } catch (OrmException err) { - // Just don't BCC everyone. Better to send a partial message to those - // we already have queued up then to fail deliver entirely to people - // who have a lower interest in the change. - log.warn("Cannot BCC users that starred updated change", err); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java index 02160ff753..63d7e789d9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java @@ -157,6 +157,7 @@ public class CommentSender extends ReplyToChangeSender { bccStarredBy(); includeWatchers(NotifyType.ALL_COMMENTS); } + removeUsersThatIgnoredTheChange(); // Add header that enables identifying comments on parsed email. // Grouping is currently done by timestamp. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java index f46eab6cb5..a563846e94 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteReviewerSender.java @@ -58,6 +58,7 @@ public class DeleteReviewerSender extends ReplyToChangeSender { ccExistingReviewers(); includeWatchers(NotifyType.ALL_COMMENTS); add(RecipientType.TO, reviewers); + removeUsersThatIgnoredTheChange(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteVoteSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteVoteSender.java index 13ee4acb15..8509f73528 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteVoteSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/DeleteVoteSender.java @@ -43,6 +43,7 @@ public class DeleteVoteSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); includeWatchers(NotifyType.ALL_COMMENTS); + removeUsersThatIgnoredTheChange(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java index 0460a70361..47115afd3d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java @@ -52,6 +52,7 @@ public class MergedSender extends ReplyToChangeSender { bccStarredBy(); includeWatchers(NotifyType.ALL_COMMENTS); includeWatchers(NotifyType.SUBMITTED_CHANGES); + removeUsersThatIgnoredTheChange(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java index 27cf2a44c3..c90000f6d5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java @@ -67,6 +67,7 @@ public class ReplacePatchSetSender extends ReplyToChangeSender { rcptToAuthors(RecipientType.CC); bccStarredBy(); includeWatchers(NotifyType.NEW_PATCHSETS, !patchSet.isDraft()); + removeUsersThatIgnoredTheChange(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RestoredSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RestoredSender.java index 7456d06fd8..6076b46afc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RestoredSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RestoredSender.java @@ -43,6 +43,7 @@ public class RestoredSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); includeWatchers(NotifyType.ALL_COMMENTS); + removeUsersThatIgnoredTheChange(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RevertedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RevertedSender.java index e590d94762..c4c0a69ebf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RevertedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/RevertedSender.java @@ -42,6 +42,7 @@ public class RevertedSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); includeWatchers(NotifyType.ALL_COMMENTS); + removeUsersThatIgnoredTheChange(); } @Override