Merge "Return 400 BAD_REQUEST when a user cannot be added as reviewer by email" into stable-3.9

2 files changed, 46 insertions, 3 deletions

diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
index e46f9e466d..b0e58c5403 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
@@ -15,6 +15,7 @@
 package com.google.gerrit.server.restapi.change;
 
 import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.CHANGE_MODIFICATION;
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
 
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -71,7 +72,7 @@ public class PostReviewers
     ReviewerModification modification =
         reviewerModifier.prepare(rsrc.getNotes(), rsrc.getUser(), input, true);
     if (modification.op == null) {
-      return Response.ok(modification.result);
+      return Response.withStatusCode(SC_BAD_REQUEST, modification.result);
     }
     try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
       try (BatchUpdate bu =
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index 647e26b623..b34974c9d2 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -100,14 +100,14 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
     // Attempt to add overly large group as reviewers.
     PushOneCommit.Result r = createChange();
     String changeId = r.getChangeId();
-    ReviewerResult result = addReviewer(changeId, largeGroup);
+    ReviewerResult result = addReviewer(changeId, largeGroup, SC_BAD_REQUEST);
     assertThat(result.input).isEqualTo(largeGroup);
     assertThat(result.confirm).isNull();
     assertThat(result.error).contains("has too many members to add them all as reviewers");
     assertThat(result.reviewers).isNull();
 
     // Attempt to add medium group without confirmation.
-    result = addReviewer(changeId, mediumGroup);
+    result = addReviewer(changeId, mediumGroup, SC_BAD_REQUEST);
     assertThat(result.input).isEqualTo(mediumGroup);
     assertThat(result.confirm).isTrue();
     assertThat(result.error)
@@ -157,6 +157,48 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void addCcEmailWithoutAccount() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String changeId = r.getChangeId();
+    String testEmailAddress = "email@without.account";
+
+    // Add a reviewer
+    ReviewerInput ri = new ReviewerInput();
+    ri.reviewer = user.email();
+    ri.state = REVIEWER;
+    ReviewerResult result = addReviewer(changeId, ri);
+    assertThat(result.input).isEqualTo(user.email());
+    assertThat(result.confirm).isNull();
+    assertThat(result.error).isNull();
+    assertThat(result.reviewers).hasSize(1);
+
+    // Add an email address that has no account to CC
+    ReviewerInput ccInput = new ReviewerInput();
+    ccInput.reviewer = testEmailAddress;
+    ccInput.state = CC;
+    ReviewerResult resultCC = addReviewer(changeId, ccInput, SC_BAD_REQUEST);
+    assertThat(resultCC.error).contains("Account '" + testEmailAddress + "' not found");
+    assertThat(resultCC.error)
+        .contains(testEmailAddress + " does not identify a registered user or group");
+  }
+
+  @Test
+  public void addReviewerEmailWithoutAccount() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String changeId = r.getChangeId();
+    String testEmailAddress = "email@without.account";
+
+    // Add a reviewer without an account
+    ReviewerInput ri = new ReviewerInput();
+    ri.reviewer = testEmailAddress;
+    ri.state = REVIEWER;
+    ReviewerResult result = addReviewer(changeId, ri, SC_BAD_REQUEST);
+    assertThat(result.error).contains("Account '" + testEmailAddress + "' not found");
+    assertThat(result.error)
+        .contains(testEmailAddress + " does not identify a registered user or group");
+  }
+
+  @Test
   public void addCcGroup() throws Exception {
     List<TestAccount> users = createAccounts(6, "addCcGroup");
     List<String> usernames = new ArrayList<>(6);