Merge "Refactor email inline comments as template data"

gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java

@@ -39,9 +39,12 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 
@@ -55,6 +58,44 @@ public class CommentSender extends ReplyToChangeSender {
     CommentSender create(Project.NameKey project, Change.Id id);
   }
 
+  private class FileCommentGroup {
+    public String filename;
+    public int patchSetId;
+    public PatchFile fileData;
+    public List<Comment> comments = new ArrayList<>();
+
+    /**
+     * @return a web link to the given patch set and file.
+     */
+    public String getLink() {
+      String url = getGerritUrl();
+      if (url == null) {
+        return null;
+      }
+
+      return new StringBuilder()
+        .append(url)
+        .append("#/c/").append(change.getId())
+        .append('/').append(patchSetId)
+        .append('/').append(KeyUtil.encode(filename))
+        .toString();
+    }
+
+    /**
+     * @return A title for the group, i.e. "Commit Message", "Merge List", or
+     * "File [[filename]]".
+     */
+    public String getTitle() {
+      if (Patch.COMMIT_MSG.equals(filename)) {
+        return "Commit Message";
+      } else if (Patch.MERGE_LIST.equals(filename)) {
+        return "Merge List";
+      } else {
+        return "File " + filename;
+      }
+    }
+  }
+
   private List<Comment> inlineComments = Collections.emptyList();
   private final CommentsUtil commentsUtil;
 
@@ -102,17 +143,53 @@ public class CommentSender extends ReplyToChangeSender {
     appendText(textTemplate("CommentFooter"));
   }
 
