commit fd238889b33ce8b00c43980086e31e1eae6d4d8b Author: Gal Paikin Date: Fri Oct 9 18:40:52 2020 +0200 Allow user preference to receive emails only when in attention set This allows specifying such a user preference that would apply too all change emails. It wouldn't apply to other emails that don't relate to changes. Specifically, if the preference is configured, and the user is not in the attention set, they will not receive an email regarding that change. Change-Id: Ib6529a710f3d2853ef03090e7d7454676e140e92 diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index 32f8656..2a59d0c 100644 --- a/Documentation/rest-api-accounts.txt +++ b/Documentation/rest-api-accounts.txt @@ -2807,9 +2807,11 @@ empty, which will default columns as determined by the frontend. |`email_strategy` || The type of email strategy to use. On `ENABLED`, the user will receive emails from Gerrit. On `CC_ON_OWN_COMMENTS` the user will also receive emails for -their own comments. On `DISABLED` the user will not receive any email -notifications from Gerrit. -Allowed values are `ENABLED`, `CC_ON_OWN_COMMENTS`, `DISABLED`. +their own comments. On `ATTENTION_SET_ONLY`, on emails about changes, the user +will receive emails only if they are in the attention set of that change. +On `DISABLED` the user will not receive any email notifications from Gerrit. +Allowed values are `ENABLED`, `CC_ON_OWN_COMMENTS`, `ATTENTION_SET_ONLY`, +`DISABLED`. |`default_base_for_merges` || The base which should be pre-selected in the 'Diff Against' drop-down list when the change screen is opened for a merge commit. @@ -2870,9 +2872,11 @@ empty, which will default columns as determined by the frontend. |`email_strategy` |optional| The type of email strategy to use. On `ENABLED`, the user will receive emails from Gerrit. On `CC_ON_OWN_COMMENTS` the user will also receive emails for -their own comments. On `DISABLED` the user will not receive any email -notifications from Gerrit. -Allowed values are `ENABLED`, `CC_ON_OWN_COMMENTS`, `DISABLED`. +their own comments. On `ATTENTION_SET_ONLY`, on emails about changes, the user +will receive emails only if they are in the attention set of that change. +On `DISABLED` the user will not receive any email notifications from Gerrit. +Allowed values are `ENABLED`, `CC_ON_OWN_COMMENTS`, `ATTENTION_SET_ONLY`, +`DISABLED`. |`default_base_for_merges` |optional| The base which should be pre-selected in the 'Diff Against' drop-down list when the change screen is opened for a merge commit. diff --git a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java index c6555b9..30514a6 100644 --- a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java +++ b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java @@ -75,6 +75,7 @@ public class GeneralPreferencesInfo { public enum EmailStrategy { ENABLED, CC_ON_OWN_COMMENTS, + ATTENTION_SET_ONLY, DISABLED } diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 7b2bf12..6637f99 100644 --- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -33,9 +33,11 @@ import com.google.gerrit.exceptions.EmailException; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.RecipientType; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.mail.MailHeader; import com.google.gerrit.server.StarredChangesUtil; +import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.mail.send.ProjectWatch.Watchers; import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchList; @@ -56,6 +58,7 @@ import java.util.Date; import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.TreeSet; import java.util.stream.Collectors; @@ -75,6 +78,7 @@ public abstract class ChangeEmail extends NotificationEmail { return ea.changeDataFactory.create(project, id); } + private final Set currentAttentionSet; protected final Change change; protected final ChangeData changeData; protected ListMultimap stars; @@ -92,6 +96,7 @@ public abstract class ChangeEmail extends NotificationEmail { this.changeData = changeData; this.change = changeData.change(); this.emailOnlyAuthors = false; + this.currentAttentionSet = getAttentionSet(); } @Override @@ -387,9 +392,19 @@ public abstract class ChangeEmail extends NotificationEmail { @Override protected void add(RecipientType rt, Account.Id to) { - if (!emailOnlyAuthors || authors.contains(to)) { - super.add(rt, to); + Optional accountState = args.accountCache.get(to); + if (!accountState.isPresent()) { + return; + } + if (accountState.get().generalPreferences().getEmailStrategy() + == EmailStrategy.ATTENTION_SET_ONLY + && !currentAttentionSet.contains(to)) { + return; + } + if (emailOnlyAuthors && !authors.contains(to)) { + return; } + super.add(rt, to); } @Override @@ -487,8 +502,8 @@ public abstract class ChangeEmail extends NotificationEmail { for (String reviewer : getEmailsByState(ReviewerStateInternal.CC)) { footers.add(MailHeader.CC.withDelimiter() + reviewer); } - for (String attentionUser : getAttentionSet()) { - footers.add(MailHeader.ATTENTION.withDelimiter() + attentionUser); + for (Account.Id attentionUser : currentAttentionSet) { + footers.add(MailHeader.ATTENTION.withDelimiter() + getNameEmailFor(attentionUser)); } } @@ -515,12 +530,12 @@ public abstract class ChangeEmail extends NotificationEmail { return reviewers; } - private Set getAttentionSet() { - Set attentionSet = new TreeSet<>(); + private Set getAttentionSet() { + Set attentionSet = new TreeSet<>(); try { attentionSet = additionsOnly(changeData.attentionSet()).stream() - .map(a -> getNameEmailFor(a.account())) + .map(a -> a.account()) .collect(Collectors.toSet()); } catch (StorageException e) { logger.atWarning().withCause(e).log("Cannot get change attention set"); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java index 976be96..08e70fb 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java @@ -41,6 +41,8 @@ import com.google.gerrit.extensions.api.changes.AttentionSetInput; import com.google.gerrit.extensions.api.changes.DeleteReviewerInput; import com.google.gerrit.extensions.api.changes.HashtagsInput; import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo; +import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -1360,6 +1362,49 @@ public class AttentionSetIT extends AbstractDaemonTest { sender.clear(); } + @Test + public void attentionSetWithEmailFilter() throws Exception { + PushOneCommit.Result r = createChange(); + + // 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()); + + // Add user to attention set. They receive an email since they are in the attention set. + change(r).addReviewer(user.id().toString()); + assertThat(sender.getMessages()).isNotEmpty(); + sender.clear(); + + // Irrelevant reply, User is still in the attention set, thus got another email. + change(r).current().review(ReviewInput.approve()); + assertThat(sender.getMessages()).isNotEmpty(); + sender.clear(); + + // Abandon the change which removes user from attention set; the user doesn't receive an email + // since they are not in the attention set. + change(r).abandon(); + assertThat(sender.getMessages()).isEmpty(); + } + + @Test + 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()); + + // Ensure emails that don't relate to changes are still sent. + gApi.accounts().id(user.id().get()).generateHttpPassword(); + assertThat(sender.getMessages()).isNotEmpty(); + } + private List getAttentionSetUpdatesForUser( PushOneCommit.Result r, TestAccount account) { return getAttentionSetUpdates(r.getChange().getId()).stream()