From f87cfe9084462a044dda177e6dca0e20c35b7077 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 20 Jan 2009 19:29:18 -0800 Subject: [PATCH] Email notifications of new changes to interested parties This way people get up-front indicators that changes are ready for review, and its in their inbox, where they can keep track of things they need to do in the near future. Signed-off-by: Shawn O. Pearce --- .../com/google/gerrit/server/ChangeMail.java | 280 +++++++++++++++--- .../com/google/gerrit/server/MimeMessage.java | 25 ++ .../com/google/gerrit/server/ssh/Receive.java | 15 + 3 files changed, 277 insertions(+), 43 deletions(-) create mode 100644 appjar/src/main/java/com/google/gerrit/server/MimeMessage.java diff --git a/appjar/src/main/java/com/google/gerrit/server/ChangeMail.java b/appjar/src/main/java/com/google/gerrit/server/ChangeMail.java index d90c9a5b96..2dee2b4f7c 100644 --- a/appjar/src/main/java/com/google/gerrit/server/ChangeMail.java +++ b/appjar/src/main/java/com/google/gerrit/server/ChangeMail.java @@ -16,10 +16,12 @@ package com.google.gerrit.server; import com.google.gerrit.client.data.ProjectCache; import com.google.gerrit.client.reviewdb.Account; +import com.google.gerrit.client.reviewdb.AccountGroupMember; import com.google.gerrit.client.reviewdb.AccountProjectWatch; import com.google.gerrit.client.reviewdb.Change; import com.google.gerrit.client.reviewdb.ChangeApproval; import com.google.gerrit.client.reviewdb.ChangeMessage; +import com.google.gerrit.client.reviewdb.Patch; import com.google.gerrit.client.reviewdb.PatchLineComment; import com.google.gerrit.client.reviewdb.PatchSet; import com.google.gerrit.client.reviewdb.PatchSetInfo; @@ -36,10 +38,12 @@ import java.net.InetAddress; import java.net.MalformedURLException; import java.net.URL; import java.net.UnknownHostException; +import java.util.Collection; import java.util.Collections; import java.util.Date; import java.util.HashSet; import java.util.List; +import java.util.Set; import javax.mail.Address; import javax.mail.MessagingException; @@ -47,7 +51,6 @@ import javax.mail.Transport; import javax.mail.Message.RecipientType; import javax.mail.internet.AddressException; import javax.mail.internet.InternetAddress; -import javax.mail.internet.MimeMessage; import javax.servlet.http.HttpServletRequest; public class ChangeMail { @@ -58,6 +61,7 @@ public class ChangeMail { private final HashSet rcptTo = new HashSet(); private MimeMessage msg; private StringBuilder body; + private boolean inFooter; private String myUrl; private Account.Id fromId; @@ -65,6 +69,8 @@ public class ChangeMail { private PatchSetInfo patchSetInfo; private ChangeMessage message; private List comments = Collections.emptyList(); + private final Set reviewers = new HashSet(); + private final Set extraCC = new HashSet(); private ReviewDb db; public ChangeMail(final GerritServer gs, final Change c) { @@ -103,10 +109,95 @@ public class ChangeMail { comments = plc; } + public void addReviewers(final Collection cc) { + reviewers.addAll(cc); + } + + public void addExtraCC(final Collection cc) { + extraCC.addAll(cc); + } + public void setReviewDb(final ReviewDb d) { db = d; } + public void sendNewChange() throws MessagingException { + if (begin("newchange")) { + add(RecipientType.TO, reviewers); + add(RecipientType.CC, extraCC); + if (patchSetInfo != null) { + // Make sure the author/committer get notice of a change that + // they will be blamed later on for writing. + // + add(RecipientType.CC, patchSetInfo.getAuthor()); + add(RecipientType.CC, patchSetInfo.getCommitter()); + } + newChangeTo(); + if (!haveRcptTo()) { + // No destinations at this point makes it very moot to mail, + // nobody was interested in the change or was told to look + // at it by the caller. + // + return; + } + + // CC the owner/uploader, but in truth these should always match + // the sender too. add will strip duplicates (if any). + // + add(RecipientType.CC, change.getOwner()); + if (patchSet != null) { + add(RecipientType.CC, patchSet.getUploader()); + } + ccSender(); + + body.append("New change "); + body.append(change.getChangeId()); + body.append(" for "); + body.append(change.getDest().getShortName()); + body.append(":\n\n"); + + if (changeUrl() != null) { + body.append(" "); + body.append(changeUrl()); + body.append("\n\n"); + } + + if (patchSetInfo != null) { + body.append(patchSetInfo.getMessage().trim()); + body.append("\n\n"); + } else { + body.append(change.getSubject().trim()); + body.append("\n\n"); + } + + if (db != null && patchSet != null) { + body.append("---\n"); + try { + for (Patch p : db.patches().byPatchSet(patchSet.getId())) { + body.append(p.getChangeType().getCode()); + body.append(' '); + body.append(p.getFileName()); + body.append('\n'); + } + } catch (OrmException e) { + // Don't bother including the files if we get a failure, + // ensure we at least send the notification message. + } + body.append("\n"); + } + + openFooter(); + if (changeUrl() != null) { + body.append("View this change at "); + body.append(changeUrl()); + body.append("\n"); + } + + msg.setMessageID(changeMessageThreadId()); + send(); + } + } + public void sendComment() throws MessagingException { if (begin("comment")) { if (message != null) { @@ -146,15 +237,65 @@ public class ChangeMail { } openFooter(); - body.append("To respond visit "); - body.append(changeUrl()); - body.append("\n"); + if (changeUrl() != null) { + body.append("To respond visit "); + body.append(changeUrl()); + body.append("\n"); + } + + initInReplyToChange(); commentTo(); send(); } } + private void newChangeTo() throws MessagingException { + final ProjectCache.Entry cacheEntry = + Common.getProjectCache().get(change.getDest().getParentKey()); + if (cacheEntry == null) { + return; + } + try { + // Try to mark interested owners with a TO and not a BCC line. + // + final Set toNotBCC = new HashSet(); + for (AccountGroupMember m : db.accountGroupMembers().byGroup( + cacheEntry.getProject().getOwnerGroupId())) { + toNotBCC.add(m.getAccountId()); + } + + // BCC anyone who has interest in this project's changes + // + for (AccountProjectWatch w : db.accountProjectWatches().notifyNewChanges( + cacheEntry.getProject().getId())) { + if (toNotBCC.contains(w.getAccountId())) { + add(RecipientType.TO, w.getAccountId()); + } else { + 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. + } + } + private void commentTo() throws MessagingException { + // Always to the owner/uploader/author/committer. These people + // have a vested interest in the change and any remarks made. + // + add(RecipientType.TO, change.getOwner()); + if (patchSet != null) { + add(RecipientType.TO, patchSet.getUploader()); + } + if (patchSetInfo != null) { + add(RecipientType.TO, patchSetInfo.getAuthor()); + add(RecipientType.TO, patchSetInfo.getCommitter()); + } + add(RecipientType.CC, reviewers); + add(RecipientType.CC, extraCC); + if (db == null) { // We need a database handle to fetch the interest list. // @@ -197,8 +338,8 @@ public class ChangeMail { initListId(messageClass); initChangeUrl(); initSubject(); - defaultTo(); body = new StringBuilder(); + inFooter = false; return true; } return false; @@ -237,10 +378,22 @@ public class ChangeMail { final String listidStr = listid.toString(); msg.setHeader("Mailing-List", "list " + listidStr); msg.setHeader("List-Id", "<" + listidStr.replace('@', '.') + ">"); + if (settingsUrl() != null) { + msg.setHeader("List-Unsubscribe", "<" + settingsUrl() + ">"); + } } private void initChangeUrl() throws MessagingException { - msg.setHeader("X-Gerrit-ChangeURL", changeUrl()); + final String u = changeUrl(); + if (u != null) { + msg.setHeader("X-Gerrit-ChangeURL", "<" + u + ">"); + } + } + + private void initInReplyToChange() throws MessagingException { + final String id = changeMessageThreadId(); + msg.setHeader("In-Reply-To", id); + msg.setHeader("References", id); } private void initSubject() throws MessagingException { @@ -259,28 +412,6 @@ public class ChangeMail { msg.setSubject(subj.toString()); } - private void defaultTo() throws MessagingException { - // Always to the owner/uploader/author/committer. These people - // have a vested interest in the change and any remarks made onit. - // - add(RecipientType.TO, change.getOwner()); - if (patchSet != null) { - add(RecipientType.TO, patchSet.getUploader()); - } - if (patchSetInfo != null) { - add(RecipientType.TO, patchSetInfo.getAuthor()); - add(RecipientType.TO, patchSetInfo.getCommitter()); - } - - if (fromId != null) { - // If we are impersonating a user, make sure they receive a CC of - // this message so they can always review and audit what we sent - // on their behalf to others. - // - add(RecipientType.CC, fromId); - } - } - private String gerritHost() { if (server.getCanonicalURL() != null) { try { @@ -309,40 +440,86 @@ public class ChangeMail { } } - private void openFooter() { - body.append("--\n"); + private String changeUrl() { + if (gerritUrl() != null) { + final StringBuilder r = new StringBuilder(); + r.append(gerritUrl()); + r.append(change.getChangeId()); + return r.toString(); + } + return null; } - private String changeUrl() { - final StringBuilder r = new StringBuilder(); - if (server.getCanonicalURL() != null) { - r.append(server.getCanonicalURL()); - } else { - r.append(myUrl); + private String settingsUrl() { + if (gerritUrl() != null) { + final StringBuilder r = new StringBuilder(); + r.append(gerritUrl()); + r.append("settings"); + return r.toString(); } + return null; + } + + private String gerritUrl() { + if (server.getCanonicalURL() != null) { + return server.getCanonicalURL(); + } + return myUrl; + } + + private String changeMessageThreadId() { + final StringBuilder r = new StringBuilder(); + r.append('<'); + r.append("gerrit"); + r.append('.'); + r.append(change.getCreatedOn().getTime()); + r.append('.'); r.append(change.getChangeId()); + if (fromId != null) { + r.append('.'); + r.append(fromId.get()); + } + r.append('@'); + r.append(gerritHost()); + r.append('>'); return r.toString(); } + private void openFooter() { + if (!inFooter) { + inFooter = true; + body.append("-- \n"); + } + } + private void send() throws MessagingException { + if (haveRcptTo()) { + ccSender(); + openFooter(); + body.append("To unsubscribe, visit "); + body.append(settingsUrl()); + body.append("\n"); + msg.setText(body.toString(), "UTF-8"); + Transport.send(msg); + } + } + + private boolean haveRcptTo() { if (rcptTo.isEmpty()) { // If we have nobody to send this message to, then all of our // selection filters previously for this type of message were // unable to match a destination. Don't bother sending it. // - return; + return false; } if (rcptTo.size() == 1 && rcptTo.contains(fromId)) { - // If the only recipient is because we CC'd the user we are sending - // from, this message isn't very worthwhile. Users shouldn't use - // us in order to email themselves. + // If the only recipient is also the sender, don't bother. // - return; + return false; } - msg.setText(body.toString(), "UTF-8"); - Transport.send(msg); + return true; } private Project.Id projectId() { @@ -352,6 +529,23 @@ public class ChangeMail { return r != null ? r.getProject().getId() : null; } + private void ccSender() throws MessagingException { + if (fromId != null) { + // If we are impersonating a user, make sure they receive a CC of + // this message so they can always review and audit what we sent + // on their behalf to others. + // + add(RecipientType.CC, fromId); + } + } + + private void add(final RecipientType rt, final Collection list) + throws MessagingException { + for (final Account.Id id : list) { + add(rt, id); + } + } + private void add(final RecipientType rt, final UserIdentity who) throws MessagingException { if (who != null && who.getAccount() != null) { diff --git a/appjar/src/main/java/com/google/gerrit/server/MimeMessage.java b/appjar/src/main/java/com/google/gerrit/server/MimeMessage.java new file mode 100644 index 0000000000..aec78e3969 --- /dev/null +++ b/appjar/src/main/java/com/google/gerrit/server/MimeMessage.java @@ -0,0 +1,25 @@ +package com.google.gerrit.server; + +import javax.mail.MessagingException; +import javax.mail.Session; + +class MimeMessage extends javax.mail.internet.MimeMessage { + MimeMessage(final Session session) { + super(session); + } + + private String messageID; + + void setMessageID(final String id) { + messageID = id; + } + + @Override + protected void updateMessageID() throws MessagingException { + if (messageID != null) { + setHeader("Message-ID", messageID); + } else { + super.updateMessageID(); + } + } +} diff --git a/appjar/src/main/java/com/google/gerrit/server/ssh/Receive.java b/appjar/src/main/java/com/google/gerrit/server/ssh/Receive.java index 5b6740bc5d..7beb20dc68 100644 --- a/appjar/src/main/java/com/google/gerrit/server/ssh/Receive.java +++ b/appjar/src/main/java/com/google/gerrit/server/ssh/Receive.java @@ -31,6 +31,7 @@ import com.google.gerrit.client.reviewdb.PatchSet; import com.google.gerrit.client.reviewdb.ReviewDb; import com.google.gerrit.client.rpc.Common; import com.google.gerrit.git.PatchSetImporter; +import com.google.gerrit.server.ChangeMail; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.GerritServer; import com.google.gwtorm.client.OrmException; @@ -64,6 +65,8 @@ import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.mail.MessagingException; + /** Receives change upload over SSH using the Git receive-pack protocol. */ class Receive extends AbstractGitCommand { private static final String NEW_CHANGE = "refs/for/"; @@ -495,6 +498,18 @@ class Receive extends AbstractGitCommand { ru.update(walk); allNewChanges.add(change.getId()); + + try { + final ChangeMail cm = new ChangeMail(server, change); + cm.setFrom(userAccount.getId()); + cm.setPatchSet(ps, imp.getPatchSetInfo()); + cm.setReviewDb(db); + cm.addReviewers(reviewerId); + cm.addExtraCC(ccId); + cm.sendNewChange(); + } catch (MessagingException e) { + // TODO Log (like everything else) email send failures + } } private void appendPatchSets() {