Mention who owns the change in comment emails
When receiving the email, knowing the change owner lets the receiver
knows it is one of their change and might refine reviewing priority for
some people.
We had that customization for a few years now, and I felt like it can
be upstreamed (ref: https://phabricator.wikimedia.org/T43608 ).
Release-Notes: Emails notifications for comments now mentions who owns the change.
Change-Id: If676e4f1b75bdbe2d39dcfe1fbe841520e81068f
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index ea52690..fe7f935 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -2566,8 +2566,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body()).doesNotContain("\nPatch Set 2: Code-Review+2\n");
assertThat(message.body())
.contains("The change is no longer submittable: Code-Review is unsatisfied now.\n");
@@ -2652,8 +2652,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body()).doesNotContain("\nPatch Set 2: Code-Review+2\n");
assertThat(message.body()).contains("\nPatch Set 2: Code-Review+1\n");
assertThat(message.body())
@@ -2740,8 +2740,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body()).contains("\nPatch Set 2: Code-Review-2\n");
assertThat(message.body())
.contains("The change is no longer submittable: Code-Review is unsatisfied now.\n");
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java
index 1094a42..5bb6dc4 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeNoLongerSubmittableIT.java
@@ -83,8 +83,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body())
.contains("The change is no longer submittable: Code-Review is unsatisfied now.\n");
assertThat(message.htmlBody())
@@ -127,8 +127,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body())
.contains("The change is no longer submittable: Code-Review is unsatisfied now.\n");
assertThat(message.htmlBody())
@@ -172,8 +172,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body())
.contains("The change is no longer submittable: Code-Review is unsatisfied now.\n");
assertThat(message.htmlBody())
@@ -263,8 +263,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body())
.contains(
"The change is no longer submittable:"
@@ -315,8 +315,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body()).doesNotContain("The change is no longer submittable");
assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable");
}
@@ -361,8 +361,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body()).doesNotContain("The change is no longer submittable");
assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable");
}
@@ -398,8 +398,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body()).doesNotContain("The change is no longer submittable");
assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable");
}
@@ -435,8 +435,8 @@
String.format(
"Attention is currently required from: %s, %s.\n"
+ "\n"
- + "%s has posted comments on this change.",
- admin.fullName(), user.fullName(), approver.fullName()));
+ + "%s has posted comments on this change by %s.",
+ admin.fullName(), user.fullName(), approver.fullName(), admin.fullName()));
assertThat(message.body()).doesNotContain("The change is no longer submittable");
assertThat(message.htmlBody()).doesNotContain("The change is no longer submittable");
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index 647e26b..985baba 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -402,7 +402,9 @@
Message m = messages.get(0);
assertThat(m.rcpt()).containsExactly(user.getNameEmail(), observer.getNameEmail());
- assertThat(m.body()).contains(admin.fullName() + " has posted comments on this change.");
+ assertThat(m.body())
+ .contains(
+ admin.fullName() + " has posted comments on this change by " + admin.fullName() + ".");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertThat(m.body()).contains("Patch Set 1: Code-Review+2");
diff --git a/resources/com/google/gerrit/server/mail/Comment.soy b/resources/com/google/gerrit/server/mail/Comment.soy
index 98ab4b2..6a67398 100644
--- a/resources/com/google/gerrit/server/mail/Comment.soy
+++ b/resources/com/google/gerrit/server/mail/Comment.soy
@@ -29,7 +29,7 @@
{@param unsatisfiedSubmitRequirements: ?}
{@param oldSubmitRequirements: ?}
{@param newSubmitRequirements: ?}
- {$fromName} has posted comments on this change.
+ {$fromName} has posted comments on this change by {$change.ownerName}.
{if $email.changeUrl} ( {$email.changeUrl} ){/if}{\n}
{if $unsatisfiedSubmitRequirements}
{\n}
