Use PolyGerrit-style URLs and project/+/changeId format in change emails Gerrit preferes the ChangeId that includes the project name. This commit adds that to the change URLs used in emails. It adapts inbound email parsing and all necessary tests. PolyGerrit is now the default so we can switch those change links in emails over to PolyGerrit's URL schema. Change-Id: I9233a1c24779a36c5f3b63cf1326c26aed17ffd5
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java index 75053c8..707056c 100644 --- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java +++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -237,7 +237,7 @@ .sorted(CommentsUtil.COMMENT_ORDER) .collect(toList()); Project.NameKey project = cd.project(); - String changeUrl = canonicalUrl.get() + "#/c/" + cd.getId().get(); + String changeUrl = canonicalUrl.get() + "c/" + cd.project().get() + "/+/" + cd.getId().get(); List<MailComment> parsedComments; if (useHtmlParser(message)) {
diff --git a/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 00a496a2..4ba3b67 100644 --- a/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/java/com/google/gerrit/server/mail/send/ChangeEmail.java
@@ -221,10 +221,7 @@ /** Get a link to the change; null if the server doesn't know its own address. */ public String getChangeUrl() { if (getGerritUrl() != null) { - final StringBuilder r = new StringBuilder(); - r.append(getGerritUrl()); - r.append(change.getChangeId()); - return r.toString(); + return getGerritUrl() + "c/" + change.getProject().get() + "/+/" + change.getChangeId(); } return null; }
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java index 988b051..a0170d2 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java
@@ -24,6 +24,7 @@ import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.mail.EmailHeader; import com.google.gerrit.mail.MailProcessingUtil; +import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.testing.FakeEmailSender; import com.google.gerrit.testing.TestTimeUtil; import java.sql.Timestamp; @@ -63,7 +64,7 @@ assertThat(emails).hasSize(1); FakeEmailSender.Message message = emails.get(0); - String changeURL = "<" + canonicalWebUrl.get() + newChange.getChange().getId().get() + ">"; + String changeURL = "<" + getChangeUrl(newChange.getChange()) + ">"; Map<String, Object> expectedHeaders = new HashMap<>(); expectedHeaders.put("Gerrit-PatchSet", "1"); @@ -100,7 +101,7 @@ assertThat(emails).hasSize(1); FakeEmailSender.Message message = emails.get(0); - String changeURL = "<" + canonicalWebUrl.get() + newChange.getChange().getId().get() + ">"; + String changeURL = "<" + getChangeUrl(newChange.getChange()) + ">"; Map<String, Object> expectedHeaders = new HashMap<>(); expectedHeaders.put("Gerrit-PatchSet", "1"); expectedHeaders.put( @@ -159,4 +160,12 @@ } } } + + private String getChangeUrl(ChangeData changeData) { + return canonicalWebUrl.get() + + "c/" + + changeData.project().get() + + "/+/" + + changeData.getId().get(); + } }
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java index b9f30d6..9ff2c05 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -46,12 +46,7 @@ // Build Message MailMessage.Builder b = messageBuilderWithDefaultFields(); String txt = - newPlaintextBody( - canonicalWebUrl.get() + "#/c/" + changeInfo._number + "/1", - "Test Message", - null, - null, - null); + newPlaintextBody(getChangeUrl(changeInfo) + "/1", "Test Message", null, null, null); b.textContent(txt + textFooterForChange(changeInfo._number, ts)); mailProcessor.process(b.build()); @@ -74,12 +69,7 @@ // Build Message MailMessage.Builder b = messageBuilderWithDefaultFields(); String txt = - newPlaintextBody( - canonicalWebUrl.get() + "#/c/" + changeInfo._number + "/1", - null, - "Some Inline Comment", - null, - null); + newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, "Some Inline Comment", null, null); b.textContent(txt + textFooterForChange(changeInfo._number, ts)); mailProcessor.process(b.build()); @@ -111,11 +101,7 @@ MailMessage.Builder b = messageBuilderWithDefaultFields(); String txt = newPlaintextBody( - canonicalWebUrl.get() + "#/c/" + changeInfo._number + "/1", - null, - null, - "Some Comment on File 1", - null); + getChangeUrl(changeInfo) + "/1", null, null, "Some Comment on File 1", null); b.textContent(txt + textFooterForChange(changeInfo._number, ts)); mailProcessor.process(b.build()); @@ -147,12 +133,7 @@ // Build Message MailMessage.Builder b = messageBuilderWithDefaultFields(); String txt = - newPlaintextBody( - canonicalWebUrl.get() + "#/c/" + changeInfo._number + "/1", - null, - "Some Inline Comment", - null, - null); + newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, "Some Inline Comment", null, null); b.textContent(txt + textFooterForChange(changeInfo._number, ts)); mailProcessor.process(b.build()); @@ -178,12 +159,7 @@ // Build Message MailMessage.Builder b = messageBuilderWithDefaultFields(); String txt = - newPlaintextBody( - canonicalWebUrl.get() + "#/c/" + changeInfo._number + "/1", - null, - "Some Inline Comment", - null, - null); + newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, "Some Inline Comment", null, null); b.textContent(txt + textFooterForChange(changeInfo._number, ts)); // Set account state to inactive @@ -211,12 +187,7 @@ // Build Message String txt = - newPlaintextBody( - canonicalWebUrl.get() + "#/c/" + changeInfo._number + "/1", - "Test Message", - null, - null, - null); + newPlaintextBody(getChangeUrl(changeInfo) + "/1", "Test Message", null, null, null); MailMessage.Builder b = messageBuilderWithDefaultFields() .from(user.emailAddress) @@ -238,12 +209,7 @@ // Build Message String txt = - newPlaintextBody( - canonicalWebUrl.get() + "#/c/" + changeInfo._number + "/1", - "Test Message", - null, - null, - null); + newPlaintextBody(getChangeUrl(changeInfo) + "/1", "Test Message", null, null, null); MailMessage.Builder b = messageBuilderWithDefaultFields() .from(user.emailAddress) @@ -257,4 +223,8 @@ assertThat(message.body()).contains("was unable to parse your email"); assertThat(message.headers()).containsKey("Subject"); } + + private String getChangeUrl(ChangeInfo changeInfo) { + return canonicalWebUrl.get() + "c/" + changeInfo.project + "/+/" + changeInfo._number; + } }
diff --git a/javatests/com/google/gerrit/mail/AbstractParserTest.java b/javatests/com/google/gerrit/mail/AbstractParserTest.java index 5219ce8..6ab4ca2 100644 --- a/javatests/com/google/gerrit/mail/AbstractParserTest.java +++ b/javatests/com/google/gerrit/mail/AbstractParserTest.java
@@ -26,7 +26,8 @@ @Ignore public class AbstractParserTest { - protected static final String CHANGE_URL = "https://gerrit-review.googlesource.com/#/changes/123"; + protected static final String CHANGE_URL = + "https://gerrit-review.googlesource.com/c/project/+/123"; protected static void assertChangeMessage(String message, MailComment comment) { assertThat(comment.fileName).isNull();
diff --git a/javatests/com/google/gerrit/mail/TextParserTest.java b/javatests/com/google/gerrit/mail/TextParserTest.java index e11321a..a5c2152 100644 --- a/javatests/com/google/gerrit/mail/TextParserTest.java +++ b/javatests/com/google/gerrit/mail/TextParserTest.java
@@ -23,7 +23,7 @@ public class TextParserTest extends AbstractParserTest { private static final String quotedFooter = "" - + "> To view, visit https://gerrit-review.googlesource.com/123\n" + + "> To view, visit https://gerrit-review.googlesource.com/c/project/+/123\n" + "> To unsubscribe, visit https://gerrit-review.googlesource.com\n" + "> \n" + "> Gerrit-MessageType: comment\n"