Merge "Add ability to modify workInProgress to PostReview"

Documentation/rest-api-changes.txt

@@ -3603,7 +3603,7 @@ describing the related changes.
 --
 
 Sets a review on a revision, optionally also publishing draft comments, setting
-labels, and adding reviewers or CCs.
+labels, adding reviewers or CCs, and modifying the work in progress property.
 
 The review must be provided in the request body as a
 link:#review-input[ReviewInput] entity.
@@ -6713,6 +6713,12 @@ 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.
+|`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.
+|`work_in_progress`         |optional|
+If true, mark the change as work in progress. It is an error for both
+`ready` and `work_in_progress` to be true.
 |============================
 
 [[review-result]]

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -105,6 +105,7 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.PostReview;
 import com.google.gerrit.server.config.AnonymousCowardNameProvider;
 import com.google.gerrit.server.git.ChangeMessageModifier;
 import com.google.gerrit.server.git.ProjectConfig;
@@ -501,6 +502,70 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_READY);
   }
 
+  @Test
+  public void reviewAndStartReview() throws Exception {
+    PushOneCommit.Result r = createWorkInProgressChange();
+    r.assertOkStatus();
+    assertThat(r.getChange().change().isWorkInProgress()).isTrue();
+
+    ReviewInput in = ReviewInput.noScore().setWorkInProgress(false);
+    gApi.changes().id(r.getChangeId()).revision("current").review(in);
+
+    ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
+    assertThat(info.workInProgress).isNull();
+  }
+
+  @Test
+  public void reviewAndMoveToWorkInProgress() throws Exception {
+    PushOneCommit.Result r = createChange();
+    r.assertOkStatus();
+    assertThat(r.getChange().change().isWorkInProgress()).isFalse();
+
+    ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
+    gApi.changes().id(r.getChangeId()).revision("current").review(in);
+
+    ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
+    assertThat(info.workInProgress).isTrue();
+  }
+
+  @Test
+  public void reviewAndSetWorkInProgressAndAddReviewerAndVote() throws Exception {
+    PushOneCommit.Result r = createChange();
+    r.assertOkStatus();
+    assertThat(r.getChange().change().isWorkInProgress()).isFalse();
+
+    ReviewInput in =
+        ReviewInput.approve().reviewer(user.email).label("Code-Review", 1).setWorkInProgress(true);
+    gApi.changes().id(r.getChangeId()).revision("current").review(in);
+
+    ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
+    assertThat(info.workInProgress).isTrue();
+    assertThat(info.reviewers.get(REVIEWER).stream().map(ai -> ai._accountId).collect(toList()))
+        .containsExactly(admin.id.get(), user.id.get());
+    assertThat(info.labels.get("Code-Review").recommended._accountId).isEqualTo(admin.id.get());
+  }
+
+  @Test
+  public void reviewWithWorkInProgressAndReadyReturnsError() throws Exception {
+    PushOneCommit.Result r = createChange();
+    r.assertOkStatus();
+    ReviewInput in = ReviewInput.noScore();
+    in.ready = true;
+    in.workInProgress = true;
+    ReviewResult result = gApi.changes().id(r.getChangeId()).revision("current").review(in);
+    assertThat(result.error).isEqualTo(PostReview.ERROR_WIP_READY_MUTUALLY_EXCLUSIVE);
+  }
+
+  @Test
+  public void reviewWithWorkInProgressByNonOwnerReturnsError() throws Exception {
+    PushOneCommit.Result r = createChange();
+    r.assertOkStatus();
+    ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
+    setApiUser(user);
+    ReviewResult result = gApi.changes().id(r.getChangeId()).revision("current").review(in);
+    assertThat(result.error).isEqualTo(PostReview.ERROR_ONLY_OWNER_CAN_MODIFY_WORK_IN_PROGRESS);
+  }
+
   @Test
   public void getAmbiguous() throws Exception {
     PushOneCommit.Result r1 = createChange();

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java

@@ -825,6 +825,44 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
         .noOneElse();
   }
 
+  @Test
+  public void noCommentAndSetWorkInProgress() throws Exception {
+    StagedChange sc = stageReviewableChange();
+    ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
+    gApi.changes().id(sc.changeId).revision("current").review(in);
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void commentAndSetWorkInProgress() throws Exception {
+    StagedChange sc = stageReviewableChange();
+    ReviewInput in = ReviewInput.noScore().message("ok").setWorkInProgress(true);
+    gApi.changes().id(sc.changeId).revision("current").review(in);
+    assertThat(sender)
+        .sent("comment", sc)
+        .cc(sc.reviewer, sc.ccer)
+        .cc(sc.reviewerByEmail, sc.ccerByEmail)
+        .bcc(sc.starrer)
+        .bcc(ALL_COMMENTS)
+        .noOneElse();
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void commentOnWipChangeAndStartReview() throws Exception {
+    StagedChange sc = stageWipChange();
+    ReviewInput in = ReviewInput.noScore().message("ok").setWorkInProgress(false);
+    gApi.changes().id(sc.changeId).revision("current").review(in);
+    assertThat(sender)
+        .sent("comment", sc)
+        .cc(sc.reviewer, sc.ccer)
+        .cc(sc.reviewerByEmail, sc.ccerByEmail)
+        .bcc(sc.starrer)
+        .bcc(ALL_COMMENTS)
+        .noOneElse();
+    assertThat(sender).notSent();
+  }
+
   private void review(TestAccount account, String changeId, EmailStrategy strategy)
       throws Exception {
     review(account, changeId, strategy, null);

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java

@@ -74,6 +74,18 @@ public class ReviewInput {
   /** Reviewers that should be added to this change. */
   public List<AddReviewerInput> reviewers;
 
+  /**
+   * If true mark the change as work in progress. It is an error for both {@link #workInProgress}
+   * and {@link #ready} to be true.
+   */
+  public boolean workInProgress;
+
+  /**
+   * If true mark the change as ready for review. It is an error for both {@link #workInProgress}
+   * and {@link #ready} to be true.
+   */
+  public boolean ready;
+
   public enum DraftHandling {
     /** Delete pending drafts on this revision only. */
     DELETE,
@@ -141,6 +153,18 @@ public class ReviewInput {
     return this;
   }
 
+  public ReviewInput setWorkInProgress(boolean workInProgress) {
+    this.workInProgress = workInProgress;
+    ready = !workInProgress;
+    return this;
+  }
+
+  public ReviewInput setReady(boolean ready) {
+    this.ready = ready;
+    workInProgress = !ready;
+    return this;
+  }
+
   public static ReviewInput recommend() {
     return new ReviewInput().label("Code-Review", 1);
   }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewResult.java

@@ -30,4 +30,7 @@ public class ReviewResult {
    * additions were requested.
    */
   @Nullable public Map<String, AddReviewerResult> reviewers;
+
+  /** Error message for non-200 responses. */
+  @Nullable public String error;
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -132,6 +132,12 @@ import org.eclipse.jgit.lib.Config;
 @Singleton
 public class PostReview
     extends RetryingRestModifyView<RevisionResource, ReviewInput, Response<ReviewResult>> {
+  public static final String ERROR_ADDING_REVIEWER = "error adding reviewer";
+  public static final String ERROR_ONLY_OWNER_CAN_MODIFY_WORK_IN_PROGRESS =
+      "only change owner can specify work_in_progress or ready";
+  public static final String ERROR_WIP_READY_MUTUALLY_EXCLUSIVE =
+      "work_in_progress and ready are mutually exclusive";
+
   private static final Gson GSON = OutputFormat.JSON_COMPACT.newGson();
   private static final int DEFAULT_ROBOT_COMMENT_SIZE_LIMIT_IN_BYTES = 1024 * 1024;
 
@@ -150,6 +156,7 @@ public class PostReview
   private final NotesMigration migration;
   private final NotifyUtil notifyUtil;
   private final Config gerritConfig;
+  private final WorkInProgressOp.Factory workInProgressOpFactory;
 
   @Inject
   PostReview(
@@ -168,7 +175,8 @@ public class PostReview
       PostReviewers postReviewers,
       NotesMigration migration,
       NotifyUtil notifyUtil,
-      @GerritServerConfig Config gerritConfig) {
+      @GerritServerConfig Config gerritConfig,
+      WorkInProgressOp.Factory workInProgressOpFactory) {
     super(retryHelper);
     this.db = db;
     this.changes = changes;
@@ -185,6 +193,7 @@ public class PostReview
     this.migration = migration;
     this.notifyUtil = notifyUtil;
     this.gerritConfig = gerritConfig;
+    this.workInProgressOpFactory = workInProgressOpFactory;
   }
 
   @Override
@@ -259,6 +268,7 @@ public class PostReview
     ReviewResult output = new ReviewResult();
     output.reviewers = reviewerJsonResults;
     if (hasError || confirm) {
+      output.error = ERROR_ADDING_REVIEWER;
       return Response.withStatusCode(SC_BAD_REQUEST, output);
     }
     output.labels = input.labels;
@@ -310,9 +320,30 @@ public class PostReview
         bu.addOp(revision.getChange().getId(), selfAddition.op);
       }
 
+      // Add WorkInProgressOp if requested.
+      if (input.ready || input.workInProgress) {
+        if (input.ready && input.workInProgress) {
+          output.error = ERROR_WIP_READY_MUTUALLY_EXCLUSIVE;
+          return Response.withStatusCode(SC_BAD_REQUEST, output);
+        }
+        if (!revision.getChange().getOwner().equals(revision.getUser().getAccountId())) {
+          output.error = ERROR_ONLY_OWNER_CAN_MODIFY_WORK_IN_PROGRESS;
+          return Response.withStatusCode(SC_BAD_REQUEST, output);
+        }
+        // Suppress notifications in WorkInProgressOp, we'll take care of
+        // them in this endpoint.
+        WorkInProgressOp.Input wipIn = new WorkInProgressOp.Input();
+        wipIn.notify = NotifyHandling.NONE;
+        bu.addOp(
+            revision.getChange().getId(),
+            workInProgressOpFactory.create(input.workInProgress, wipIn));
+      }
+
+      // Add the review op.
       bu.addOp(
           revision.getChange().getId(),
           new Op(revision.getPatchSet().getId(), input, accountsToNotify));
+
       bu.execute();
 
       for (PostReviewers.Addition reviewerResult : reviewerResults) {
@@ -326,12 +357,20 @@ public class PostReview
   }
 
   private NotifyHandling defaultNotify(Change c, ReviewInput in) {
-    if (ChangeMessagesUtil.isAutogenerated(in.tag)) {
-      // Autogenerated comments default to lower notify levels.
-      return c.isWorkInProgress() ? NotifyHandling.OWNER : NotifyHandling.OWNER_REVIEWERS;
+    boolean workInProgress = c.isWorkInProgress();
+    if (in.workInProgress) {
+      workInProgress = true;
+    }
+    if (in.ready) {
+      workInProgress = false;
     }
 
-    if (c.isWorkInProgress() && !c.hasReviewStarted()) {
+    if (ChangeMessagesUtil.isAutogenerated(in.tag)) {
+      // Autogenerated comments default to lower notify levels.
+      return workInProgress ? NotifyHandling.OWNER : NotifyHandling.OWNER_REVIEWERS;
+    }
+
+    if (workInProgress && !c.hasReviewStarted()) {
       // If review hasn't started we want to minimize recipients, no matter who
       // the author is.
       return NotifyHandling.OWNER;

gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java

@@ -14,9 +14,11 @@
 
 package com.google.gerrit.server.change;
 
+import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -35,7 +37,9 @@ import com.google.inject.assistedinject.Assisted;
 /* Set work in progress or ready for review state on a change */
 public class WorkInProgressOp implements BatchUpdateOp {
   public static class Input {
-    String message;
+    @Nullable String message;
+
+    @Nullable NotifyHandling notify;
 
     public Input() {}
 
@@ -53,6 +57,7 @@ public class WorkInProgressOp implements BatchUpdateOp {
   private final PatchSetUtil psUtil;
   private final boolean workInProgress;
   private final Input in;
+  private final NotifyHandling notify;
 
   private Change change;
   private ChangeNotes notes;
@@ -71,6 +76,9 @@ public class WorkInProgressOp implements BatchUpdateOp {
     this.psUtil = psUtil;
     this.workInProgress = workInProgress;
     this.in = in;
+    notify =
+        MoreObjects.firstNonNull(
+            in.notify, workInProgress ? NotifyHandling.NONE : NotifyHandling.ALL);
   }
 
   @Override
@@ -113,12 +121,12 @@ public class WorkInProgressOp implements BatchUpdateOp {
 
   @Override
   public void postUpdate(Context ctx) {
-    if (workInProgress) {
+    if (workInProgress || notify.ordinal() < NotifyHandling.OWNER_REVIEWERS.ordinal()) {
       return;
     }
     email
         .create(
-            NotifyHandling.ALL,
+            notify,
             ImmutableListMultimap.of(),
             notes,
             ps,