From a57155dcb3a5f1875a12f321bf30327393276acb Mon Sep 17 00:00:00 2001 From: Dmitrii Filippov Date: Wed, 12 Apr 2023 13:47:29 +0000 Subject: Revert "Disallow adding inactive reviewer" This reverts commit e84b33e5b345b31a130890a80ebd55451f335fa7. Reason for revert: the following use-cases were broken by the original change: 1. Revert a change where one of reviewers/cc is inactive. 2. Removing an inactive reviewer from the "Reply" dialog. Release-Notes: skip Google-Bug-Id: b/277053099 Change-Id: I7ff68c37e78c32a4dc9ce07208918862ebf118c1 (cherry picked from commit 17022251627494d22d0429b44ed8e9c226723cbc) --- .../gerrit/server/change/ReviewerModifier.java | 11 ------- .../gerrit/acceptance/api/change/ChangeIT.java | 35 ++++++++++++++-------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java index 49bd28acbf..b5e01810a5 100644 --- a/java/com/google/gerrit/server/change/ReviewerModifier.java +++ b/java/com/google/gerrit/server/change/ReviewerModifier.java @@ -113,7 +113,6 @@ public class ReviewerModifier { private enum FailureType { NOT_FOUND, - INACTIVE, OTHER; } @@ -233,9 +232,6 @@ public class ReviewerModifier { ReviewerModification byAccountId = byAccountId(input, notes, user); - if (byAccountId.isFailure() && byAccountId.failureType.equals(FailureType.INACTIVE)) { - return byAccountId; - } ReviewerModification wholeGroup = null; if (!byAccountId.exactMatchFound) { wholeGroup = addWholeGroup(input, notes, user, confirmed, allowGroup, allowByEmail); @@ -289,13 +285,6 @@ public class ReviewerModifier { } else { reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser(); } - if (reviewerUser.getAccount().inactive()) { - return fail( - input, - FailureType.INACTIVE, - String.format( - "Cannot add inactive account %s, %s", reviewerUser.getAccountId(), input.reviewer)); - } if (input.reviewer.equalsIgnoreCase(reviewerUser.getName()) || input.reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) { exactMatchFound = true; diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index b5c9aab09a..118b31b9d9 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -125,6 +125,7 @@ import com.google.gerrit.extensions.api.changes.RevertInput; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling; import com.google.gerrit.extensions.api.changes.ReviewResult; +import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.api.changes.ReviewerInput; import com.google.gerrit.extensions.api.changes.ReviewerResult; import com.google.gerrit.extensions.api.changes.RevisionApi; @@ -1370,23 +1371,26 @@ public class ChangeIT extends AbstractDaemonTest { } @Test - public void addReviewerThatIsInactiveByUsername_notAllowed() throws Exception { + public void addReviewerThatIsInactiveByUsername() throws Exception { PushOneCommit.Result result = createChange(); String username = name("new-user"); - Account.Id accountId = accountOperations.newAccount().username(username).inactive().create(); + Account.Id id = accountOperations.newAccount().username(username).inactive().create(); ReviewerInput in = new ReviewerInput(); in.reviewer = username; ReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in); assertThat(r.input).isEqualTo(in.reviewer); - assertThat(r.error) - .isEqualTo(String.format("Cannot add inactive account %s, %s", accountId, username)); + assertThat(r.error).isNull(); + assertThat(r.reviewers).hasSize(1); + ReviewerInfo reviewer = r.reviewers.get(0); + assertThat(reviewer._accountId).isEqualTo(id.get()); + assertThat(reviewer.username).isEqualTo(username); } @Test - public void addReviewerThatIsInactiveById_notAllowed() throws Exception { + public void addReviewerThatIsInactiveById() throws Exception { PushOneCommit.Result result = createChange(); String username = name("new-user"); @@ -1397,28 +1401,33 @@ public class ChangeIT extends AbstractDaemonTest { ReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in); assertThat(r.input).isEqualTo(in.reviewer); - - assertThat(r.error) - .isEqualTo(String.format("Cannot add inactive account %s, %s", id, id.get())); + assertThat(r.error).isNull(); + assertThat(r.reviewers).hasSize(1); + ReviewerInfo reviewer = r.reviewers.get(0); + assertThat(reviewer._accountId).isEqualTo(id.get()); + assertThat(reviewer.username).isEqualTo(username); } @Test - public void addReviewerThatIsInactiveByEmail_notAllowed() throws Exception { + public void addReviewerThatIsInactiveByEmail() throws Exception { ConfigInput conf = new ConfigInput(); conf.enableReviewerByEmail = InheritableBoolean.TRUE; gApi.projects().name(project.get()).config(conf); PushOneCommit.Result result = createChange(); String username = "user@domain.com"; - Account.Id accountId = accountOperations.newAccount().username(username).inactive().create(); + Account.Id id = accountOperations.newAccount().username(username).inactive().create(); ReviewerInput in = new ReviewerInput(); in.reviewer = username; in.state = ReviewerState.CC; ReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in); - assertThat(r.input).isEqualTo(in.reviewer); - assertThat(r.error) - .isEqualTo(String.format("Cannot add inactive account %s, %s", accountId, username)); + assertThat(r.input).isEqualTo(username); + assertThat(r.error).isNull(); + assertThat(r.ccs).hasSize(1); + AccountInfo reviewer = r.ccs.get(0); + assertThat(reviewer._accountId).isEqualTo(id.get()); + assertThat(reviewer.username).isEqualTo(username); } @Test -- cgit v1.2.3