opendev/gerrit
Code Issues Proposed changes

Fix Change-Id in revert email

Browse Source 
When a change is reverted we send two emails:
1. one email for the creation of the revert change
2. one email for posting a message on the change that is reverted

The second email should contain the Change-Id of the change on which the
message is posted (and not the Change-Id of the newly created revert
change).

Add a test to verify that both emails are triggered as expected.

Change-Id: I34ec258a0f63fed82c67f181542b29ef280a72b4
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2018-03-14 16:53:55 +01:00
parent 2040fc3c23
commit 424205ba06
3 changed files with 83 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/server/restapi/change/Revert.java
View File

@ -235,7 +235,7 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
try (BatchUpdate bu = updateFactory.create(db.get(), project, user, now)) {
bu.setRepository(git, revWalk, oi);
bu.insertChange(ins);
bu.addOp(changeId, new NotifyOp(notes.getChange(), ins));
bu.addOp(changeId, new NotifyOp(changeToRevert, ins));
bu.addOp(changeToRevert.getId(), new PostRevertedMessageOp(computedChangeId));
bu.execute();
}
@ -278,14 +278,12 @@ public class Revert extends RetryingRestModifyView<ChangeResource, RevertInput,
@Override
public void postUpdate(Context ctx) throws Exception {
changeReverted.fire(change, ins.getChange(), ctx.getWhen());
Change.Id changeId = ins.getChange().getId();
try {
RevertedSender cm = revertedSenderFactory.create(ctx.getProject(), changeId);
RevertedSender cm = revertedSenderFactory.create(ctx.getProject(), change.getId());
cm.setFrom(ctx.getAccountId());
cm.setChangeMessage(ins.getChangeMessage().getMessage(), ctx.getWhen());
cm.send();
} catch (Exception err) {
log.error("Cannot send email for revert change " + changeId, err);
log.error("Cannot send email for revert change " + change.getId(), err);
}
}
}

16
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@ -674,6 +674,22 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(revertChange.revertOf).isEqualTo(gApi.changes().id(r.getChangeId()).get()._number);
}
@Test
public void revertNotifications() throws Exception {
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).addReviewer(user.email);
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
sender.clear();
ChangeInfo revertChange = gApi.changes().id(r.getChangeId()).revert().get();
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(2);
assertThat(sender.getMessages(revertChange.changeId, "newchange")).hasSize(1);
assertThat(sender.getMessages(r.getChangeId(), "revert")).hasSize(1);
}
@Test
public void revertPreservesReviewersAndCcs() throws Exception {
PushOneCommit.Result r = createChange();

86
javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
View File

@ -2219,17 +2219,22 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, sc.owner);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.cc(sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
@Test
@ -2237,18 +2242,23 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, sc.owner);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.ccer)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.cc(sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
@Test
@ -2256,19 +2266,24 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, sc.owner, CC_ON_OWN_COMMENTS);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.cc(sc.owner)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.to(sc.owner)
.cc(sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
@Test
@ -2276,19 +2291,24 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, sc.owner, CC_ON_OWN_COMMENTS);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.owner, sc.ccer)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.to(sc.owner)
.cc(sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
@Test
@ -2296,17 +2316,23 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, other);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.cc(sc.owner, sc.reviewer, sc.ccer, admin)
.to(sc.owner)
.cc(sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
@Test
@ -2314,18 +2340,24 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, other);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.ccer)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.cc(sc.owner, sc.reviewer, sc.ccer, admin)
.to(sc.owner)
.cc(sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
@Test
@ -2333,19 +2365,24 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isFalse();
StagedChange sc = stageChange();
revert(sc, other, CC_ON_OWN_COMMENTS);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.ccer, sc.watchingProjectOwner, admin)
.cc(other)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.to(other)
.cc(sc.owner, sc.reviewer, sc.ccer, admin)
.to(sc.owner)
.cc(other, sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
@Test
@ -2353,19 +2390,24 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
assume().that(notesMigration.readChanges()).isTrue();
StagedChange sc = stageChange();
revert(sc, other, CC_ON_OWN_COMMENTS);
// email for the newly created revert change
assertThat(sender)
.sent("newchange", sc)
.to(sc.owner, sc.reviewer, sc.watchingProjectOwner, admin)
.cc(sc.ccer, other)
.bcc(NEW_CHANGES, NEW_PATCHSETS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
// email for the change that is reverted
assertThat(sender)
.sent("revert", sc)
.to(other)
.cc(sc.owner, sc.reviewer, sc.ccer, admin)
.to(sc.owner)
.cc(other, sc.reviewer, sc.ccer, admin)
.cc(sc.reviewerByEmail, sc.ccerByEmail)
.bcc(sc.starrer)
.bcc(ALL_COMMENTS)
.noOneElse(); // TODO(logan): Why not starrer/reviewers-by-email?
.noOneElse();
}
private StagedChange stageChange() throws Exception {