opendev/gerrit
Code Issues Proposed changes

CommentSender: Use UrlFormatter to get URLs for file and comments

Browse Source 
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
This commit is contained in:
David Pursehouse
2019-06-18 14:40:16 +09:00
parent 01271ec9c7
commit 29ab7f8a5d
4 changed files with 53 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
java/com/google/gerrit/server/config/UrlFormatter.java
View File

@@ -47,6 +47,19 @@ public interface UrlFormatter {
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()

32
java/com/google/gerrit/server/mail/send/CommentSender.java
View File

@@ -75,21 +75,19 @@ public class CommentSender extends ReplyToChangeSender {
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 @@ public class CommentSender extends ReplyToChangeSender {
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 @@ public class CommentSender extends ReplyToChangeSender {
// 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.

2
java/com/google/gerrit/server/mail/send/OutgoingEmail.java
View File

@@ -298,7 +298,7 @@ public abstract class OutgoingEmail {
return null;
}
public String getGerritUrl() {
private String getGerritUrl() {
return args.urlFormatter.get().getWebUrl().orElse(null);
}

32
javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
View File

@@ -629,13 +629,17 @@ public class CommentsIT extends AbstractDaemonTest {
+ "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 @@ public class CommentsIT extends AbstractDaemonTest {
+ "\n"
+ "\n"
+ url
+ "#/c/"
+ "c/"
+ project.get()
+ "/+/"
+ c
+ "/1/a.txt@1 \n"
+ "PS1, Line 1: boring\n"
@@ -651,13 +657,17 @@ public class CommentsIT extends AbstractDaemonTest {
+ "\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 @@ public class CommentsIT extends AbstractDaemonTest {
+ "\n"
+ "\n"
+ url
+ "#/c/"
+ "c/"
+ project.get()
+ "/+/"
+ c
+ "/2/a.txt@a2 \n"
+ "PS2, Line 2: \n"
@@ -673,7 +685,9 @@ public class CommentsIT extends AbstractDaemonTest {
+ "\n"
+ "\n"
+ url
+ "#/c/"
+ "c/"
+ project.get()
+ "/+/"
+ c
+ "/2/a.txt@1 \n"
+ "PS2, Line 1: interesting\n"
@@ -681,7 +695,9 @@ public class CommentsIT extends AbstractDaemonTest {
+ "\n"
+ "\n"
+ url
+ "#/c/"
+ "c/"
+ project.get()
+ "/+/"
+ c
+ "/2/a.txt@2 \n"
+ "PS2, Line 2: nten\n"