opendev/gerrit
Code Issues Proposed changes

Use PolyGerrit-style URLs and project/+/changeId format in change emails

Browse Source 
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
This commit is contained in:
Patrick Hiesel
2018-07-26 15:12:57 +02:00
parent f71363fe0c
commit a0203dbea6
6 changed files with 27 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/google/gerrit/server/mail/receive/MailProcessor.java
View File

@@ -237,7 +237,7 @@ public class MailProcessor {
.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)) {

5
java/com/google/gerrit/server/mail/send/ChangeEmail.java
View File

@@ -221,10 +221,7 @@ public abstract class ChangeEmail extends NotificationEmail {
/** 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;
}

13
javatests/com/google/gerrit/acceptance/server/mail/MailMetadataIT.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
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 @@ public class MailMetadataIT extends AbstractDaemonTest {
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 @@ public class MailMetadataIT extends AbstractDaemonTest {
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 @@ public class MailMetadataIT extends AbstractDaemonTest {
}
}
}
private String getChangeUrl(ChangeData changeData) {
return canonicalWebUrl.get()
+ "c/"
+ changeData.project().get()
+ "/+/"
+ changeData.getId().get();
}
}

52
javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
View File

@@ -46,12 +46,7 @@ public class MailProcessorIT extends AbstractMailIT {
// 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 @@ public class MailProcessorIT extends AbstractMailIT {
// 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 @@ public class MailProcessorIT extends AbstractMailIT {
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 @@ public class MailProcessorIT extends AbstractMailIT {
// 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 @@ public class MailProcessorIT extends AbstractMailIT {
// 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 @@ public class MailProcessorIT extends AbstractMailIT {
// 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 @@ public class MailProcessorIT extends AbstractMailIT {
// 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 @@ public class MailProcessorIT extends AbstractMailIT {
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;
}
}

3
javatests/com/google/gerrit/mail/AbstractParserTest.java
View File

@@ -26,7 +26,8 @@ import org.junit.Ignore;
@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();

2
javatests/com/google/gerrit/mail/TextParserTest.java
View File

@@ -23,7 +23,7 @@ import org.junit.Test;
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"