diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 0f3be1329b..89012433f8 100644 --- a/Documentation/rest-api-changes.txt +++ b/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]] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index edffdd48cc..425f4199ba 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/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(); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java index 5e0caf205f..f4c12b305c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java +++ b/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); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java index 6b234199b5..2c945be86e 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java +++ b/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 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); } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewResult.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewResult.java index d7729246c3..761f26063d 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewResult.java +++ b/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 reviewers; + + /** Error message for non-200 responses. */ + @Nullable public String error; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index b11db15d75..147654d40b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/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> { + 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; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java index 5c27936393..b5a8220f50 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java +++ b/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,