diff --git a/Documentation/index.txt b/Documentation/index.txt index 7647aaf7bc..77904e551b 100644 --- a/Documentation/index.txt +++ b/Documentation/index.txt @@ -19,6 +19,7 @@ User Guide * link:access-control.html[Access Controls] * link:error-messages.html[Error Messages] * link:rest-api.html[REST API] +* link:user-notify.html[Subscribing to Email Notifications] * link:user-submodules.html[Subscribing to Git Submodules] Installation diff --git a/Documentation/user-notify.txt b/Documentation/user-notify.txt new file mode 100644 index 0000000000..ae3c2d09c5 --- /dev/null +++ b/Documentation/user-notify.txt @@ -0,0 +1,127 @@ +Gerrit Code Review - Email Notifications +======================================== + +Description +----------- + +Gerrit can automatically notify users by email when new changes are +uploaded for review, after comments have been posted on a change, +or after the change has been submitted to a branch. + +User Level Settings +------------------- + +Individual users can configure email subscriptions by editing +watched projects through Settings > Watched Projects with the web UI. + +Specific projects may be watched, or the special project +`All-Projects` can be watched to watch all projects that +are visible to the user. + +Change search expressions can be used to filter change notifications +to specific subsets, for example `branch:master` to only see changes +proposed for the master branch. + +Project Level Settings +---------------------- + +Project owners and site administrators can configure project level +notifications, enabling Gerrit Code Review to automatically send +emails to team mailing lists, or groups of users. Project settings +are stored inside of the `refs/meta/config` branch of each Git +repository, and are placed inside of the `project.config` file. + +To edit the project level notify settings, ensure the project owner +has Push permission already granted for the `refs/meta/config` +branch. Consult link:access-control.html[access controls] for +details on how access permissions work. + +Initialize a temporary Git repository to edit the configuration: +==== + mkdir cfg_dir + cd cfg_dir + git init +==== + +Download the existing configuration from Gerrit: +==== + git fetch ssh://localhost:29418/project refs/meta/config + git checkout FETCH_HEAD +==== + +Enable notifications to an email address by adding to +`project.config`, this can be done using the `git config` command: +==== + git config -f project.config --add notify.team.email team-address@example.com + git config -f project.config --add notify.team.email paranoid-manager@example.com +==== + +Examining the project.config file with any text editor should show +a new notify section describing the email addresses to deliver to: +---- + [notify "team"] + email = team-address@example.com + email = paranoid-manager@example.com +---- + +Each notify section within a single project.config file must have a +unique name. The section name itself does not matter and may later +appear in the web UI. Naming a section after the email address or +group it delivers to is typical. Multiple sections can be specified +if different filters are needed. + +Commit the configuration change, and push it back: +==== + git commit -a -m "Notify team-address@example.com of changes" + git push ssh://localhost:29418/project HEAD:refs/meta/config +==== + +[[notify.name.email]]notify..email:: ++ +List of email addresses to send matching notifications to. Each +email address should be placed on its own line. ++ +Internal groups within Gerrit Code Review can also be named using +`group NAME` syntax. If this format is used the group's UUID must +also appear in the corresponding `groups` file. Gerrit will expand +the group membership and BCC all current users. + +[[notify.name.type]]notify..type:: ++ +Types of notifications to send. If not specified, all notifications +are sent. ++ +* `new_changes`: Only newly created changes. +* `all_comments`: Only comments on existing changes. +* `submitted_changes`: Only changes that have been submitted. +* `all`: All notifications. + ++ +Like email, this variable may be a list of options. + +[[notify.name.filter]]notify..filter:: ++ +link:user-search.html[Change search expression] to match changes that +should be sent to the emails named in this section. Within a Git-style +configuration file double quotes around complex operator values may +need to be escaped, e.g. `filter = branch:\"^(maint|stable)-.*\"`. + +When sending email to a bare email address in a notify block, Gerrit +Code Review ignores read access controls and assumes the administrator +has set the filtering options correctly. Project owners can implement +security filtering by adding the `visibleto:groupname` predicate to +the filter expression, for example: + +==== + [notify "Developers"] + email = team-address@example.com + filter = visibleto:Developers +==== + +When sending email to an internal group, the internal group's read +access is automatically checked by Gerrit and therefore does not +need to use the `visibleto:` operator in the filter. + +GERRIT +------ +Part of link:index.html[Gerrit Code Review] diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java index 93e6fb322d..e592101b21 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java @@ -22,7 +22,7 @@ import com.google.gwtorm.client.StringKey; public final class AccountProjectWatch { public enum NotifyType { - NEW_CHANGES, ALL_COMMENTS, SUBMITTED_CHANGES + NEW_CHANGES, ALL_COMMENTS, SUBMITTED_CHANGES, ALL } public static final String FILTER_ALL = "*"; @@ -159,6 +159,12 @@ public final class AccountProjectWatch { case SUBMITTED_CHANGES: notifySubmittedChanges = v; break; + + case ALL: + notifyNewChanges = v; + notifyAllComments = v; + notifySubmittedChanges = v; + break; } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/NotifyConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotifyConfig.java new file mode 100644 index 0000000000..ba2833dc8e --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotifyConfig.java @@ -0,0 +1,104 @@ +// 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.git; + +import com.google.common.base.Strings; +import com.google.common.collect.Sets; +import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; +import com.google.gerrit.server.mail.Address; + +import java.util.EnumSet; +import java.util.Set; + +public class NotifyConfig implements Comparable { + private String name; + private EnumSet types = EnumSet.of(NotifyType.ALL); + private String filter; + + private Set groups = Sets.newHashSet(); + private Set
addresses = Sets.newHashSet(); + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public boolean isNotify(NotifyType type) { + return types.contains(type) || types.contains(NotifyType.ALL); + } + + public EnumSet getNotify() { + return types; + } + + public void setTypes(EnumSet newTypes) { + types = EnumSet.copyOf(newTypes); + } + + public String getFilter() { + return filter; + } + + public void setFilter(String filter) { + if ("*".equals(filter)) { + this.filter = null; + } else { + this.filter = Strings.emptyToNull(filter); + } + } + + public Set getGroups() { + return groups; + } + + public Set
getAddresses() { + return addresses; + } + + public void addEmail(GroupReference group) { + groups.add(group); + } + + public void addEmail(Address address) { + addresses.add(address); + } + + @Override + public int compareTo(NotifyConfig o) { + return name.compareTo(o.name); + } + + @Override + public int hashCode() { + return name.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (obj instanceof NotifyConfig) { + return compareTo((NotifyConfig) obj) == 0; + } + return false; + } + + @Override + public String toString() { + return "NotifyConfig[" + name + " = " + addresses + " + " + groups + "]"; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index 0fe00eae4b..75c9bd11f1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -16,6 +16,8 @@ package com.google.gerrit.server.git; import static com.google.gerrit.common.data.Permission.isPermission; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.ContributorAgreement; import com.google.gerrit.common.data.GlobalCapability; @@ -23,17 +25,21 @@ import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule.Action; +import com.google.gerrit.common.data.RefConfigSection; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project.State; import com.google.gerrit.reviewdb.client.Project.SubmitType; import com.google.gerrit.server.account.GroupCache; -import com.google.gerrit.common.data.RefConfigSection; +import com.google.gerrit.server.config.ConfigUtil; +import com.google.gerrit.server.mail.Address; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.util.StringUtils; import java.io.BufferedReader; import java.io.IOException; @@ -41,6 +47,7 @@ import java.io.StringReader; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -67,6 +74,11 @@ public class ProjectConfig extends VersionedMetaData { private static final String KEY_AUTO_VERIFY = "autoVerify"; private static final String KEY_AGREEMENT_URL = "agreementUrl"; + private static final String NOTIFY = "notify"; + private static final String KEY_EMAIL = "email"; + private static final String KEY_FILTER = "filter"; + private static final String KEY_TYPE = "type"; + private static final String CAPABILITY = "capability"; private static final String RECEIVE = "receive"; @@ -91,6 +103,7 @@ public class ProjectConfig extends VersionedMetaData { private Map groupsByUUID; private Map accessSections; private Map contributorAgreements; + private Map notifySections; private List validationErrors; private ObjectId rulesId; @@ -185,6 +198,10 @@ public class ProjectConfig extends VersionedMetaData { contributorAgreements.put(section.getName(), section); } + public Collection getNotifyConfigs() { + return notifySections.values(); + } + public GroupReference resolve(AccountGroup group) { return resolve(GroupReference.forGroup(group)); } @@ -275,6 +292,7 @@ public class ProjectConfig extends VersionedMetaData { loadAccountsSection(rc, groupsByName); loadContributorAgreements(rc, groupsByName); loadAccessSections(rc, groupsByName); + loadNotifySections(rc, groupsByName); } private void loadAccountsSection( @@ -318,6 +336,67 @@ public class ProjectConfig extends VersionedMetaData { } } + /** + * Parses the [notify] sections out of the configuration file. + * + * 
+   *   [notify "reviewers"]
+   *     email = group Reviewers
+   *     type = new_changes
+   *
+   *   [notify "dev-team"]
+   *     email = dev-team@example.com
+   *     filter = branch:master
+   *
+   *   [notify "qa"]
+   *     email = qa@example.com
+   *     filter = branch:\"^(maint|stable)-.*\"
+   *     type = submitted_changes
+   *
+ */ + private void loadNotifySections( + Config rc, Map groupsByName) { + notifySections = Maps.newHashMap(); + for (String sectionName : rc.getSubsections(NOTIFY)) { + NotifyConfig n = new NotifyConfig(); + n.setName(sectionName); + n.setFilter(rc.getString(NOTIFY, sectionName, KEY_FILTER)); + + EnumSet types = EnumSet.noneOf(NotifyType.class); + types.addAll(ConfigUtil.getEnumList(rc, + NOTIFY, sectionName, KEY_TYPE, + NotifyType.ALL)); + n.setTypes(types); + + for (String dst : rc.getStringList(NOTIFY, sectionName, KEY_EMAIL)) { + if (dst.startsWith("group ")) { + String groupName = dst.substring(6).trim(); + GroupReference ref = groupsByName.get(groupName); + if (ref == null) { + ref = new GroupReference(null, groupName); + groupsByName.put(ref.getName(), ref); + } + if (ref.getUUID() != null) { + n.addEmail(ref); + } else { + error(new ValidationError(PROJECT_CONFIG, + "group \"" + ref.getName() + "\" not in " + GROUP_LIST)); + } + } else if (dst.startsWith("user ")) { + error(new ValidationError(PROJECT_CONFIG, dst + " not supported")); + } else { + try { + n.addEmail(Address.parse(dst)); + } catch (IllegalArgumentException err) { + error(new ValidationError(PROJECT_CONFIG, + "notify section \"" + sectionName + "\" has invalid email \"" + dst + "\"")); + } + } + } + notifySections.put(sectionName, n); + } + } + private void loadAccessSections( Config rc, Map groupsByName) { accessSections = new HashMap(); @@ -458,6 +537,7 @@ public class ProjectConfig extends VersionedMetaData { saveAccountsSection(rc, keepGroups); saveContributorAgreements(rc, keepGroups); saveAccessSections(rc, keepGroups); + saveNotifySections(rc, keepGroups); groupsByUUID.keySet().retainAll(keepGroups); saveConfig(PROJECT_CONFIG, rc); @@ -493,6 +573,47 @@ public class ProjectConfig extends VersionedMetaData { } } + private void saveNotifySections( + Config rc, Set keepGroups) { + for (NotifyConfig nc : sort(notifySections.values())) { + List email = Lists.newArrayList(); + for (GroupReference gr : nc.getGroups()) { + if (gr.getUUID() != null) { + keepGroups.add(gr.getUUID()); + } + email.add(new PermissionRule(gr).asString(false)); + } + Collections.sort(email); + + List addrs = Lists.newArrayList(); + for (Address addr : nc.getAddresses()) { + addrs.add(addr.toString()); + } + Collections.sort(addrs); + email.addAll(addrs); + + if (email.isEmpty()) { + rc.unset(NOTIFY, nc.getName(), KEY_EMAIL); + } else { + rc.setStringList(NOTIFY, nc.getName(), KEY_EMAIL, email); + } + + if (nc.getNotify().equals(EnumSet.of(NotifyType.ALL))) { + rc.unset(NOTIFY, nc.getName(), KEY_TYPE); + } else { + List types = Lists.newArrayListWithCapacity(4); + for (NotifyType t : NotifyType.values()) { + if (nc.isNotify(t)) { + types.add(StringUtils.toLowerCase(t.name())); + } + } + rc.setStringList(NOTIFY, nc.getName(), KEY_TYPE, types); + } + + set(rc, NOTIFY, nc.getName(), KEY_FILTER, nc.getFilter()); + } + } + private List ruleToStringList( List list, Set keepGroups) { List rules = new ArrayList(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java index 7fabcfe1eb..0eb3dfe367 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -38,7 +39,7 @@ public class AbandonedSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); - bccWatchesNotifyAllComments(); + bccWatches(NotifyType.ALL_COMMENTS); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java index 624e626b7b..4e9ed2b830 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java @@ -54,6 +54,19 @@ public class Address { return email; } + @Override + public int hashCode() { + return email.hashCode(); + } + + @Override + public boolean equals(Object other) { + if (other instanceof Address) { + return email.equals(((Address) other).email); + } + return false; + } + @Override public String toString() { try { 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 100275e8b9..e1a1725022 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 @@ -14,18 +14,25 @@ package com.google.gerrit.server.mail; +import com.google.common.collect.Lists; +import com.google.common.collect.Sets; +import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.AccountGroupInclude; +import com.google.gerrit.reviewdb.client.AccountGroupMember; import com.google.gerrit.reviewdb.client.AccountProjectWatch; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.PatchSetInfo; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.StarredChange; -import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.git.NotifyConfig; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; @@ -34,17 +41,17 @@ import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.QueryParseException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeQueryBuilder; +import com.google.gerrit.server.query.change.SingleGroupUser; import com.google.gwtorm.server.OrmException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.text.MessageFormat; -import java.util.ArrayList; import java.util.Collections; import java.util.Date; import java.util.HashSet; -import java.util.List; +import java.util.Queue; import java.util.Set; import java.util.TreeSet; @@ -305,53 +312,148 @@ public abstract class ChangeEmail extends OutgoingEmail { // 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); } } - /** BCC any user who has set "notify all comments" on this project. */ - protected void bccWatchesNotifyAllComments() { + /** BCC users and groups that want notification of events. */ + protected void bccWatches(NotifyType type) { try { - // BCC anyone else who has interest in this project's changes - // - for (final AccountProjectWatch w : getWatches()) { - if (w.isNotify(NotifyType.ALL_COMMENTS)) { - add(RecipientType.BCC, w.getAccountId()); - } + Watchers matching = getWatches(type); + for (Account.Id user : matching.accounts) { + add(RecipientType.BCC, user); + } + for (Address addr : matching.emails) { + add(RecipientType.BCC, addr); } } catch (OrmException 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. + log.warn("Cannot BCC watchers for " + type, err); } } /** Returns all watches that are relevant */ - protected final List getWatches() throws OrmException { + protected final Watchers getWatches(NotifyType type) throws OrmException { + Watchers matching = new Watchers(); if (changeData == null) { - return Collections.emptyList(); + return matching; } - List matching = new ArrayList(); Set projectWatchers = new HashSet(); for (AccountProjectWatch w : args.db.get().accountProjectWatches() .byProject(change.getProject())) { projectWatchers.add(w.getAccountId()); - add(matching, w); - } - - for (AccountProjectWatch w : args.db.get().accountProjectWatches() - .byProject(args.allProjectsName)) { - if (!projectWatchers.contains(w.getAccountId())) { + if (w.isNotify(type)) { add(matching, w); } } - return Collections.unmodifiableList(matching); + for (AccountProjectWatch w : args.db.get().accountProjectWatches() + .byProject(args.allProjectsName)) { + if (!projectWatchers.contains(w.getAccountId()) && w.isNotify(type)) { + add(matching, w); + } + } + + ProjectState state = projectState; + while (state != null) { + for (NotifyConfig nc : state.getConfig().getNotifyConfigs()) { + if (nc.isNotify(type)) { + try { + add(matching, nc, state.getProject().getNameKey()); + } catch (QueryParseException e) { + log.warn(String.format( + "Project %s has invalid notify %s filter \"%s\": %s", + state.getProject().getName(), nc.getName(), + nc.getFilter(), e.getMessage())); + } + } + } + state = state.getParentState(); + } + + return matching; + } + + protected static class Watchers { + protected final Set accounts = Sets.newHashSet(); + protected final Set
emails = Sets.newHashSet(); } @SuppressWarnings("unchecked") - private void add(List matching, AccountProjectWatch w) + private void add(Watchers matching, NotifyConfig nc, Project.NameKey project) + throws OrmException, QueryParseException { + for (GroupReference ref : nc.getGroups()) { + AccountGroup group = args.groupCache.get(ref.getUUID()); + if (group == null) { + log.warn(String.format( + "Project %s has invalid group %s in notify section %s", + project.get(), ref.getName(), nc.getName())); + continue; + } + + if (group.getType() != AccountGroup.Type.INTERNAL) { + log.warn(String.format( + "Project %s cannot use group %s of type %s in notify section %s", + project.get(), ref.getName(), group.getType(), nc.getName())); + continue; + } + + ChangeQueryBuilder qb = args.queryBuilder.create(new SingleGroupUser( + args.capabilityControlFactory, + ref.getUUID())); + qb.setAllowFile(true); + Predicate p = qb.is_visible(); + if (nc.getFilter() != null) { + p = Predicate.and(qb.parse(nc.getFilter()), p); + p = args.queryRewriter.get().rewrite(p); + } + if (p.match(changeData)) { + recursivelyAddAllAccounts(matching, group); + } + } + + if (!nc.getAddresses().isEmpty()) { + if (nc.getFilter() != null) { + ChangeQueryBuilder qb = args.queryBuilder.create(args.anonymousUser); + qb.setAllowFile(true); + Predicate p = qb.parse(nc.getFilter()); + p = args.queryRewriter.get().rewrite(p); + if (p.match(changeData)) { + matching.emails.addAll(nc.getAddresses()); + } + } else { + matching.emails.addAll(nc.getAddresses()); + } + } + } + + private void recursivelyAddAllAccounts(Watchers matching, AccountGroup group) + throws OrmException { + Set seen = Sets.newHashSet(); + Queue scan = Lists.newLinkedList(); + scan.add(group.getId()); + seen.add(group.getId()); + while (!scan.isEmpty()) { + AccountGroup.Id next = scan.remove(); + for (AccountGroupMember m : args.db.get().accountGroupMembers() + .byGroup(next)) { + matching.accounts.add(m.getAccountId()); + } + for (AccountGroupInclude m : args.db.get().accountGroupIncludes() + .byGroup(next)) { + if (seen.add(m.getIncludeId())) { + scan.add(m.getIncludeId()); + } + } + } + } + + @SuppressWarnings("unchecked") + private void add(Watchers matching, AccountProjectWatch w) throws OrmException { IdentifiedUser user = args.identifiedUserFactory.create(args.db, w.getAccountId()); @@ -363,13 +465,13 @@ public abstract class ChangeEmail extends OutgoingEmail { p = Predicate.and(qb.parse(w.getFilter()), p); p = args.queryRewriter.get().rewrite(p); if (p.match(changeData)) { - matching.add(w); + matching.accounts.add(w.getAccountId()); } } catch (QueryParseException e) { // Ignore broken filter expressions. } } else if (p.match(changeData)) { - matching.add(w); + matching.accounts.add(w.getAccountId()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index f054ee8949..749b11f6c3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchLineComment; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.patch.PatchFile; import com.google.gerrit.server.patch.PatchList; @@ -68,7 +69,7 @@ public class CommentSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); - bccWatchesNotifyAllComments(); + bccWatches(NotifyType.ALL_COMMENTS); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java index c6f716ddae..9b82c715a0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java @@ -15,74 +15,65 @@ package com.google.gerrit.server.mail; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.AccountGroup; -import com.google.gerrit.reviewdb.client.AccountGroupMember; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; -import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; -import com.google.gerrit.server.account.GroupCache; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.ssh.SshInfo; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; -import java.util.HashSet; -import java.util.Set; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** Notify interested parties of a brand new change. */ public class CreateChangeSender extends NewChangeSender { + private static final Logger log = + LoggerFactory.getLogger(CreateChangeSender.class); + public static interface Factory { public CreateChangeSender create(Change change); } - private final GroupCache groupCache; - @Inject public CreateChangeSender(EmailArguments ea, @AnonymousCowardName String anonymousCowardName, SshInfo sshInfo, - GroupCache groupCache, @Assisted Change c) { + @Assisted Change c) { super(ea, anonymousCowardName, sshInfo, c); - this.groupCache = groupCache; } @Override protected void init() throws EmailException { super.init(); - bccWatchers(); - } - - private void bccWatchers() { try { + // BCC anyone who has interest in this project's changes // Try to mark interested owners with a TO and not a BCC line. // - final Set owners = new HashSet(); - for (AccountGroup.UUID uuid : getProjectOwners()) { - AccountGroup group = groupCache.get(uuid); - if (group != null) { - for (AccountGroupMember m : args.db.get().accountGroupMembers() - .byGroup(group.getId())) { - owners.add(m.getAccountId()); - } + Watchers matching = getWatches(NotifyType.NEW_CHANGES); + for (Account.Id user : matching.accounts) { + if (isOwnerOfProjectOrBranch(user)) { + add(RecipientType.TO, user); + } else { + add(RecipientType.BCC, user); } } - - // BCC anyone who has interest in this project's changes - // - for (final AccountProjectWatch w : getWatches()) { - if (w.isNotify(NotifyType.NEW_CHANGES)) { - if (owners.contains(w.getAccountId())) { - add(RecipientType.TO, w.getAccountId()); - } else { - add(RecipientType.BCC, w.getAccountId()); - } - } + for (Address addr : matching.emails) { + add(RecipientType.BCC, addr); } } catch (OrmException 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. + log.warn("Cannot BCC watchers for new change", err); } } + + private boolean isOwnerOfProjectOrBranch(Account.Id user) { + return projectState != null + && change != null + && projectState.controlFor(args.identifiedUserFactory.create(user)) + .controlForRef(change.getDest()) + .isOwner(); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java index 68f78d0a57..fa49b06ce7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java @@ -15,9 +15,11 @@ package com.google.gerrit.server.mail; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser.GenericFactory; import com.google.gerrit.server.account.AccountCache; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.CanonicalWebUrl; @@ -44,6 +46,8 @@ class EmailArguments { final EmailSender emailSender; final PatchSetInfoFactory patchSetInfoFactory; final IdentifiedUser.GenericFactory identifiedUserFactory; + final CapabilityControl.Factory capabilityControlFactory; + final AnonymousUser anonymousUser; final Provider urlProvider; final AllProjectsName allProjectsName; @@ -58,6 +62,8 @@ class EmailArguments { PatchListCache patchListCache, FromAddressGenerator fromAddressGenerator, EmailSender emailSender, PatchSetInfoFactory patchSetInfoFactory, GenericFactory identifiedUserFactory, + CapabilityControl.Factory capabilityControlFactory, + AnonymousUser anonymousUser, @CanonicalWebUrl @Nullable Provider urlProvider, AllProjectsName allProjectsName, ChangeQueryBuilder.Factory queryBuilder, @@ -72,6 +78,8 @@ class EmailArguments { this.emailSender = emailSender; this.patchSetInfoFactory = patchSetInfoFactory; this.identifiedUserFactory = identifiedUserFactory; + this.capabilityControlFactory = capabilityControlFactory; + this.anonymousUser = anonymousUser; this.urlProvider = urlProvider; this.allProjectsName = allProjectsName; this.queryBuilder = queryBuilder; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java index 3590b8a90a..70b2d7fa2b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java @@ -17,7 +17,6 @@ package com.google.gerrit.server.mail; import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.AccountProjectWatch; import com.google.gerrit.reviewdb.client.ApprovalCategory; import com.google.gerrit.reviewdb.client.ApprovalCategoryValue; import com.google.gerrit.reviewdb.client.Change; @@ -53,8 +52,8 @@ public class MergedSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); - bccWatchesNotifyAllComments(); - bccWatchesNotifySubmittedChanges(); + bccWatches(NotifyType.ALL_COMMENTS); + bccWatches(NotifyType.SUBMITTED_CHANGES); } @Override @@ -140,20 +139,4 @@ public class MergedSender extends ReplyToChangeSender { } m.put(ca.getCategoryId(), ca); } - - private void bccWatchesNotifySubmittedChanges() { - try { - // BCC anyone else who has interest in this project's changes - // - for (final AccountProjectWatch w : getWatches()) { - if (w.isNotify(NotifyType.SUBMITTED_CHANGES)) { - add(RecipientType.BCC, w.getAccountId()); - } - } - } catch (OrmException 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. - } - } } 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 de8628f990..caa441dfce 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 @@ -14,6 +14,7 @@ package com.google.gerrit.server.mail; +import com.google.common.collect.Sets; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.UserIdentity; import com.google.gerrit.server.account.AccountState; @@ -34,14 +35,13 @@ import java.io.StringReader; import java.io.StringWriter; import java.net.MalformedURLException; import java.net.URL; -import java.util.ArrayList; import java.util.Collection; import java.util.Date; import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; +import java.util.Set; /** Sends an email to one or more interested parties. */ public abstract class OutgoingEmail { @@ -53,7 +53,7 @@ public abstract class OutgoingEmail { protected String messageClass; private final HashSet rcptTo = new HashSet(); private final Map headers; - private final List
smtpRcptTo = new ArrayList
(); + private final Set
smtpRcptTo = Sets.newHashSet(); private Address smtpFromAddress; private StringBuilder body; protected VelocityContext velocityContext; @@ -282,7 +282,7 @@ public abstract class OutgoingEmail { return false; } - if (rcptTo.size() == 1 && rcptTo.contains(fromId)) { + if (smtpRcptTo.size() == 1 && rcptTo.size() == 1 && rcptTo.contains(fromId)) { // If the only recipient is also the sender, don't bother. // return false; @@ -324,14 +324,15 @@ public abstract class OutgoingEmail { protected void add(final RecipientType rt, final Address addr) { if (addr != null && addr.email != null && addr.email.length() > 0) { if (args.emailSender.canEmail(addr.email)) { - smtpRcptTo.add(addr); - switch (rt) { - case TO: - ((EmailHeader.AddressList) headers.get(HDR_TO)).add(addr); - break; - case CC: - ((EmailHeader.AddressList) headers.get(HDR_CC)).add(addr); - break; + if (smtpRcptTo.add(addr)) { + switch (rt) { + case TO: + ((EmailHeader.AddressList) headers.get(HDR_TO)).add(addr); + break; + case CC: + ((EmailHeader.AddressList) headers.get(HDR_CC)).add(addr); + break; + } } } else { log.warn("Not emailing " + addr.email + " (prohibited by allowrcpt)"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java index c9afddea57..946c29fcda 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -38,7 +39,7 @@ public class RestoredSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); - bccWatchesNotifyAllComments(); + bccWatches(NotifyType.ALL_COMMENTS); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java index 964bfed62b..033bd5656e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.mail; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -37,7 +38,7 @@ public class RevertedSender extends ReplyToChangeSender { ccAllApprovals(); bccStarredBy(); - bccWatchesNotifyAllComments(); + bccWatches(NotifyType.ALL_COMMENTS); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/Predicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/Predicate.java index 77e082d2c3..b0f12b5587 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/Predicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/Predicate.java @@ -43,6 +43,12 @@ import java.util.List; * @type type of object the predicate can evaluate in memory. */ public abstract class Predicate { + /** A predicate that matches any input, always, with no cost. */ + @SuppressWarnings("unchecked") + public static Predicate any() { + return (Predicate) Any.INSTANCE; + } + /** Combine the passed predicates into a single AND node. */ public static Predicate and(final Predicate... that) { if (that.length == 1) { @@ -120,4 +126,36 @@ public abstract class Predicate { @Override public abstract boolean equals(Object other); + + private static class Any extends Predicate { + private static final Any INSTANCE = new Any(); + + private Any() { + } + + @Override + public Predicate copy(Collection> children) { + return this; + } + + @Override + public boolean match(T object) { + return true; + } + + @Override + public int getCost() { + return 0; + } + + @Override + public int hashCode() { + return System.identityHashCode(this); + } + + @Override + public boolean equals(Object other) { + return other == this; + } + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java index cdce217240..270b2e7d26 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java @@ -27,15 +27,15 @@ import java.util.Collection; import java.util.Collections; import java.util.Set; -final class SingleGroupUser extends CurrentUser { +public final class SingleGroupUser extends CurrentUser { private final GroupMembership groups; - SingleGroupUser(CapabilityControl.Factory capabilityControlFactory, + public SingleGroupUser(CapabilityControl.Factory capabilityControlFactory, AccountGroup.UUID groupId) { this(capabilityControlFactory, Collections.singleton(groupId)); } - SingleGroupUser(CapabilityControl.Factory capabilityControlFactory, + public SingleGroupUser(CapabilityControl.Factory capabilityControlFactory, Set groups) { super(capabilityControlFactory, AccessPath.UNKNOWN); this.groups = new ListGroupMembership(groups);