From a19b319af18e912d85964b7b989a84b1b078169b Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Tue, 13 May 2014 16:19:19 -0700
Subject: [PATCH] List reviewers with dummy approvals on closed changes

Teams that allow submit bypassing review in some form or other may
want to add reviewers to a change after it has been submitted. This
was allowed, but the result was not being returned by ChangeJson in
the closed change case.

Change-Id: Ibfd3728993d9eacfe0e0002275d9a956d6e79024
---
 .../acceptance/api/change/ChangeIT.java       | 21 +++++++++++++++++++
 .../gerrit/server/change/ChangeJson.java      |  9 ++++----
 2 files changed, 26 insertions(+), 4 deletions(-)

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 f6d62abedf..d656e1e83f 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
@@ -129,6 +129,27 @@ public class ChangeIT extends AbstractDaemonTest {
     assertEquals(ImmutableSet.of(user.id), getReviewers(cApi.get()));
   }
 
+  @Test
+  public void addReviewerToClosedChange() throws GitAPIException,
+      IOException, RestApiException {
+    PushOneCommit.Result r = createChange();
+    ChangeApi cApi = gApi.changes().id("p~master~" + r.getChangeId());
+    cApi.revision(r.getCommit().name())
+        .review(ReviewInput.approve());
+    cApi.revision(r.getCommit().name())
+        .submit();
+
+    assertEquals(ImmutableSet.of(admin.getId()), getReviewers(cApi.get()));
+
+    AddReviewerInput in = new AddReviewerInput();
+    in.reviewer = user.email;
+    gApi.changes()
+        .id("p~master~" + r.getChangeId())
+        .addReviewer(in);
+    assertEquals(ImmutableSet.of(admin.getId(), user.id),
+        getReviewers(cApi.get()));
+  }
+
   @Test
   public void createEmptyChange() throws RestApiException {
     ChangeInfo in = new ChangeInfo();
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 975fbb1518..13a33c9700 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
@@ -565,19 +565,20 @@ public class ChangeJson {
       allUsers.add(psa.getAccountId());
     }
 
+    // We can only approximately reconstruct what the submit rule evaluator
+    // would have done. These should really come from a stored submit record.
     Set<String> labelNames = Sets.newHashSet();
     Multimap<Account.Id, PatchSetApproval> current = HashMultimap.create();
     for (PatchSetApproval a : cd.currentApprovals()) {
       LabelType type = labelTypes.byLabel(a.getLabelId());
-      if (type != null && a.getValue() != 0) {
+      if (type != null) {
         labelNames.add(type.getName());
+        // Not worth the effort to distinguish between votable/non-votable for 0
+        // values on closed changes, since they can't vote anyway.
         current.put(a.getAccountId(), a);
       }
     }
 
-    // We can only approximately reconstruct what the submit rule evaluator
-    // would have done. These should really come from a stored submit record.
-    //
     // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
     Map<String, LabelInfo> labels = new TreeMap<>(labelTypes.nameComparator());
     for (String name : labelNames) {