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
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index cf71f79..416d859 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
@@ -294,14 +294,14 @@
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);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CreateChangeSenderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CreateChangeSenderIT.java
index 3f151de..36dcb5d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CreateChangeSenderIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/CreateChangeSenderIT.java
@@ -35,11 +35,7 @@
@Test
public void createWipChange() throws Exception {
StagedPreChange spc = stagePreChange("refs/for/master%wip", NEW_CHANGES, NEW_PATCHSETS);
- assertThat(sender)
- .sent("newchange", spc)
- .notTo(spc.owner)
- .to(spc.watchingProjectOwner)
- .bcc(NEW_CHANGES, NEW_PATCHSETS);
+ assertThat(sender).notSent();
}
@Test
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ReplacePatchSetSenderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ReplacePatchSetSenderIT.java
index 8d9a069..8589527 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ReplacePatchSetSenderIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/mail/ReplacePatchSetSenderIT.java
@@ -71,7 +71,7 @@
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 @@
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 @@
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 @@
.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 @@
}
@Test
- public void newPatchSetByOtherOnReviewableChangeNotifyOwnerInNoteDb() throws Exception {
- assume().that(notesMigration.readChanges()).isTrue();
+ public void newPatchSetByOtherOnReviewableChangeNotifyOwner() throws Exception {
StagedChange sc = stageReviewableChange(NEW_PATCHSETS);
pushTo(sc, "refs/for/master%notify=OWNER", other);
- assertThat(sender)
- .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);
+ assertThat(sender).notSent();
}
@Test
- public void newPatchSetByOtherOnReviewableChangeNotifyOwnerInReviewDb() 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();
+ public void newPatchSetByOtherOnReviewableChangeOwnerSelfCcNotifyOwner() throws Exception {
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) // 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);
+ // 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();
}
@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 @@
}
@Test
- public void newPatchSetByOwnerOnReviewableChangeToWipInNoteDb() throws Exception {
- assume().that(notesMigration.readChanges()).isTrue();
+ public void newPatchSetByOwnerOnReviewableChangeToWip() throws Exception {
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)
- .to(sc.reviewer)
- .to(sc.reviewerByEmail)
- .cc(sc.ccer)
- .cc(sc.ccerByEmail)
- .bcc(sc.starrer)
- .bcc(NEW_PATCHSETS);
+ assertThat(sender).notSent();
}
@Test
- public void newPatchSetByOwnerOnReviewableChangeToWipInReviewDb() 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();
+ public void newPatchSetOnWipChange() throws Exception {
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)
- .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);
+ assertThat(sender).notSent();
}
@Test
@@ -379,35 +290,121 @@
}
@Test
- public void newPatchSetOnReviewableWipChangeInNoteDb() throws Exception {
- assume().that(notesMigration.readChanges()).isTrue();
+ public void newPatchSetOnReviewableWipChange() throws Exception {
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);
- // TODO(logan): This shouldn't notify.
+ pushTo(sc, "refs/for/master%ready", sc.owner);
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 {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 14042ba..11330cb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -1273,7 +1273,7 @@
+ "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 @@
}
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 @@
.setExtraCC(recipients.getCcOnly())
.setApprovals(approvals)
.setMessage(msg.toString())
- .setNotify(magicBranch.notify)
+ .setNotify(magicBranch.getNotify())
.setAccountsToNotify(magicBranch.getAccountsToNotify())
.setRequestScopePropagator(requestScopePropagator)
.setSendMail(true)
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
index a7b8133..71c2ab1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
@@ -443,8 +443,7 @@
}
}
- NotifyHandling notify =
- magicBranch != null && magicBranch.notify != null ? magicBranch.notify : NotifyHandling.ALL;
+ NotifyHandling notify = magicBranch != null ? magicBranch.getNotify(notes) : NotifyHandling.ALL;
if (shouldPublishComments()) {
emailCommentsFactory
@@ -489,7 +488,7 @@
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());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java
index f0b61db..b5dbc84 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ReplacePatchSetSender.java
+++ b/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 @@
//
reviewers.remove(fromId);
}
- add(RecipientType.TO, reviewers);
- add(RecipientType.CC, extraCC);
+ if (notify == NotifyHandling.ALL || notify == NotifyHandling.OWNER_REVIEWERS) {
+ add(RecipientType.TO, reviewers);
+ add(RecipientType.CC, extraCC);
+ }
rcptToAuthors(RecipientType.CC);
bccStarredBy();
includeWatchers(
