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
2016-11-18 14:32:05 -08:00
parent d1091dfc81
commit deb7f68c4a
10 changed files with 44 additions and 82 deletions
--- 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 @@ public abstract class ChangeEmail extends NotificationEmail {
    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) {
--- 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 @@ public class CommentSender extends ReplyToChangeSender {
  @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) {
--- 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 @@ public abstract class NotificationEmail extends OutgoingEmail {
    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());
  }
 }
--- 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.MalformedURLException;
 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 @@ public abstract class OutgoingEmail {
  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 @@ public abstract class OutgoingEmail {

  protected void setupSoyContext() {
    soyContext = new HashMap<>();
+    footers = new ArrayList<>();

    soyContext.put("messageClass", messageClass);
+    soyContext.put("footers", footers);

    soyContextEmailData = new HashMap<>();
    soyContextEmailData.put("settingsUrl", getSettingsUrl());
--- 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}
--- 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">
--- 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}
--- 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}
--- 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}
--- 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}