Merge "Implement batch removal of reviewers in PostReview"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 0122615..1581274 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -7754,7 +7754,7 @@
have been granted `labelAs-NAME` permission for all keys of labels.
|`reviewers` |optional|
A list of link:rest-api-changes.html#reviewer-input[ReviewerInput]
-representing reviewers that should be added to the change.
+representing reviewers that should be added/removed to/from the change.
|`ready` |optional|
If true, and if the change is work in progress, then start review.
It is an error for both `ready` and `work_in_progress` to be true.
@@ -7788,7 +7788,7 @@
|`reviewers` |optional|
Map of account or group identifier to
link:rest-api-changes.html#reviewer-result[ReviewerResult]
-representing the outcome of adding as a reviewer.
+representing the outcome of adding/removing a reviewer.
Absent if no reviewer additions were requested.
|`ready` |optional|
If true, the change was moved from WIP to ready for review as a result of this
@@ -7819,24 +7819,25 @@
[[reviewer-input]]
=== ReviewerInput
-The `ReviewerInput` entity contains information for adding a reviewer
-to a change.
+The `ReviewerInput` entity contains information for adding or removing
+reviewers to/from the change.
[options="header",cols="1,^1,5"]
|=============================
|Field Name ||Description
|`reviewer` ||
The link:rest-api-accounts.html#account-id[ID] of one account that
-should be added as reviewer or the link:rest-api-groups.html#group-id[
+should be added/removed as reviewer or the link:rest-api-groups.html#group-id[
ID] of one internal group for which all members should be added as reviewers. +
If an ID identifies both an account and a group, only the account is
added as reviewer to the change.
External groups, such as LDAP groups, will be silently omitted from a
link:#set-review[set-review] or
-link:rest-api-changes.html#add-reviewer[add-reviewer] call.
+link:rest-api-changes.html#add-reviewer[add-reviewer] call. A group can only be
+specified for adding reviewers, not for removing them.
|`state` |optional|
-Add reviewer in this state. Possible reviewer states are `REVIEWER`
-and `CC`. If not given, defaults to `REVIEWER`.
+Add reviewer in this state. Possible reviewer states are `REVIEWER`,
+`CC` and `REMOVED`. If not given, defaults to `REVIEWER`.
|`confirmed` |optional|
Whether adding the reviewer is confirmed. +
The Gerrit server may be configured to
@@ -7855,8 +7856,8 @@
[[reviewer-result]]
=== ReviewerResult
-The `ReviewerResult` entity describes the result of adding a reviewer to a
-change.
+The `ReviewerResult` entity describes the result of modifying reviewers of
+a change.
[options="header",cols="1,^1,5"]
|===========================
@@ -7870,7 +7871,12 @@
|`ccs` |optional|
The newly CCed accounts as a list of
link:rest-api-accounts.html#account-info[AccountInfo] entities. This field will
-only appear if the requested `state` for the reviewer was `CC`.
+only appear if the requested `state` for the reviewer was `CC`.
+|`removed` |optional|
+The newly removed accounts as a list of
+link:rest-api-accounts.html#account-info[AccountInfo] entities.
+This field will only appear if the requested `state` for the reviewer was
+`REMOVED`.
|`error` |optional|
Error message explaining why the reviewer could not be added. +
If a group was specified in the input and an error is returned, it
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index 9da0d0f..87831e4 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -62,7 +62,7 @@
*/
public String onBehalfOf;
- /** Reviewers that should be added to this change. */
+ /** Reviewers that should be added to this change or removed from it. */
public List<ReviewerInput> reviewers;
/**
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewerResult.java b/java/com/google/gerrit/extensions/api/changes/ReviewerResult.java
index 324275a..1713674 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewerResult.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewerResult.java
@@ -18,17 +18,17 @@
import com.google.gerrit.extensions.common.AccountInfo;
import java.util.List;
-/** Result object representing the outcome of a request to add a reviewer. */
+/** Result object representing the outcome of a request to add/remove a reviewer. */
public class ReviewerResult {
- /** The identifier of an account or group that was to be added as a reviewer. */
+ /** The identifier of an account or group that was to be added/removed as a reviewer. */
public String input;
- /** If non-null, a string describing why the reviewer could not be added. */
+ /** If non-null, a string describing why the reviewer could not be added/removed. */
@Nullable public String error;
/**
* Non-null and true if the reviewer cannot be added without explicit confirmation. This may be
- * the case for groups of a certain size.
+ * the case for groups of a certain size. For removals, it's always false.
*/
@Nullable public Boolean confirm;
@@ -39,11 +39,14 @@
@Nullable public List<ReviewerInfo> reviewers;
/**
- * List of accounts CCed on the change. The size of this list may be greater than one (e.g. when a
- * group is CCed). Null if no accounts were CCed or if reviewers is non-null.
+ * List of new accounts CCed on the change. The size of this list may be greater than one (e.g.
+ * when a group is CCed). Null if no accounts were CCed.
*/
@Nullable public List<AccountInfo> ccs;
+ /** An account removed from the change. Null if no accounts were removed. */
+ @Nullable public AccountInfo removed;
+
/**
* Constructs a partially initialized result for the given reviewer.
*
diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java
index f79c9fc..1d6fb3c 100644
--- a/java/com/google/gerrit/server/change/AddReviewersOp.java
+++ b/java/com/google/gerrit/server/change/AddReviewersOp.java
@@ -23,7 +23,6 @@
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toList;
-import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
@@ -31,7 +30,6 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -42,7 +40,6 @@
import com.google.gerrit.server.extensions.events.ReviewerAdded;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
import com.google.inject.Inject;
@@ -52,7 +49,7 @@
import java.util.List;
import java.util.Set;
-public class AddReviewersOp implements BatchUpdateOp {
+public class AddReviewersOp extends ReviewerOp {
public interface Factory {
/**
@@ -75,34 +72,6 @@
boolean forGroup);
}
- @AutoValue
- public abstract static class Result {
- public abstract ImmutableList<PatchSetApproval> addedReviewers();
-
- public abstract ImmutableList<Address> addedReviewersByEmail();
-
- public abstract ImmutableList<Account.Id> addedCCs();
-
- public abstract ImmutableList<Address> addedCCsByEmail();
-
- static Builder builder() {
- return new AutoValue_AddReviewersOp_Result.Builder();
- }
-
- @AutoValue.Builder
- abstract static class Builder {
- abstract Builder setAddedReviewers(Iterable<PatchSetApproval> addedReviewers);
-
- abstract Builder setAddedReviewersByEmail(Iterable<Address> addedReviewersByEmail);
-
- abstract Builder setAddedCCs(Iterable<Account.Id> addedCCs);
-
- abstract Builder setAddedCCsByEmail(Iterable<Address> addedCCsByEmail);
-
- abstract Result build();
- }
- }
-
private final ApprovalsUtil approvalsUtil;
private final PatchSetUtil psUtil;
private final ReviewerAdded reviewerAdded;
@@ -121,10 +90,7 @@
private Collection<Account.Id> addedCCs = ImmutableList.of();
private Collection<Address> addedCCsByEmail = ImmutableList.of();
- private boolean sendEmail = true;
private Change change;
- private PatchSet patchSet;
- private Result opResult;
@Inject
AddReviewersOp(
@@ -152,17 +118,6 @@
this.forGroup = forGroup;
}
- // TODO(dborowitz): This mutable setter is ugly, but a) it's less ugly than adding boolean args
- // all the way through the constructor stack, and b) this class is slated to be completely
- // rewritten.
- public void suppressEmail() {
- this.sendEmail = false;
- }
-
- void setPatchSet(PatchSet patchSet) {
- this.patchSet = requireNonNull(patchSet);
- }
-
@Override
public boolean updateChange(ChangeContext ctx) throws RestApiException, IOException {
change = ctx.getChange();
@@ -252,8 +207,10 @@
change,
Lists.transform(addedReviewers, PatchSetApproval::accountId),
addedCCs,
+ ImmutableSet.of(),
addedReviewersByEmail,
addedCCsByEmail,
+ ImmutableSet.of(),
ctx.getNotify(change.getId()));
}
if (!addedReviewers.isEmpty()) {
@@ -266,9 +223,4 @@
ctx.getChangeData(change), patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
}
}
-
- public Result getResult() {
- checkState(opResult != null, "Batch update wasn't executed yet");
- return opResult;
- }
}
diff --git a/java/com/google/gerrit/server/change/ChangeMessages.java b/java/com/google/gerrit/server/change/ChangeMessages.java
index 6f2e1ef..787f036 100644
--- a/java/com/google/gerrit/server/change/ChangeMessages.java
+++ b/java/com/google/gerrit/server/change/ChangeMessages.java
@@ -31,6 +31,7 @@
public String reviewerInvalid;
public String reviewerNotFoundUserOrGroup;
+ public String groupRemovalIsNotAllowed;
public String groupIsNotAllowed;
public String groupHasTooManyMembers;
public String groupManyMembersConfirmation;
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
index 2505024..839d7f1 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
@@ -22,14 +22,13 @@
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.mail.send.DeleteReviewerSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
-import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.Collections;
-public class DeleteReviewerByEmailOp implements BatchUpdateOp {
+public class DeleteReviewerByEmailOp extends ReviewerOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -74,22 +73,25 @@
@Override
public void postUpdate(PostUpdateContext ctx) {
- try {
- NotifyResolver.Result notify = ctx.getNotify(change.getId());
- if (!notify.shouldNotify()) {
- return;
+ opResult = Result.builder().setDeletedReviewerByEmail(reviewer).build();
+ if (sendEmail) {
+ try {
+ NotifyResolver.Result notify = ctx.getNotify(change.getId());
+ if (!notify.shouldNotify()) {
+ return;
+ }
+ DeleteReviewerSender emailSender =
+ deleteReviewerSenderFactory.create(ctx.getProject(), change.getId());
+ emailSender.setFrom(ctx.getAccountId());
+ emailSender.addReviewersByEmail(Collections.singleton(reviewer));
+ emailSender.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn());
+ emailSender.setNotify(notify);
+ emailSender.setMessageId(
+ messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()));
+ emailSender.send();
+ } catch (Exception err) {
+ logger.atSevere().withCause(err).log("Cannot email update for change %s", change.getId());
}
- DeleteReviewerSender emailSender =
- deleteReviewerSenderFactory.create(ctx.getProject(), change.getId());
- emailSender.setFrom(ctx.getAccountId());
- emailSender.addReviewersByEmail(Collections.singleton(reviewer));
- emailSender.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn());
- emailSender.setNotify(notify);
- emailSender.setMessageId(
- messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()));
- emailSender.send();
- } catch (Exception err) {
- logger.atSevere().withCause(err).log("Cannot email update for change %s", change.getId());
}
}
}
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index b1fc67d..095a19b 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelTypes;
-import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.EmailException;
@@ -35,6 +34,7 @@
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.extensions.events.ReviewerDeleted;
import com.google.gerrit.server.mail.send.DeleteReviewerSender;
@@ -44,7 +44,6 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.RemoveReviewerControl;
-import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoView;
@@ -56,11 +55,11 @@
import java.util.HashMap;
import java.util.Map;
-public class DeleteReviewerOp implements BatchUpdateOp {
+public class DeleteReviewerOp extends ReviewerOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
- DeleteReviewerOp create(AccountState reviewerAccount, DeleteReviewerInput input);
+ DeleteReviewerOp create(Account reviewerAccount, DeleteReviewerInput input);
}
private final ApprovalsUtil approvalsUtil;
@@ -73,13 +72,13 @@
private final RemoveReviewerControl removeReviewerControl;
private final ProjectCache projectCache;
private final MessageIdGenerator messageIdGenerator;
+ private final AccountCache accountCache;
- private final AccountState reviewer;
+ private final Account reviewer;
private final DeleteReviewerInput input;
ChangeMessage changeMessage;
Change currChange;
- PatchSet currPs;
Map<String, Short> newApprovals = new HashMap<>();
Map<String, Short> oldApprovals = new HashMap<>();
@@ -95,7 +94,8 @@
RemoveReviewerControl removeReviewerControl,
ProjectCache projectCache,
MessageIdGenerator messageIdGenerator,
- @Assisted AccountState reviewerAccount,
+ AccountCache accountCache,
+ @Assisted Account reviewerAccount,
@Assisted DeleteReviewerInput input) {
this.approvalsUtil = approvalsUtil;
this.psUtil = psUtil;
@@ -107,6 +107,7 @@
this.removeReviewerControl = removeReviewerControl;
this.projectCache = projectCache;
this.messageIdGenerator = messageIdGenerator;
+ this.accountCache = accountCache;
this.reviewer = reviewerAccount;
this.input = input;
}
@@ -114,15 +115,18 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws AuthException, ResourceNotFoundException, PermissionBackendException, IOException {
- Account.Id reviewerId = reviewer.account().id();
+ Account.Id reviewerId = reviewer.id();
// Check of removing this reviewer (even if there is no vote processed by the loop below) is OK
removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), reviewerId);
if (!approvalsUtil.getReviewers(ctx.getNotes()).all().contains(reviewerId)) {
- throw new ResourceNotFoundException();
+ throw new ResourceNotFoundException(
+ String.format(
+ "Reviewer %s doesn't exist in the change, hence can't delete it",
+ reviewer.getName()));
}
currChange = ctx.getChange();
- currPs = psUtil.current(ctx.getNotes());
+ setPatchSet(psUtil.current(ctx.getNotes()));
LabelTypes labelTypes =
projectCache
@@ -141,14 +145,14 @@
? "cc"
: "reviewer";
StringBuilder msg = new StringBuilder();
- msg.append(String.format("Removed %s %s", ccOrReviewer, reviewer.account().fullName()));
+ msg.append(String.format("Removed %s %s", ccOrReviewer, reviewer.fullName()));
StringBuilder removedVotesMsg = new StringBuilder();
removedVotesMsg.append(" with the following votes:\n\n");
boolean votesRemoved = false;
for (PatchSetApproval a : approvals(ctx, reviewerId)) {
// Check if removing this vote is OK
removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
- if (a.patchSetId().equals(currPs.id()) && a.value() != 0) {
+ if (a.patchSetId().equals(patchSet.id()) && a.value() != 0) {
oldApprovals.put(a.label(), a.value());
removedVotesMsg
.append("* ")
@@ -166,7 +170,7 @@
} else {
msg.append(".");
}
- ChangeUpdate update = ctx.getUpdate(currPs.id());
+ ChangeUpdate update = ctx.getUpdate(patchSet.id());
update.removeReviewer(reviewerId);
changeMessage =
@@ -178,26 +182,31 @@
@Override
public void postUpdate(PostUpdateContext ctx) {
+ opResult = Result.builder().setDeletedReviewer(reviewer.id()).build();
+
NotifyResolver.Result notify = ctx.getNotify(currChange.getId());
- if (input.notify == null
- && currChange.isWorkInProgress()
- && !oldApprovals.isEmpty()
- && notify.handling().compareTo(NotifyHandling.OWNER) < 0) {
- // Override NotifyHandling from the context to notify owner if votes were removed on a WIP
- // change.
- notify = notify.withHandling(NotifyHandling.OWNER);
- }
- try {
- if (notify.shouldNotify()) {
- emailReviewers(ctx.getProject(), currChange, changeMessage, notify, ctx.getRepoView());
+ if (sendEmail) {
+ if (input.notify == null
+ && currChange.isWorkInProgress()
+ && !oldApprovals.isEmpty()
+ && notify.handling().compareTo(NotifyHandling.OWNER) < 0) {
+ // Override NotifyHandling from the context to notify owner if votes were removed on a WIP
+ // change.
+ notify = notify.withHandling(NotifyHandling.OWNER);
}
- } catch (Exception err) {
- logger.atSevere().withCause(err).log("Cannot email update for change %s", currChange.getId());
+ try {
+ if (notify.shouldNotify()) {
+ emailReviewers(ctx.getProject(), currChange, changeMessage, notify, ctx.getRepoView());
+ }
+ } catch (Exception err) {
+ logger.atSevere().withCause(err).log(
+ "Cannot email update for change %s", currChange.getId());
+ }
}
reviewerDeleted.fire(
ctx.getChangeData(currChange),
- currPs,
- reviewer,
+ patchSet,
+ accountCache.get(reviewer.id()).orElse(AccountState.forAccount(reviewer)),
ctx.getAccount(),
changeMessage.getMessage(),
newApprovals,
@@ -227,14 +236,14 @@
RepoView repoView)
throws EmailException {
Account.Id userId = user.get().getAccountId();
- if (userId.equals(reviewer.account().id())) {
+ if (userId.equals(reviewer.id())) {
// The user knows they removed themselves, don't bother emailing them.
return;
}
DeleteReviewerSender emailSender =
deleteReviewerSenderFactory.create(projectName, change.getId());
emailSender.setFrom(userId);
- emailSender.addReviewers(Collections.singleton(reviewer.account().id()));
+ emailSender.addReviewers(Collections.singleton(reviewer.id()));
emailSender.setChangeMessage(changeMessage.getMessage(), changeMessage.getWrittenOn());
emailSender.setNotify(notify);
emailSender.setMessageId(
diff --git a/java/com/google/gerrit/server/change/DeleteReviewersUtil.java b/java/com/google/gerrit/server/change/DeleteReviewersUtil.java
new file mode 100644
index 0000000..a4f306b
--- /dev/null
+++ b/java/com/google/gerrit/server/change/DeleteReviewersUtil.java
@@ -0,0 +1,85 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Address;
+import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
+import com.google.gerrit.extensions.api.changes.ReviewerInput;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.update.BatchUpdate;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.util.Collection;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+
+public class DeleteReviewersUtil {
+ private final DeleteReviewerOp.Factory deleteReviewerOpFactory;
+ private final DeleteReviewerByEmailOp.Factory deleteReviewerByEmailOpFactory;
+ private final AccountResolver accountResolver;
+ private final ApprovalsUtil approvalsUtil;
+
+ @Inject
+ DeleteReviewersUtil(
+ DeleteReviewerOp.Factory deleteReviewerOpFactory,
+ DeleteReviewerByEmailOp.Factory deleteReviewerByEmailOpFactory,
+ AccountResolver accountResolver,
+ ApprovalsUtil approvalsUtil) {
+ this.deleteReviewerOpFactory = deleteReviewerOpFactory;
+ this.deleteReviewerByEmailOpFactory = deleteReviewerByEmailOpFactory;
+ this.accountResolver = accountResolver;
+ this.approvalsUtil = approvalsUtil;
+ }
+
+ public void addDeleteReviewerOpToBatchUpdate(
+ BatchUpdate batchUpdate, ChangeNotes changeNotes, ReviewerInput reviewerInput)
+ throws IOException, ConfigInvalidException, AuthException, ResourceNotFoundException {
+
+ try {
+ AccountResolver.Result result =
+ accountResolver.resolveIgnoreVisibility(reviewerInput.reviewer);
+ if (fetchAccountIds(changeNotes).contains(result.asUniqueUser().getAccountId())) {
+ DeleteReviewerInput deleteReviewerInput = new DeleteReviewerInput();
+ deleteReviewerInput.notify = reviewerInput.notify;
+ deleteReviewerInput.notifyDetails = reviewerInput.notifyDetails;
+ batchUpdate.addOp(
+ changeNotes.getChangeId(),
+ deleteReviewerOpFactory.create(result.asUnique().account(), deleteReviewerInput));
+ return;
+ } else {
+ return;
+ }
+ } catch (AccountResolver.UnresolvableAccountException e) {
+ if (e.isSelf()) {
+ throw new AuthException(e.getMessage(), e);
+ }
+ }
+ Address address = Address.tryParse(reviewerInput.reviewer);
+ if (address != null && changeNotes.getReviewersByEmail().all().contains(address)) {
+ batchUpdate.addOp(changeNotes.getChangeId(), deleteReviewerByEmailOpFactory.create(address));
+ return;
+ }
+
+ throw new ResourceNotFoundException(reviewerInput.reviewer);
+ }
+
+ private Collection<Account.Id> fetchAccountIds(ChangeNotes changeNotes) {
+ return approvalsUtil.getReviewers(changeNotes).all();
+ }
+}
diff --git a/java/com/google/gerrit/server/change/ModifyReviewersEmail.java b/java/com/google/gerrit/server/change/ModifyReviewersEmail.java
index a7327f6..cb747f6 100644
--- a/java/com/google/gerrit/server/change/ModifyReviewersEmail.java
+++ b/java/com/google/gerrit/server/change/ModifyReviewersEmail.java
@@ -55,19 +55,25 @@
Change change,
Collection<Account.Id> added,
Collection<Account.Id> copied,
+ Collection<Account.Id> removed,
Collection<Address> addedByEmail,
Collection<Address> copiedByEmail,
+ Collection<Address> removedByEmail,
NotifyResolver.Result notify) {
- // The user knows they added themselves, don't bother emailing them.
+ // The user knows they added/removed themselves, don't bother emailing them.
Account.Id userId = user.getAccountId();
ImmutableList<Account.Id> immutableToMail =
added.stream().filter(id -> !id.equals(userId)).collect(toImmutableList());
ImmutableList<Account.Id> immutableToCopy =
copied.stream().filter(id -> !id.equals(userId)).collect(toImmutableList());
+ ImmutableList<Account.Id> immutableToRemove =
+ removed.stream().filter(id -> !id.equals(userId)).collect(toImmutableList());
if (immutableToMail.isEmpty()
&& immutableToCopy.isEmpty()
+ && immutableToRemove.isEmpty()
&& addedByEmail.isEmpty()
- && copiedByEmail.isEmpty()) {
+ && copiedByEmail.isEmpty()
+ && removedByEmail.isEmpty()) {
return;
}
@@ -77,6 +83,7 @@
Project.NameKey projectNameKey = change.getProject();
ImmutableList<Address> immutableAddedByEmail = ImmutableList.copyOf(addedByEmail);
ImmutableList<Address> immutableCopiedByEmail = ImmutableList.copyOf(copiedByEmail);
+ ImmutableList<Address> immutableRemovedByEmail = ImmutableList.copyOf(removedByEmail);
@SuppressWarnings("unused")
Future<?> possiblyIgnoredError =
@@ -91,6 +98,8 @@
emailSender.addReviewersByEmail(immutableAddedByEmail);
emailSender.addExtraCC(immutableToCopy);
emailSender.addExtraCCByEmail(immutableCopiedByEmail);
+ emailSender.addRemovedReviewers(immutableToRemove);
+ emailSender.addRemovedByEmailReviewers(immutableRemovedByEmail);
emailSender.setMessageId(
messageIdGenerator.fromChangeUpdate(
change.getProject(), change.currentPatchSetId()));
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index b45b6a9..6f05072 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -19,12 +19,14 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
+import static com.google.gerrit.extensions.client.ReviewerState.REMOVED;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Ordering;
import com.google.common.collect.Streams;
@@ -39,6 +41,7 @@
import com.google.gerrit.entities.GroupDescription;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.extensions.api.changes.ReviewerInput;
@@ -155,6 +158,8 @@
private final Provider<AnonymousUser> anonymousProvider;
private final AddReviewersOp.Factory addReviewersOpFactory;
private final OutgoingEmailValidator validator;
+ private final DeleteReviewerOp.Factory deleteReviewerOpFactory;
+ private final DeleteReviewerByEmailOp.Factory deleteReviewerByEmailOpFactory;
@Inject
ReviewerModifier(
@@ -168,7 +173,9 @@
ProjectCache projectCache,
Provider<AnonymousUser> anonymousProvider,
AddReviewersOp.Factory addReviewersOpFactory,
- OutgoingEmailValidator validator) {
+ OutgoingEmailValidator validator,
+ DeleteReviewerOp.Factory deleteReviewerOpFactory,
+ DeleteReviewerByEmailOp.Factory deleteReviewerByEmailOpFactory) {
this.accountResolver = accountResolver;
this.permissionBackend = permissionBackend;
this.groupResolver = groupResolver;
@@ -180,6 +187,8 @@
this.anonymousProvider = anonymousProvider;
this.addReviewersOpFactory = addReviewersOpFactory;
this.validator = validator;
+ this.deleteReviewerOpFactory = deleteReviewerOpFactory;
+ this.deleteReviewerByEmailOpFactory = deleteReviewerByEmailOpFactory;
}
/**
@@ -209,7 +218,7 @@
.orElseThrow(illegalState(notes.getProjectName()))
.is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL);
- ReviewerModification byAccountId = addByAccountId(input, notes, user);
+ ReviewerModification byAccountId = byAccountId(input, notes, user);
ReviewerModification wholeGroup = null;
if (!byAccountId.exactMatchFound) {
@@ -244,15 +253,14 @@
newReviewerInput(user.getUserName().orElse(null), CC, NotifyHandling.NONE),
revision.getNotes(),
revision.getUser(),
- ImmutableSet.of(user.getAccountId()),
+ ImmutableSet.of(user.asIdentifiedUser().getAccount()),
null,
true,
false);
}
@Nullable
- private ReviewerModification addByAccountId(
- ReviewerInput input, ChangeNotes notes, CurrentUser user)
+ private ReviewerModification byAccountId(ReviewerInput input, ChangeNotes notes, CurrentUser user)
throws PermissionBackendException, IOException, ConfigInvalidException {
IdentifiedUser reviewerUser;
boolean exactMatchFound = false;
@@ -273,7 +281,7 @@
input,
notes,
user,
- ImmutableSet.of(reviewerUser.getAccountId()),
+ ImmutableSet.of(reviewerUser.getAccount()),
null,
exactMatchFound,
false);
@@ -319,7 +327,14 @@
MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
}
- Set<Account.Id> reviewers = new HashSet<>();
+ if (input.state().equals(REMOVED)) {
+ return fail(
+ input,
+ FailureType.OTHER,
+ MessageFormat.format(ChangeMessages.get().groupRemovalIsNotAllowed, group.getName()));
+ }
+
+ Set<Account> reviewers = new HashSet<>();
Set<Account> members;
try {
members = groupMembers.listAccounts(group.getGroupUUID(), notes.getProjectName());
@@ -356,7 +371,7 @@
for (Account member : members) {
if (isValidReviewer(notes.getChange().getDest(), member)) {
- reviewers.add(member.id());
+ reviewers.add(member);
}
}
@@ -412,8 +427,8 @@
public class ReviewerModification {
public final ReviewerResult result;
- @Nullable public final AddReviewersOp op;
- public final ImmutableSet<Account.Id> reviewers;
+ @Nullable public final ReviewerOp op;
+ public final ImmutableSet<Account> reviewers;
public final ImmutableSet<Address> reviewersByEmail;
@Nullable final IdentifiedUser caller;
final boolean exactMatchFound;
@@ -435,7 +450,7 @@
ReviewerInput input,
ChangeNotes notes,
CurrentUser caller,
- @Nullable Iterable<Account.Id> reviewers,
+ @Nullable Iterable<Account> reviewers,
@Nullable Iterable<Address> reviewersByEmail,
boolean exactMatchFound,
boolean forGroup) {
@@ -452,14 +467,40 @@
this.reviewersByEmail =
reviewersByEmail == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewersByEmail);
this.caller = caller.asIdentifiedUser();
- op = addReviewersOpFactory.create(this.reviewers, this.reviewersByEmail, state(), forGroup);
+ if (state().equals(REMOVED)) {
+ // only one is set.
+ checkState(
+ (this.reviewers.size() == 1 && this.reviewersByEmail.isEmpty())
+ || (this.reviewers.isEmpty() && this.reviewersByEmail.size() == 1));
+ if (this.reviewers.size() >= 1) {
+ checkState(this.reviewers.size() == 1);
+ DeleteReviewerInput deleteReviewerInput = new DeleteReviewerInput();
+ deleteReviewerInput.notify = input.notify;
+ deleteReviewerInput.notifyDetails = input.notifyDetails;
+ op =
+ deleteReviewerOpFactory.create(
+ Iterables.getOnlyElement(this.reviewers.asList()), deleteReviewerInput);
+ } else {
+ checkState(this.reviewersByEmail.size() == 1);
+ op =
+ deleteReviewerByEmailOpFactory.create(
+ Iterables.getOnlyElement(this.reviewersByEmail.asList()));
+ }
+ } else {
+ op =
+ addReviewersOpFactory.create(
+ this.reviewers.stream().map(Account::id).collect(toImmutableSet()),
+ this.reviewersByEmail,
+ state(),
+ forGroup);
+ }
this.exactMatchFound = exactMatchFound;
}
- private ImmutableSet<Account.Id> omitOwner(ChangeNotes notes, Iterable<Account.Id> reviewers) {
+ private ImmutableSet<Account> omitOwner(ChangeNotes notes, Iterable<Account> reviewers) {
return reviewers != null
? Streams.stream(reviewers)
- .filter(id -> !id.equals(notes.getChange().getOwner()))
+ .filter(account -> !account.id().equals(notes.getChange().getOwner()))
.collect(toImmutableSet())
: ImmutableSet.of();
}
@@ -470,31 +511,52 @@
// Generate result details and fill AccountLoader. This occurs outside
// the Op because the accounts are in a different table.
- AddReviewersOp.Result opResult = op.getResult();
- if (state() == CC) {
- result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
- for (Account.Id accountId : opResult.addedCCs()) {
- result.ccs.add(json.format(new ReviewerInfo(accountId.get()), accountId, cd));
- }
- accountLoaderFactory.create(true).fill(result.ccs);
- for (Address a : opResult.addedCCsByEmail()) {
- result.ccs.add(new AccountInfo(a.name(), a.email()));
- }
- } else {
- result.reviewers = Lists.newArrayListWithCapacity(opResult.addedReviewers().size());
- for (PatchSetApproval psa : opResult.addedReviewers()) {
- // New reviewers have value 0, don't bother normalizing.
- result.reviewers.add(
- json.format(
- new ReviewerInfo(psa.accountId().get()),
- psa.accountId(),
- cd,
- ImmutableList.of(psa)));
- }
- accountLoaderFactory.create(true).fill(result.reviewers);
- for (Address a : opResult.addedReviewersByEmail()) {
- result.reviewers.add(ReviewerInfo.byEmail(a.name(), a.email()));
- }
+ ReviewerOp.Result opResult = op.getResult();
+ switch (state()) {
+ case CC:
+ result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
+ for (Account.Id accountId : opResult.addedCCs()) {
+ result.ccs.add(json.format(new ReviewerInfo(accountId.get()), accountId, cd));
+ }
+ accountLoaderFactory.create(true).fill(result.ccs);
+ for (Address a : opResult.addedCCsByEmail()) {
+ result.ccs.add(new AccountInfo(a.name(), a.email()));
+ }
+ break;
+ case REVIEWER:
+ result.reviewers = Lists.newArrayListWithCapacity(opResult.addedReviewers().size());
+ for (PatchSetApproval psa : opResult.addedReviewers()) {
+ // New reviewers have value 0, don't bother normalizing.
+ result.reviewers.add(
+ json.format(
+ new ReviewerInfo(psa.accountId().get()),
+ psa.accountId(),
+ cd,
+ ImmutableList.of(psa)));
+ }
+ accountLoaderFactory.create(true).fill(result.reviewers);
+ for (Address a : opResult.addedReviewersByEmail()) {
+ result.reviewers.add(ReviewerInfo.byEmail(a.name(), a.email()));
+ }
+ break;
+ case REMOVED:
+ if (opResult.deletedReviewer().isPresent()) {
+ result.removed =
+ json.format(
+ new ReviewerInfo(opResult.deletedReviewer().get().get()),
+ opResult.deletedReviewer().get(),
+ cd);
+ accountLoaderFactory.create(true).fill(ImmutableList.of(result.removed));
+ } else if (opResult.deletedReviewerByEmail().isPresent()) {
+ result.removed =
+ new AccountInfo(
+ opResult.deletedReviewerByEmail().get().name(),
+ opResult.deletedReviewerByEmail().get().email());
+ }
+ break;
+ default:
+ throw new IllegalStateException(
+ String.format("Illegal ReviewerState argument is %s", state().name()));
}
}
@@ -573,7 +635,7 @@
// We never call updateRepo on the addition ops, which is only ok because it's a no-op.
public void updateChange(ChangeContext ctx, PatchSet patchSet)
- throws RestApiException, IOException {
+ throws RestApiException, IOException, PermissionBackendException {
for (ReviewerModification addition : modifications()) {
addition.op.setPatchSet(patchSet);
addition.op.updateChange(ctx);
diff --git a/java/com/google/gerrit/server/change/ReviewerOp.java b/java/com/google/gerrit/server/change/ReviewerOp.java
new file mode 100644
index 0000000..716ac5e
--- /dev/null
+++ b/java/com/google/gerrit/server/change/ReviewerOp.java
@@ -0,0 +1,99 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.requireNonNull;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Address;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.ChangeContext;
+import java.io.IOException;
+import java.util.Optional;
+
+public class ReviewerOp implements BatchUpdateOp {
+ protected boolean sendEmail = true;
+ protected PatchSet patchSet;
+ protected Result opResult;
+
+ // TODO(dborowitz): This mutable setter is ugly, but a) it's less ugly than adding boolean args
+ // all the way through the constructor stack, and b) this class is slated to be completely
+ // rewritten.
+ public void suppressEmail() {
+ this.sendEmail = false;
+ }
+
+ void setPatchSet(PatchSet patchSet) {
+ this.patchSet = requireNonNull(patchSet);
+ }
+
+ @AutoValue
+ public abstract static class Result {
+ public abstract ImmutableList<PatchSetApproval> addedReviewers();
+
+ public abstract ImmutableList<Address> addedReviewersByEmail();
+
+ public abstract ImmutableList<Account.Id> addedCCs();
+
+ public abstract ImmutableList<Address> addedCCsByEmail();
+
+ public abstract Optional<Account.Id> deletedReviewer();
+
+ public abstract Optional<Address> deletedReviewerByEmail();
+
+ static Builder builder() {
+ return new AutoValue_ReviewerOp_Result.Builder()
+ .setAddedReviewers(ImmutableList.of())
+ .setAddedReviewersByEmail(ImmutableList.of())
+ .setAddedCCs(ImmutableList.of())
+ .setAddedCCsByEmail(ImmutableList.of());
+ }
+
+ @AutoValue.Builder
+ abstract static class Builder {
+ abstract Builder setAddedReviewers(Iterable<PatchSetApproval> addedReviewers);
+
+ abstract Builder setAddedReviewersByEmail(Iterable<Address> addedReviewersByEmail);
+
+ abstract Builder setAddedCCs(Iterable<Account.Id> addedCCs);
+
+ abstract Builder setAddedCCsByEmail(Iterable<Address> addedCCsByEmail);
+
+ abstract Builder setDeletedReviewerByEmail(Address deletedReviewerByEmail);
+
+ abstract Builder setDeletedReviewer(Account.Id deletedReviewer);
+
+ abstract Result build();
+ }
+ }
+
+ public Result getResult() {
+ checkState(opResult != null, "Batch update wasn't executed yet");
+ return opResult;
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx)
+ throws RestApiException, IOException, PermissionBackendException {
+ return false;
+ }
+}
diff --git a/java/com/google/gerrit/server/mail/send/NewChangeSender.java b/java/com/google/gerrit/server/mail/send/NewChangeSender.java
index ee9a328..001de52 100644
--- a/java/com/google/gerrit/server/mail/send/NewChangeSender.java
+++ b/java/com/google/gerrit/server/mail/send/NewChangeSender.java
@@ -31,6 +31,8 @@
private final Set<Address> reviewersByEmail = new HashSet<>();
private final Set<Account.Id> extraCC = new HashSet<>();
private final Set<Address> extraCCByEmail = new HashSet<>();
+ private final Set<Account.Id> removedReviewers = new HashSet<>();
+ private final Set<Address> removedByEmailReviewers = new HashSet<>();
protected NewChangeSender(EmailArguments args, ChangeData changeData) {
super(args, "newchange", changeData);
@@ -52,10 +54,17 @@
extraCCByEmail.addAll(cc);
}
+ public void addRemovedReviewers(Collection<Account.Id> removed) {
+ removedReviewers.addAll(removed);
+ }
+
+ public void addRemovedByEmailReviewers(Collection<Address> removed) {
+ removedByEmailReviewers.addAll(removed);
+ }
+
@Override
protected void init() throws EmailException {
super.init();
-
String threadId = getChangeMessageThreadId();
setHeader("References", threadId);
@@ -71,6 +80,8 @@
case OWNER_REVIEWERS:
reviewers.stream().forEach(r -> add(RecipientType.TO, r, true));
addByEmail(RecipientType.TO, reviewersByEmail, true);
+ removedReviewers.stream().forEach(r -> add(RecipientType.TO, r, true));
+ addByEmail(RecipientType.TO, removedByEmailReviewers, true);
break;
}
@@ -96,10 +107,25 @@
return names;
}
+ public List<String> getRemovedReviewerNames() {
+ if (removedReviewers.isEmpty() && removedByEmailReviewers.isEmpty()) {
+ return null;
+ }
+ List<String> names = new ArrayList<>();
+ for (Account.Id id : removedReviewers) {
+ names.add(getNameFor(id));
+ }
+ for (Address address : removedByEmailReviewers) {
+ names.add(address.name());
+ }
+ return names;
+ }
+
@Override
protected void setupSoyContext() {
super.setupSoyContext();
soyContext.put("ownerName", getNameFor(change.getOwner()));
soyContextEmailData.put("reviewerNames", getReviewerNames());
+ soyContextEmailData.put("removedReviewerNames", getRemovedReviewerNames());
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteReviewer.java b/java/com/google/gerrit/server/restapi/change/DeleteReviewer.java
index 3e4a483..db8e9de 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteReviewer.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteReviewer.java
@@ -64,7 +64,7 @@
if (rsrc.isByEmail()) {
op = deleteReviewerByEmailOpFactory.create(rsrc.getReviewerByEmail());
} else {
- op = deleteReviewerOpFactory.create(rsrc.getReviewerUser().state(), input);
+ op = deleteReviewerOpFactory.create(rsrc.getReviewerUser().getAccount(), input);
}
bu.addOp(rsrc.getChange().getId(), op);
bu.execute();
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index b5f2ab1..8c1e655 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -16,6 +16,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
@@ -91,13 +92,13 @@
import com.google.gerrit.server.PublishCommentUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.account.AccountResolver;
-import com.google.gerrit.server.change.AddReviewersOp.Result;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.EmailReviewComments;
import com.google.gerrit.server.change.ModifyReviewersEmail;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.ReviewerModifier;
import com.google.gerrit.server.change.ReviewerModifier.ReviewerModification;
+import com.google.gerrit.server.change.ReviewerOp.Result;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.WorkInProgressOp;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -313,7 +314,7 @@
try (BatchUpdate bu =
updateFactory.create(revision.getChange().getProject(), revision.getUser(), ts)) {
- Account.Id id = revision.getUser().getAccountId();
+ Account account = revision.getUser().asIdentifiedUser().getAccount();
boolean ccOrReviewer = false;
if (input.labels != null && !input.labels.isEmpty()) {
ccOrReviewer = input.labels.values().stream().anyMatch(v -> v != 0);
@@ -326,7 +327,7 @@
// Check if user was already CCed or reviewing prior to this review.
ReviewerSet currentReviewers =
approvalsUtil.getReviewers(revision.getChangeResource().getNotes());
- ccOrReviewer = currentReviewers.all().contains(id);
+ ccOrReviewer = currentReviewers.all().contains(account.id());
if (ccOrReviewer) {
logger.atFine().log("calling user is already cc/reviewer on the change");
}
@@ -339,7 +340,7 @@
for (ReviewerModification reviewerResult : reviewerResults) {
reviewerResult.op.suppressEmail(); // Send a single batch email below.
bu.addOp(revision.getChange().getId(), reviewerResult.op);
- if (!ccOrReviewer && reviewerResult.reviewers.contains(id)) {
+ if (!ccOrReviewer && reviewerResult.reviewers.contains(account)) {
logger.atFine().log("calling user is explicitly added as reviewer or CC");
ccOrReviewer = true;
}
@@ -443,24 +444,39 @@
try (TraceContext.TraceTimer ignored = newTimer("batchEmailReviewers")) {
List<Account.Id> to = new ArrayList<>();
List<Account.Id> cc = new ArrayList<>();
+ List<Account.Id> removed = new ArrayList<>();
List<Address> toByEmail = new ArrayList<>();
List<Address> ccByEmail = new ArrayList<>();
+ List<Address> removedByEmail = new ArrayList<>();
for (ReviewerModification modification : reviewerModifications) {
Result reviewAdditionResult = modification.op.getResult();
if (modification.state() == ReviewerState.REVIEWER
&& (!reviewAdditionResult.addedReviewers().isEmpty()
|| !reviewAdditionResult.addedReviewersByEmail().isEmpty())) {
- to.addAll(modification.reviewers);
+ to.addAll(modification.reviewers.stream().map(Account::id).collect(toImmutableSet()));
toByEmail.addAll(modification.reviewersByEmail);
} else if (modification.state() == ReviewerState.CC
&& (!reviewAdditionResult.addedCCs().isEmpty()
|| !reviewAdditionResult.addedCCsByEmail().isEmpty())) {
- cc.addAll(modification.reviewers);
+ cc.addAll(modification.reviewers.stream().map(Account::id).collect(toImmutableSet()));
ccByEmail.addAll(modification.reviewersByEmail);
+ } else if (modification.state() == ReviewerState.REMOVED
+ && (reviewAdditionResult.deletedReviewer().isPresent()
+ || reviewAdditionResult.deletedReviewerByEmail().isPresent())) {
+ reviewAdditionResult.deletedReviewer().ifPresent(d -> removed.add(d));
+ reviewAdditionResult.deletedReviewerByEmail().ifPresent(d -> removedByEmail.add(d));
}
}
modifyReviewersEmail.emailReviewersAsync(
- user.asIdentifiedUser(), change, to, cc, toByEmail, ccByEmail, notify);
+ user.asIdentifiedUser(),
+ change,
+ to,
+ cc,
+ removed,
+ toByEmail,
+ ccByEmail,
+ removedByEmail,
+ notify);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index dee7861..b54e9cd 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -157,6 +157,7 @@
import com.google.gerrit.index.query.PostFilterPredicate;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.testing.TestChangeETagComputation;
import com.google.gerrit.server.git.ChangeMessageModifier;
@@ -185,6 +186,7 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.sql.Timestamp;
+import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@@ -2113,6 +2115,46 @@
}
@Test
+ public void deleteGroupFromReviewersFails() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ // create a group named "kobe" with one user: lee
+ String myGroupUserEmail = "lee@example.com";
+ String myGroupUserFullname = "lee";
+ accountOperations
+ .newAccount()
+ .username("lee")
+ .preferredEmail(myGroupUserEmail)
+ .fullname(myGroupUserFullname)
+ .create();
+
+ String groupName = "kobe";
+ String testGroup = groupOperations.newGroup().name(groupName).create().get();
+ GroupApi groupApi = gApi.groups().id(testGroup);
+ groupApi.description("test group");
+ groupApi.addMembers(myGroupUserFullname);
+
+ // add the user as reviewer.
+ gApi.changes().id(r.getChangeId()).addReviewer(myGroupUserFullname);
+
+ // fail to remove that user via group.
+ ReviewResult reviewResult =
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(ReviewInput.create().reviewer(testGroup, REMOVED, /* confirmed= */ true));
+
+ assertThat(reviewResult.error).isEqualTo("error adding reviewer");
+
+ ReviewerInput in = new ReviewerInput();
+ in.reviewer = testGroup;
+ in.state = REMOVED;
+ ReviewerResult reviewerResult = gApi.changes().id(r.getChangeId()).addReviewer(in);
+ assertThat(reviewerResult.error)
+ .isEqualTo(MessageFormat.format(ChangeMessages.get().groupRemovalIsNotAllowed, groupName));
+ }
+
+ @Test
@UseClockStep
public void addSelfAsReviewer() throws Exception {
PushOneCommit.Result r = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 34679bd..b79be80 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -14,6 +14,7 @@
package com.google.gerrit.acceptance.api.change;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.util.stream.Collectors.toList;
@@ -47,6 +48,7 @@
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
+import com.google.gerrit.extensions.api.changes.ReviewResult;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.AccountInfo;
@@ -55,6 +57,7 @@
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.events.CommentAddedListener;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
@@ -66,6 +69,7 @@
import com.google.gerrit.server.restapi.change.PostReview;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.gerrit.server.update.CommentsRejectedException;
+import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -74,6 +78,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.regex.Pattern;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
@@ -693,6 +698,186 @@
assertThat(testSubmitRule.count).isEqualTo(1);
}
+ @Test
+ public void deletingReviewers() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ TestAccount user2 = accountCreator.user2();
+
+ // add user and user2
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(ReviewInput.create().reviewer(user.email()).reviewer(user2.email()));
+
+ sender.clear();
+
+ // remove user and user2
+ ReviewResult reviewResult =
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true)
+ .reviewer(user2.email(), ReviewerState.REMOVED, /* confirmed= */ true));
+
+ assertThat(
+ reviewResult.reviewers.values().stream()
+ .map(a -> a.removed.name)
+ .collect(toImmutableSet()))
+ .containsExactly(user.fullName(), user2.fullName());
+
+ assertThat(gApi.changes().id(r.getChangeId()).reviewers()).isEmpty();
+
+ // Ensure only one batch email was sent for this operation
+ FakeEmailSender.Message message = Iterables.getOnlyElement(sender.getMessages());
+ assertThat(message.body())
+ .containsMatch(
+ Pattern.quote("removed ")
+ + "("
+ + Pattern.quote(String.format("%s, %s", user.fullName(), user2.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s, %s", user2.fullName(), user.fullName()))
+ + ")");
+ assertThat(message.htmlBody())
+ .containsMatch(
+ Pattern.quote("removed ")
+ + "("
+ + Pattern.quote(String.format("%s and %s", user.fullName(), user2.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s and %s", user2.fullName(), user.fullName()))
+ + ")");
+ }
+
+ @Test
+ public void addingAndDeletingReviewers() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ TestAccount user2 = accountCreator.user2();
+ TestAccount user3 = accountCreator.create("user3", "user3@email.com", "user3", "user3");
+ TestAccount user4 = accountCreator.create("user4", "user4@email.com", "user4", "user4");
+
+ // add user and user2
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(ReviewInput.create().reviewer(user.email()).reviewer(user2.email()));
+
+ sender.clear();
+
+ // remove user and user2 while adding user3 and user4
+ ReviewResult reviewResult =
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true)
+ .reviewer(user2.email(), ReviewerState.REMOVED, /* confirmed= */ true)
+ .reviewer(user3.email())
+ .reviewer(user4.email()));
+
+ assertThat(
+ reviewResult.reviewers.values().stream()
+ .filter(a -> a.removed != null)
+ .map(a -> a.removed.name)
+ .collect(toImmutableSet()))
+ .containsExactly(user.fullName(), user2.fullName());
+ assertThat(
+ reviewResult.reviewers.values().stream()
+ .filter(a -> a.reviewers != null)
+ .map(a -> Iterables.getOnlyElement(a.reviewers).name)
+ .collect(toImmutableSet()))
+ .containsExactly(user3.fullName(), user4.fullName());
+
+ assertThat(
+ gApi.changes().id(r.getChangeId()).reviewers().stream()
+ .map(a -> a.name)
+ .collect(toImmutableSet()))
+ .containsExactly(user3.fullName(), user4.fullName());
+
+ // Ensure only one batch email was sent for this operation
+ FakeEmailSender.Message message = Iterables.getOnlyElement(sender.getMessages());
+ assertThat(message.body())
+ .containsMatch(
+ Pattern.quote("Hello ")
+ + "("
+ + Pattern.quote(String.format("%s, %s", user3.fullName(), user4.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s, %s", user4.fullName(), user3.fullName()))
+ + ")");
+ assertThat(message.htmlBody())
+ .containsMatch(
+ "("
+ + Pattern.quote(String.format("%s and %s", user3.fullName(), user4.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s and %s", user4.fullName(), user3.fullName()))
+ + ")"
+ + Pattern.quote(" to <strong>review</strong> this change"));
+
+ assertThat(message.body())
+ .containsMatch(
+ Pattern.quote("removed ")
+ + "("
+ + Pattern.quote(String.format("%s, %s", user.fullName(), user2.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s, %s", user2.fullName(), user.fullName()))
+ + ")");
+ assertThat(message.htmlBody())
+ .containsMatch(
+ Pattern.quote("removed ")
+ + "("
+ + Pattern.quote(String.format("%s and %s", user.fullName(), user2.fullName()))
+ + "|"
+ + Pattern.quote(String.format("%s and %s", user2.fullName(), user.fullName()))
+ + ")");
+ }
+
+ @Test
+ public void deletingNonExistingReviewerFails() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ ResourceNotFoundException resourceNotFoundException =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () ->
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true)));
+ assertThat(resourceNotFoundException)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Reviewer %s doesn't exist in the change, hence can't delete it", user.fullName()));
+ }
+
+ @Test
+ public void addingAndDeletingSameReviewerFails() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ ResourceNotFoundException resourceNotFoundException =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () ->
+ gApi.changes()
+ .id(r.getChangeId())
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email())
+ .reviewer(user.email(), ReviewerState.REMOVED, true)));
+ assertThat(resourceNotFoundException)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Reviewer %s doesn't exist in the change," + " hence can't delete it",
+ user.fullName()));
+ }
+
private static class TestListener implements CommentAddedListener {
public CommentAddedListener.Event lastCommentAddedEvent;
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index 41bedbc..db218b4 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -442,6 +442,7 @@
assertThat(result.labels).isNull();
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(3);
+
ReviewerResult reviewerResult = result.reviewers.get(largeGroup);
assertThat(reviewerResult).isNotNull();
assertThat(reviewerResult.confirm).isNull();
@@ -539,6 +540,7 @@
ReviewResult result = review(r.getChangeId(), r.getCommit().name(), input);
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(1);
+
ReviewerResult reviewerResult = result.reviewers.get(user.email());
assertThat(reviewerResult).isNotNull();
assertThat(reviewerResult.confirm).isNull();
@@ -564,6 +566,7 @@
ReviewResult result = review(r.getChangeId(), r.getCommit().name(), input);
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(2);
+
ReviewerResult reviewerResult = result.reviewers.get(group1);
assertThat(reviewerResult.error).isNull();
assertThat(reviewerResult.reviewers).hasSize(2);
diff --git a/resources/com/google/gerrit/server/change/ChangeMessages.properties b/resources/com/google/gerrit/server/change/ChangeMessages.properties
index 3763d8e..c78f5a3 100644
--- a/resources/com/google/gerrit/server/change/ChangeMessages.properties
+++ b/resources/com/google/gerrit/server/change/ChangeMessages.properties
@@ -7,6 +7,7 @@
reviewerInvalid = {0} is not a valid user identifier
reviewerNotFoundUserOrGroup = {0} does not identify a registered user or group
+groupRemovalIsNotAllowed = Groups can't be removed from reviewers, so can't remove {0}.
groupIsNotAllowed = The group {0} cannot be added as reviewer.
groupHasTooManyMembers = The group {0} has too many members to add them all as reviewers.
groupManyMembersConfirmation = The group {0} has {1} members. Do you want to add them all as reviewers?
diff --git a/resources/com/google/gerrit/server/mail/NewChange.soy b/resources/com/google/gerrit/server/mail/NewChange.soy
index 0fe8835..aa2b946 100644
--- a/resources/com/google/gerrit/server/mail/NewChange.soy
+++ b/resources/com/google/gerrit/server/mail/NewChange.soy
@@ -23,23 +23,35 @@
{template NewChange kind="text"}
{@param change: ?}
{@param email: ?}
+ {@param fromName: ?}
{@param ownerName: ?}
{@param patchSet: ?}
{@param projectName: ?}
- {if $email.reviewerNames}
- Hello{sp}
- {for $reviewerName in $email.reviewerNames}
- {if not isFirst($reviewerName)},{sp}{/if}
- {$reviewerName}
- {/for},
+ {if $email.reviewerNames or $email.removedReviewerNames}
+ {if $email.reviewerNames}
+ Hello{sp}
+ {for $reviewerName in $email.reviewerNames}
+ {if not isFirst($reviewerName)},{sp}{/if}
+ {$reviewerName}
+ {/for},
- {\n}
- {\n}
+ {\n}
+ {\n}
- I'd like you to do a code review.
-
+ I'd like you to do a code review.
+ {\n}
+ {/if}
+ {if $email.removedReviewerNames}
+ {$fromName} has removed{sp}
+ {for $reviewerName in $email.removedReviewerNames}
+ {if not isFirst($reviewerName)},{sp}{/if}
+ {$reviewerName}
+ {/for}{sp}
+ from this change.{sp}
+ {\n}
+ {/if}
{if $email.changeUrl}
- {sp}Please visit
+ Please visit
{\n}
{\n}
diff --git a/resources/com/google/gerrit/server/mail/NewChangeHtml.soy b/resources/com/google/gerrit/server/mail/NewChangeHtml.soy
index dbb3d8a..272c3ef 100644
--- a/resources/com/google/gerrit/server/mail/NewChangeHtml.soy
+++ b/resources/com/google/gerrit/server/mail/NewChangeHtml.soy
@@ -26,16 +26,35 @@
{@param patchSet: ?}
{@param projectName: ?}
<p>
- {if $email.reviewerNames}
- {$fromName} would like{sp}
- {for $reviewerName in $email.reviewerNames}
- {if not isFirst($reviewerName)}
- {if isLast($reviewerName)}{sp}and{else},{/if}{sp}
- {/if}
- {$reviewerName}
- {/for}{sp}
- to <strong>review</strong> this change
- {if $fromName != $ownerName}{sp}authored by {$ownerName}{/if}.
+ {if $email.reviewerNames or $email.removedReviewerNames}
+ {if $email.reviewerNames}
+ {$fromName} would like{sp}
+ {for $reviewerName in $email.reviewerNames}
+ {if not isFirst($reviewerName)}
+ {if isLast($reviewerName)}{sp}and{else},{/if}{sp}
+ {/if}
+ {$reviewerName}
+ {/for}{sp}
+ to <strong>review</strong> this change
+ {if $fromName != $ownerName}{sp}authored by {$ownerName}{/if}.
+ {\n}
+ {/if}
+ {if $email.removedReviewerNames}
+ <p>
+ {$fromName}{sp}
+ <strong>
+ removed{sp}
+ {for $reviewerName in $email.removedReviewerNames}
+ {if not isFirst($reviewerName)}
+ {if isLast($reviewerName)}{sp}and{else},{/if}{sp}
+ {/if}
+ {$reviewerName}
+ {/for}
+ </strong>{sp}
+ from this change.
+ </p>
+ {\n}
+ {/if}
{else}
{$ownerName} has uploaded this change for <strong>review</strong>.
{/if}
