Persist CCs on Git push Previously, we have sent an initial email to CCs added upon push, but have not persisted them. This change adds logic and tests to do so. CCs have native support in NoteDb and legacy support in ReviewDb where they are added as reviewers instead. This mirrors what we do in PostReviewers and PostReview. Change-Id: I4177526707141ffe676a5e4db534aeb740b7d53f
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java index 6ca8384..fe1d0b7 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -16,9 +16,11 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.GitUtil.pushHead; +import static java.util.stream.Collectors.toList; import static org.junit.Assert.assertEquals; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; @@ -29,15 +31,19 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; +import com.google.gerrit.testutil.TestNotesMigration; import com.google.gwtorm.server.OrmException; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Stream; import org.eclipse.jgit.api.TagCommand; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.PersonIdent; @@ -138,6 +144,7 @@ private final ChangeNotes.Factory notesFactory; private final ApprovalsUtil approvalsUtil; private final Provider<InternalChangeQuery> queryProvider; + private final TestNotesMigration notesMigration; private final ReviewDb db; private final TestRepository<?> testRepo; @@ -155,6 +162,7 @@ ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, Provider<InternalChangeQuery> queryProvider, + TestNotesMigration notesMigration, @Assisted ReviewDb db, @Assisted PersonIdent i, @Assisted TestRepository<?> testRepo) @@ -163,6 +171,7 @@ notesFactory, approvalsUtil, queryProvider, + notesMigration, db, i, testRepo, @@ -176,6 +185,7 @@ ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, Provider<InternalChangeQuery> queryProvider, + TestNotesMigration notesMigration, @Assisted ReviewDb db, @Assisted PersonIdent i, @Assisted TestRepository<?> testRepo, @@ -185,6 +195,7 @@ notesFactory, approvalsUtil, queryProvider, + notesMigration, db, i, testRepo, @@ -199,6 +210,7 @@ ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, Provider<InternalChangeQuery> queryProvider, + TestNotesMigration notesMigration, @Assisted ReviewDb db, @Assisted PersonIdent i, @Assisted TestRepository<?> testRepo, @@ -210,6 +222,7 @@ notesFactory, approvalsUtil, queryProvider, + notesMigration, db, i, testRepo, @@ -224,13 +237,24 @@ ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, Provider<InternalChangeQuery> queryProvider, + TestNotesMigration notesMigration, @Assisted ReviewDb db, @Assisted PersonIdent i, @Assisted TestRepository<?> testRepo, @Assisted String subject, @Assisted Map<String, String> files) throws Exception { - this(notesFactory, approvalsUtil, queryProvider, db, i, testRepo, subject, files, null); + this( + notesFactory, + approvalsUtil, + queryProvider, + notesMigration, + db, + i, + testRepo, + subject, + files, + null); } @AssistedInject @@ -238,6 +262,7 @@ ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, Provider<InternalChangeQuery> queryProvider, + TestNotesMigration notesMigration, @Assisted ReviewDb db, @Assisted PersonIdent i, @Assisted TestRepository<?> testRepo, @@ -250,6 +275,7 @@ notesFactory, approvalsUtil, queryProvider, + notesMigration, db, i, testRepo, @@ -262,6 +288,7 @@ ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, Provider<InternalChangeQuery> queryProvider, + TestNotesMigration notesMigration, ReviewDb db, PersonIdent i, TestRepository<?> testRepo, @@ -274,6 +301,7 @@ this.notesFactory = notesFactory; this.approvalsUtil = approvalsUtil; this.queryProvider = queryProvider; + this.notesMigration = notesMigration; this.subject = subject; this.files = files; this.changeId = changeId; @@ -392,16 +420,36 @@ public void assertChange( Change.Status expectedStatus, String expectedTopic, TestAccount... expectedReviewers) throws OrmException { + assertChange( + expectedStatus, expectedTopic, Arrays.asList(expectedReviewers), ImmutableList.of()); + } + + public void assertChange( + Change.Status expectedStatus, + String expectedTopic, + List<TestAccount> expectedReviewers, + List<TestAccount> expectedCcs) + throws OrmException { Change c = getChange().change(); assertThat(c.getSubject()).isEqualTo(resSubj); assertThat(c.getStatus()).isEqualTo(expectedStatus); assertThat(Strings.emptyToNull(c.getTopic())).isEqualTo(expectedTopic); - assertReviewers(c, expectedReviewers); + if (notesMigration.readChanges()) { + assertReviewers(c, ReviewerStateInternal.REVIEWER, expectedReviewers); + assertReviewers(c, ReviewerStateInternal.CC, expectedCcs); + } else { + assertReviewers( + c, + ReviewerStateInternal.REVIEWER, + Stream.concat(expectedReviewers.stream(), expectedCcs.stream()).collect(toList())); + } } - private void assertReviewers(Change c, TestAccount... expectedReviewers) throws OrmException { + private void assertReviewers( + Change c, ReviewerStateInternal state, List<TestAccount> expectedReviewers) + throws OrmException { Iterable<Account.Id> actualIds = - approvalsUtil.getReviewers(db, notesFactory.createChecked(db, c)).all(); + approvalsUtil.getReviewers(db, notesFactory.createChecked(db, c)).byState(state); assertThat(actualIds) .containsExactlyElementsIn(Sets.newHashSet(TestAccount.ids(expectedReviewers))); }
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java index 3ab4a88..7acb135 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/TestAccount.java
@@ -32,10 +32,6 @@ return accounts.stream().map(a -> a.id).collect(toList()); } - public static List<Account.Id> ids(TestAccount... accounts) { - return ids(Arrays.asList(accounts)); - } - public static List<String> names(List<TestAccount> accounts) { return accounts.stream().map(a -> a.fullName).collect(toList()); }
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index b52136e..cdb8fbb 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -323,10 +323,13 @@ String topic = "my/topic"; PushOneCommit.Result r = pushTo("refs/for/master/" + topic + "%cc=" + user.email); r.assertOkStatus(); - r.assertChange(Change.Status.NEW, topic); + r.assertChange( + Change.Status.NEW, + topic, + ImmutableList.of(), + ImmutableList.of(user)); // cc several users - TestAccount user2 = accounts.create("another-user", "another.user@example.com", "Another User"); r = pushTo( "refs/for/master/" @@ -336,9 +339,14 @@ + ",cc=" + user.email + ",cc=" - + user2.email); + + accounts.user2().email); r.assertOkStatus(); - r.assertChange(Change.Status.NEW, topic); + // Check that admin isn't CC'd as they own the change + r.assertChange( + Change.Status.NEW, + topic, + ImmutableList.of(), + ImmutableList.of(user, accounts.user2())); // cc non-existing user String nonExistingEmail = "non.existing@example.com";
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 5391635..80d644f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -51,6 +51,7 @@ import com.google.gerrit.server.mail.send.CreateChangeSender; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchProjectException; @@ -68,6 +69,7 @@ import java.io.IOException; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -100,6 +102,7 @@ private final CommitValidators.Factory commitValidatorsFactory; private final RevisionCreated revisionCreated; private final CommentAdded commentAdded; + private final NotesMigration migration; private final Change.Id changeId; private final PatchSet.Id psId; @@ -147,6 +150,7 @@ CommitValidators.Factory commitValidatorsFactory, CommentAdded commentAdded, RevisionCreated revisionCreated, + NotesMigration migration, @Assisted Change.Id changeId, @Assisted ObjectId commitId, @Assisted String refName) { @@ -162,6 +166,7 @@ this.commitValidatorsFactory = commitValidatorsFactory; this.revisionCreated = revisionCreated; this.commentAdded = commentAdded; + this.migration = migration; this.changeId = changeId; this.psId = new PatchSet.Id(changeId, INITIAL_PATCH_SET_ID); @@ -408,6 +413,14 @@ */ update.fixStatus(change.getStatus()); + Set<Account.Id> reviewersToAdd = new HashSet<>(reviewers); + if (migration.readChanges()) { + approvalsUtil.addCcs( + ctx.getNotes(), update, filterOnChangeVisibility(db, ctx.getNotes(), extraCC)); + } else { + reviewersToAdd.addAll(extraCC); + } + LabelTypes labelTypes = ctl.getProjectControl().getLabelTypes(); approvalsUtil.addReviewers( db, @@ -416,7 +429,7 @@ change, patchSet, patchSetInfo, - filterOnChangeVisibility(db, ctx.getNotes(), reviewers), + filterOnChangeVisibility(db, ctx.getNotes(), reviewersToAdd), Collections.<Account.Id>emptySet()); approvalsUtil.addApprovalsForNewPatchSet( db, update, labelTypes, patchSet, ctx.getControl(), approvals);