Send comments as a separate email when published on push
When a user uses the new publish-comments option, the new patch set
email didn't contain the contents of the comments that were being
published. Actually getting these combined into one email will require a
fair bit of refactoring of templates.
In the meantime, just send the comments in a separate email, which from
the recipients' perspective is the same as if this feature didn't exist
and the author published comments via the web UI immediately after
pushing.
Change-Id: I20e542e1fb044ee678855fc83afcfaa6df65b326
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index b9b6812..2ce0b3b 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -69,12 +69,14 @@
import com.google.gerrit.testutil.TestTimeUtil;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.regex.Pattern;
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
@@ -1509,11 +1511,36 @@
.containsExactly("comment1", "comment2", "comment3");
assertThat(getLastMessage(r.getChangeId())).isEqualTo("Uploaded patch set 3.\n\n(3 comments)");
- Message m = Iterables.getLast(sender.getMessages());
- assertThat(m.body()).contains("I'd like you to reexamine a change");
- // TODO(dborowitz): The template currently doesn't include the ChangeMessage (aka cover letter)
- // text at all. Update this to look for the (3 commnets) text once that's fixed.
- assertThat(m.body()).doesNotMatch("[Cc]omment");
+ List<String> messages =
+ sender
+ .getMessages()
+ .stream()
+ .map(m -> m.body())
+ .sorted(Comparator.comparingInt(m -> m.contains("reexamine") ? 0 : 1))
+ .collect(toList());
+ assertThat(messages).hasSize(2);
+
+ assertThat(messages.get(0)).contains("Gerrit-MessageType: newpatchset");
+ assertThat(messages.get(0)).contains("I'd like you to reexamine a change");
+ assertThat(messages.get(0)).doesNotContain("Uploaded patch set 3");
+
+ assertThat(messages.get(1)).contains("Gerrit-MessageType: comment");
+ assertThat(messages.get(1))
+ .containsMatch(
+ Pattern.compile(
+ // A little weird that the comment email contains this text, but it's actually
+ // what's in the ChangeMessage. Really we should fuse the emails into one, but until
+ // then, this test documents the current behavior.
+ "Uploaded patch set 3\\.\n"
+ + "\n"
+ + "\\(3 comments\\)\\n.*"
+ + "PS1, Line 1:.*"
+ + "comment1\\n.*"
+ + "PS1, Line 1:.*"
+ + "comment2\\n.*"
+ + "PS2, Line 1:.*"
+ + "comment3\\n",
+ Pattern.DOTALL));
}
@Test
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
index a86e163..382af51 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
@@ -23,6 +23,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -42,11 +43,13 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.change.ChangeKindCache;
+import com.google.gerrit.server.change.EmailReviewComments;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.ReceiveCommits.MagicBranchInput;
import com.google.gerrit.server.mail.MailUtil.MailRecipients;
import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
+import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectControl;
@@ -104,6 +107,7 @@
private final ChangeKindCache changeKindCache;
private final ChangeMessagesUtil cmUtil;
private final CommentsUtil commentsUtil;
+ private final EmailReviewComments.Factory emailCommentsFactory;
private final ExecutorService sendEmailExecutor;
private final RevisionCreated revisionCreated;
private final CommentAdded commentAdded;
@@ -127,7 +131,7 @@
private final MailRecipients recipients = new MailRecipients();
private RevCommit commit;
private ReceiveCommand cmd;
- private Change change;
+ private ChangeNotes notes;
private PatchSet newPatchSet;
private ChangeKind changeKind;
private ChangeMessage msg;
@@ -146,6 +150,7 @@
ChangeKindCache changeKindCache,
ChangeMessagesUtil cmUtil,
CommentsUtil commentsUtil,
+ EmailReviewComments.Factory emailCommentsFactory,
RevisionCreated revisionCreated,
CommentAdded commentAdded,
MergedByPushOp.Factory mergedByPushOpFactory,
@@ -171,6 +176,7 @@
this.changeKindCache = changeKindCache;
this.cmUtil = cmUtil;
this.commentsUtil = commentsUtil;
+ this.emailCommentsFactory = emailCommentsFactory;
this.revisionCreated = revisionCreated;
this.commentAdded = commentAdded;
this.mergedByPushOpFactory = mergedByPushOpFactory;
@@ -218,13 +224,14 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
- change = ctx.getChange();
+ notes = ctx.getNotes();
+ Change change = notes.getChange();
if (change == null || change.getStatus().isClosed()) {
rejectMessage = CHANGE_IS_CLOSED;
return false;
}
if (groups.isEmpty()) {
- PatchSet prevPs = psUtil.current(ctx.getDb(), ctx.getNotes());
+ PatchSet prevPs = psUtil.current(ctx.getDb(), notes);
groups = prevPs != null ? prevPs.getGroups() : ImmutableList.<String>of();
}
@@ -240,7 +247,7 @@
approvals.putAll(magicBranch.labels);
Set<String> hashtags = magicBranch.hashtags;
if (hashtags != null && !hashtags.isEmpty()) {
- hashtags.addAll(ctx.getNotes().getHashtags());
+ hashtags.addAll(notes.getHashtags());
update.setHashtags(hashtags);
}
if (magicBranch.topic != null && !magicBranch.topic.equals(ctx.getChange().getTopic())) {
@@ -260,7 +267,7 @@
change.setWorkInProgress(true);
update.setWorkInProgress(true);
}
- if (magicBranch.shouldPublishComments()) {
+ if (shouldPublishComments()) {
comments = publishComments(ctx);
}
}
@@ -425,47 +432,35 @@
@Override
public void postUpdate(Context ctx) throws Exception {
if (changeKind != ChangeKind.TRIVIAL_REBASE) {
- Runnable sender =
- new Runnable() {
- @Override
- public void run() {
- try {
- ReplacePatchSetSender cm =
- replacePatchSetFactory.create(
- projectControl.getProject().getNameKey(), change.getId());
- cm.setFrom(ctx.getAccountId());
- cm.setPatchSet(newPatchSet, info);
- cm.setChangeMessage(msg.getMessage(), ctx.getWhen());
- if (magicBranch != null) {
- cm.setNotify(magicBranch.notify);
- cm.setAccountsToNotify(magicBranch.getAccountsToNotify());
- }
- cm.addReviewers(recipients.getReviewers());
- cm.addExtraCC(recipients.getCcOnly());
- cm.send();
- } catch (Exception e) {
- log.error("Cannot send email for new patch set " + newPatchSet.getId(), e);
- }
- }
-
- @Override
- public String toString() {
- return "send-email newpatchset";
- }
- };
-
+ // TODO(dborowitz): Merge email templates so we only have to send one.
+ Runnable e = new ReplaceEmailTask(ctx);
if (requestScopePropagator != null) {
@SuppressWarnings("unused")
- Future<?> possiblyIgnoredError =
- sendEmailExecutor.submit(requestScopePropagator.wrap(sender));
+ Future<?> possiblyIgnoredError = sendEmailExecutor.submit(requestScopePropagator.wrap(e));
} else {
- sender.run();
+ e.run();
}
}
NotifyHandling notify =
magicBranch != null && magicBranch.notify != null ? magicBranch.notify : NotifyHandling.ALL;
- revisionCreated.fire(change, newPatchSet, ctx.getAccount(), ctx.getWhen(), notify);
+
+ if (shouldPublishComments()) {
+ emailCommentsFactory
+ .create(
+ notify,
+ magicBranch != null ? magicBranch.getAccountsToNotify() : ImmutableListMultimap.of(),
+ notes,
+ newPatchSet,
+ ctx.getUser().asIdentifiedUser(),
+ msg,
+ comments,
+ msg.getMessage(),
+ ImmutableList.of()) // TODO(dborowitz): Include labels.
+ .sendAsync();
+ }
+
+ revisionCreated.fire(notes.getChange(), newPatchSet, ctx.getAccount(), ctx.getWhen(), notify);
try {
fireCommentAddedEvent(ctx);
} catch (Exception e) {
@@ -476,6 +471,40 @@
}
}
+ private class ReplaceEmailTask implements Runnable {
+ private final Context ctx;
+
+ private ReplaceEmailTask(Context ctx) {
+ this.ctx = ctx;
+ }
+
+ @Override
+ public void run() {
+ try {
+ ReplacePatchSetSender cm =
+ replacePatchSetFactory.create(
+ projectControl.getProject().getNameKey(), notes.getChangeId());
+ cm.setFrom(ctx.getAccount().getId());
+ cm.setPatchSet(newPatchSet, info);
+ cm.setChangeMessage(msg.getMessage(), ctx.getWhen());
+ if (magicBranch != null) {
+ cm.setNotify(magicBranch.notify);
+ cm.setAccountsToNotify(magicBranch.getAccountsToNotify());
+ }
+ cm.addReviewers(recipients.getReviewers());
+ cm.addExtraCC(recipients.getCcOnly());
+ cm.send();
+ } catch (Exception e) {
+ log.error("Cannot send email for new patch set " + newPatchSet.getId(), e);
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "send-email newpatchset";
+ }
+ }
+
private void fireCommentAddedEvent(Context ctx) throws OrmException {
if (approvals.isEmpty()) {
return;
@@ -487,7 +516,7 @@
* show a transition from an oldValue of 0 to the new value.
*/
ChangeControl changeControl =
- changeControlFactory.controlFor(ctx.getDb(), change, ctx.getUser());
+ changeControlFactory.controlFor(ctx.getDb(), notes.getChange(), ctx.getUser());
List<LabelType> labels = changeControl.getLabelTypes().getLabelTypes();
Map<String, Short> allApprovals = new HashMap<>();
Map<String, Short> oldApprovals = new HashMap<>();
@@ -503,7 +532,13 @@
}
commentAdded.fire(
- change, newPatchSet, ctx.getAccount(), null, allApprovals, oldApprovals, ctx.getWhen());
+ notes.getChange(),
+ newPatchSet,
+ ctx.getAccount(),
+ null,
+ allApprovals,
+ oldApprovals,
+ ctx.getWhen());
}
public PatchSet getPatchSet() {
@@ -511,7 +546,7 @@
}
public Change getChange() {
- return change;
+ return notes.getChange();
}
public String getRejectMessage() {
@@ -546,4 +581,8 @@
return null;
}
}
+
+ private boolean shouldPublishComments() {
+ return magicBranch != null && magicBranch.shouldPublishComments();
+ }
}
