Downgrade default notify for new patch sets in WIP

Previously, notify would default to ALL unless explicitly set via
magicbranch push option. Now, the default is downgraded to OWNER when
the following conditions are met:

    1. The --wip push option is set
    2. The change is WIP pre-push and --ready isn't set

Change-Id: Ied563ae0126f26837e9365f4650967fda591dbc7

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java

@@ -294,14 +294,14 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest {
       return username + "@example.com";
     }
 
-    TestAccount testAccount(String name) throws Exception {
+    public TestAccount testAccount(String name) throws Exception {
       String username = name(name);
       TestAccount account = accountCreator.create(username, email(username), name);
       accountsByEmail.put(account.email, account);
       return account;
     }
 
-    TestAccount testAccount(String name, String groupName) throws Exception {
+    public TestAccount testAccount(String name, String groupName) throws Exception {
       String username = name(name);
       TestAccount account = accountCreator.create(username, email(username), name, groupName);
       accountsByEmail.put(account.email, account);

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

@@ -35,11 +35,7 @@ public class CreateChangeSenderIT extends AbstractNotificationTest {
   @Test
   public void createWipChange() throws Exception {
     StagedPreChange spc = stagePreChange("refs/for/master%wip", NEW_CHANGES, NEW_PATCHSETS);
-    assertThat(sender)
+    assertThat(sender).notSent();
-        .sent("newchange", spc)
-        .notTo(spc.owner)
-        .to(spc.watchingProjectOwner)
-        .bcc(NEW_CHANGES, NEW_PATCHSETS);
   }
 
   @Test

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

@@ -71,7 +71,7 @@ public class ReplacePatchSetSenderIT extends AbstractNotificationTest {
     pushTo(sc, "refs/for/master", other);
     assertThat(sender)
         .sent("newpatchset", sc)
-        .notTo(sc.owner)
+        .notTo(sc.owner) // TODO(logan): This email shouldn't come from the owner.
         .to(sc.reviewer, other)
         .to(sc.reviewerByEmail)
         .cc(sc.ccer)
@@ -87,7 +87,7 @@ public class ReplacePatchSetSenderIT extends AbstractNotificationTest {
     pushTo(sc, "refs/for/master", other);
     assertThat(sender)
         .sent("newpatchset", sc)
-        .notTo(sc.owner)
+        .notTo(sc.owner) // TODO(logan): This email shouldn't come from the owner.
         .to(sc.reviewer, sc.ccer, other)
         .notTo(sc.reviewerByEmail, sc.ccerByEmail)
         .bcc(sc.starrer)
@@ -129,6 +129,7 @@ public class ReplacePatchSetSenderIT extends AbstractNotificationTest {
     pushTo(sc, "refs/for/master%notify=OWNER_REVIEWERS", other);
     assertThat(sender)
         .sent("newpatchset", sc)
+        // TODO(logan): This email shouldn't come from the owner.
         .notTo(sc.owner, sc.starrer, other)
         .to(sc.reviewer)
         .to(sc.reviewerByEmail)
@@ -147,7 +148,7 @@ public class ReplacePatchSetSenderIT extends AbstractNotificationTest {
         .sent("newpatchset", sc)
         .to(sc.reviewer, sc.ccer)
         .notTo(sc.starrer, other)
-        .notTo(sc.owner) // TODO(logan): Why?
+        .notTo(sc.owner) // TODO(logan): This email shouldn't come from the owner.
         .notTo(sc.reviewerByEmail, sc.ccerByEmail)
         .notTo(NEW_PATCHSETS);
   }
@@ -183,67 +184,27 @@ public class ReplacePatchSetSenderIT extends AbstractNotificationTest {
   }
 
   @Test
-  public void newPatchSetByOtherOnReviewableChangeNotifyOwnerInNoteDb() throws Exception {
+  public void newPatchSetByOtherOnReviewableChangeNotifyOwner() throws Exception {
-    assume().that(notesMigration.readChanges()).isTrue();
     StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
     pushTo(sc, "refs/for/master%notify=OWNER", other);
-    assertThat(sender)
+    assertThat(sender).notSent();
-        .sent("newpatchset", sc)
-        .notTo(sc.owner, sc.starrer, other)
-        .to(sc.reviewer) // TODO(logan): Why?
-        .cc(sc.ccer) // TODO(logan): Why?
-        .notTo(sc.reviewerByEmail, sc.ccerByEmail)
-        .notTo(NEW_PATCHSETS);
   }
 
   @Test
-  public void newPatchSetByOtherOnReviewableChangeNotifyOwnerInReviewDb() throws Exception {
+  public void newPatchSetByOtherOnReviewableChangeOwnerSelfCcNotifyOwner() throws Exception {
-    assume().that(notesMigration.readChanges()).isFalse();
-    StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
-    pushTo(sc, "refs/for/master%notify=OWNER", other);
-    assertThat(sender)
-        .sent("newpatchset", sc)
-        .to(sc.reviewer, sc.ccer) // TODO(logan): Why?
-        .notTo(sc.owner, sc.starrer, other)
-        .notTo(sc.reviewerByEmail, sc.ccerByEmail)
-        .notTo(NEW_PATCHSETS);
-  }
-
-  @Test
-  public void newPatchSetByOtherOnReviewableChangeOwnerSelfCcNotifyOwnerInNoteDb()
-      throws Exception {
-    assume().that(notesMigration.readChanges()).isTrue();
     StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
     pushTo(sc, "refs/for/master%notify=OWNER", other, EmailStrategy.CC_ON_OWN_COMMENTS);
-    assertThat(sender)
+    // TODO(logan): This email shouldn't come from the owner, and that's why
-        .sent("newpatchset", sc)
+    // no email is currently sent (owner isn't CCing self).
-        .to(sc.owner)
+    assertThat(sender).notSent();
-        .to(sc.reviewer) // TODO(logan): Why?
-        .cc(sc.ccer) // TODO(logan): Why?
-        .notTo(sc.starrer, other)
-        .notTo(sc.reviewerByEmail, sc.ccerByEmail)
-        .notTo(NEW_PATCHSETS);
-  }
-
-  @Test
-  public void newPatchSetByOtherOnReviewableChangeOwnerSelfCcNotifyOwnerInReviewDb()
-      throws Exception {
-    assume().that(notesMigration.readChanges()).isFalse();
-    StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
-    pushTo(sc, "refs/for/master%notify=OWNER", other, EmailStrategy.CC_ON_OWN_COMMENTS);
-    assertThat(sender)
-        .sent("newpatchset", sc)
-        .to(sc.owner)
-        .to(sc.reviewer, sc.ccer) // TODO(logan): Why?
-        .notTo(sc.starrer, other)
-        .notTo(sc.reviewerByEmail, sc.ccerByEmail)
-        .notTo(NEW_PATCHSETS);
   }
 
   @Test
   public void newPatchSetByOtherOnReviewableChangeNotifyNone() throws Exception {
     StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
     pushTo(sc, "refs/for/master%notify=NONE", other);
+    // TODO(logan): This email shouldn't come from the owner, and that's why
+    // no email is currently sent (owner isn't CCing self).
     assertThat(sender).notSent();
   }
 
@@ -255,67 +216,17 @@ public class ReplacePatchSetSenderIT extends AbstractNotificationTest {
   }
 
   @Test
-  public void newPatchSetByOwnerOnReviewableChangeToWipInNoteDb() throws Exception {
+  public void newPatchSetByOwnerOnReviewableChangeToWip() throws Exception {
-    assume().that(notesMigration.readChanges()).isTrue();
     StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
     pushTo(sc, "refs/for/master%wip", sc.owner);
-    // TODO(logan): This shouldn't notify.
+    assertThat(sender).notSent();
-    assertThat(sender)
-        .sent("newpatchset", sc)
-        .notTo(sc.owner)
-        .to(sc.reviewer)
-        .to(sc.reviewerByEmail)
-        .cc(sc.ccer)
-        .cc(sc.ccerByEmail)
-        .bcc(sc.starrer)
-        .bcc(NEW_PATCHSETS);
   }
 
   @Test
-  public void newPatchSetByOwnerOnReviewableChangeToWipInReviewDb() throws Exception {
+  public void newPatchSetOnWipChange() throws Exception {
-    assume().that(notesMigration.readChanges()).isFalse();
-    StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
-    pushTo(sc, "refs/for/master%wip", sc.owner);
-    // TODO(logan): This shouldn't notify.
-    assertThat(sender)
-        .sent("newpatchset", sc)
-        .notTo(sc.owner)
-        .notTo(sc.reviewerByEmail, sc.ccerByEmail)
-        .to(sc.reviewer, sc.ccer)
-        .bcc(sc.starrer)
-        .bcc(NEW_PATCHSETS);
-  }
-
-  @Test
-  public void newPatchSetOnWipChangeInNoteDb() throws Exception {
-    assume().that(notesMigration.readChanges()).isTrue();
     StagedChange sc = stageWipChange(NEW_PATCHSETS);
     pushTo(sc, "refs/for/master%wip", sc.owner);
-    // TODO(logan): This shouldn't notify.
+    assertThat(sender).notSent();
-    assertThat(sender)
-        .sent("newpatchset", sc)
-        .notTo(sc.owner)
-        .to(sc.reviewer)
-        .to(sc.reviewerByEmail)
-        .cc(sc.ccer)
-        .cc(sc.ccerByEmail)
-        .bcc(sc.starrer)
-        .bcc(NEW_PATCHSETS);
-  }
-
-  @Test
-  public void newPatchSetOnWipChangeInReviewDb() throws Exception {
-    assume().that(notesMigration.readChanges()).isFalse();
-    StagedChange sc = stageWipChange(NEW_PATCHSETS);
-    pushTo(sc, "refs/for/master%wip", sc.owner);
-    // TODO(logan): This shouldn't notify.
-    assertThat(sender)
-        .sent("newpatchset", sc)
-        .notTo(sc.owner)
-        .notTo(sc.reviewerByEmail, sc.ccerByEmail)
-        .to(sc.reviewer, sc.ccer)
-        .bcc(sc.starrer)
-        .bcc(NEW_PATCHSETS);
   }
 
   @Test
@@ -379,35 +290,121 @@ public class ReplacePatchSetSenderIT extends AbstractNotificationTest {
   }
 
   @Test
-  public void newPatchSetOnReviewableWipChangeInNoteDb() throws Exception {
+  public void newPatchSetOnReviewableWipChange() throws Exception {
-    assume().that(notesMigration.readChanges()).isTrue();
     StagedChange sc = stageReviewableWipChange(NEW_PATCHSETS);
     pushTo(sc, "refs/for/master%wip", sc.owner);
-    // TODO(logan): This shouldn't notify.
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void newPatchSetOnReviewableChangeAddingReviewerInNoteDb() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
+    TestAccount newReviewer = sc.testAccount("newReviewer");
+    pushTo(sc, "refs/for/master%r=" + newReviewer.username, sc.owner);
+    assertThat(sender)
+        .sent("newpatchset", sc)
+        .notTo(sc.owner)
+        .to(sc.reviewer, newReviewer)
+        .cc(sc.ccer)
+        .to(sc.reviewerByEmail)
+        .cc(sc.ccerByEmail)
+        .bcc(sc.starrer)
+        .bcc(NEW_PATCHSETS);
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void newPatchSetOnReviewableChangeAddingReviewerInReviewDb() throws Exception {
+    assume().that(notesMigration.readChanges()).isFalse();
+    StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
+    TestAccount newReviewer = sc.testAccount("newReviewer");
+    pushTo(sc, "refs/for/master%r=" + newReviewer.username, sc.owner);
+    assertThat(sender)
+        .sent("newpatchset", sc)
+        .notTo(sc.owner)
+        .to(sc.reviewer, sc.ccer, newReviewer)
+        .to(sc.reviewerByEmail)
+        .cc(sc.ccerByEmail)
+        .bcc(sc.starrer)
+        .bcc(NEW_PATCHSETS);
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void newPatchSetOnWipChangeAddingReviewer() throws Exception {
+    StagedChange sc = stageWipChange(NEW_PATCHSETS);
+    TestAccount newReviewer = sc.testAccount("newReviewer");
+    pushTo(sc, "refs/for/master%r=" + newReviewer.username, sc.owner);
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void newPatchSetOnWipChangeAddingReviewerNotifyAllInNoteDb() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    StagedChange sc = stageWipChange(NEW_PATCHSETS);
+    TestAccount newReviewer = sc.testAccount("newReviewer");
+    pushTo(sc, "refs/for/master%notify=ALL,r=" + newReviewer.username, sc.owner);
+    assertThat(sender)
+        .sent("newpatchset", sc)
+        .notTo(sc.owner)
+        .to(sc.reviewer, newReviewer)
+        .cc(sc.ccer)
+        .to(sc.reviewerByEmail)
+        .cc(sc.ccerByEmail)
+        .bcc(sc.starrer)
+        .bcc(NEW_PATCHSETS);
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void newPatchSetOnWipChangeAddingReviewerNotifyAllInReviewDb() throws Exception {
+    assume().that(notesMigration.readChanges()).isFalse();
+    StagedChange sc = stageWipChange(NEW_PATCHSETS);
+    TestAccount newReviewer = sc.testAccount("newReviewer");
+    pushTo(sc, "refs/for/master%notify=ALL,r=" + newReviewer.username, sc.owner);
+    assertThat(sender)
+        .sent("newpatchset", sc)
+        .notTo(sc.owner)
+        .to(sc.reviewer, sc.ccer, newReviewer)
+        .to(sc.reviewerByEmail)
+        .cc(sc.ccerByEmail)
+        .bcc(sc.starrer)
+        .bcc(NEW_PATCHSETS);
+    assertThat(sender).notSent();
+  }
+
+  @Test
+  public void newPatchSetOnWipChangeSettingReadyInNoteDb() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    StagedChange sc = stageWipChange(NEW_PATCHSETS);
+    pushTo(sc, "refs/for/master%ready", sc.owner);
     assertThat(sender)
         .sent("newpatchset", sc)
         .notTo(sc.owner)
         .to(sc.reviewer)
-        .to(sc.reviewerByEmail)
         .cc(sc.ccer)
+        .to(sc.reviewerByEmail)
         .cc(sc.ccerByEmail)
         .bcc(sc.starrer)
         .bcc(NEW_PATCHSETS);
+    assertThat(sender).notSent();
   }
 
   @Test
-  public void newPatchSetOnReviewableWipChangeInReviewDb() throws Exception {
+  public void newPatchSetOnWipChangeSettingReadyInReviewDb() throws Exception {
     assume().that(notesMigration.readChanges()).isFalse();
     StagedChange sc = stageWipChange(NEW_PATCHSETS);
-    pushTo(sc, "refs/for/master%wip", sc.owner);
+    pushTo(sc, "refs/for/master%ready", sc.owner);
-    // TODO(logan): This shouldn't notify.
     assertThat(sender)
         .sent("newpatchset", sc)
         .notTo(sc.owner)
-        .notTo(sc.reviewerByEmail, sc.ccerByEmail)
         .to(sc.reviewer, sc.ccer)
+        .to(sc.reviewerByEmail)
+        .cc(sc.ccerByEmail)
         .bcc(sc.starrer)
         .bcc(NEW_PATCHSETS);
+    assertThat(sender).notSent();
   }
 
   private void pushTo(StagedChange sc, String ref, TestAccount by) throws Exception {

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -1273,7 +1273,7 @@ public class ReceiveCommits {
               + "should be sent. Allowed values are NONE, OWNER, "
               + "OWNER_REVIEWERS, ALL. If not set, the default is ALL."
     )
-    NotifyHandling notify = NotifyHandling.ALL;
+    private NotifyHandling notify;
 
     @Option(name = "--notify-to", metaVar = "USER", usage = "user that should be notified")
     List<Account.Id> tos = new ArrayList<>();
@@ -1430,6 +1430,26 @@ public class ReceiveCommits {
       }
       return ref.substring(0, split);
     }
+
+    public NotifyHandling getNotify() {
+      if (notify != null) {
+        return notify;
+      }
+      if (workInProgress) {
+        return NotifyHandling.OWNER;
+      }
+      return NotifyHandling.ALL;
+    }
+
+    public NotifyHandling getNotify(ChangeNotes notes) {
+      if (notify != null) {
+        return notify;
+      }
+      if (workInProgress || (!ready && notes.getChange().isWorkInProgress())) {
+        return NotifyHandling.OWNER;
+      }
+      return NotifyHandling.ALL;
+    }
   }
 
   /**
@@ -2197,7 +2217,7 @@ public class ReceiveCommits {
                 .setExtraCC(recipients.getCcOnly())
                 .setApprovals(approvals)
                 .setMessage(msg.toString())
-                .setNotify(magicBranch.notify)
+                .setNotify(magicBranch.getNotify())
                 .setAccountsToNotify(magicBranch.getAccountsToNotify())
                 .setRequestScopePropagator(requestScopePropagator)
                 .setSendMail(true)

gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java

@@ -443,8 +443,7 @@ public class ReplaceOp implements BatchUpdateOp {
       }
     }
 
-    NotifyHandling notify =
+    NotifyHandling notify = magicBranch != null ? magicBranch.getNotify(notes) : NotifyHandling.ALL;
-        magicBranch != null && magicBranch.notify != null ? magicBranch.notify : NotifyHandling.ALL;
 
     if (shouldPublishComments()) {
       emailCommentsFactory
@@ -489,7 +488,7 @@ public class ReplaceOp implements BatchUpdateOp {
         cm.setPatchSet(newPatchSet, info);
         cm.setChangeMessage(msg.getMessage(), ctx.getWhen());
         if (magicBranch != null) {
-          cm.setNotify(magicBranch.notify);
+          cm.setNotify(magicBranch.getNotify(notes));
           cm.setAccountsToNotify(magicBranch.getAccountsToNotify());
         }
         cm.addReviewers(recipients.getReviewers());

gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.mail.send;
 
 import com.google.gerrit.common.errors.EmailException;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.extensions.api.changes.RecipientType;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -62,8 +63,10 @@ public class ReplacePatchSetSender extends ReplyToChangeSender {
       //
       reviewers.remove(fromId);
     }
-    add(RecipientType.TO, reviewers);
+    if (notify == NotifyHandling.ALL || notify == NotifyHandling.OWNER_REVIEWERS) {
-    add(RecipientType.CC, extraCC);
+      add(RecipientType.TO, reviewers);
+      add(RecipientType.CC, extraCC);
+    }
     rcptToAuthors(RecipientType.CC);
     bccStarredBy();
     includeWatchers(