diff options
author | Daniele Sassoli <danielesassoli@gmail.com> | 2023-04-12 15:28:09 +0100 |
---|---|---|
committer | Diego Zambelli Sessona <diego.sessona@gmail.com> | 2023-11-03 07:13:23 +0000 |
commit | 3742096c34a57b6ac5a51703d188fe9b231bf8fd (patch) | |
tree | 0aa7eb37c2019a26c877ade38c3fbf8b3d10f10e | |
parent | ea3417d2c51d4ff83ed6f1358ef35f4b0383eb69 (diff) |
Return 400 BAD_REQUEST when a user cannot be added as reviewer by email
When trying to add a user that cannot be added as a reviewer
on a change, the API was returning a 200 Status Code with an
error message.
This was happening in particular when adding as a reviewer a
user by email who does not have an account in Gerrit.
Bug: Issue 40015460
Release-Notes: Return 400 BAD_REQUEST when a reviewer cannot be added by email
Change-Id: Ice3d6f05f058f9933fef344f1b38c667b2c7abbe
-rw-r--r-- | java/com/google/gerrit/server/restapi/change/PostReviewers.java | 3 | ||||
-rw-r--r-- | javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java | 46 |
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); |