PostReview: Fix email for removing an email without Gerrit account as CC
If reviewer.enableByEmail in project.config is true, it's possible to CC
an email for which no Gerrit account exists. Removing this CC via the
PostReview REST endpoint (click on the pencil icon next to the CC,
remove the CC in the Reply dialog and click SEND) triggered an email
that said:
<user> removed null from this change.
When removing the CC via a DELETE reviewer request (click on Remove CC
in the user popup), the triggered email was correct (also see change
I4895bca04):
<user> removed <email> from this change.
Deleting a reviewer via PostReview sends the email via NewChangeSender.
NewChangeSender#getRemovedReviewerNames() set address.name() as the
reviewer name, which returned null since the reviewer doesn't have a
name. Changing this to address.toString(), same as in
DeleteReviewerSender#getReviewerNames() fixed this. DeleteReviewerSender
is used when a reviewer is deleted via DeleteReviewer where the email is
correct.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Release-Notes: skip
Bug: Google b/240670892
Change-Id: I00baa40bc1e5f0e47c911b6211c57168982a8959
diff --git a/java/com/google/gerrit/server/mail/send/NewChangeSender.java b/java/com/google/gerrit/server/mail/send/NewChangeSender.java
index 001de52..e899fc5 100644
--- a/java/com/google/gerrit/server/mail/send/NewChangeSender.java
+++ b/java/com/google/gerrit/server/mail/send/NewChangeSender.java
@@ -116,7 +116,7 @@
names.add(getNameFor(id));
}
for (Address address : removedByEmailReviewers) {
- names.add(address.name());
+ names.add(address.toString());
}
return names;
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 8b5eaf8..aaefb2d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -4817,7 +4817,7 @@
}
@Test
- public void ccNonExistentAccountByEmailThenRemove() throws Exception {
+ public void ccNonExistentAccountByEmailThenRemoveByDelete() throws Exception {
// Create a project that allows reviewers by email.
Project.NameKey project = projectOperations.newProject().create();
try (ProjectConfigUpdate u = updateProject(project)) {
@@ -4862,6 +4862,54 @@
.contains(String.format("%s has removed %s", admin.fullName(), reviewerInput.reviewer));
}
+ @Test
+ public void ccNonExistentAccountByEmailThenRemoveByPostReview() throws Exception {
+ // Create a project that allows reviewers by email.
+ Project.NameKey project = projectOperations.newProject().create();
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateProject(
+ b ->
+ b.setBooleanConfig(
+ BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL, InheritableBoolean.TRUE));
+ u.save();
+ }
+
+ // Create a change
+ TestRepository<?> testRepo = cloneProject(project, admin);
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ // Add an email as a CC for which no Gerrit account exists.
+ sender.clear();
+ ReviewerInput reviewerInput = new ReviewerInput();
+ reviewerInput.state = CC;
+ reviewerInput.reviewer = "email-without-account@example.com";
+ gApi.changes().id(r.getChangeId()).addReviewer(reviewerInput);
+
+ // Check that the email was added as a CC and an email was sent.
+ AccountInfo ccedAccountInfo =
+ Iterables.getOnlyElement(
+ gApi.changes().id(r.getChangeId()).get().reviewers.get(ReviewerState.CC));
+ assertThat(ccedAccountInfo.email).isEqualTo(reviewerInput.reviewer);
+ assertThat(ccedAccountInfo._accountId).isNull();
+ assertThat(ccedAccountInfo.name).isNull();
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .contains(String.format("%s has uploaded this change for review", admin.fullName()));
+
+ // Remove the CC.
+ sender.clear();
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.reviewer(reviewerInput.reviewer, ReviewerState.REMOVED, /* confirmed= */ false);
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+
+ // Check that the email was removed as a CC and an email was sent.
+ assertThat(gApi.changes().id(r.getChangeId()).get().reviewers).isEmpty();
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .contains(String.format("%s has removed %s", admin.fullName(), reviewerInput.reviewer));
+ }
+
private PushOneCommit.Result createWorkInProgressChange() throws Exception {
return pushTo("refs/for/master%wip");
}
