opendev/gerrit
Code Issues Proposed changes

Merge "Add WIP awareness to PutMessage notifications"

Browse Source
This commit is contained in:
Logan Hanks
2017-07-07 16:52:18 +00:00
committed by Gerrit Code Review
parent 6a216fc633 a1e68dca14
commit 720257c525
7 changed files with 329 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
Documentation/rest-api-changes.txt
View File

@@ -3154,6 +3154,17 @@ link:project-configuration.html#require-change-id[Require Change-Id] was specifi
}
----
.Notifications
An email will be sent using the "newpatchset" template.
[options="header",cols="1,1"]
|=============================
|WIP State |Default
|Ready for review|owner, reviewers, CCs, stars, NEW_PATCHSETS watchers
|Work in progress|owner
|=============================
[[revision-endpoints]]
== Revision Endpoints
@@ -5960,7 +5971,7 @@ the commit message of a change.
Notify handling that defines to whom email notifications should be sent
after the commit message was updated. +
Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. +
If not set, the default is `ALL`.
If not set, the default is `OWNER` for WIP changes and `ALL` otherwise.
|`notify_details`|optional|
Additional information about whom to notify about the update as a map
of recipient type to link:#notify-info[NotifyInfo] entity.

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

@@ -43,6 +43,8 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.CommitMessageInput;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.project.Util;
import org.junit.Before;
@@ -1473,10 +1475,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
pushTo(sc, "refs/for/master", other);
// TODO(logan): This should include owner but currently doesn't because
// it's sent *from* the owner.
assertThat(sender)
.sent("newpatchset", sc)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer, other)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
@@ -1494,7 +1495,6 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
.sent("newpatchset", sc)
.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)
.bcc(NEW_PATCHSETS)
.noOneElse();
@@ -1507,7 +1507,8 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
pushTo(sc, "refs/for/master", other, EmailStrategy.CC_ON_OWN_COMMENTS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer, other)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer, other)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
@@ -1522,7 +1523,8 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
pushTo(sc, "refs/for/master", other, EmailStrategy.CC_ON_OWN_COMMENTS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer, sc.ccer, other)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer, sc.ccer, other)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
@@ -1533,10 +1535,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
pushTo(sc, "refs/for/master%notify=OWNER_REVIEWERS", other);
// TODO(logan): This should include owner but currently doesn't because
// it's sent *from* the owner.
assertThat(sender)
.sent("newpatchset", sc)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
@@ -1549,9 +1550,11 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageReviewableChange();
pushTo(sc, "refs/for/master%notify=OWNER_REVIEWERS", other);
// TODO(logan): This should include owner but currently doesn't because
// it's sent *from* the owner.
assertThat(sender).sent("newpatchset", sc).to(sc.reviewer, sc.ccer).noOneElse();
assertThat(sender)
.sent("newpatchset", sc)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer, sc.ccer)
.noOneElse();
}
@Test
@@ -1562,7 +1565,8 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
pushTo(sc, "refs/for/master%notify=OWNER_REVIEWERS", other, EmailStrategy.CC_ON_OWN_COMMENTS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.noOneElse();
@@ -1574,7 +1578,11 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageReviewableChange();
pushTo(sc, "refs/for/master%notify=OWNER_REVIEWERS", other, EmailStrategy.CC_ON_OWN_COMMENTS);
assertThat(sender).sent("newpatchset", sc).to(sc.owner, sc.reviewer, sc.ccer).noOneElse();
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.reviewer, sc.ccer)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.noOneElse();
}
@Test
@@ -1797,10 +1805,246 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
private void pushTo(StagedChange sc, String ref, TestAccount by, EmailStrategy emailStrategy)
throws Exception {
setEmailStrategy(sc.owner, emailStrategy);
setEmailStrategy(by, emailStrategy);
pushFactory.create(db, by.getIdent(), sc.repo, sc.changeId).to(ref).assertOkStatus();
}
@Test
public void editCommitMessageEditByOwnerOnReviewableChangeInNoteDb() throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, sc.owner);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.reviewer)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
@Test
public void editCommitMessageEditByOwnerOnReviewableChangeInReviewDb() throws Exception {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, sc.owner);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.reviewer, sc.ccer)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
@Test
public void editCommitMessageEditByOtherOnReviewableChangeInNoteDb() throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
@Test
public void editCommitMessageEditByOtherOnReviewableChangeInReviewDb() throws Exception {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer, sc.ccer)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeOwnerSelfCcInNoteDb() throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, CC_ON_OWN_COMMENTS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer, other)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeOwnerSelfCcInReviewDb() throws Exception {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, CC_ON_OWN_COMMENTS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer, sc.ccer, other)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeNotifyOwnerReviewersInNoteDb()
throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, OWNER_REVIEWERS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeNotifyOwnerReviewersInReviewDb()
throws Exception {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, OWNER_REVIEWERS);
assertThat(sender).sent("newpatchset", sc).to(sc.owner, sc.reviewer, sc.ccer).noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeOwnerSelfCcNotifyOwnerReviewersInNoteDb()
throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, OWNER_REVIEWERS, CC_ON_OWN_COMMENTS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer)
.cc(sc.ccer, other)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeOwnerSelfCcNotifyOwnerReviewersInReviewDb()
throws Exception {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, OWNER_REVIEWERS, CC_ON_OWN_COMMENTS);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.owner, sc.reviewer, sc.ccer)
.cc(other)
.noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeNotifyOwner() throws Exception {
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, OWNER);
assertThat(sender).sent("newpatchset", sc).to(sc.owner).noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeOwnerSelfCcNotifyOwner() throws Exception {
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, OWNER, CC_ON_OWN_COMMENTS);
assertThat(sender).sent("newpatchset", sc).to(sc.owner).cc(other).noOneElse();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeNotifyNone() throws Exception {
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, NONE);
assertThat(sender).notSent();
}
@Test
public void editCommitMessageByOtherOnReviewableChangeOwnerSelfCcNotifyNone() throws Exception {
StagedChange sc = stageReviewableChange();
editCommitMessage(sc, other, NONE, CC_ON_OWN_COMMENTS);
assertThat(sender).notSent();
}
@Test
public void editCommitMessageOnWipChange() throws Exception {
StagedChange sc = stageWipChange();
editCommitMessage(sc, sc.owner);
assertThat(sender).notSent();
}
@Test
public void editCommitMessageByOtherOnWipChange() throws Exception {
StagedChange sc = stageWipChange();
editCommitMessage(sc, other);
assertThat(sender).sent("newpatchset", sc).to(sc.owner).noOneElse();
}
@Test
public void editCommitMessageByOtherOnWipChangeSelfCc() throws Exception {
StagedChange sc = stageWipChange();
editCommitMessage(sc, other, CC_ON_OWN_COMMENTS);
assertThat(sender).sent("newpatchset", sc).to(sc.owner).cc(other).noOneElse();
}
@Test
public void editCommitMessageOnWipChangeNotifyAllInNoteDb() throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageWipChange();
editCommitMessage(sc, sc.owner, ALL);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.reviewer)
.cc(sc.ccer)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
@Test
public void editCommitMessageOnWipChangeNotifyAllInReviewDb() throws Exception {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageWipChange();
editCommitMessage(sc, sc.owner, ALL);
assertThat(sender)
.sent("newpatchset", sc)
.to(sc.reviewer, sc.ccer)
.bcc(sc.starrer)
.bcc(NEW_PATCHSETS)
.noOneElse();
}
private void editCommitMessage(StagedChange sc, TestAccount by) throws Exception {
editCommitMessage(sc, by, null, ENABLED);
}
private void editCommitMessage(StagedChange sc, TestAccount by, @Nullable NotifyHandling notify)
throws Exception {
editCommitMessage(sc, by, notify, ENABLED);
}
private void editCommitMessage(StagedChange sc, TestAccount by, EmailStrategy emailStrategy)
throws Exception {
editCommitMessage(sc, by, null, emailStrategy);
}
private void editCommitMessage(
StagedChange sc, TestAccount by, @Nullable NotifyHandling notify, EmailStrategy emailStrategy)
throws Exception {
setEmailStrategy(by, emailStrategy);
CommitInfo commit = gApi.changes().id(sc.changeId).revision("current").commit(false);
CommitMessageInput in = new CommitMessageInput();
in.message = "update\n" + commit.message;
in.notify = notify;
gApi.changes().id(sc.changeId).setMessage(in);
}
/*
* RestoredSender tests.
*/
@@ -1914,7 +2158,6 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assertThat(sender)
.sent("revert", sc)
.notTo(sc.owner)
.cc(sc.reviewer, sc.ccer, admin)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?

