opendev/gerrit
Code Issues Proposed changes

Refactor email inline comments as template data

Browse Source 
Previously, the inline comment section of comment emails was constructed
by a set of Java methods in the CommentSender class. The comments were
built into a large string containing file groupings, file URLs, file
quotations, patch set IDs, line numbers, parent comment messages, and
comment messages formatted together. For text emails, this string can be
merely included directly in the email body, and was provided to the
templates under the `inlineComments` key.

With this change, CommentSender is modified to model the inline comments
information into a list of groups, organized identically to the grouping
found in the `inlineComments` string.

These groups are provided to the Soy template as a hierarchical list of
maps under the `commentFiles` key. The template now loops over this
data, recreating the same grouped comment list format.

The `getInlineComments` method is preserved for VTL support, and is
refactored to use the same group data.

Change-Id: I8816f349abf2d1c397177a0d3c105835a5b228bf
This commit is contained in:
Wyatt Allen
2016-09-27 11:37:20 -07:00
parent e47fe47537
commit 11bff0a6c6
3 changed files with 359 additions and 132 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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;
int currentPatchSetId = -1;
PatchFile currentFileData = null;
// Loop over the comments and collect them into groups based on the file
// location of the comment.
FileCommentGroup currentGroup = null;
for (Comment c : inlineComments) {
if (!c.key.filename.equals(currentFileName)
|| c.key.patchSetId != currentPatchSetId) {
String link = makeLink(change.getId(), c.key);
if (link != null) {
cmts.append(link).append('\n');
}
if (Patch.COMMIT_MSG.equals(c.key.filename)) {
cmts.append("Commit Message:\n\n");
} 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 it's a new group:
if (currentGroup == null
|| !c.key.filename.equals(currentGroup.filename)
|| c.key.patchSetId != currentGroup.patchSetId) {
currentGroup = new FileCommentGroup();
currentGroup.filename = c.key.filename;
currentGroup.patchSetId = c.key.patchSetId;
groups.add(currentGroup);
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) {
appendComment(cmts, lines, currentFileData, c);
if (currentGroup.fileData != null) {
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;
Comment.Range range = comment.range;
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');
if (comment.range != null) {
appendRangedComment(out, contextLines, currentFileData, comment);
} else {
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);
}
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);
}
appendLineComment(out, contextLines, currentFileData, comment);
}
}
/**
* 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;
Comment.Key key = new Comment.Key(child.parentUuid, child.key.filename,
child.key.patchSetId);
try {
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');
}
Optional<Comment> parent = getParent(child);
if (parent.isPresent()) {
out.append("> ")
.append(getShortenedCommentMessage(parent.get()))
.append('\n');
}
}
// Makes a link back to the given patch set and file.
private String makeLink(Change.Id changeId, Comment.Key key) {
String url = getGerritUrl();
if (url == null) {
return null;
/**
* Get the parent comment of a given comment.
* @param child the comment with a potential parent comment.
* @return an optional comment that will be present if the given comment has
* 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()
.append(url)
.append("#/c/").append(changeId)
.append('/').append(key.patchSetId)
.append('/').append(KeyUtil.encode(key.filename))
.toString();
Optional<Comment> parent;
Comment.Key key = new Comment.Key(child.parentUuid, child.key.filename,
child.key.patchSetId);
try {
return commentsUtil.get(args.db.get(), changeData.notes(), key);
} 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());
soyContextEmailData.put("hasInlineComments", hasInlineComments());
soyContext.put("commentFiles", getCommentGroupsTemplateData());
}
private String getLine(PatchFile fileInfo, short side, int lineNbr) {