diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java index 1fc91d28d0..49c86029af 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java @@ -16,14 +16,19 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; +import static com.google.gerrit.testutil.GerritServerTests.isNoteDbTestEnabled; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.RestSession; +import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.ChangeStatus; +import com.google.gerrit.extensions.client.ReviewerState; +import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.LabelInfo; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.testutil.ConfigSuite; @@ -33,6 +38,7 @@ import org.eclipse.jgit.lib.Config; import org.junit.Test; import java.io.IOException; +import java.util.Collection; public class DraftChangeIT extends AbstractDaemonTest { @ConfigSuite.Config @@ -109,6 +115,38 @@ public class DraftChangeIT extends AbstractDaemonTest { r.assertErrorStatus("draft workflow is disabled"); } + @Test + public void listApprovalsOnDraftChange() throws Exception { + assume().that(isAllowDrafts()).isTrue(); + PushOneCommit.Result result = createDraftChange(); + result.assertOkStatus(); + String changeId = result.getChangeId(); + String triplet = project.get() + "~master~" + changeId; + + gApi.changes().id(triplet).addReviewer(user.fullName); + + ChangeInfo info = get(triplet); + LabelInfo label = info.labels.get("Code-Review"); + assertThat(label.all).hasSize(1); + assertThat(label.all.get(0)._accountId).isEqualTo(user.id.get()); + assertThat(label.all.get(0).value).isEqualTo(0); + + ReviewerState rs = isNoteDbTestEnabled() + ? ReviewerState.REVIEWER : ReviewerState.CC; + Collection ccs = info.reviewers.get(rs); + assertThat(ccs).hasSize(1); + assertThat(ccs.iterator().next()._accountId).isEqualTo(user.id.get()); + + setApiUser(user); + gApi.changes().id(triplet).current().review(ReviewInput.recommend()); + setApiUser(admin); + + label = get(triplet).labels.get("Code-Review"); + assertThat(label.all).hasSize(1); + assertThat(label.all.get(0)._accountId).isEqualTo(user.id.get()); + assertThat(label.all.get(0).value).isEqualTo(1); + } + private PushOneCommit.Result createDraftChange() throws Exception { return pushTo("refs/drafts/master"); } diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java index 9d0676e0d7..e87cadd21d 100644 --- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java +++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java @@ -19,6 +19,7 @@ import com.google.gerrit.client.rpc.NativeString; import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.common.data.LabelValue; import com.google.gerrit.common.data.SubmitRecord; +import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; @@ -29,10 +30,13 @@ import com.google.gwt.core.client.JsArrayString; import com.google.gwtjsonrpc.client.impl.ser.JavaSqlTimestamp_JsonSerializer; import java.sql.Timestamp; +import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; @@ -133,6 +137,23 @@ public class ChangeInfo extends JavaScriptObject { public final native JsArray removableReviewers() /*-{ return this.removable_reviewers; }-*/; + private final native NativeMap> _reviewers() + /*-{ return this.reviewers; }-*/; + public final Map> reviewers() { + NativeMap> reviewers = _reviewers(); + Map> result = new HashMap<>(); + for (String k : reviewers.keySet()) { + ReviewerState state = ReviewerState.valueOf(k.toUpperCase()); + List accounts = result.get(state); + if (accounts == null) { + accounts = new ArrayList<>(); + result.put(state, accounts); + } + accounts.addAll(Natives.asList(reviewers.get(k))); + } + return result; + } + public final native boolean hasActions() /*-{ return this.hasOwnProperty('actions') }-*/; public final native NativeMap actions() /*-{ return this.actions; }-*/; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java index 48071cccb8..e04509bfe4 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java @@ -29,6 +29,7 @@ import com.google.gerrit.client.rpc.NativeMap; import com.google.gerrit.client.rpc.NativeString; import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.client.ui.RemoteSuggestBox; +import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.reviewdb.client.Change; import com.google.gwt.core.client.GWT; import com.google.gwt.core.client.JavaScriptObject; @@ -52,7 +53,9 @@ import com.google.gwt.user.client.ui.UIObject; import com.google.gwtexpui.safehtml.client.SafeHtml; import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder; +import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Set; @@ -205,15 +208,9 @@ public class Reviewers extends Composite { } private void display(ChangeInfo info) { - Map r = new HashMap<>(); - Map cc = new HashMap<>(); - for (LabelInfo label : Natives.asList(info.allLabels().values())) { - if (label.all() != null) { - for (ApprovalInfo ai : Natives.asList(label.all())) { - (ai.value() != 0 ? r : cc).put(ai._accountId(), ai); - } - } - } + Map> reviewers = info.reviewers(); + Map r = byAccount(reviewers, ReviewerState.REVIEWER); + Map cc = byAccount(reviewers, ReviewerState.CC); for (Integer i : r.keySet()) { cc.remove(i); } @@ -237,6 +234,19 @@ public class Reviewers extends Composite { } } + private static Map byAccount( + Map> reviewers, ReviewerState state) { + List accounts = reviewers.get(state); + if (accounts == null) { + return Collections.emptyMap(); + } + Map result = new HashMap<>(); + for (AccountInfo a : accounts) { + result.put(a._accountId(), a); + } + return result; + } + private static Map votable(ChangeInfo change) { Map d = new HashMap<>(); for (String name : change.labels()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 2e425564fe..cb7338b00d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -158,6 +158,7 @@ public class ChangeJson { private final GpgApiAdapter gpgApi; private AccountLoader accountLoader; + private Map> submitRecords; private FixInput fix; @AssistedInject @@ -485,14 +486,22 @@ public class ChangeJson { } private List submitRecords(ChangeData cd) throws OrmException { - if (cd.getSubmitRecords() != null) { - return cd.getSubmitRecords(); + // Maintain our own cache rather than using cd.getSubmitRecords(), + // since the latter may not have used the same values for + // fastEvalLabels/allowDraft/etc. + // TODO(dborowitz): Handle this better at the ChangeData level. + if (submitRecords == null) { + submitRecords = new HashMap<>(); } - cd.setSubmitRecords(new SubmitRuleEvaluator(cd) + List records = submitRecords.get(cd.getId()); + if (records == null) { + records = new SubmitRuleEvaluator(cd) .setFastEvalLabels(true) .setAllowDraft(true) - .evaluate()); - return cd.getSubmitRecords(); + .evaluate(); + submitRecords.put(cd.getId(), records); + } + return records; } private Map labelsFor(ChangeControl ctl,