Merge changes If69cf4d0,Ic2aad5eb * changes: Move emailOnlyAuthors and emailiOnlyAttentionSetIfEnabled to isRecipientAllowed. Remove NotificationEmail class.
diff --git a/java/com/google/gerrit/server/mail/send/BranchEmailUtils.java b/java/com/google/gerrit/server/mail/send/BranchEmailUtils.java new file mode 100644 index 0000000..acba4ea --- /dev/null +++ b/java/com/google/gerrit/server/mail/send/BranchEmailUtils.java
@@ -0,0 +1,98 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.mail.send; + +import com.google.common.collect.Iterables; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.entities.BranchNameKey; +import com.google.gerrit.mail.MailHeader; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** Contains utils for email notification related to the events on project+branch. */ +class BranchEmailUtils { + + /** Set a reasonable list id so that filters can be used to sort messages. */ + static void setListIdHeader(OutgoingEmail email, BranchNameKey branch) { + email.setHeader( + "List-Id", + "<gerrit-" + branch.project().get().replace('/', '-') + "." + email.getGerritHost() + ">"); + if (email.getSettingsUrl() != null) { + email.setHeader("List-Unsubscribe", "<" + email.getSettingsUrl() + ">"); + } + } + + /** Add branch information to soy template params. */ + static void addBranchData(OutgoingEmail email, EmailArguments args, BranchNameKey branch) { + Map<String, Object> soyContext = email.getSoyContext(); + Map<String, Object> soyContextEmailData = email.getSoyContextEmailData(); + + String projectName = branch.project().get(); + soyContext.put("projectName", projectName); + // shortProjectName is the project name with the path abbreviated. + soyContext.put("shortProjectName", getShortProjectName(projectName)); + + // instanceAndProjectName is the instance's name followed by the abbreviated project path + soyContext.put( + "instanceAndProjectName", + getInstanceAndProjectName(args.instanceNameProvider.get(), projectName)); + soyContext.put("addInstanceNameInSubject", args.addInstanceNameInSubject); + + soyContextEmailData.put("sshHost", getSshHost(email.getGerritHost(), args.sshAddresses)); + + Map<String, String> branchData = new HashMap<>(); + branchData.put("shortName", branch.shortName()); + soyContext.put("branch", branchData); + + email.addFooter(MailHeader.PROJECT.withDelimiter() + branch.project().get()); + email.addFooter(MailHeader.BRANCH.withDelimiter() + branch.shortName()); + } + + @Nullable + private static String getSshHost(String gerritHost, List<String> sshAddresses) { + String host = Iterables.getFirst(sshAddresses, null); + if (host == null) { + return null; + } + if (host.startsWith("*:")) { + return gerritHost + host.substring(1); + } + return host; + } + + /** Shortens project/repo name to only show part after the last '/'. */ + static String getShortProjectName(String projectName) { + int lastIndexSlash = projectName.lastIndexOf('/'); + if (lastIndexSlash == 0) { + return projectName.substring(1); // Remove the first slash + } + if (lastIndexSlash == -1) { // No slash in the project name + return projectName; + } + + return "..." + projectName.substring(lastIndexSlash + 1); + } + + /** Returns a project/repo name that includes instance as prefix. */ + static String getInstanceAndProjectName(String instanceName, String projectName) { + if (instanceName == null || instanceName.isEmpty()) { + return getShortProjectName(projectName); + } + // Extract the project name (everything after the last slash) and prepends it with gerrit's + // instance name + return instanceName + "/" + projectName.substring(projectName.lastIndexOf('/') + 1); + } +}
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java index ec81cf6..e8c43dc 100644 --- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -26,6 +26,7 @@ import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Address; import com.google.gerrit.entities.AttentionSetUpdate; +import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.ChangeSizeBucket; import com.google.gerrit.entities.NotifyConfig.NotifyType; @@ -43,6 +44,7 @@ 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.mail.send.ProjectWatch.Watchers.WatcherList; import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.DiffNotAvailableException; import com.google.gerrit.server.patch.DiffOptions; @@ -78,7 +80,7 @@ import org.eclipse.jgit.util.TemporaryBuffer; /** Sends an email to one or more interested parties. */ -public abstract class ChangeEmail extends NotificationEmail { +public abstract class ChangeEmail extends OutgoingEmail { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @@ -100,19 +102,23 @@ protected PatchSetInfo patchSetInfo; protected String changeMessage; protected Instant timestamp; + protected BranchNameKey branch; protected ProjectState projectState; protected Set<Account.Id> authors; protected boolean emailOnlyAuthors; protected boolean emailOnlyAttentionSetIfEnabled; + // Watchers ignore attention set rules. + protected Set<Account.Id> watchers = new HashSet<>(); protected ChangeEmail(EmailArguments args, String messageClass, ChangeData changeData) { - super(args, messageClass, changeData.change().getDest()); + super(args, messageClass); this.changeData = changeData; change = changeData.change(); emailOnlyAuthors = false; emailOnlyAttentionSetIfEnabled = true; currentAttentionSet = getAttentionSet(); + branch = changeData.change().getDest(); } @Override @@ -202,6 +208,7 @@ } super.init(); + BranchEmailUtils.setListIdHeader(this, branch); if (timestamp != null) { setHeader(FieldName.DATE, timestamp); } @@ -395,8 +402,38 @@ } } - @Override - protected final Watchers getWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) { + /** Include users and groups that want notification of events. */ + protected void includeWatchers(NotifyType type) { + includeWatchers(type, true); + } + + /** Include users and groups that want notification of events. */ + protected void includeWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) { + try { + Watchers matching = getWatchers(type, includeWatchersFromNotifyConfig); + addWatchers(RecipientType.TO, matching.to); + addWatchers(RecipientType.CC, matching.cc); + addWatchers(RecipientType.BCC, matching.bcc); + } catch (StorageException err) { + // Just don't CC 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. + logger.atWarning().withCause(err).log("Cannot BCC watchers for %s", type); + } + } + + /** Add users or email addresses to the TO, CC, or BCC list. */ + private void addWatchers(RecipientType type, WatcherList watcherList) { + watchers.addAll(watcherList.accounts); + for (Account.Id user : watcherList.accounts) { + addByAccountId(type, user); + } + for (Address addr : watcherList.emails) { + addByEmail(type, addr); + } + } + + private final Watchers getWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) { if (!NotifyHandling.ALL.equals(notify.handling())) { return new Watchers(); } @@ -438,38 +475,11 @@ } @Override - protected void addByAccountId(RecipientType rt, Account.Id to) { - 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 (emailOnlyAttentionSetIfEnabled - && accountState.isPresent() - && accountState.get().generalPreferences().getEmailStrategy() - == EmailStrategy.ATTENTION_SET_ONLY - && !currentAttentionSet.contains(to)) { - return; - } - } - if (emailOnlyAuthors && !authors.contains(to)) { - return; - } - super.addByAccountId(rt, to); - } - - @Override protected boolean isRecipientAllowed(Address addr) throws PermissionBackendException { if (!projectState.statePermitsRead()) { return false; } + return args.permissionBackend .user(args.anonymousUser.get()) .change(changeData) @@ -481,6 +491,20 @@ if (!projectState.statePermitsRead()) { return false; } + if (emailOnlyAuthors && !authors.contains(to)) { + return false; + } + if (!watchers.contains(to)) { + Optional<AccountState> accountState = args.accountCache.get(to); + if (emailOnlyAttentionSetIfEnabled + && accountState.isPresent() + && accountState.get().generalPreferences().getEmailStrategy() + == EmailStrategy.ATTENTION_SET_ONLY + && !currentAttentionSet.contains(to)) { + return false; + } + } + return args.permissionBackend.absentUser(to).change(changeData).test(ChangePermission.READ); } @@ -517,6 +541,7 @@ @Override protected void setupSoyContext() { super.setupSoyContext(); + BranchEmailUtils.addBranchData(this, args, branch); soyContext.put("changeId", change.getKey().get()); soyContext.put("coverLetter", getCoverLetter());
diff --git a/java/com/google/gerrit/server/mail/send/NotificationEmail.java b/java/com/google/gerrit/server/mail/send/NotificationEmail.java deleted file mode 100644 index c943724..0000000 --- a/java/com/google/gerrit/server/mail/send/NotificationEmail.java +++ /dev/null
@@ -1,155 +0,0 @@ -// Copyright (C) 2012 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.server.mail.send; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.Iterables; -import com.google.common.flogger.FluentLogger; -import com.google.gerrit.common.Nullable; -import com.google.gerrit.entities.Account; -import com.google.gerrit.entities.Address; -import com.google.gerrit.entities.BranchNameKey; -import com.google.gerrit.entities.NotifyConfig.NotifyType; -import com.google.gerrit.exceptions.EmailException; -import com.google.gerrit.exceptions.StorageException; -import com.google.gerrit.extensions.api.changes.RecipientType; -import com.google.gerrit.mail.MailHeader; -import com.google.gerrit.server.mail.send.ProjectWatch.Watchers; -import com.google.gerrit.server.mail.send.ProjectWatch.Watchers.WatcherList; -import java.util.HashMap; -import java.util.Map; - -/** Common class for notifications that are related to a project and branch */ -public abstract class NotificationEmail extends OutgoingEmail { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - - protected BranchNameKey branch; - - protected NotificationEmail(EmailArguments args, String messageClass, BranchNameKey branch) { - super(args, messageClass); - this.branch = branch; - } - - @Override - protected void init() throws EmailException { - super.init(); - setListIdHeader(); - } - - private void setListIdHeader() { - // Set a reasonable list id so that filters can be used to sort messages - setHeader( - "List-Id", - "<gerrit-" + branch.project().get().replace('/', '-') + "." + getGerritHost() + ">"); - if (getSettingsUrl() != null) { - setHeader("List-Unsubscribe", "<" + getSettingsUrl() + ">"); - } - } - - /** Include users and groups that want notification of events. */ - protected void includeWatchers(NotifyType type) { - includeWatchers(type, true); - } - - /** Include users and groups that want notification of events. */ - protected void includeWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig) { - try { - Watchers matching = getWatchers(type, includeWatchersFromNotifyConfig); - addWatchers(RecipientType.TO, matching.to); - addWatchers(RecipientType.CC, matching.cc); - addWatchers(RecipientType.BCC, matching.bcc); - } catch (StorageException err) { - // Just don't CC 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. - logger.atWarning().withCause(err).log("Cannot BCC watchers for %s", type); - } - } - - /** Returns all watchers that are relevant */ - protected abstract Watchers getWatchers(NotifyType type, boolean includeWatchersFromNotifyConfig); - - /** Add users or email addresses to the TO, CC, or BCC list. */ - protected void addWatchers(RecipientType type, WatcherList watcherList) { - for (Account.Id user : watcherList.accounts) { - addWatcher(type, user); - } - for (Address addr : watcherList.emails) { - addByEmail(type, addr); - } - } - - protected abstract void addWatcher(RecipientType type, Account.Id to); - - @Nullable - public String getSshHost() { - String host = Iterables.getFirst(args.sshAddresses, null); - if (host == null) { - return null; - } - if (host.startsWith("*:")) { - return getGerritHost() + host.substring(1); - } - return host; - } - - @Override - protected void setupSoyContext() { - super.setupSoyContext(); - - String projectName = branch.project().get(); - soyContext.put("projectName", projectName); - // shortProjectName is the project name with the path abbreviated. - soyContext.put("shortProjectName", getShortProjectName(projectName)); - - // instanceAndProjectName is the instance's name followed by the abbreviated project path - soyContext.put( - "instanceAndProjectName", - getInstanceAndProjectName(args.instanceNameProvider.get(), projectName)); - soyContext.put("addInstanceNameInSubject", args.addInstanceNameInSubject); - - soyContextEmailData.put("sshHost", getSshHost()); - - Map<String, String> branchData = new HashMap<>(); - branchData.put("shortName", branch.shortName()); - soyContext.put("branch", branchData); - - footers.add(MailHeader.PROJECT.withDelimiter() + branch.project().get()); - footers.add(MailHeader.BRANCH.withDelimiter() + branch.shortName()); - } - - @VisibleForTesting - protected static String getShortProjectName(String projectName) { - int lastIndexSlash = projectName.lastIndexOf('/'); - if (lastIndexSlash == 0) { - return projectName.substring(1); // Remove the first slash - } - if (lastIndexSlash == -1) { // No slash in the project name - return projectName; - } - - return "..." + projectName.substring(lastIndexSlash + 1); - } - - @VisibleForTesting - protected static String getInstanceAndProjectName(String instanceName, String projectName) { - if (instanceName == null || instanceName.isEmpty()) { - return getShortProjectName(projectName); - } - // Extract the project name (everything after the last slash) and prepends it with gerrit's - // instance name - return instanceName + "/" + projectName.substring(projectName.lastIndexOf('/') + 1); - } -}
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index d00c874..aba8f62 100644 --- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -661,6 +661,26 @@ soyContext.put("email", soyContextEmailData); } + /** Mutable map of parameters passed into email templates when rendering. */ + public Map<String, Object> getSoyContext() { + return this.soyContext; + } + + // TODO: It's not clear why we need this explicit separation. Probably worth + // simplifying. + /** Mutable content of `email` parameter in the templates. */ + public Map<String, Object> getSoyContextEmailData() { + return this.soyContextEmailData; + } + + /** + * Add a line to email footer with additional information. Typically, in the form of {@literal + * <key>: <value>}. + */ + public void addFooter(String footer) { + footers.add(footer); + } + private String getInstanceName() { return args.instanceNameProvider.get(); }
diff --git a/javatests/com/google/gerrit/server/mail/send/NotificationEmailTest.java b/javatests/com/google/gerrit/server/mail/send/BranchEmailUtilsTest.java similarity index 88% rename from javatests/com/google/gerrit/server/mail/send/NotificationEmailTest.java rename to javatests/com/google/gerrit/server/mail/send/BranchEmailUtilsTest.java index b87c4a1..3c60b79 100644 --- a/javatests/com/google/gerrit/server/mail/send/NotificationEmailTest.java +++ b/javatests/com/google/gerrit/server/mail/send/BranchEmailUtilsTest.java
@@ -15,12 +15,12 @@ package com.google.gerrit.server.mail.send; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.server.mail.send.NotificationEmail.getInstanceAndProjectName; -import static com.google.gerrit.server.mail.send.NotificationEmail.getShortProjectName; +import static com.google.gerrit.server.mail.send.BranchEmailUtils.getInstanceAndProjectName; +import static com.google.gerrit.server.mail.send.BranchEmailUtils.getShortProjectName; import org.junit.Test; -public class NotificationEmailTest { +public class BranchEmailUtilsTest { @Test public void instanceAndProjectName() throws Exception { assertThat(getInstanceAndProjectName("test", "/my/api")).isEqualTo("test/api");