opendev/gerrit
Code Issues Proposed changes

Add WIP awareness to PutMessage notifications

Browse Source 
This change adds notification integration tests for this new API
endpoint. For convenience, PutMessage.Input is moved to common
extensions and renamed MessageInput, so it can be incorporated into the
ChangeApi interface.

Bug: Issue 6615
Change-Id: If419d713e4586038f04cc079245fed825acfb647
This commit is contained in:
Logan Hanks
2017-06-29 15:13:27 -07:00
parent 3f52f803e5
commit a1e68dca14
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
@@ -5958,7 +5969,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();