Include reviewers that can't vote on any label

When a user is added as a reviewer on a change an arbitrary label
is chosen and a value of 0 is inserted into PatchSetApprovals. If
the user was not permitted to place any score on the change the
server dropped them from the label information, preventing display
in the ApprovalTable of the web UI.

This was missed up until now because most servers grant something
like Code-Review -1..+1 to Registered Users, so everyone can always
vote on a change. However if this grant was not given reviewers
did not appear to be added to the change.

The rewrite of the method is easier to follow. It now proceeds
through the table in a row-wise fashion, ensuring every column is
filled out. This is backwards from the way the JSON is organized,
which uses a column-first ordering. Column-first in this particular
function is difficult because the server must ensure every row
has all columns populated, and the column matrix is what is sparse
for a user. Column-first also requires rebuilding or caching the
ChangeControl for each reviewer, row-first permits this to be
trivially retained in a local variable.

Change-Id: I194c4805c042e0d0c5a21f47bc12ab0724306446

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -69,8 +69,6 @@ import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.ssh.SshInfo;
-import com.google.gerrit.server.workflow.CategoryFunction;
-import com.google.gerrit.server.workflow.FunctionState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -116,7 +114,6 @@ public class ChangeJson {
 
   private final Provider<ReviewDb> db;
   private final LabelTypes labelTypes;
-  private final FunctionState.Factory functionState;
   private final CurrentUser user;
   private final AnonymousUser anonymous;
   private final IdentifiedUser.GenericFactory userFactory;
@@ -136,7 +133,6 @@ public class ChangeJson {
   ChangeJson(
       Provider<ReviewDb> db,
       LabelTypes at,
-      FunctionState.Factory fs,
       CurrentUser u,
       AnonymousUser au,
       IdentifiedUser.GenericFactory uf,
@@ -148,7 +144,6 @@ public class ChangeJson {
       Urls urls) {
     this.db = db;
     this.labelTypes = at;
-    this.functionState = fs;
     this.user = u;
     this.anonymous = au;
     this.userFactory = uf;
@@ -424,50 +419,46 @@ public class ChangeJson {
 
   private void setAllApprovals(ChangeData cd,
       Map<String, LabelInfo> labels) throws OrmException {
-    cd.allApprovals(db);
-    ChangeControl ctl = control(cd);
-    if (ctl == null) {
+    ChangeControl baseCtrl = control(cd);
+    if (baseCtrl == null) {
       return;
     }
-    Collection<PatchSetApproval> approvals = cd.currentApprovals(db);
-    FunctionState fs =
-        functionState.create(ctl, cd.change(db).currentPatchSetId(), approvals);
-    for (LabelType lt : labelTypes.getLabelTypes()) {
-      CategoryFunction.forType(lt).run(lt, fs);
-    }
 
-    Multimap<Account.Id, String> existing =
-        HashMultimap.create(approvals.size(), labels.size());
-    for (PatchSetApproval psa : approvals) {
-      LabelType lt = labelTypes.byId(psa.getCategoryId().get());
-      if (lt == null) {
-        continue;
-      }
-      LabelInfo p = labels.get(lt.getName());
-      if (p == null) {
-        continue; // TODO: support arbitrary labels.
-      }
-      if (!getRange(ctl, psa.getAccountId(), lt.getName()).isEmpty()) {
-        p.addApproval(approvalInfo(psa.getAccountId(), psa.getValue()));
-      }
-      existing.put(psa.getAccountId(), lt.getName());
-    }
-
-    // Add dummy approvals for all permitted labels for each user even if they
-    // do not exist in the DB.
     Set<Account.Id> allUsers = Sets.newHashSet();
     for (PatchSetApproval psa : cd.allApprovals(db)) {
       allUsers.add(psa.getAccountId());
     }
-    for (Account.Id user : allUsers) {
-      for (Map.Entry<String, LabelInfo> le : labels.entrySet()) {
-        if (existing.containsEntry(user, le.getKey())) {
+
+    Multimap<Account.Id, PatchSetApproval> current = HashMultimap.create();
+    for (PatchSetApproval a : cd.currentApprovals(db)) {
+      current.put(a.getAccountId(), a);
+    }
+
+    for (Account.Id accountId : allUsers) {
+      IdentifiedUser user = userFactory.create(accountId);
+      ChangeControl ctl = baseCtrl.forUser(user);
+      Map<String, ApprovalInfo> byLabel =
+          Maps.newHashMapWithExpectedSize(labels.size());
+
+      for (String name : labels.keySet()) {
+        ApprovalInfo ai = approvalInfo(accountId, 0);
+        byLabel.put(name, ai);
+        labels.get(name).addApproval(ai);
+      }
+      for (PatchSetApproval psa : current.get(accountId)) {
+        // TODO Support arbitrary labels placed by a reviewer.
+        LabelType lt = labelTypes.byId(psa.getCategoryId().get());
+        if (lt == null) {
           continue;
         }
-        LabelInfo p = le.getValue();
-        if (!getRange(ctl, user, le.getKey()).isEmpty()) {
-          p.addApproval(approvalInfo(user, (short) 0));
+
+        ApprovalInfo info = byLabel.get(lt.getName());
+        if (info == null) {
+          continue;
         }
+
+        PermissionRange r = ctl.getRange(Permission.forLabel(lt.getName()));
+        info.value = r != null ? r.squash(psa.getValue()) : 0;
       }
     }
   }
@@ -520,12 +511,6 @@ public class ChangeJson {
     return labels;
   }
 
-  private PermissionRange getRange(ChangeControl control, Account.Id user,
-      String label) {
-    return control.forUser(userFactory.create(user))
-        .getRange(Permission.forLabel(label));
-  }
-
   private AccountInfo accountInfo(PatchSetApproval psa) {
     return accountLoader.get(psa.getAccountId());
   }