Implement batch removal of reviewers in PostReview
Currently, it's fully supported to add multiple reviewers in PostReview,
and it's supported to delete reviewers via the delete reviewer endpoint
(only one per endpoint). However, it is not supported to delete
multiple reviewers in one endpoint in any way. This may cause issues
since currently to delete multiple reviewers the frontend calls the
delete reviewer endpoint many times.
This change extends PostReview to support deletion of reviewers on top
of additions. We ensure only one BatchUpdate is performed,
and only one email is sent.
It's unclear why we're sending the emails via NewChange.soy, but it is
out of scope for this change.
Change-Id: I088a8dc9bcf1390bc30e829ba33d1b52ce126c7b
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 69bbc54..fb3e3ec 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -7757,7 +7757,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.
@@ -7791,7 +7791,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
@@ -7822,24 +7822,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
@@ -7858,8 +7859,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"]
|===========================
@@ -7873,7 +7874,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}
