PostReviewersOp: Pass correct to/CC arguments to emailReviewers
The emailReviewers method takes 4 lists for added reviewers/CCs by
name/email. A given PostReviewersOp is only capable of populating at
most 2 of those lists, since a single op is tied to a single
ReviewerState. However, the factory arguments and instance fields used
the name "reviewers".
This naming confusion caused a bug: the "reviewersByEmail" field was
always passed as the "addedByEmail" argument to emailReviewers, even
when the "reviewers" were actually CCs.
Restructure PostReviewersOp to produce the 4 output lists from its
updateChange method, and sanity-check the results. These 4 lists are
populated in the Result as well as passed to emailReviewers.
In rewriting the logic, I discovered another few subtle issues that it
made sense to fix:
* Previously, we never filtered the list of by-email reviewers being
added to exclude existing reviewers. The ultimate result was
idempotent when parsed back into a ReviewerByEmailSet, but it may
have resulted in redundant NoteDb updates and redundant results
returned in the REST API.
* Previously, if the results of adding reviewers by ID resulted in no
update, the whole method would short-circuit, even if there were
pending reviewers to add by email. This case couldn't happen with
the current API interface, as all callers of PostReviewersOp.Factory
pass either IDs or addresses, not both.
Change-Id: I96f31af4d958ad331bbee23fd92d0b2055b3ad1d
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
index dd6e457..9577fb3 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
@@ -493,7 +493,7 @@
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), accountId, cd));
}
accountLoaderFactory.create(true).fill(result.ccs);
- for (Address a : reviewersByEmail) {
+ for (Address a : opResult.addedCCsByEmail()) {
result.ccs.add(new AccountInfo(a.getName(), a.getEmail()));
}
} else {
@@ -508,7 +508,7 @@
ImmutableList.of(psa)));
}
accountLoaderFactory.create(true).fill(result.reviewers);
- for (Address a : reviewersByEmail) {
+ for (Address a : opResult.addedReviewersByEmail()) {
result.reviewers.add(ReviewerInfo.byEmail(a.getName(), a.getEmail()));
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewersOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewersOp.java
index 6379393..8d411b4 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewersOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewersOp.java
@@ -16,12 +16,14 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
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.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Streams;
@@ -53,16 +55,30 @@
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
-import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Set;
public class PostReviewersOp implements BatchUpdateOp {
public interface Factory {
+
+ /**
+ * Create a new op.
+ *
+ * <p>Users may be added by account or by email addresses, as determined by {@code accountIds}
+ * and {@code addresses}. The reviewer state for both accounts and email addresses is determined
+ * by {@code state}.
+ *
+ * @param accountIds account IDs to add.
+ * @param addresses email addresses to add.
+ * @param state resulting reviewer state.
+ * @param notify notification handling.
+ * @param accountsToNotify additional accounts to notify.
+ * @return batch update operation.
+ */
PostReviewersOp create(
- Set<Account.Id> reviewers,
- Collection<Address> reviewersByEmail,
+ Set<Account.Id> accountIds,
+ Collection<Address> addresses,
ReviewerState state,
@Nullable NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify);
@@ -72,17 +88,25 @@
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_PostReviewersOp_Result.Builder();
}
@AutoValue.Builder
abstract static class Builder {
- abstract Builder setAddedReviewers(ImmutableList<PatchSetApproval> addedReviewers);
+ abstract Builder setAddedReviewers(Iterable<PatchSetApproval> addedReviewers);
- abstract Builder setAddedCCs(ImmutableList<Account.Id> addedCCs);
+ abstract Builder setAddedReviewersByEmail(Iterable<Address> addedReviewersByEmail);
+
+ abstract Builder setAddedCCs(Iterable<Account.Id> addedCCs);
+
+ abstract Builder setAddedCCsByEmail(Iterable<Address> addedCCsByEmail);
abstract Result build();
}
@@ -97,15 +121,19 @@
private final NotesMigration migration;
private final Provider<IdentifiedUser> user;
private final Provider<ReviewDb> dbProvider;
- private final Set<Account.Id> reviewers;
- private final Collection<Address> reviewersByEmail;
+ private final Set<Account.Id> accountIds;
+ private final Collection<Address> addresses;
private final ReviewerState state;
private final NotifyHandling notify;
private final ListMultimap<RecipientType, Account.Id> accountsToNotify;
- private List<PatchSetApproval> addedReviewers = new ArrayList<>();
- private Collection<Account.Id> addedCCs = new ArrayList<>();
- private Collection<Address> addedCCsByEmail = new ArrayList<>();
+ // Unlike addedCCs, addedReviewers is a PatchSetApproval because the AddReviewerResult returned
+ // via the REST API is supposed to include vote information.
+ private List<PatchSetApproval> addedReviewers = ImmutableList.of();
+ private Collection<Address> addedReviewersByEmail = ImmutableList.of();
+ private Collection<Account.Id> addedCCs = ImmutableList.of();
+ private Collection<Address> addedCCsByEmail = ImmutableList.of();
+
private Change change;
private PatchSet patchSet;
private Result opResult;
@@ -121,8 +149,8 @@
NotesMigration migration,
Provider<IdentifiedUser> user,
Provider<ReviewDb> dbProvider,
- @Assisted Set<Account.Id> reviewers,
- @Assisted Collection<Address> reviewersByEmail,
+ @Assisted Set<Account.Id> accountIds,
+ @Assisted Collection<Address> addresses,
@Assisted ReviewerState state,
@Assisted @Nullable NotifyHandling notify,
@Assisted ListMultimap<RecipientType, Account.Id> accountsToNotify) {
@@ -137,8 +165,8 @@
this.user = user;
this.dbProvider = dbProvider;
- this.reviewers = reviewers;
- this.reviewersByEmail = reviewersByEmail;
+ this.accountIds = accountIds;
+ this.addresses = addresses;
this.state = state;
this.notify = notify;
this.accountsToNotify = accountsToNotify;
@@ -148,14 +176,11 @@
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
change = ctx.getChange();
- if (!reviewers.isEmpty()) {
+ if (!accountIds.isEmpty()) {
if (migration.readChanges() && state == CC) {
addedCCs =
approvalsUtil.addCcs(
- ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), reviewers);
- if (addedCCs.isEmpty()) {
- return false;
- }
+ ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), accountIds);
} else {
addedReviewers =
approvalsUtil.addReviewers(
@@ -164,35 +189,76 @@
ctx.getUpdate(change.currentPatchSetId()),
projectCache.checkedGet(change.getProject()).getLabelTypes(change.getDest()),
change,
- reviewers);
- if (addedReviewers.isEmpty()) {
- return false;
- }
+ accountIds);
}
}
- for (Address a : reviewersByEmail) {
- ctx.getUpdate(change.currentPatchSetId())
- .putReviewerByEmail(a, ReviewerStateInternal.fromReviewerState(state));
+ ImmutableList<Address> addressesToAdd = ImmutableList.of();
+ ReviewerStateInternal internalState = ReviewerStateInternal.fromReviewerState(state);
+ if (migration.readChanges()) {
+ // TODO(dborowitz): This behavior should live in ApprovalsUtil or something, like addCcs does.
+ ImmutableSet<Address> existing = ctx.getNotes().getReviewersByEmail().byState(internalState);
+ addressesToAdd =
+ addresses.stream().filter(a -> !existing.contains(a)).collect(toImmutableList());
+
+ if (state == CC) {
+ addedCCsByEmail = addressesToAdd;
+ } else {
+ addedReviewersByEmail = addressesToAdd;
+ }
+ for (Address a : addressesToAdd) {
+ ctx.getUpdate(change.currentPatchSetId()).putReviewerByEmail(a, internalState);
+ }
}
+ if (addedCCs.isEmpty() && addedReviewers.isEmpty() && addressesToAdd.isEmpty()) {
+ return false;
+ }
+
+ checkAdded();
patchSet = psUtil.current(dbProvider.get(), ctx.getNotes());
return true;
}
+ private void checkAdded() {
+ // Should only affect either reviewers or CCs, not both. But the logic in updateChange is
+ // complex, so programmer error is conceivable.
+ boolean addedAnyReviewers = !addedReviewers.isEmpty() || !addedReviewersByEmail.isEmpty();
+ boolean addedAnyCCs = !addedCCs.isEmpty() || !addedCCsByEmail.isEmpty();
+ checkState(
+ !(addedAnyReviewers && addedAnyCCs),
+ "should not have added both reviewers and CCs:\n"
+ + "Arguments:\n"
+ + " accountIds=%s\n"
+ + " addresses=%s\n"
+ + "Results:\n"
+ + " addedReviewers=%s\n"
+ + " addedReviewersByEmail=%s\n"
+ + " addedCCs=%s\n"
+ + " addedCCsByEmail=%s",
+ accountIds,
+ addresses,
+ addedReviewers,
+ addedReviewersByEmail,
+ addedCCs,
+ addedCCsByEmail);
+ }
+
@Override
public void postUpdate(Context ctx) throws Exception {
opResult =
Result.builder()
- .setAddedReviewers(ImmutableList.copyOf(addedReviewers))
- .setAddedCCs(ImmutableList.copyOf(addedCCs))
+ .setAddedReviewers(addedReviewers)
+ .setAddedReviewersByEmail(addedReviewersByEmail)
+ .setAddedCCs(addedCCs)
+ .setAddedCCsByEmail(addedCCsByEmail)
.build();
postReviewersEmail.emailReviewers(
user.get(),
change,
Lists.transform(addedReviewers, PatchSetApproval::getAccountId),
- addedCCs == null ? ImmutableList.of() : addedCCs,
- reviewersByEmail,
+ addedCCs,
+ addedReviewersByEmail,
addedCCsByEmail,
notify,
accountsToNotify,
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
index 2e95e29..257c88b 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
@@ -335,12 +335,7 @@
assertThat(info._accountId).isNull();
assertThat(info.email).isEqualTo(input.reviewer);
- // TODO(dborowitz): Shouldn't report that existing reviewer by email were added the second time.
- result = gApi.changes().id(r.getChangeId()).addReviewer(input);
- assertThat(result.reviewers).hasSize(1);
- info = result.reviewers.get(0);
- assertThat(info._accountId).isNull();
- assertThat(info.email).isEqualTo(input.reviewer);
+ assertThat(gApi.changes().id(r.getChangeId()).addReviewer(input).reviewers).isEmpty();
}
@Test
@@ -358,12 +353,7 @@
assertThat(info._accountId).isNull();
assertThat(info.email).isEqualTo(input.reviewer);
- // TODO(dborowitz): Shouldn't report that existing CC by email were added the second time.
- result = gApi.changes().id(r.getChangeId()).addReviewer(input);
- assertThat(result.ccs).hasSize(1);
- info = result.ccs.get(0);
- assertThat(info._accountId).isNull();
- assertThat(info.email).isEqualTo(input.reviewer);
+ assertThat(gApi.changes().id(r.getChangeId()).addReviewer(input).ccs).isEmpty();
}
private static String toRfcAddressString(AccountInfo info) {
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index 8b80358..2daccc0 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -566,11 +566,10 @@
addReviewerToReviewableChangeInNoteDbByOwnerCcingSelfNotifyNone(batch());
}
- @Test
- public void addNonUserReviewerByEmailInNoteDbSingly() throws Exception {
+ private void addNonUserReviewerByEmailInNoteDb(Adder adder) throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
- addReviewer(singly(ReviewerState.REVIEWER), sc.changeId, sc.owner, "nonexistent@example.com");
+ addReviewer(adder, sc.changeId, sc.owner, "nonexistent@example.com");
assertThat(sender)
.sent("newchange", sc)
.to("nonexistent@example.com")
@@ -580,13 +579,22 @@
}
@Test
+ public void addNonUserReviewerByEmailInNoteDbSingly() throws Exception {
+ addNonUserReviewerByEmailInNoteDb(singly(ReviewerState.REVIEWER));
+ }
+
+ @Test
public void addNonUserReviewerByEmailInNoteDbBatch() throws Exception {
+ addNonUserReviewerByEmailInNoteDb(batch(ReviewerState.REVIEWER));
+ }
+
+ private void addNonUserCcByEmailInNoteDb(Adder adder) throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
- addReviewer(batch(ReviewerState.REVIEWER), sc.changeId, sc.owner, "nonexistent@example.com");
+ addReviewer(adder, sc.changeId, sc.owner, "nonexistent@example.com");
assertThat(sender)
.sent("newchange", sc)
- .to("nonexistent@example.com")
+ .cc("nonexistent@example.com")
.cc(sc.reviewer)
.cc(sc.ccerByEmail, sc.reviewerByEmail)
.noOneElse();
@@ -594,28 +602,12 @@
@Test
public void addNonUserCcByEmailInNoteDbSingly() throws Exception {
- assume().that(notesMigration.readChanges()).isTrue();
- StagedChange sc = stageReviewableChange();
- addReviewer(singly(ReviewerState.CC), sc.changeId, sc.owner, "nonexistent@example.com");
- assertThat(sender)
- .sent("newchange", sc)
- .to("nonexistent@example.com") // TODO(dborowitz): Should be cc.
- .cc(sc.reviewer)
- .cc(sc.ccerByEmail, sc.reviewerByEmail)
- .noOneElse();
+ addNonUserCcByEmailInNoteDb(singly(ReviewerState.CC));
}
@Test
public void addNonUserCcByEmailInNoteDbBatch() throws Exception {
- assume().that(notesMigration.readChanges()).isTrue();
- StagedChange sc = stageReviewableChange();
- addReviewer(batch(ReviewerState.CC), sc.changeId, sc.owner, "nonexistent@example.com");
- assertThat(sender)
- .sent("newchange", sc)
- .cc("nonexistent@example.com")
- .cc(sc.reviewer)
- .cc(sc.ccerByEmail, sc.reviewerByEmail)
- .noOneElse();
+ addNonUserCcByEmailInNoteDb(batch(ReviewerState.CC));
}
private interface Adder {