+  /**
+   * No longer used outside Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
   public boolean hasInlineComments() {
     return !inlineComments.isEmpty();
   }
 
+  /**
+   * No longer used outside Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
   public String getInlineComments() {
     return getInlineComments(1);
   }
 
+  /**
+   * No longer used outside Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
   public String getInlineComments(int lines) {
     StringBuilder cmts = new StringBuilder();
+    for (FileCommentGroup group : getGroupedInlineComments()) {
+      String link = group.getLink();
+      if (link != null) {
+        cmts.append(link).append('\n');
+      }
+      cmts.append(group.getTitle()).append(":\n\n");
+      for (Comment c : group.comments) {
+        appendComment(cmts, lines, group.fileData, c);
+      }
+      cmts.append("\n\n");
+    }
+    return cmts.toString();
+  }
+
+  /**
+   * @return a list of FileCommentGroup objects representing the inline comments
+   * grouped by the file.
+   */
+  private List<CommentSender.FileCommentGroup> getGroupedInlineComments() {
+    List<CommentSender.FileCommentGroup> groups = new ArrayList<>();
     try (Repository repo = getRepository()) {
+      // Get the patch list:
       PatchList patchList = null;
       if (repo != null) {
         try {
@@ -122,29 +199,21 @@ public class CommentSender extends ReplyToChangeSender {
         }
       }
 
-      String currentFileName = null;
+      // Loop over the comments and collect them into groups based on the file
-      int currentPatchSetId = -1;
+      // location of the comment.
-      PatchFile currentFileData = null;
+      FileCommentGroup currentGroup = null;
       for (Comment c : inlineComments) {
-        if (!c.key.filename.equals(currentFileName)
+        // If it's a new group:
-            || c.key.patchSetId != currentPatchSetId) {
+        if (currentGroup == null
-          String link = makeLink(change.getId(), c.key);
+            || !c.key.filename.equals(currentGroup.filename)
-          if (link != null) {
+            || c.key.patchSetId != currentGroup.patchSetId) {
-            cmts.append(link).append('\n');
+          currentGroup = new FileCommentGroup();
-          }
+          currentGroup.filename = c.key.filename;
-          if (Patch.COMMIT_MSG.equals(c.key.filename)) {
+          currentGroup.patchSetId = c.key.patchSetId;
-            cmts.append("Commit Message:\n\n");
+          groups.add(currentGroup);
-          } else if (Patch.MERGE_LIST.equals(c.key.filename)) {
-            cmts.append("Merge List:\n\n");
-          } else {
-            cmts.append("File ").append(c.key.filename).append(":\n\n");
-          }
-          currentFileName = c.key.filename;
-          currentPatchSetId = c.key.patchSetId;
-
           if (patchList != null) {
             try {
-              currentFileData =
+              currentGroup.fileData =
                   new PatchFile(repo, patchList, c.key.filename);
             } catch (IOException e) {
               log.warn(String.format(
@@ -152,20 +221,25 @@ public class CommentSender extends ReplyToChangeSender {
                   c.key.filename,
                   patchList.getNewId().name(),
                   projectState.getProject().getName()), e);
-              currentFileData = null;
+              currentGroup.fileData = null;
             }
           }
         }
 
-        if (currentFileData != null) {
+        if (currentGroup.fileData != null) {
-          appendComment(cmts, lines, currentFileData, c);
+          currentGroup.comments.add(c);
         }
-        cmts.append("\n\n");
       }
     }
-    return cmts.toString();
+
+    return groups;
   }
 
+  /**
+   * No longer used except for Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
   private void appendComment(StringBuilder out, int contextLines,
       PatchFile currentFileData, Comment comment) {
     if (comment instanceof RobotComment) {
@@ -176,62 +250,107 @@ public class CommentSender extends ReplyToChangeSender {
          .append(robotComment.robotRunId)
          .append("):\n");
     }
-    short side = comment.side;
+    if (comment.range != null) {
-    Comment.Range range = comment.range;
+      appendRangedComment(out, contextLines, currentFileData, comment);
-    if (range != null) {
-      String prefix = "PS" + comment.key.patchSetId
-        + ", Line " + range.startLine + ": ";
-      for (int n = range.startLine; n <= range.endLine; n++) {
-        out.append(n == range.startLine
-            ? prefix
-            : Strings.padStart(": ", prefix.length(), ' '));
-        String s = getLine(currentFileData, side, n);
-        if (n == range.startLine && n == range.endLine) {
-          s = s.substring(
-              Math.min(range.startChar, s.length()),
-              Math.min(range.endChar, s.length()));
-        } else if (n == range.startLine) {
-          s = s.substring(Math.min(range.startChar, s.length()));
-        } else if (n == range.endLine) {
-          s = s.substring(0, Math.min(range.endChar, s.length()));
-        }
-        out.append(s).append('\n');
-      }
-      appendQuotedParent(out, comment);
-      out.append(comment.message.trim()).append('\n');
     } else {
-      int lineNbr = comment.lineNbr;
+      appendLineComment(out, contextLines, currentFileData, comment);
-
-      // Initialize maxLines to the known line number.
-      int maxLines = lineNbr;
-
-      try {
-        maxLines = currentFileData.getLineCount(side);
-      } catch (IOException err) {
-        // The file could not be read, leave the max as is.
-        log.warn(String.format("Failed to read file %s on side %d",
-            comment.key.filename, side), err);
-      } catch (NoSuchEntityException err) {
-        // The file could not be read, leave the max as is.
-        log.warn(String.format("Side %d of file %s didn't exist",
-             side, comment.key.filename), err);
-      }
-
-      final int startLine = Math.max(1, lineNbr - contextLines + 1);
-      final int stopLine = Math.min(maxLines, lineNbr + contextLines);
-
-      for (int line = startLine; line <= lineNbr; ++line) {
-        appendFileLine(out, currentFileData, side, line);
-      }
-      appendQuotedParent(out, comment);
-      out.append(comment.message.trim()).append('\n');
-
-      for (int line = lineNbr + 1; line < stopLine; ++line) {
-        appendFileLine(out, currentFileData, side, line);
-      }
     }
   }
 
+  /**
+   * No longer used except for Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
+  private void appendRangedComment(StringBuilder out, int contextLines,
+      PatchFile fileData, Comment comment) {
+    String prefix = getCommentLinePrefix(comment);
+    String emptyPrefix = Strings.padStart(": ", prefix.length(), ' ');
+    boolean firstLine = true;
+    for (String line : getLinesByRange(comment.range, fileData, comment.side)) {
+      out.append(firstLine ? prefix : emptyPrefix)
+          .append(line)
+          .append('\n');
+      firstLine = false;
+    }
+    appendQuotedParent(out, comment);
+    out.append(comment.message.trim()).append('\n');
+  }
+
+  private String getCommentLinePrefix(Comment comment) {
+    int lineNbr = comment.range == null ?
+        comment.lineNbr : comment.range.startLine;
+    return "PS" + comment.key.patchSetId + ", Line " + lineNbr + ": ";
+  }
+
+  /**
+   * @return the lines of file content in fileData that are encompassed by range
+   * on the given side.
+   */
+  private List<String> getLinesByRange(Comment.Range range,
+      PatchFile fileData, short side) {
+    List<String> lines = new ArrayList<>();
+
+    for (int n = range.startLine; n <= range.endLine; n++) {
+      String s = getLine(fileData, side, n);
+      if (n == range.startLine && n == range.endLine) {
+        s = s.substring(
+            Math.min(range.startChar, s.length()),
+            Math.min(range.endChar, s.length()));
+      } else if (n == range.startLine) {
+        s = s.substring(Math.min(range.startChar, s.length()));
+      } else if (n == range.endLine) {
+        s = s.substring(0, Math.min(range.endChar, s.length()));
+      }
+      lines.add(s);
+    }
+    return lines;
+  }
+
+  /**
+   * No longer used except for Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
+  private void appendLineComment(StringBuilder out, int contextLines,
+      PatchFile currentFileData, Comment comment) {
+    short side = comment.side;
+    int lineNbr = comment.lineNbr;
+
+    // Initialize maxLines to the known line number.
+    int maxLines = lineNbr;
+
+    try {
+      maxLines = currentFileData.getLineCount(side);
+    } catch (IOException err) {
+      // The file could not be read, leave the max as is.
+      log.warn(String.format("Failed to read file %s on side %d",
+          comment.key.filename, side), err);
+    } catch (NoSuchEntityException err) {
+      // The file could not be read, leave the max as is.
+      log.warn(String.format("Side %d of file %s didn't exist",
+           side, comment.key.filename), err);
+    }
+
+    int startLine = Math.max(1, lineNbr - contextLines + 1);
+    int stopLine = Math.min(maxLines, lineNbr + contextLines);
+
+    for (int line = startLine; line <= lineNbr; ++line) {
+      appendFileLine(out, currentFileData, side, line);
+    }
+    appendQuotedParent(out, comment);
+    out.append(comment.message.trim()).append('\n');
+
+    for (int line = lineNbr + 1; line < stopLine; ++line) {
+      appendFileLine(out, currentFileData, side, line);
+    }
+  }
+
+  /**
+   * No longer used except for Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
   private void appendFileLine(StringBuilder cmts, PatchFile fileData,
       short side, int line) {
     String lineStr = getLine(fileData, side, line);
@@ -242,45 +361,131 @@ public class CommentSender extends ReplyToChangeSender {
         .append("\n");
   }
 
+  /**
+   * No longer used except for Velocity. Remove this method when VTL support is
+   * removed.
+   */
+  @Deprecated
   private void appendQuotedParent(StringBuilder out, Comment child) {
-    if (child.parentUuid != null) {
+    Optional<Comment> parent = getParent(child);
-      Optional<Comment> parent;
+    if (parent.isPresent()) {
-      Comment.Key key = new Comment.Key(child.parentUuid, child.key.filename,
+      out.append("> ")
-          child.key.patchSetId);
+          .append(getShortenedCommentMessage(parent.get()))
-      try {
+          .append('\n');
-        parent = commentsUtil.get(args.db.get(), changeData.notes(), key);
-      } catch (OrmException e) {
-        log.warn("Could not find the parent of this comment: "
-            + child.toString());
-        parent = Optional.empty();
-      }
-      if (parent.isPresent()) {
-        String msg = parent.get().message.trim();
-        if (msg.length() > 75) {
-          msg = msg.substring(0, 75);
-        }
-        int lf = msg.indexOf('\n');
-        if (lf > 0) {
-          msg = msg.substring(0, lf);
-        }
-        out.append("> ").append(msg).append('\n');
-      }
     }
   }
 
-  // Makes a link back to the given patch set and file.
+  /**
-  private String makeLink(Change.Id changeId, Comment.Key key) {
+   * Get the parent comment of a given comment.
-    String url = getGerritUrl();
+   * @param child the comment with a potential parent comment.
-    if (url == null) {
+   * @return an optional comment that will be  present if the given comment has
-      return null;
+   * a parent, and is empty if it does not.
+   */
+  private Optional<Comment> getParent(Comment child) {
+    if (child.parentUuid == null) {
+      return Optional.empty();
     }
 
-    return new StringBuilder()
+    Optional<Comment> parent;
-      .append(url)
+    Comment.Key key = new Comment.Key(child.parentUuid, child.key.filename,
-      .append("#/c/").append(changeId)
+          child.key.patchSetId);
-      .append('/').append(key.patchSetId)
+    try {
-      .append('/').append(KeyUtil.encode(key.filename))
+      return commentsUtil.get(args.db.get(), changeData.notes(), key);
-      .toString();
+    } catch (OrmException e) {
+      log.warn("Could not find the parent of this comment: "
+          + child.toString());
+      return Optional.empty();
+    }
+  }
+
+  /**
+   * Retrieve the file lines refered to by a comment.
+   * @param comment The comment that refers to some file contents. The comment
+   *     may be a line comment or a ranged comment.
+   * @param fileData The file on which the comment appears.
+   * @return file contents referred to by the comment. If the comment is a line
+   *     comment, the result will be a list of one string. Otherwise it will be
+   *     a list of one or more strings.
+   */
+  private List<String> getLinesOfComment(Comment comment, PatchFile fileData) {
+    List<String> lines = new ArrayList<>();
+    if (comment.range == null) {
+      lines.add(getLine(fileData, comment.side, comment.lineNbr));
+    } else {
+      lines.addAll(getLinesByRange(comment.range, fileData, comment.side));
+    }
+    return lines;
+  }
+
+  /**
+   * @return a shortened version of the given comment's message. Will be
+   * shortened to 75 characters or the first line, whichever is shorter.
+   */
+  private String getShortenedCommentMessage(Comment comment) {
+    String msg = comment.message.trim();
+    if (msg.length() > 75) {
+      msg = msg.substring(0, 75);
+    }
+    int lf = msg.indexOf('\n');
+    if (lf > 0) {
+      msg = msg.substring(0, lf);
+    }
+    return msg;
+  }
+
+  /**
+   * @return grouped inline comment data mapped to data structures that are
+   * suitable for passing into Soy.
+   */
+  private List<Map<String, Object>> getCommentGroupsTemplateData() {
+    List<Map<String, Object>> commentGroups = new ArrayList<>();
+
+    for (CommentSender.FileCommentGroup group : getGroupedInlineComments()) {
+      Map<String, Object> groupData = new HashMap<>();
+      groupData.put("link", group.getLink());
+      groupData.put("title", group.getTitle());
+      groupData.put("patchSetId", group.patchSetId);
+
+      List<Map<String, Object>> commentsList = new ArrayList<>();
+      for (Comment comment : group.comments) {
+        Map<String, Object> commentData = new HashMap<>();
+        commentData.put("lines", getLinesOfComment(comment, group.fileData));
+        commentData.put("message", comment.message.trim());
+
+        String prefix = getCommentLinePrefix(comment);
+        commentData.put("linePrefix", prefix);
+        commentData.put("linePrefixEmpty",
+            Strings.padStart(": ", prefix.length(), ' '));
+
+        if (comment.range == null) {
+          commentData.put("startLine", comment.lineNbr);
+        } else {
+          commentData.put("startLine", comment.range.startLine);
+          commentData.put("endLine", comment.range.endLine);
+        }
+
+        if (comment instanceof RobotComment) {
+          RobotComment robotComment = (RobotComment) comment;
+          commentData.put("isRobotComment", true);
+          commentData.put("robotId", robotComment.robotId);
+          commentData.put("robotRunId", robotComment.robotRunId);
+        } else {
+          commentData.put("isRobotComment", false);
+        }
+
+        Optional<Comment> parent = getParent(comment);
+        if (parent.isPresent()) {
+          commentData.put("parentMessage",
+              getShortenedCommentMessage(parent.get()));
+        }
+
+        commentsList.add(commentData);
+      }
+      groupData.put("comments", commentsList);
+
+      commentGroups.add(groupData);
+    }
+    return commentGroups;
   }
 
   private Repository getRepository() {
@@ -294,8 +499,7 @@ public class CommentSender extends ReplyToChangeSender {
   @Override
   protected void setupSoyContext() {
     super.setupSoyContext();
-    soyContextEmailData.put("inlineComments", getInlineComments());
+    soyContext.put("commentFiles", getCommentGroupsTemplateData());
-    soyContextEmailData.put("hasInlineComments", hasInlineComments());
   }
 
   private String getLine(PatchFile fileInfo, short side, int lineNbr) {

gerrit-server/src/main/resources/com/google/gerrit/server/mail/Comment.soy

@@ -23,24 +23,47 @@
  * @param coverLetter
  * @param email
  * @param fromName
+ * @param commentFiles
  */
 {template .Comment autoescape="strict" kind="text"}
-  {if $coverLetter or $email.hasInlineComments}
+  {$fromName} has posted comments on this change.
-    {$fromName} has posted comments on this change.
+  {if $email.changeUrl} ( {$email.changeUrl} ){/if}{\n}
-    {if $email.changeUrl} ( {$email.changeUrl} ){/if}{\n}
-    {\n}
-    Change subject: {$change.subject}{\n}
-    ......................................................................{\n}
-    {if $coverLetter}
-      {\n}
-      {\n}
-      {$coverLetter}
-    {/if}
-    {if $email.hasInlineComments}
-      {\n}
-      {\n}
-      {$email.inlineComments}
-    {/if}
-  {/if}
   {\n}
-{/template}
+  Change subject: {$change.subject}{\n}
+  ......................................................................{\n}
+  {if $coverLetter}
+    {\n}
+    {\n}
+    {$coverLetter}{\n}
+    {\n}
+  {/if}
+
+  {foreach $group in $commentFiles}
+    {$group.link}{\n}
+    {$group.title}:{\n}
+    {\n}
+
+    {foreach $comment in $group.comments}
+      {if $comment.isRobotComment}
+        Robot Comment from {$comment.robotId} (run ID {$comment.robotRunId}):
+        {\n}
+      {/if}
+
+      {foreach $line in $comment.lines}
+        {if isFirst($line)}
+          {$comment.linePrefix}
+        {else}
+          {$comment.linePrefixEmpty}
+        {/if}
+        {$line}{\n}
+      {/foreach}
+      {if $comment.parentMessage}
+        >{sp}{$comment.parentMessage}{\n}
+      {/if}
+      {$comment.message}{\n}
+      {\n}
+      {\n}
+    {/foreach}
+  {/foreach}
+  {\n}
+{/template}

gerrit-server/src/main/resources/com/google/gerrit/server/mail/CommentFooter.soy

@@ -20,10 +20,10 @@
  * 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 email
+ * @param commentFiles
  */
 {template .CommentFooter autoescape="strict" kind="text"}
-  {if $email.hasInlineComments}
+  {if length($commentFiles) > 0}
     Gerrit-HasComments: Yes
   {else}
     Gerrit-HasComments: No