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)
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index 49bd28a..b5e0181 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -113,7 +113,6 @@
private enum FailureType {
NOT_FOUND,
- INACTIVE,
OTHER;
}
@@ -233,9 +232,6 @@
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 @@
} 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 b5c9aab..118b31b 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.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 @@
}
@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 @@
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
