Make implicit reviewers feature actually work on GWT UI
This was presumably fixed in Ia5e6b9791 and in I8b735db4f. It turns out, that labels map is not null in GWT UI, as it is the case in PG, but also non empty, even in non-voting case: Code-Review: 0. Fix the optimization check to account for zero votes. Bug: Issue 4638 Change-Id: I6d9a2cc42ec51e6b1df13b96cf4bcdd082c87f60
This commit is contained in:
parent
126d496953
commit
39bc108a39
10
WORKSPACE
10
WORKSPACE
@ -696,12 +696,20 @@ maven_jar(
|
|||||||
sha1 = "42a25dc3219429f0e5d060061f71acb49bf010a0",
|
sha1 = "42a25dc3219429f0e5d060061f71acb49bf010a0",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
TRUTH_VERS = "0.31"
|
||||||
|
|
||||||
maven_jar(
|
maven_jar(
|
||||||
name = "truth",
|
name = "truth",
|
||||||
artifact = "com.google.truth:truth:0.31",
|
artifact = "com.google.truth:truth:" + TRUTH_VERS,
|
||||||
sha1 = "1a926b0cb2879fd32efbb3716ee8bab040f4218b",
|
sha1 = "1a926b0cb2879fd32efbb3716ee8bab040f4218b",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
maven_jar(
|
||||||
|
name = "truth-java8-extension",
|
||||||
|
artifact = "com.google.truth.extensions:truth-java8-extension:" + TRUTH_VERS,
|
||||||
|
sha1 = "a7e80e631f2bf4ecc2b99ad1e33059eb0dcc6ea0",
|
||||||
|
)
|
||||||
|
|
||||||
maven_jar(
|
maven_jar(
|
||||||
name = "easymock",
|
name = "easymock",
|
||||||
artifact = "org.easymock:easymock:3.1", # When bumping the version
|
artifact = "org.easymock:easymock:3.1", # When bumping the version
|
||||||
|
@ -41,6 +41,7 @@ java_library2(
|
|||||||
"//gerrit-server:testutil",
|
"//gerrit-server:testutil",
|
||||||
"//gerrit-server/src/main/prolog:common",
|
"//gerrit-server/src/main/prolog:common",
|
||||||
"//lib:truth",
|
"//lib:truth",
|
||||||
|
"//lib:truth-java8-extension",
|
||||||
"//lib/auto:auto-value",
|
"//lib/auto:auto-value",
|
||||||
"//lib/httpcomponents:fluent-hc",
|
"//lib/httpcomponents:fluent-hc",
|
||||||
"//lib/httpcomponents:httpclient",
|
"//lib/httpcomponents:httpclient",
|
||||||
|
@ -15,6 +15,7 @@
|
|||||||
package com.google.gerrit.acceptance.api.change;
|
package com.google.gerrit.acceptance.api.change;
|
||||||
|
|
||||||
import static com.google.common.truth.Truth.assertThat;
|
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.common.truth.TruthJUnit.assume;
|
||||||
import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
|
import static com.google.gerrit.acceptance.GitUtil.assertPushOk;
|
||||||
import static com.google.gerrit.acceptance.GitUtil.pushHead;
|
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 com.google.gerrit.server.project.Util.value;
|
||||||
import static java.util.concurrent.TimeUnit.SECONDS;
|
import static java.util.concurrent.TimeUnit.SECONDS;
|
||||||
import static java.util.stream.Collectors.toList;
|
import static java.util.stream.Collectors.toList;
|
||||||
|
import static java.util.stream.Collectors.toSet;
|
||||||
import static org.junit.Assert.fail;
|
import static org.junit.Assert.fail;
|
||||||
|
|
||||||
import com.google.common.collect.ImmutableList;
|
import com.google.common.collect.ImmutableList;
|
||||||
|
import com.google.common.collect.ImmutableMap;
|
||||||
import com.google.common.collect.ImmutableSet;
|
import com.google.common.collect.ImmutableSet;
|
||||||
import com.google.common.collect.Iterables;
|
import com.google.common.collect.Iterables;
|
||||||
import com.google.common.collect.Lists;
|
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.RebaseInput;
|
||||||
import com.google.gerrit.extensions.api.changes.RecipientType;
|
import com.google.gerrit.extensions.api.changes.RecipientType;
|
||||||
import com.google.gerrit.extensions.api.changes.ReviewInput;
|
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.changes.RevisionApi;
|
||||||
import com.google.gerrit.extensions.api.projects.BranchInput;
|
import com.google.gerrit.extensions.api.projects.BranchInput;
|
||||||
import com.google.gerrit.extensions.client.ChangeKind;
|
import com.google.gerrit.extensions.client.ChangeKind;
|
||||||
@ -114,6 +118,8 @@ import java.util.HashMap;
|
|||||||
import java.util.Iterator;
|
import java.util.Iterator;
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
|
import java.util.Optional;
|
||||||
|
import java.util.Set;
|
||||||
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
|
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
|
||||||
import org.eclipse.jgit.junit.TestRepository;
|
import org.eclipse.jgit.junit.TestRepository;
|
||||||
import org.eclipse.jgit.lib.Constants;
|
import org.eclipse.jgit.lib.Constants;
|
||||||
@ -945,17 +951,41 @@ public class ChangeIT extends AbstractDaemonTest {
|
|||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void implicitlyCcOnNonVotingReview() throws Exception {
|
public void implicitlyCcOnNonVotingReviewPgStyle() throws Exception {
|
||||||
PushOneCommit.Result r = createChange();
|
PushOneCommit.Result r = createChange();
|
||||||
setApiUser(user);
|
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();
|
// Exact request format made by PG UI at ddc6b7160fe416fed9e7e3180489d44c82fd64f8.
|
||||||
// If we're not reading from NoteDb, then the CCed user will be returned
|
ReviewInput in = new ReviewInput();
|
||||||
// in the REVIEWER state.
|
in.drafts = DraftHandling.PUBLISH_ALL_REVISIONS;
|
||||||
ReviewerState state = notesMigration.readChanges() ? CC : REVIEWER;
|
in.labels = ImmutableMap.of();
|
||||||
assertThat(c.reviewers.get(state).stream().map(ai -> ai._accountId).collect(toList()))
|
in.message = "comment";
|
||||||
.containsExactly(user.id.get());
|
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
|
@Test
|
||||||
@ -2290,6 +2320,20 @@ public class ChangeIT extends AbstractDaemonTest {
|
|||||||
return changeResourceFactory.create(ctls.get(0));
|
return changeResourceFactory.create(ctls.get(0));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private Optional<ReviewerState> getReviewerState(String changeId, Account.Id accountId)
|
||||||
|
throws Exception {
|
||||||
|
ChangeInfo c = gApi.changes().id(changeId).get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
|
||||||
|
Set<ReviewerState> 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 {
|
private void setChangeStatus(Change.Id id, Change.Status newStatus) throws Exception {
|
||||||
try (BatchUpdate batchUpdate =
|
try (BatchUpdate batchUpdate =
|
||||||
updateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
|
updateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
|
||||||
|
@ -244,7 +244,10 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
|
|||||||
batchUpdateFactory.create(
|
batchUpdateFactory.create(
|
||||||
db.get(), revision.getChange().getProject(), revision.getUser(), ts)) {
|
db.get(), revision.getChange().getProject(), revision.getUser(), ts)) {
|
||||||
Account.Id id = bu.getUser().getAccountId();
|
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) {
|
if (!ccOrReviewer) {
|
||||||
// Check if user was already CCed or reviewing prior to this review.
|
// Check if user was already CCed or reviewing prior to this review.
|
||||||
|
11
lib/BUILD
11
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(
|
java_library(
|
||||||
name = "javassist",
|
name = "javassist",
|
||||||
data = ["//lib:LICENSE-DO_NOT_DISTRIBUTE"],
|
data = ["//lib:LICENSE-DO_NOT_DISTRIBUTE"],
|
||||||
|
Loading…
Reference in New Issue
Block a user