opendev/gerrit
Code Issues Proposed changes

Ensure ApprovalTable contains cells for all users/labels

Browse Source 
When a user is added to a review, we insert a dummy PatchSetApproval
in an arbitrary category without considering whether the user can vote
on that category. The call to labelNormalizer.normalize() in
ChangeJson (correctly) omits results where the user is not permitted
to vote. This meant if the user can't vote in the dummy category, we
were omitting the user row entirely.

Instead, ensure the "all" lists in the returned ChangeInfo describe a
dense approval table. Cells with value 0 are votable; cells with no
value are not votable. Even if a user cannot vote on *any* categories,
they should still show up in the table. Update ApprovalTable to know
how to handle null values.

Change-Id: I49d7d73b73aa2ac9590bd043fa68dc973e62c5a1
This commit is contained in:
Dave Borowitz
2013-03-22 14:20:33 -07:00
parent 08e5e85aee
commit a30db916df
5 changed files with 57 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
Documentation/rest-api-changes.txt
View File

@@ -1656,7 +1656,10 @@ In addition `ApprovalInfo` has the following fields:
[options="header",width="50%",cols="1,^1,5"]
|===========================
|Field Name ||Description
|`value` ||The vote that the user has given for the label.
|`value` |optional|
The vote that the user has given for the label. If present and zero, the
user is permitted to vote on the label. If absent, the user is not
permitted to vote on that label.
|===========================
[[change-info]]

15
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
View File

@@ -218,12 +218,15 @@ public class ApprovalTable extends Composite {
ad.setCanRemove(removableReviewers.contains(id));
byUser.put(id, ad);
}
ad.votable(name);
ad.value(name, ai.value());
if (formatValue(ai.value()).equals(max)) {
ad.approved(name);
} else if (formatValue(ai.value()).equals(min)) {
ad.rejected(name);
if (ai.has_value()) {
ad.votable(name);
ad.value(name, ai.value());
String fv = formatValue(ai.value());
if (fv.equals(max)) {
ad.approved(name);
} else if (fv.equals(min)) {
ad.rejected(name);
}
}
}
}

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java
View File

@@ -150,6 +150,7 @@ public class ChangeInfo extends JavaScriptObject {
}
public static class ApprovalInfo extends AccountInfo {
public final native boolean has_value() /*-{ return this.hasOwnProperty('value'); }-*/;
public final native short value() /*-{ return this.value; }-*/;
protected ApprovalInfo() {

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

@@ -25,7 +25,9 @@ import static com.google.gerrit.common.changes.ListChangesOption.DETAILED_LABELS
import static com.google.gerrit.common.changes.ListChangesOption.LABELS;
import com.google.common.base.Joiner;
import com.google.common.base.Objects;
import com.google.common.base.Strings;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.LinkedListMultimap;
@@ -34,6 +36,7 @@ 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.common.collect.Table;
import com.google.gerrit.common.changes.ListChangesOption;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
@@ -426,42 +429,45 @@ public class ChangeJson {
return;
}
// All users ever added, even if they can't vote on one or all labels.
Set<Account.Id> allUsers = Sets.newHashSet();
Multimap<Account.Id, PatchSetApproval> current = HashMultimap.create();
PatchSet.Id psId = baseCtrl.getChange().currentPatchSetId();
for (PatchSetApproval psa : labelNormalizer.normalize(
baseCtrl, cd.allApprovals(db))) {
ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals =
cd.allApprovalsMap(db);
for (PatchSetApproval psa : allApprovals.values()) {
allUsers.add(psa.getAccountId());
if (psa.getPatchSetId().equals(psId)) {
current.put(psa.getAccountId(), psa);
}
}
List<PatchSetApproval> currentList = labelNormalizer.normalize(
baseCtrl, allApprovals.get(baseCtrl.getChange().currentPatchSetId()));
// Most recent, normalized vote on each label for the current patch set by
// each user (may be 0).
Table<Account.Id, String, PatchSetApproval> current = HashBasedTable.create(
allUsers.size(), baseCtrl.getLabelTypes().getLabelTypes().size());
for (PatchSetApproval psa : currentList) {
current.put(psa.getAccountId(), psa.getLabel(), psa);
}
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()) {
LabelType lt = ctl.getLabelTypes().byLabel(name);
if (lt != null && labelNormalizer.canVote(ctl, lt, accountId)) {
ApprovalInfo ai = approvalInfo(accountId, 0);
byLabel.put(name, ai);
labels.get(name).addApproval(ai);
}
}
for (PatchSetApproval psa : current.get(accountId)) {
LabelType lt = ctl.getLabelTypes().byLabel(psa.getLabelId());
for (Map.Entry<String, LabelInfo> e : labels.entrySet()) {
LabelType lt = ctl.getLabelTypes().byLabel(e.getKey());
if (lt == null) {
// Ignore submit record for undefined label; likely the submit rule
// author didn't intend for the label to show up in the table.
continue;
}
ApprovalInfo info = byLabel.get(lt.getName());
if (info == null) {
continue;
Integer value;
PatchSetApproval psa = current.get(accountId, lt.getName());
if (psa != null) {
value = Integer.valueOf(psa.getValue());
} else {
// Either the user cannot vote on this label, or there just wasn't a
// dummy approval for this label. Explicitly check whether the user
// can vote on this label.
value = labelNormalizer.canVote(ctl, lt, accountId) ? 0 : null;
}
info.value = psa.getValue();
e.getValue().addApproval(approvalInfo(accountId, value));
}
}
}
@@ -519,7 +525,7 @@ public class ChangeJson {
short val = psa.getValue();
ApprovalInfo info = byLabel.get(type.getName());
if (info != null) {
info.value = val;
info.value = Integer.valueOf(val);
}
LabelInfo li = labels.get(type.getName());
@@ -544,7 +550,7 @@ public class ChangeJson {
return labels;
}
private ApprovalInfo approvalInfo(Account.Id id, int value) {
private ApprovalInfo approvalInfo(Account.Id id, Integer value) {
ApprovalInfo ai = new ApprovalInfo(id);
ai.value = value;
accountLoader.put(ai);
@@ -619,7 +625,7 @@ public class ChangeJson {
continue;
}
for (ApprovalInfo ai : label.all) {
if (ctl.canRemoveReviewer(ai._id, ai.value)) {
if (ctl.canRemoveReviewer(ai._id, Objects.firstNonNull(ai.value, 0))) {
removable.add(ai._id);
} else {
fixed.add(ai._id);
@@ -914,7 +920,7 @@ public class ChangeJson {
}
static class ApprovalInfo extends AccountInfo {
int value;
Integer value;
ApprovalInfo(Account.Id id) {
super(id);

9
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -388,7 +388,14 @@ public class ChangeData {
return ImmutableList.copyOf(allApprovalsMap(db).values());
}
private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovalsMap(
/**
* @param db review database.
* @return all patch set approvals for the change (regardless of whether
* {@link #limitToPatchSets(Collection)} was previously called), keyed by
* ID, ordered by timestamp within each patch set.
* @throws OrmException an error occurred reading the database.
*/
public ListMultimap<PatchSet.Id, PatchSetApproval> allApprovalsMap(
Provider<ReviewDb> db) throws OrmException {
if (allApprovals == null) {
allApprovals = ArrayListMultimap.create();