opendev/gerrit
Code Issues Proposed changes

Treat 0 as permitted but not voting in REST label info

Browse Source 
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
This commit is contained in:
Dave Borowitz 2013-02-14 09:25:01 -08:00 committed by Gerrit Code Review
parent 051e7313ba
commit 327b825bec
2 changed files with 98 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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<ReviewDb> 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<ReviewDb> 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<PatchSetApproval> approvals = null;
if (detailed) {
approvals = cd.currentApprovals(db);
setAllApprovals(labels, approvals);
setAllApprovals(cd, labels, approvals);
}
for (Map.Entry<String, LabelInfo> e : labels.entrySet()) {
ApprovalType type = approvalTypes.byLabel(e.getKey());
@ -404,25 +414,61 @@ public class ChangeJson {
return approvals;
}
private void setAllApprovals(Map<String, LabelInfo> labels,
Collection<PatchSetApproval> approvals) {
private void setAllApprovals(ChangeData cd,
Map<String, LabelInfo> labels,
Collection<PatchSetApproval> 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<Account.Id, String> 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<Account.Id, Collection<String>> ue
: existing.asMap().entrySet()) {
for (Map.Entry<String, LabelInfo> 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<String> 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 {

41
gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
View File

@ -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<ReviewDb> db;
private final IdentifiedUser.GenericFactory userFactory;
private final ApprovalTypes approvalTypes;
private final FunctionState.Factory functionState;
private final AccountInfo.Loader.Factory accountLoaderFactory;
@Inject
ReviewerJson(Provider<ReviewDb> 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<ReviewerInfo> format(Collection<ReviewerResource> rsrcs) throws OrmException {
public List<ReviewerInfo> format(Collection<ReviewerResource> rsrcs)
throws OrmException {
List<ReviewerInfo> 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.<ReviewerResource> of(rsrc));
}
public ReviewerInfo format(ReviewerInfo out, ChangeControl control,
public ReviewerInfo format(ReviewerInfo out, ChangeControl ctl,
List<PatchSetApproval> 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;
}