CommentSender: Use UrlFormatter to get URLs for file and comments
CommentSender is hard coded to provide the direct URL to a filename
within a patch, and to the inline comments. The hard-coded URLs are
in the legacy format, meaning they do not include the project name
and are inconsistent with other links included in notifications and
console messages from Gerrit.
Hard-coding the URLs also means it's not possible for plugins to
provide alternative URLs.
Add new methods on the UrlFormatter interface, and provide default
implementations that include the project name, thus making the URLs
consistent with other URLs emitted by Gerrit, and also allowing
plugins to provide alternatives.
Change-Id: I0991dea120856c2c2bf1304749961e28b0b73a24
diff --git a/java/com/google/gerrit/server/config/UrlFormatter.java b/java/com/google/gerrit/server/config/UrlFormatter.java
index 066a3ca..3e94c30 100644
--- a/java/com/google/gerrit/server/config/UrlFormatter.java
+++ b/java/com/google/gerrit/server/config/UrlFormatter.java
@@ -47,6 +47,19 @@
return getWebUrl().map(url -> url + "c/" + project.get() + "/+/" + id.get());
}
+ /** Returns the URL for viewing a file in a given patch set of a change. */
+ default Optional<String> getPatchFileView(Change change, int patchsetId, String filename) {
+ return getChangeViewUrl(change.getProject(), change.getId())
+ .map(url -> url + "/" + patchsetId + "/" + filename);
+ }
+
+ /** Returns the URL for viewing a comment in a file in a given patch set of a change. */
+ default Optional<String> getInlineCommentView(
+ Change change, int patchsetId, String filename, short side, int startLine) {
+ return getPatchFileView(change, patchsetId, filename)
+ .map(url -> url + String.format("@%s%d", side == 0 ? "a" : "", startLine));
+ }
+
/** Returns a URL pointing to a section of the settings page. */
default Optional<String> getSettingsUrl(@Nullable String section) {
return getWebUrl()
diff --git a/java/com/google/gerrit/server/mail/send/CommentSender.java b/java/com/google/gerrit/server/mail/send/CommentSender.java
index 69a7926..e78a60a 100644
--- a/java/com/google/gerrit/server/mail/send/CommentSender.java
+++ b/java/com/google/gerrit/server/mail/send/CommentSender.java
@@ -75,21 +75,19 @@
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;
- }
+ public String getFileLink() {
+ return args.urlFormatter
+ .get()
+ .getPatchFileView(change, patchSetId, KeyUtil.encode(filename))
+ .orElse(null);
+ }
- return new StringBuilder()
- .append(url)
- .append("#/c/")
- .append(change.getId())
- .append('/')
- .append(patchSetId)
- .append('/')
- .append(KeyUtil.encode(filename))
- .toString();
+ /** @return a web link to a comment within a given patch set and file. */
+ public String getCommentLink(short side, int startLine) {
+ return args.urlFormatter
+ .get()
+ .getInlineCommentView(change, patchSetId, KeyUtil.encode(filename), side, startLine)
+ .orElse(null);
}
/**
@@ -392,7 +390,7 @@
for (CommentSender.FileCommentGroup group : getGroupedInlineComments(repo)) {
Map<String, Object> groupData = new HashMap<>();
- groupData.put("link", group.getLink());
+ groupData.put("link", group.getFileLink());
groupData.put("title", group.getTitle());
groupData.put("patchSetId", group.patchSetId);
@@ -421,11 +419,9 @@
// Set the comment link.
if (comment.lineNbr == 0) {
- commentData.put("link", group.getLink());
- } else if (comment.side == 0) {
- commentData.put("link", group.getLink() + "@a" + startLine);
+ commentData.put("link", group.getFileLink());
} else {
- commentData.put("link", group.getLink() + '@' + startLine);
+ commentData.put("link", group.getCommentLink(comment.side, startLine));
}
// Set robot comment data.
diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
index 1ef3b25..b0f1e75 100644
--- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
+++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java
@@ -298,7 +298,7 @@
return null;
}
- public String getGerritUrl() {
+ private String getGerritUrl() {
return args.urlFormatter.get().getWebUrl().orElse(null);
}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index e9d2611..535d461 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -629,13 +629,17 @@
+ "comments\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/1/a.txt \n"
+ "File a.txt:\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/1/a.txt@a2 \n"
+ "PS1, Line 2: \n"
@@ -643,7 +647,9 @@
+ "\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/1/a.txt@1 \n"
+ "PS1, Line 1: boring\n"
@@ -651,13 +657,17 @@
+ "\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/2/a.txt \n"
+ "File a.txt:\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/2/a.txt@a1 \n"
+ "PS2, Line 1: \n"
@@ -665,7 +675,9 @@
+ "\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/2/a.txt@a2 \n"
+ "PS2, Line 2: \n"
@@ -673,7 +685,9 @@
+ "\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/2/a.txt@1 \n"
+ "PS2, Line 1: interesting\n"
@@ -681,7 +695,9 @@
+ "\n"
+ "\n"
+ url
- + "#/c/"
+ + "c/"
+ + project.get()
+ + "/+/"
+ c
+ "/2/a.txt@2 \n"
+ "PS2, Line 2: nten\n"
