Silence new reviewer notifications on WIP changes

When PostReviewers is used to add a single reviewer to a WIP change,
NotifyHandling will now default to NONE.

When PostReview is used to add a batch of reviewers, the notify property
of each of the nested AddReviewerInputs is ignored and only the
top-level ReviewInput's notify applies. For WIP changes, this top-level
property's default value is also NONE.

Change-Id: Ia2af9c1377e0aff1e5ce8646bc89d687dcab810e

Documentation/rest-api-changes.txt

@@ -2988,6 +2988,17 @@ Reviewers without Gerrit accounts can only be added on changes visible to anonym
   }
 ----
 
+.Notifications
+
+An email will be sent using the "newchange" template.
+
+[options="header",cols="1,1,1"]
+|=============================
+|WIP State       |Default|notify=ALL
+|Ready for review|owner, reviewers, CCs|owner, reviewers, CCs
+|Work in progress|not sent|owner, reviewers, CCs
+|=============================
+
 [[delete-reviewer]]
 === Delete Reviewer
 --
@@ -3530,7 +3541,8 @@ describing the related changes.
 'POST /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/review'
 --
 
-Sets a review on a revision.
+Sets a review on a revision, optionally also publishing draft comments, setting
+labels, and adding reviewers or CCs.
 
 The review must be provided in the request body as a
 link:#review-input[ReviewInput] entity.
@@ -3538,7 +3550,7 @@ link:#review-input[ReviewInput] entity.
 A review cannot be set on a change edit. Trying to post a review for a
 change edit fails with `409 Conflict`.
 
-This API method can be used to set labels:
+Here is an example of using this method to set labels:
 
 .Request
 ----
@@ -3594,7 +3606,7 @@ if you set a label but weren't previously a reviewer on this CL).
 ----
 
 It is also possible to add one or more reviewers or CCs
-to a change simultaneously with a review.
+to a change simultaneously with a review:
 
 .Request
 ----
@@ -3623,6 +3635,9 @@ link:#reviewer-input[ReviewerInput]. The corresponding result of
 adding each reviewer will be returned in a map of inputs to
 link:#add-reviewer-result[AddReviewerResult]s.
 
+NOTE: The notify property of each ReviewerInput is *ignored*. Use the notify
+property of the top-level link:#review-input[ReviewInput] instead.
+
 .Response
 ----
   HTTP/1.1 200 OK

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

@@ -1261,6 +1261,40 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
   }
 
+  @Test
+  public void notificationsForAddedWorkInProgressReviewers() throws Exception {
+    AddReviewerInput in = new AddReviewerInput();
+    in.reviewer = user.email;
+    ReviewInput batchIn = new ReviewInput();
+    batchIn.reviewers = ImmutableList.of(in);
+
+    // Added reviewers not notified by default.
+    PushOneCommit.Result r = createWorkInProgressChange();
+    gApi.changes().id(r.getChangeId()).addReviewer(in);
+    assertThat(sender.getMessages()).hasSize(0);
+
+    // Default notification handling can be overridden.
+    r = createWorkInProgressChange();
+    in.notify = NotifyHandling.OWNER_REVIEWERS;
+    gApi.changes().id(r.getChangeId()).addReviewer(in);
+    assertThat(sender.getMessages()).hasSize(1);
+    sender.clear();
+
+    // Reviewers added via PostReview also not notified by default.
+    // In this case, the child ReviewerInput has a notify=OWNER_REVIEWERS
+    // that should be ignored.
+    r = createWorkInProgressChange();
+    gApi.changes().id(r.getChangeId()).revision("current").review(batchIn);
+    assertThat(sender.getMessages()).hasSize(0);
+
+    // Top-level notify property can force notifications when adding reviewer
+    // via PostReview.
+    r = createWorkInProgressChange();
+    batchIn.notify = NotifyHandling.OWNER_REVIEWERS;
+    gApi.changes().id(r.getChangeId()).revision("current").review(batchIn);
+    assertThat(sender.getMessages()).hasSize(1);
+  }
+
   @Test
   public void addReviewerWithNoteDbWhenDummyApprovalInReviewDbExists() throws Exception {
     assume().that(notesMigration.enabled()).isTrue();

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

@@ -54,7 +54,7 @@ public class ReviewInput {
   public DraftHandling drafts;
 
   /** Who to send email notifications to after review is stored. */
-  public NotifyHandling notify = NotifyHandling.ALL;
+  public NotifyHandling notify;
 
   public Map<RecipientType, NotifyInfo> notifyDetails;
 

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

@@ -226,8 +226,8 @@ public class PostReview
     }
 
     if (input.notify == null) {
-      log.warn("notify = null; assuming notify = NONE");
-      input.notify = NotifyHandling.NONE;
+      input.notify =
+          revision.getChange().isWorkInProgress() ? NotifyHandling.OWNER : NotifyHandling.ALL;
     }
 
     ListMultimap<RecipientType, Account.Id> accountsToNotify =

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

@@ -238,7 +238,10 @@ public class PostReviewersOp implements BatchUpdateOp {
 
     try {
       AddReviewerSender cm = addReviewerSenderFactory.create(change.getProject(), change.getId());
-      cm.setNotify(MoreObjects.firstNonNull(notify, NotifyHandling.ALL));
+      // Default to silent operation on WIP changes.
+      NotifyHandling defaultNotifyHandling =
+          change.isWorkInProgress() ? NotifyHandling.NONE : NotifyHandling.ALL;
+      cm.setNotify(MoreObjects.firstNonNull(notify, defaultNotifyHandling));
       cm.setAccountsToNotify(accountsToNotify);
       cm.setFrom(userId);
       cm.addReviewers(toMail);