diff options
author | David Åkerman <davidak@axis.com> | 2022-01-17 16:26:19 +0100 |
---|---|---|
committer | David Åkerman <davidak@axis.com> | 2022-03-15 11:51:23 +0100 |
commit | 03ba8b11c9f0d5c4a87a8bf58fb30e4270e37a54 (patch) | |
tree | 69cee60ddcb4597dfd0cd682bdd196530f442fde | |
parent | 5fff60a37742db5026b6eae9fe52b4c5dc5287ba (diff) |
Separate reviewer-recipients from project-watch-recipients
To ensure that project-watch emails are sent regardless of other
notfication configurations. Without this fix you will not get any
email notifications for watched projects when you have set
"Only when I am in the Attention-Set" and you are outside of
the attention-set.
Release-Notes: Fixed lost watch notifications when attention-set-only is configured
Bug: Issue 14912
Change-Id: I0707c588f23f13c617cdfb565171adff12833ea9
4 files changed, 45 insertions, 20 deletions
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 8d76e23211..7890d35961 100644 --- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -397,14 +397,24 @@ public abstract class ChangeEmail extends NotificationEmail { @Override protected void add(RecipientType rt, Account.Id to) { - Optional<AccountState> accountState = args.accountCache.get(to); - if (!accountState.isPresent()) { - return; - } - if (accountState.get().generalPreferences().getEmailStrategy() - == EmailStrategy.ATTENTION_SET_ONLY - && !currentAttentionSet.contains(to)) { - return; + addRecipient(rt, to, /* isWatcher= */ false); + } + + /** This bypasses the EmailStrategy.ATTENTION_SET_ONLY strategy when adding the recipient. */ + @Override + protected void addWatcher(RecipientType rt, Account.Id to) { + addRecipient(rt, to, /* isWatcher= */ true); + } + + private void addRecipient(RecipientType rt, Account.Id to, boolean isWatcher) { + if (!isWatcher) { + Optional<AccountState> accountState = args.accountCache.get(to); + if (accountState.isPresent() + && accountState.get().generalPreferences().getEmailStrategy() + == EmailStrategy.ATTENTION_SET_ONLY + && !currentAttentionSet.contains(to)) { + return; + } } if (emailOnlyAuthors && !authors.contains(to)) { return; diff --git a/java/com/google/gerrit/server/mail/send/CreateChangeSender.java b/java/com/google/gerrit/server/mail/send/CreateChangeSender.java index b78dc62ce7..2ab73a8484 100644 --- a/java/com/google/gerrit/server/mail/send/CreateChangeSender.java +++ b/java/com/google/gerrit/server/mail/send/CreateChangeSender.java @@ -60,7 +60,7 @@ public class CreateChangeSender extends NewChangeSender { // TODO(hiesel): Remove special handling for owners StreamSupport.stream(matching.all().accounts.spliterator(), false) .filter(this::isOwnerOfProjectOrBranch) - .forEach(acc -> add(RecipientType.TO, acc)); + .forEach(acc -> addWatcher(RecipientType.TO, acc)); // Add everyone else. Owners added above will not be duplicated. add(RecipientType.TO, matching.to); add(RecipientType.CC, matching.cc); diff --git a/java/com/google/gerrit/server/mail/send/NotificationEmail.java b/java/com/google/gerrit/server/mail/send/NotificationEmail.java index 5ffd9284b7..91310ce094 100644 --- a/java/com/google/gerrit/server/mail/send/NotificationEmail.java +++ b/java/com/google/gerrit/server/mail/send/NotificationEmail.java @@ -82,13 +82,15 @@ public abstract class NotificationEmail extends OutgoingEmail { /** Add users or email addresses to the TO, CC, or BCC list. */ protected void add(RecipientType type, Watchers.List list) { for (Account.Id user : list.accounts) { - add(type, user); + addWatcher(type, user); } for (Address addr : list.emails) { add(type, addr); } } + protected abstract void addWatcher(RecipientType type, Account.Id to); + public String getSshHost() { String host = Iterables.getFirst(args.sshAddresses, null); if (host == null) { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java index 61eef63b5f..5c5094908a 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java @@ -1438,11 +1438,7 @@ public class AttentionSetIT extends AbstractDaemonTest { // Add preference for the user such that they only receive an email on changes that require // their attention. - requestScopeOperations.setApiUser(user.id()); - GeneralPreferencesInfo prefs = gApi.accounts().self().getPreferences(); - prefs.emailStrategy = EmailStrategy.ATTENTION_SET_ONLY; - gApi.accounts().self().setPreferences(prefs); - requestScopeOperations.setApiUser(admin.id()); + setEmailStrategyForUser(EmailStrategy.ATTENTION_SET_ONLY); // Add user to attention set. They receive an email since they are in the attention set. change(r).addReviewer(user.id().toString()); @@ -1464,17 +1460,34 @@ public class AttentionSetIT extends AbstractDaemonTest { public void attentionSetWithEmailFilterImpactingOnlyChangeEmails() throws Exception { // Add preference for the user such that they only receive an email on changes that require // their attention. - requestScopeOperations.setApiUser(user.id()); - GeneralPreferencesInfo prefs = gApi.accounts().self().getPreferences(); - prefs.emailStrategy = EmailStrategy.ATTENTION_SET_ONLY; - gApi.accounts().self().setPreferences(prefs); - requestScopeOperations.setApiUser(admin.id()); + setEmailStrategyForUser(EmailStrategy.ATTENTION_SET_ONLY); // Ensure emails that don't relate to changes are still sent. gApi.accounts().id(user.id().get()).generateHttpPassword(); assertThat(sender.getMessages()).isNotEmpty(); } + @Test + public void outsideAttentionSet_watchProjectEmailReceived() throws Exception { + setEmailStrategyForUser(EmailStrategy.ATTENTION_SET_ONLY); + + requestScopeOperations.setApiUser(user.id()); + watch(project.get()); + + createChange(); + + assertThat(sender.getMessages()).isNotEmpty(); + sender.clear(); + } + + private void setEmailStrategyForUser(EmailStrategy es) throws Exception { + requestScopeOperations.setApiUser(user.id()); + GeneralPreferencesInfo prefs = gApi.accounts().self().getPreferences(); + prefs.emailStrategy = es; + gApi.accounts().self().setPreferences(prefs); + requestScopeOperations.setApiUser(admin.id()); + } + private List<AttentionSetUpdate> getAttentionSetUpdatesForUser( PushOneCommit.Result r, TestAccount account) { return getAttentionSetUpdates(r.getChange().getId()).stream() |