From 327b825bec0633cc50dacd45e619730e9d8986e9 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 14 Feb 2013 09:25:01 -0800 Subject: [PATCH] Treat 0 as permitted but not voting in REST label info A 0 value in the DB exists for only the default approval category, and regardless of permissions. But the ApprovalTable treats a vote with value 0 as an empty cell that is votable. Reconcile these in the REST handlers by always looking at all relevant categories and checking the relevant permissions. Change-Id: Ia65c9f8f5bd65eddbf001266d02f77de5f061e7c --- .../gerrit/server/change/ChangeJson.java | 77 ++++++++++++++++--- .../gerrit/server/change/ReviewerJson.java | 41 ++++++++-- 2 files changed, 98 insertions(+), 20 deletions(-) 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 8799b822d2..b57b060a0e 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 @@ -26,11 +26,13 @@ import static com.google.gerrit.common.changes.ListChangesOption.LABELS; import com.google.common.base.Joiner; import com.google.common.base.Strings; +import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Multimap; import com.google.common.collect.Sets; import com.google.gerrit.common.changes.ListChangesOption; import com.google.gerrit.common.data.ApprovalType; @@ -65,6 +67,8 @@ 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; @@ -109,8 +113,10 @@ public class ChangeJson { private final Provider db; private final ApprovalTypes approvalTypes; + private final FunctionState.Factory functionState; private final CurrentUser user; private final AnonymousUser anonymous; + private final IdentifiedUser.GenericFactory userFactory; private final ChangeControl.GenericFactory changeControlGenericFactory; private final PatchSetInfoFactory patchSetInfoFactory; private final PatchListCache patchListCache; @@ -127,9 +133,11 @@ public class ChangeJson { ChangeJson( Provider db, ApprovalTypes at, + FunctionState.Factory fs, CurrentUser u, AnonymousUser au, - ChangeControl.GenericFactory gf, + IdentifiedUser.GenericFactory uf, + ChangeControl.GenericFactory ccf, PatchSetInfoFactory psi, PatchListCache plc, AccountInfo.Loader.Factory ailf, @@ -137,9 +145,11 @@ public class ChangeJson { Urls urls) { this.db = db; this.approvalTypes = at; + this.functionState = fs; this.user = u; this.anonymous = au; - this.changeControlGenericFactory = gf; + this.userFactory = uf; + this.changeControlGenericFactory = ccf; this.patchSetInfoFactory = psi; this.patchListCache = plc; this.accountLoaderFactory = ailf; @@ -318,7 +328,7 @@ public class ChangeJson { Collection approvals = null; if (detailed) { approvals = cd.currentApprovals(db); - setAllApprovals(labels, approvals); + setAllApprovals(cd, labels, approvals); } for (Map.Entry e : labels.entrySet()) { ApprovalType type = approvalTypes.byLabel(e.getKey()); @@ -404,25 +414,61 @@ public class ChangeJson { return approvals; } - private void setAllApprovals(Map labels, - Collection approvals) { + private void setAllApprovals(ChangeData cd, + Map labels, + Collection approvals) throws OrmException { + ChangeControl ctl = cd.changeControl(); + FunctionState fs = + functionState.create(ctl, cd.change(db).currentPatchSetId(), approvals); + for (ApprovalType at : approvalTypes.getApprovalTypes()) { + CategoryFunction.forCategory(at.getCategory()).run(at, fs); + } + + Multimap existing = + HashMultimap.create(approvals.size(), labels.size()); for (PatchSetApproval psa : approvals) { ApprovalType at = approvalTypes.byId(psa.getCategoryId()); if (at == null) { continue; } - LabelInfo p = labels.get(at.getCategory().getLabelName()); + String name = at.getCategory().getLabelName(); + LabelInfo p = labels.get(name); if (p == null) { continue; // TODO: support arbitrary labels. } - if (p.all == null) { - p.all = Lists.newArrayList(); + if (!getRange(ctl, psa.getAccountId(), name).isEmpty()) { + p.addApproval(approvalInfo(psa.getAccountId(), psa.getValue())); } - ApprovalInfo ai = new ApprovalInfo(psa.getAccountId()); - accountLoader.put(ai); - ai.value = psa.getValue(); - p.all.add(ai); + existing.put(psa.getAccountId(), at.getCategory().getLabelName()); } + + // Add dummy approvals for all permitted labels for each user even if they + // do not exist in the DB. + for (Map.Entry> ue + : existing.asMap().entrySet()) { + for (Map.Entry le : labels.entrySet()) { + if (ue.getValue().contains(le.getKey())) { + continue; + } + LabelInfo p = le.getValue(); + if (!getRange(ctl, ue.getKey(), le.getKey()).isEmpty()) { + p.addApproval(approvalInfo(ue.getKey(), (short) 0)); + } + } + } + } + + private PermissionRange getRange(ChangeControl control, Account.Id user, + String label) { + return control.forUser(userFactory.create(user)) + .getRange(Permission.forLabel(label)); + } + + private ApprovalInfo approvalInfo(Account.Id id, short value) { + ApprovalInfo ai = new ApprovalInfo(id); + ai.value = value; + accountLoader.put(ai); + return ai; } private static boolean isOnlyZero(Collection values) { @@ -773,6 +819,13 @@ public class ChangeJson { Short value; Boolean optional; + + void addApproval(ApprovalInfo ai) { + if (all == null) { + all = Lists.newArrayList(); + } + all.add(ai); + } } static class ApprovalInfo extends AccountInfo { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java index e5fd78648f..23fdeadab1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java @@ -14,18 +14,23 @@ package com.google.gerrit.server.change; +import static com.google.gerrit.reviewdb.client.ApprovalCategoryValue.formatValue; + import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.gerrit.common.data.ApprovalType; import com.google.gerrit.common.data.ApprovalTypes; +import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRange; +import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.ApprovalCategoryValue; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.workflow.CategoryFunction; import com.google.gerrit.server.workflow.FunctionState; import com.google.gwtorm.server.OrmException; @@ -38,22 +43,26 @@ import java.util.Map; public class ReviewerJson { private final Provider db; + private final IdentifiedUser.GenericFactory userFactory; private final ApprovalTypes approvalTypes; private final FunctionState.Factory functionState; private final AccountInfo.Loader.Factory accountLoaderFactory; @Inject ReviewerJson(Provider db, + IdentifiedUser.GenericFactory userFactory, ApprovalTypes approvalTypes, FunctionState.Factory functionState, AccountInfo.Loader.Factory accountLoaderFactory) { this.db = db; + this.userFactory = userFactory; this.approvalTypes = approvalTypes; this.functionState = functionState; this.accountLoaderFactory = accountLoaderFactory; } - public List format(Collection rsrcs) throws OrmException { + public List format(Collection rsrcs) + throws OrmException { List infos = Lists.newArrayListWithCapacity(rsrcs.size()); AccountInfo.Loader loader = accountLoaderFactory.create(true); for (ReviewerResource rsrc : rsrcs) { @@ -69,33 +78,49 @@ public class ReviewerJson { return format(ImmutableList. of(rsrc)); } - public ReviewerInfo format(ReviewerInfo out, ChangeControl control, + public ReviewerInfo format(ReviewerInfo out, ChangeControl ctl, List approvals) throws OrmException { - PatchSet.Id psId = control.getChange().currentPatchSetId(); + PatchSet.Id psId = ctl.getChange().currentPatchSetId(); if (approvals == null) { approvals = db.get().patchSetApprovals() .byPatchSetUser(psId, out._id).toList(); } - FunctionState fs = functionState.create(control, psId, approvals); + FunctionState fs = functionState.create(ctl, psId, approvals); for (ApprovalType at : approvalTypes.getApprovalTypes()) { CategoryFunction.forCategory(at.getCategory()).run(at, fs); } out.approvals = Maps.newHashMapWithExpectedSize(approvals.size()); for (PatchSetApproval ca : approvals) { - for (PermissionRange pr : control.getLabelRanges()) { - if (pr.getMin() != 0 || pr.getMax() != 0) { + for (PermissionRange pr : ctl.getLabelRanges()) { + if (!pr.isEmpty()) { // TODO: Support arbitrary labels. ApprovalType at = approvalTypes.byId(ca.getCategoryId()); if (at != null) { out.approvals.put(at.getCategory().getLabelName(), - ApprovalCategoryValue.formatValue(ca.getValue())); + formatValue(ca.getValue())); } + } + } + } + + // Add dummy approvals for all permitted labels for the user even if they + // do not exist in the DB. + ChangeData cd = new ChangeData(ctl); + PatchSet ps = cd.currentPatchSet(db); + if (ps != null) { + for (SubmitRecord rec : ctl.canSubmit(db.get(), ps, cd, true, false)) { + for (SubmitRecord.Label label : rec.labels) { + String name = label.label; + if (!out.approvals.containsKey(name) + && !ctl.getRange(Permission.forLabel(name)).isEmpty()) { + out.approvals.put(name, formatValue((short) 0)); } } } } + if (out.approvals.isEmpty()) { out.approvals = null; }