Improve email footer format Gerrit change emails include metadata to support mail filters and mail search. In HTML emails, these are included in the email using a hidden block to avoid visual clutter. However, with [1], it is possible for the footer content to change from message to message in such a way that Gmail quoting moves the delta text into a block that is not hidden. Refactor the footer generation to embed each line in a hidden block. In this way, the content remains hidden when it is changed, and mail search/filters function as before. The list of footers is defined in the email classes rather than the templates to simplify this embedding. Whitespace around footers in HTML emails is partially preserved to improve readability in the message source. [1] I42f3c4fa6fd19ceac137c99fad42952fcf7b7d6b Bug: Issue 4936 Change-Id: Ic6713a587b14a6b0ecf658f4a047675afa97a854
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java index b30eb81..83ffab6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -1,4 +1,4 @@ -// Copyright (C) 2010 The Android Open Source Project +// Copyright (C) 2016 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. @@ -467,12 +467,20 @@ patchSetData.put("refName", patchSet.getRefName()); soyContext.put("patchSet", patchSetData); - soyContext.put("reviewerEmails", - getEmailsByState(ReviewerStateInternal.REVIEWER)); - soyContext.put("ccEmails", - getEmailsByState(ReviewerStateInternal.CC)); - // TODO(wyatta): patchSetInfo + + footers.add("Gerrit-MessageType: " + messageClass); + footers.add("Gerrit-Change-Id: " + change.getKey().get()); + footers.add("Gerrit-Change-Number: " + + Integer.toString(change.getChangeId())); + footers.add("Gerrit-PatchSet: " + patchSet.getPatchSetId()); + footers.add("Gerrit-Owner: " + getNameEmailFor(change.getOwner())); + for (String reviewer : getEmailsByState(ReviewerStateInternal.REVIEWER)) { + footers.add("Gerrit-Reviewer: " + reviewer); + } + for (String reviewer : getEmailsByState(ReviewerStateInternal.CC)) { + footers.add("Gerrit-CC: " + reviewer); + } } private Set<String> getEmailsByState(ReviewerStateInternal state) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java index d27c94e..0cc4152 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java
@@ -588,12 +588,19 @@ @Override protected void setupSoyContext() { super.setupSoyContext(); + boolean hasComments = false; try (Repository repo = getRepository()) { - soyContext.put("commentFiles", getCommentGroupsTemplateData(repo)); + List<Map<String, Object>> files = getCommentGroupsTemplateData(repo); + soyContext.put("commentFiles", files); + hasComments = !files.isEmpty(); } + soyContext.put("commentTimestamp", getCommentTimestamp()); soyContext.put("coverLetterBlocks", commentBlocksToSoyData(CommentFormatter.parse(getCoverLetter()))); + + footers.add("Gerrit-Comment-Date: " + getCommentTimestamp()); + footers.add("Gerrit-HasComments: " + (hasComments ? "Yes" : "No")); } private String getLine(PatchFile fileInfo, short side, int lineNbr) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NotificationEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NotificationEmail.java index baf0d93..0ded9d8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NotificationEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/NotificationEmail.java
@@ -123,5 +123,8 @@ Map<String, String> branchData = new HashMap<>(); branchData.put("shortName", branch.getShortName()); soyContext.put("branch", branchData); + + footers.add("Gerrit-Project: " + branch.getParentKey().get()); + footers.add("Gerrit-Branch: " + branch.getShortName()); } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index 1051b9d..fcbd790 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -48,11 +48,13 @@ import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Collection; import java.util.Date; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.List; import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; @@ -74,6 +76,7 @@ protected VelocityContext velocityContext; protected Map<String, Object> soyContext; protected Map<String, Object> soyContextEmailData; + protected List<String> footers; protected final EmailArguments args; protected Account.Id fromId; protected NotifyHandling notify = NotifyHandling.ALL; @@ -462,8 +465,10 @@ protected void setupSoyContext() { soyContext = new HashMap<>(); + footers = new ArrayList<>(); soyContext.put("messageClass", messageClass); + soyContext.put("footers", footers); soyContextEmailData = new HashMap<>(); soyContextEmailData.put("settingsUrl", getSettingsUrl());
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooter.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooter.soy index 4958bde..a034872 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooter.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooter.soy
@@ -19,15 +19,7 @@ /** * The .ChangeFooter template will determine the contents of the footer text * that will be appended to ALL emails related to changes. - * @param branch - * @param ccEmails - * @param change - * @param changeId * @param email - * @param messageClass - * @param patchSet - * @param projectName - * @param reviewerEmails */ {template .ChangeFooter autoescape="strict" kind="text"} --{sp} @@ -44,18 +36,4 @@ {if $email.changeUrl or $email.settingsUrl} {\n} {/if} - - Gerrit-MessageType: {$messageClass}{\n} - Gerrit-Change-Id: {$changeId}{\n} - Gerrit-Change-Number: {$change.changeNumber}{\n} - Gerrit-PatchSet: {$patchSet.patchSetId}{\n} - Gerrit-Project: {$projectName}{\n} - Gerrit-Branch: {$branch.shortName}{\n} - Gerrit-Owner: {$change.ownerEmail}{\n} - {foreach $reviewer in $reviewerEmails} - Gerrit-Reviewer: {$reviewer}{\n} - {/foreach} - {foreach $reviewer in $ccEmails} - Gerrit-CC: {$reviewer}{\n} - {/foreach} {/template}
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooterHtml.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooterHtml.soy index 28b2c28..5091cfe 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooterHtml.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/ChangeFooterHtml.soy
@@ -17,21 +17,9 @@ {namespace com.google.gerrit.server.mail.template} /** - * @param branch - * @param ccEmails - * @param change - * @param changeId * @param email - * @param messageClass - * @param patchSet - * @param projectName - * @param reviewerEmails */ {template .ChangeFooterHtml autoescape="strict" kind="html"} - {let $footerStyle kind="css"} - display: none; - {/let} - {if $email.changeUrl or $email.settingsUrl} <p> {if $email.changeUrl} @@ -44,22 +32,6 @@ </p> {/if} - <p style="{$footerStyle}"> - Gerrit-MessageType: {$messageClass}<br/> - Gerrit-Change-Id: {$changeId}<br/> - Gerrit-Change-Number: {$change.changeNumber}<br/> - Gerrit-PatchSet: {$patchSet.patchSetId}<br/> - Gerrit-Project: {$projectName}<br/> - Gerrit-Branch: {$branch.shortName}<br/> - Gerrit-Owner: {$change.ownerEmail} - {foreach $reviewer in $reviewerEmails} - Gerrit-Reviewer: {$reviewer}</br> - {/foreach} - {foreach $reviewer in $ccEmails} - Gerrit-CC: {$reviewer}</br> - {/foreach} - </p> - {if $email.changeUrl} <div itemscope itemtype="http://schema.org/EmailMessage"> <div itemscope itemprop="action" itemtype="http://schema.org/ViewAction">
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy index cce313b..73fdfba 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy
@@ -20,14 +20,6 @@ * The .CommentFooter template will determine the contents of the footer text * that will be appended to emails related to a user submitting comments on * changes. - * @param commentFiles - * @param commentTimestamp */ {template .CommentFooter autoescape="strict" kind="text"} - Gerrit-Comment-Date: {$commentTimestamp} - {if length($commentFiles) > 0} - Gerrit-HasComments: Yes - {else} - Gerrit-HasComments: No - {/if} {/template}
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooterHtml.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooterHtml.soy index 84af859..7bf28e7 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooterHtml.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooterHtml.soy
@@ -16,21 +16,5 @@ {namespace com.google.gerrit.server.mail.template} -/** - * @param commentFiles - * @param commentTimestamp - */ {template .CommentFooterHtml autoescape="strict" kind="html"} - {let $footerStyle kind="css"} - display: none; - {/let} - - <p style="{$footerStyle}"> - Gerrit-Comment-Date: {$commentTimestamp} - {if length($commentFiles) > 0} - Gerrit-HasComments: Yes - {else} - Gerrit-HasComments: No - {/if} - </p> {/template}
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Footer.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Footer.soy index 6467e95..24db2fd 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Footer.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/Footer.soy
@@ -20,6 +20,10 @@ * The .Footer template will determine the contents of the footer text * appended to the end of all outgoing emails after the ChangeFooter and * CommentFooter. + * @param footers */ -{template .Footer} +{template .Footer autoescape="strict" kind="text"} + {foreach $footer in $footers} + {$footer}{\n} + {/foreach} {/template}
diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/FooterHtml.soy b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/FooterHtml.soy index 9befa51..9f9c503 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mail/FooterHtml.soy +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mail/FooterHtml.soy
@@ -16,5 +16,14 @@ {namespace com.google.gerrit.server.mail.template} +/** + * @param footers + */ {template .FooterHtml autoescape="strict" kind="html"} + {\n} + {\n} + {foreach $footer in $footers} + <div style="display:none">{sp}{$footer}{sp}</div>{\n} + {/foreach} + {\n} {/template}