Reuse ReviewerAdder from ReplaceOp

Change-Id: I1034505eb7615dbba599424bd79066bcbf9b7737

java/com/google/gerrit/server/git/receive/ReplaceOp.java

@@ -14,7 +14,9 @@
 
 package com.google.gerrit.server.git.receive;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
+import static com.google.gerrit.server.change.ReviewerAdder.newAddReviewerInputFromCommitIdentity;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
@@ -23,12 +25,17 @@ import static org.eclipse.jgit.lib.Constants.R_HEADS;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.Streams;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.extensions.api.changes.AddReviewerResult;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.extensions.client.ChangeKind;
+import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -45,6 +52,10 @@ import com.google.gerrit.server.PublishCommentUtil;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeKindCache;
 import com.google.gerrit.server.change.EmailReviewComments;
+import com.google.gerrit.server.change.ReviewerAdder;
+import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
 import com.google.gerrit.server.config.SendEmailExecutor;
 import com.google.gerrit.server.extensions.events.CommentAdded;
 import com.google.gerrit.server.extensions.events.RevisionCreated;
@@ -75,6 +86,7 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -117,6 +129,7 @@ public class ReplaceOp implements BatchUpdateOp {
   private final PatchSetUtil psUtil;
   private final ReplacePatchSetSender.Factory replacePatchSetFactory;
   private final ProjectCache projectCache;
+  private final ReviewerAdder reviewerAdder;
 
   private final ProjectState projectState;
   private final Branch.NameKey dest;
@@ -142,6 +155,7 @@ public class ReplaceOp implements BatchUpdateOp {
   private String rejectMessage;
   private MergedByPushOp mergedByPushOp;
   private RequestScopePropagator requestScopePropagator;
+  private ReviewerAdditionList reviewerAdditions;
 
   @Inject
   ReplaceOp(
@@ -161,6 +175,7 @@ public class ReplaceOp implements BatchUpdateOp {
       ReplacePatchSetSender.Factory replacePatchSetFactory,
       ProjectCache projectCache,
       @SendEmailExecutor ExecutorService sendEmailExecutor,
+      ReviewerAdder reviewerAdder,
       @Assisted ProjectState projectState,
       @Assisted Branch.NameKey dest,
       @Assisted boolean checkMergedInto,
@@ -188,6 +203,7 @@ public class ReplaceOp implements BatchUpdateOp {
     this.replacePatchSetFactory = replacePatchSetFactory;
     this.projectCache = projectCache;
     this.sendEmailExecutor = sendEmailExecutor;
+    this.reviewerAdder = reviewerAdder;
 
     this.projectState = projectState;
     this.dest = dest;
@@ -228,7 +244,8 @@ public class ReplaceOp implements BatchUpdateOp {
 
   @Override
   public boolean updateChange(ChangeContext ctx)
-      throws RestApiException, OrmException, IOException, PermissionBackendException {
+      throws RestApiException, OrmException, IOException, PermissionBackendException,
+          ConfigInvalidException {
     notes = ctx.getNotes();
     Change change = notes.getChange();
     if (change == null || change.getStatus().isClosed()) {
@@ -294,7 +311,8 @@ public class ReplaceOp implements BatchUpdateOp {
             psDescription);
 
     update.setPsDescription(psDescription);
-    recipients.add(getRecipientsFromFooters(accountResolver, commit.getFooterLines()));
+    MailRecipients fromFooters = getRecipientsFromFooters(accountResolver, commit.getFooterLines());
+    recipients.add(fromFooters);
     recipients.remove(ctx.getAccountId());
     ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx.getNotes());
     MailRecipients oldRecipients = getRecipientsFromReviewers(cd.reviewers());
@@ -313,15 +331,21 @@ public class ReplaceOp implements BatchUpdateOp {
         ctx.getRevWalk(),
         ctx.getRepoView().getConfig(),
         newApprovals);
-    approvalsUtil.addReviewers(
-        ctx.getDb(),
-        update,
-        projectState.getLabelTypes(),
-        change,
-        newPatchSet,
-        info,
-        recipients.getReviewers(),
-        oldRecipients.getAll());
+
+    reviewerAdditions =
+        reviewerAdder.prepare(
+            ctx.getNotes(),
+            ctx.getUser(),
+            getReviewerInputs(recipients, ctx.getChange(), info),
+            true);
+    Optional<ReviewerAddition> reviewerError = reviewerAdditions.getFailures().stream().findFirst();
+    if (reviewerError.isPresent()) {
+      // TODO(dborowitz): How best to report this? Erroring out would match existing
+      // ReceiveCommits behavior, but it may be worth rethinking.
+      AddReviewerResult result = reviewerError.get().result;
+      throw new BadRequestException("Failed to add reviewer " + result.input + ": " + result.error);
+    }
+    reviewerAdditions.updateChange(ctx, newPatchSet);
 
     // Check if approvals are changing in with this update. If so, add current user to reviewers.
     // Note that this is done separately as addReviewers is filtering out the change owner as
@@ -344,6 +368,44 @@ public class ReplaceOp implements BatchUpdateOp {
     return true;
   }
 
+  private static ImmutableList<InternalAddReviewerInput> getReviewerInputs(
+      MailRecipients recipients, Change change, PatchSetInfo psInfo) {
+    // Disable individual emails when adding reviewers, as all reviewers will receive the single
+    // bulk new change email.
+    return Streams.concat(
+            recipients
+                .getReviewers()
+                .stream()
+                .distinct()
+                .map(id -> newAddReviewerInput(id, ReviewerState.REVIEWER)),
+            recipients
+                .getCcOnly()
+                .stream()
+                .distinct()
+                .map(id -> newAddReviewerInput(id, ReviewerState.CC)),
+            Streams.stream(
+                newAddReviewerInputFromCommitIdentity(
+                    change, psInfo.getAuthor().getAccount(), NotifyHandling.NONE)),
+            Streams.stream(
+                newAddReviewerInputFromCommitIdentity(
+                    change, psInfo.getCommitter().getAccount(), NotifyHandling.NONE)))
+        .collect(toImmutableList());
+  }
+
+  private static InternalAddReviewerInput newAddReviewerInput(
+      Account.Id accountId, ReviewerState state) {
+    // Disable individual emails when adding reviewers, as all reviewers will receive the single
+    // bulk new patch set email.
+    InternalAddReviewerInput input =
+        ReviewerAdder.newAddReviewerInput(accountId, state, NotifyHandling.NONE);
+
+    // Ignore failures for reasons like the reviewer being inactive or being unable to see the
+    // change. See discussion in ChangeInserter.
+    input.otherFailureBehavior = ReviewerAdder.FailureBehavior.IGNORE;
+
+    return input;
+  }
+
   private ChangeMessage createChangeMessage(ChangeContext ctx, String reviewMessage)
       throws OrmException, IOException {
     String approvalMessage =
@@ -455,6 +517,7 @@ public class ReplaceOp implements BatchUpdateOp {
 
   @Override
   public void postUpdate(Context ctx) throws Exception {
+    reviewerAdditions.postUpdate(ctx);
     if (changeKind != ChangeKind.TRIVIAL_REBASE) {
       // TODO(dborowitz): Merge email templates so we only have to send one.
       Runnable e = new ReplaceEmailTask(ctx);

javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -2259,6 +2259,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
       String name = "reviewers for " + (i + 1);
       if (expectedReviewer != null) {
         assertThat(cd.reviewers().all()).named(name).containsExactly(expectedReviewer.getId());
+        // Remove reviewer from PS1 so we can test adding this same reviewer on PS2 below.
         gApi.changes().id(cd.getId().get()).reviewer(expectedReviewer.getId().toString()).remove();
       }
       assertThat(byCommit(c).reviewers().all()).named(name).isEmpty();