ChangeJson: Fix approvals for draft changes

In If07a6e67 we started caching the submit records produced by MergeOp
back in the ChangeData. An unintended side effect of this was to reuse
those cached submit records in ChangeJson, since MergeOp is called
when setting the submittable bit. The SubmitRuleEvaluator in MergeOp
runs with allowDrafts = false, meaning no submit records would appear
later in ChangeJson when we run initLabels on draft changes. Another
side effect is making the reviewer list empty on the change screen,
which still uses the labels map rather than the reviewers map.

Fix this in a slightly hacky way by using a cache specific to
ChangeJson which guarantees that the right value of allowDrafts is
set.

Change-Id: Ifb95f8796388facb05cddb2870f1d6d649e25088

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<AccountInfo> 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");
   }

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<Change.Id, List<SubmitRecord>> submitRecords;
   private FixInput fix;
 
   @AssistedInject
@@ -485,14 +486,22 @@ public class ChangeJson {
   }
 
   private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException {
-    if (cd.getSubmitRecords() != null) {
+    // Maintain our own cache rather than using cd.getSubmitRecords(),
-      return 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<SubmitRecord> records = submitRecords.get(cd.getId());
+    if (records == null) {
+      records = new SubmitRuleEvaluator(cd)
         .setFastEvalLabels(true)
         .setAllowDraft(true)
-        .evaluate());
+        .evaluate();
-    return cd.getSubmitRecords();
+      submitRecords.put(cd.getId(), records);
+    }
+    return records;
   }
 
   private Map<String, LabelInfo> labelsFor(ChangeControl ctl,