opendev/gerrit
Code Issues Proposed changes

Sort approvals in REST endpoints and approval table

Browse Source 
Document REST endpoints to return approvals within a given list by
timestamp, although the timestamp is not exposed. Sort labels by
corresponding ApprovalCategory position.

On the client side, sort users by the owner first, followed by users
with nonzero votes.

Change-Id: I68cd21e4a62b76bd7de2bf1dd1e18e8675dd34db
This commit is contained in:
Dave Borowitz 2013-02-14 12:58:28 -08:00
parent ed5364b937
commit 0f697c84d0
6 changed files with 91 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java
View File

@ -18,6 +18,7 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
@ -25,6 +26,13 @@ import java.util.Map;
import java.util.Set;
public class ApprovalDetail {
public static final Comparator<ApprovalDetail> SORT =
new Comparator<ApprovalDetail>() {
public int compare(ApprovalDetail o1, ApprovalDetail o2) {
return o1.hasNonZero - o2.hasNonZero;
}
};
protected Account.Id account;
protected List<PatchSetApproval> approvals;
protected boolean canRemove;
@ -33,6 +41,7 @@ public class ApprovalDetail {
private transient Set<String> approved;
private transient Set<String> rejected;
private transient Map<String, Integer> values;
private transient int hasNonZero;
protected ApprovalDetail() {
}
@ -59,6 +68,7 @@ public class ApprovalDetail {
approved = new HashSet<String>();
}
approved.add(label);
hasNonZero = 1;
}
public void rejected(String label) {
@ -66,6 +76,7 @@ public class ApprovalDetail {
rejected = new HashSet<String>();
}
rejected.add(label);
hasNonZero = 1;
}
public void votable(String label) {
@ -80,6 +91,9 @@ public class ApprovalDetail {
values = new HashMap<String, Integer>();
}
values.put(label, value);
if (value != 0) {
hasNonZero = 1;
}
}
public boolean isApproved(String label) {

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

@ -53,6 +53,7 @@ import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
@ -156,8 +157,11 @@ public class ApprovalTable extends Composite {
} else {
displayHeader(change.labels());
table.resizeRows(1 + byUser.size());
List<ApprovalDetail> sorted =
new ArrayList<ApprovalDetail>(byUser.values());
Collections.sort(sorted, ApprovalDetail.SORT);
int i = 1;
for (ApprovalDetail ad : byUser.values()) {
for (ApprovalDetail ad : sorted) {
displayRow(i++, ad, change, accounts.get(ad.getAccount().get()));
}
table.setVisible(true);

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

@ -322,7 +322,8 @@ public class ChangeJson {
return Collections.emptyMap();
}
Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
Map<String, LabelInfo> labels =
Maps.newTreeMap(LabelOrdering.create(approvalTypes));
initLabels(cd, labels, standard);
Collection<PatchSetApproval> approvals = null;

36
gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java Normal file
View File

@ -0,0 +1,36 @@
// Copyright (C) 2013 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.change;
import com.google.common.base.Function;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.data.ApprovalType;
import com.google.gerrit.common.data.ApprovalTypes;
class LabelOrdering {
public static Ordering<String> create(final ApprovalTypes approvalTypes) {
return Ordering.natural().nullsLast().onResultOf(
new Function<String, Short>() {
@Override
public Short apply(String n) {
ApprovalType at = approvalTypes.byLabel(n);
return at != null ? at.getCategory().getPosition() : null;
}
}).compound(Ordering.natural());
}
private LabelOrdering() {
}
}

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

@ -79,8 +79,8 @@ public class ReviewerJson {
PatchSet.Id psId = ctl.getChange().currentPatchSetId();
if (approvals == null) {
approvals = db.get().patchSetApprovals()
.byPatchSetUser(psId, out._id).toList();
approvals = ChangeData.sortApprovals(db.get().patchSetApprovals()
.byPatchSetUser(psId, out._id));
}
FunctionState fs = functionState.create(ctl, psId, approvals);
@ -88,7 +88,7 @@ public class ReviewerJson {
CategoryFunction.forCategory(at.getCategory()).run(at, fs);
}
out.approvals = Maps.newHashMapWithExpectedSize(approvals.size());
out.approvals = Maps.newTreeMap(LabelOrdering.create(approvalTypes));
for (PatchSetApproval ca : approvals) {
for (PermissionRange pr : ctl.getLabelRanges()) {
if (!pr.isEmpty()) {

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

@ -14,10 +14,12 @@
package com.google.gerrit.server.query.change;
import com.google.common.base.Function;
import com.google.common.collect.ArrayListMultimap;
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.Ordering;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Change;
@ -46,6 +48,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@ -55,6 +58,19 @@ import java.util.Map;
import java.util.Set;
public class ChangeData {
private static Ordering<PatchSetApproval> SORT_APPROVALS = Ordering.natural()
.onResultOf(new Function<PatchSetApproval, Timestamp>() {
@Override
public Timestamp apply(PatchSetApproval a) {
return a.getGranted();
}
});
public static List<PatchSetApproval> sortApprovals(
Iterable<PatchSetApproval> approvals) {
return SORT_APPROVALS.sortedCopy(approvals);
}
public static void ensureChangeLoaded(
Provider<ReviewDb> db, List<ChangeData> changes) throws OrmException {
Map<Change.Id, ChangeData> missing = Maps.newHashMap();
@ -102,7 +118,7 @@ public class ChangeData {
int idx = 0;
for (ChangeData cd : changes) {
if (cd.currentApprovals == null && cd.approvals == null) {
cd.currentApprovals = pending.get(idx++).toList();
cd.currentApprovals = sortApprovals(pending.get(idx++));
}
}
}
@ -114,8 +130,8 @@ public class ChangeData {
private PatchSet currentPatchSet;
private Set<PatchSet.Id> limitedIds;
private Collection<PatchSet> patches;
private Multimap<PatchSet.Id, PatchSetApproval> approvals;
private Collection<PatchSetApproval> currentApprovals;
private ListMultimap<PatchSet.Id, PatchSetApproval> approvals;
private List<PatchSetApproval> currentApprovals;
private String[] currentFiles;
private Collection<PatchLineComment> comments;
private Collection<TrackingId> trackingIds;
@ -254,7 +270,7 @@ public class ChangeData {
return currentPatchSet;
}
public Collection<PatchSetApproval> currentApprovals(Provider<ReviewDb> db)
public List<PatchSetApproval> currentApprovals(Provider<ReviewDb> db)
throws OrmException {
if (currentApprovals == null) {
Change c = change(db);
@ -264,8 +280,8 @@ public class ChangeData {
(limitedIds == null || limitedIds.contains(c.currentPatchSetId()))) {
return approvals.get(c.currentPatchSetId());
} else {
currentApprovals = db.get().patchSetApprovals()
.byPatchSet(c.currentPatchSetId()).toList();
currentApprovals = sortApprovals(db.get().patchSetApprovals()
.byPatchSet(c.currentPatchSetId()));
}
}
return currentApprovals;
@ -318,29 +334,30 @@ public class ChangeData {
/**
* @param db review database.
* @return patch set approvals for the change. If
* @return patch set approvals for the change in timestamp order. If
* {@link #limitToPatchSets(Collection)} was previously called, only contains
* approvals for the patches with the specified IDs.
* @throws OrmException an error occurred reading the database.
*/
public Collection<PatchSetApproval> approvals(Provider<ReviewDb> db)
public List<PatchSetApproval> approvals(Provider<ReviewDb> db)
throws OrmException {
return approvalsMap(db).values();
return sortApprovals(approvalsMap(db).values());
}
/**
* @param db review database.
* @return patch set approvals for the change, keyed by ID. If
* @return patch set approvals for the change, keyed by ID, ordered by
* timestamp within each patch set. If
* {@link #limitToPatchSets(Collection)} was previously called, only
* contains approvals for the patches with the specified IDs.
* @throws OrmException an error occurred reading the database.
*/
public Multimap<PatchSet.Id,PatchSetApproval> approvalsMap(
public ListMultimap<PatchSet.Id,PatchSetApproval> approvalsMap(
Provider<ReviewDb> db) throws OrmException {
if (approvals == null) {
approvals = ArrayListMultimap.create();
for (PatchSetApproval psa :
db.get().patchSetApprovals().byChange(legacyId)) {
for (PatchSetApproval psa : sortApprovals(
db.get().patchSetApprovals().byChange(legacyId))) {
if (limitedIds == null || limitedIds.contains(legacyId)) {
approvals.put(psa.getPatchSetId(), psa);
}