9
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
View File

@@ -19,6 +19,7 @@ import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.CommitMessageInput;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.MergePatchSetInput;
import com.google.gerrit.extensions.common.RobotCommentInfo;
@@ -193,6 +194,9 @@ public interface ChangeApi {
/** Create a new patch set with a new commit message. */
void setMessage(String message) throws RestApiException;
/** Create a new patch set with a new commit message. */
void setMessage(CommitMessageInput in) throws RestApiException;
/** Set hashtags on a change */
void setHashtags(HashtagsInput input) throws RestApiException;
@@ -438,6 +442,11 @@ public interface ChangeApi {
throw new NotImplementedException();
}
@Override
public void setMessage(CommitMessageInput in) throws RestApiException {
throw new NotImplementedException();
}
@Override
public EditInfo getEdit() throws RestApiException {
throw new NotImplementedException();

30
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/CommitMessageInput.java Normal file
View File

@@ -0,0 +1,30 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.extensions.common;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.NotifyInfo;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.restapi.DefaultInput;
import java.util.Map;
public class CommitMessageInput {
@DefaultInput public String message;
@Nullable public NotifyHandling notify;
public Map<RecipientType, NotifyInfo> notifyDetails;
}

14
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@@ -39,6 +39,7 @@ import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.CommitMessageInput;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.MergePatchSetInput;
import com.google.gerrit.extensions.common.RobotCommentInfo;
@@ -515,11 +516,16 @@ class ChangeApiImpl implements ChangeApi {
}
@Override
public void setMessage(String in) throws RestApiException {
public void setMessage(String msg) throws RestApiException {
CommitMessageInput in = new CommitMessageInput();
in.message = msg;
setMessage(in);
}
@Override
public void setMessage(CommitMessageInput in) throws RestApiException {
try {
PutMessage.Input input = new PutMessage.Input();
input.message = in;
putMessage.apply(change, input);
putMessage.apply(change, in);
} catch (Exception e) {
throw asRestApiException("Cannot edit commit message", e);
}

3
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
@@ -180,7 +181,7 @@ public class PatchSetInserter implements BatchUpdateOp {
}
public PatchSetInserter setNotify(NotifyHandling notify) {
this.notify = notify;
this.notify = Preconditions.checkNotNull(notify);
return this;
}

24
gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java
View File

@@ -17,11 +17,9 @@ package com.google.gerrit.server.change;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.NotifyInfo;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.common.CommitMessageInput;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.DefaultInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -46,7 +44,6 @@ import com.google.gwtorm.server.OrmException;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.List;
import java.util.Map;
import java.util.TimeZone;
import javax.inject.Inject;
import javax.inject.Provider;
@@ -63,15 +60,7 @@ import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
public class PutMessage
extends RetryingRestModifyView<ChangeResource, PutMessage.Input, Response<?>> {
public static class Input {
@DefaultInput public String message;
public NotifyHandling notify = NotifyHandling.ALL;
public Map<RecipientType, NotifyInfo> notifyDetails;
}
extends RetryingRestModifyView<ChangeResource, CommitMessageInput, Response<?>> {
private final GitRepositoryManager repositoryManager;
private final Provider<CurrentUser> currentUserProvider;
@@ -106,7 +95,7 @@ public class PutMessage
@Override
protected Response<String> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource resource, Input input)
BatchUpdate.Factory updateFactory, ChangeResource resource, CommitMessageInput input)
throws IOException, UnchangedCommitMessageException, RestApiException, UpdateException,
PermissionBackendException, OrmException, ConfigInvalidException {
PatchSet ps = psUtil.current(db.get(), resource.getNotes());
@@ -127,6 +116,11 @@ public class PutMessage
resource.getChange().getKey().get(),
sanitizedCommitMessage);
NotifyHandling notify = input.notify;
if (notify == null) {
notify = resource.getChange().isWorkInProgress() ? NotifyHandling.OWNER : NotifyHandling.ALL;
}
try (Repository repository = repositoryManager.openRepository(resource.getProject());
RevWalk revWalk = new RevWalk(repository);
ObjectInserter objectInserter = repository.newObjectInserter()) {
@@ -152,7 +146,7 @@ public class PutMessage
inserter.setMessage(
String.format("Patch Set %s: Commit message was updated.", psId.getId()));
inserter.setDescription("Edit commit message");
inserter.setNotify(input.notify);
inserter.setNotify(notify);
inserter.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails));
bu.addOp(resource.getChange().getId(), inserter);
bu.execute();