diff --git a/WORKSPACE b/WORKSPACE index 3839b2169a..f7bfb5980d 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -696,12 +696,20 @@ maven_jar( sha1 = "42a25dc3219429f0e5d060061f71acb49bf010a0", ) +TRUTH_VERS = "0.31" + maven_jar( name = "truth", - artifact = "com.google.truth:truth:0.31", + artifact = "com.google.truth:truth:" + TRUTH_VERS, sha1 = "1a926b0cb2879fd32efbb3716ee8bab040f4218b", ) +maven_jar( + name = "truth-java8-extension", + artifact = "com.google.truth.extensions:truth-java8-extension:" + TRUTH_VERS, + sha1 = "a7e80e631f2bf4ecc2b99ad1e33059eb0dcc6ea0", +) + maven_jar( name = "easymock", artifact = "org.easymock:easymock:3.1", # When bumping the version diff --git a/gerrit-acceptance-framework/BUILD b/gerrit-acceptance-framework/BUILD index 69f132b873..db5a3004a8 100644 --- a/gerrit-acceptance-framework/BUILD +++ b/gerrit-acceptance-framework/BUILD @@ -41,6 +41,7 @@ java_library2( "//gerrit-server:testutil", "//gerrit-server/src/main/prolog:common", "//lib:truth", + "//lib:truth-java8-extension", "//lib/auto:auto-value", "//lib/httpcomponents:fluent-hc", "//lib/httpcomponents:httpclient", diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 998abbff8b..df82e211b7 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.api.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.assertPushOk; import static com.google.gerrit.acceptance.GitUtil.pushHead; @@ -31,9 +32,11 @@ import static com.google.gerrit.server.project.Util.category; import static com.google.gerrit.server.project.Util.value; import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toSet; import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; @@ -57,6 +60,7 @@ import com.google.gerrit.extensions.api.changes.NotifyInfo; import com.google.gerrit.extensions.api.changes.RebaseInput; import com.google.gerrit.extensions.api.changes.RecipientType; import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling; import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.api.projects.BranchInput; import com.google.gerrit.extensions.client.ChangeKind; @@ -114,6 +118,8 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Optional; +import java.util.Set; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Constants; @@ -945,17 +951,41 @@ public class ChangeIT extends AbstractDaemonTest { } @Test - public void implicitlyCcOnNonVotingReview() throws Exception { + public void implicitlyCcOnNonVotingReviewPgStyle() throws Exception { PushOneCommit.Result r = createChange(); setApiUser(user); - gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(new ReviewInput()); + assertThat(getReviewerState(r.getChangeId(), user.id)).isEmpty(); - ChangeInfo c = gApi.changes().id(r.getChangeId()).get(); - // If we're not reading from NoteDb, then the CCed user will be returned - // in the REVIEWER state. - ReviewerState state = notesMigration.readChanges() ? CC : REVIEWER; - assertThat(c.reviewers.get(state).stream().map(ai -> ai._accountId).collect(toList())) - .containsExactly(user.id.get()); + // Exact request format made by PG UI at ddc6b7160fe416fed9e7e3180489d44c82fd64f8. + ReviewInput in = new ReviewInput(); + in.drafts = DraftHandling.PUBLISH_ALL_REVISIONS; + in.labels = ImmutableMap.of(); + in.message = "comment"; + in.reviewers = ImmutableList.of(); + gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in); + + // If we're not reading from NoteDb, then the CCed user will be returned in the REVIEWER state. + assertThat(getReviewerState(r.getChangeId(), user.id)) + .hasValue(notesMigration.readChanges() ? CC : REVIEWER); + } + + @Test + public void implicitlyCcOnNonVotingReviewGwtStyle() throws Exception { + PushOneCommit.Result r = createChange(); + setApiUser(user); + assertThat(getReviewerState(r.getChangeId(), user.id)).isEmpty(); + + // Exact request format made by GWT UI at ddc6b7160fe416fed9e7e3180489d44c82fd64f8. + ReviewInput in = new ReviewInput(); + in.labels = ImmutableMap.of("Code-Review", (short) 0); + in.strictLabels = true; + in.drafts = DraftHandling.PUBLISH_ALL_REVISIONS; + in.message = "comment"; + gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in); + + // If we're not reading from NoteDb, then the CCed user will be returned in the REVIEWER state. + assertThat(getReviewerState(r.getChangeId(), user.id)) + .hasValue(notesMigration.readChanges() ? CC : REVIEWER); } @Test @@ -2290,6 +2320,20 @@ public class ChangeIT extends AbstractDaemonTest { return changeResourceFactory.create(ctls.get(0)); } + private Optional getReviewerState(String changeId, Account.Id accountId) + throws Exception { + ChangeInfo c = gApi.changes().id(changeId).get(EnumSet.of(ListChangesOption.DETAILED_LABELS)); + Set states = + c.reviewers + .entrySet() + .stream() + .filter(e -> e.getValue().stream().anyMatch(a -> a._accountId == accountId.get())) + .map(e -> e.getKey()) + .collect(toSet()); + assertThat(states.size()).named(states.toString()).isAtMost(1); + return states.stream().findFirst(); + } + private void setChangeStatus(Change.Id id, Change.Status newStatus) throws Exception { try (BatchUpdate batchUpdate = updateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 790c24145d..d0c0e29982 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -244,7 +244,10 @@ public class PostReview implements RestModifyView batchUpdateFactory.create( db.get(), revision.getChange().getProject(), revision.getUser(), ts)) { Account.Id id = bu.getUser().getAccountId(); - boolean ccOrReviewer = input.labels != null && !input.labels.isEmpty(); + boolean ccOrReviewer = false; + if (input.labels != null && !input.labels.isEmpty()) { + ccOrReviewer = input.labels.values().stream().filter(v -> v != 0).findFirst().isPresent(); + } if (!ccOrReviewer) { // Check if user was already CCed or reviewing prior to this review. diff --git a/lib/BUILD b/lib/BUILD index ca0fec3443..fe1933c8a7 100644 --- a/lib/BUILD +++ b/lib/BUILD @@ -230,6 +230,17 @@ java_library( ], ) +java_library( + name = "truth-java8-extension", + data = ["//lib:LICENSE-DO_NOT_DISTRIBUTE"], + visibility = ["//visibility:public"], + exports = [ + ":guava", + ":truth", + "@truth-java8-extension//jar", + ], +) + java_library( name = "javassist", data = ["//lib:LICENSE-DO_NOT_DISTRIBUTE"],