From 0f697c84d05e449224aa9e883f19d84ecf6a0164 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 14 Feb 2013 12:58:28 -0800 Subject: [PATCH] Sort approvals in REST endpoints and approval table 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 --- .../gerrit/common/data/ApprovalDetail.java | 14 ++++++ .../gerrit/client/changes/ApprovalTable.java | 6 ++- .../gerrit/server/change/ChangeJson.java | 3 +- .../gerrit/server/change/LabelOrdering.java | 36 +++++++++++++++ .../gerrit/server/change/ReviewerJson.java | 6 +-- .../server/query/change/ChangeData.java | 45 +++++++++++++------ 6 files changed, 91 insertions(+), 19 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java index 86d70d4cad..d931742507 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java @@ -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 SORT = + new Comparator() { + public int compare(ApprovalDetail o1, ApprovalDetail o2) { + return o1.hasNonZero - o2.hasNonZero; + } + }; + protected Account.Id account; protected List approvals; protected boolean canRemove; @@ -33,6 +41,7 @@ public class ApprovalDetail { private transient Set approved; private transient Set rejected; private transient Map values; + private transient int hasNonZero; protected ApprovalDetail() { } @@ -59,6 +68,7 @@ public class ApprovalDetail { approved = new HashSet(); } approved.add(label); + hasNonZero = 1; } public void rejected(String label) { @@ -66,6 +76,7 @@ public class ApprovalDetail { rejected = new HashSet(); } rejected.add(label); + hasNonZero = 1; } public void votable(String label) { @@ -80,6 +91,9 @@ public class ApprovalDetail { values = new HashMap(); } values.put(label, value); + if (value != 0) { + hasNonZero = 1; + } } public boolean isApproved(String label) { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java index 1eb6b88a60..2c9b48bb7c 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java @@ -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 sorted = + new ArrayList(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); 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 b57b060a0e..4e9e330f6c 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 @@ -322,7 +322,8 @@ public class ChangeJson { return Collections.emptyMap(); } - Map labels = Maps.newLinkedHashMap(); + Map labels = + Maps.newTreeMap(LabelOrdering.create(approvalTypes)); initLabels(cd, labels, standard); Collection approvals = null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java new file mode 100644 index 0000000000..d7304ae6f7 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/LabelOrdering.java @@ -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 create(final ApprovalTypes approvalTypes) { + return Ordering.natural().nullsLast().onResultOf( + new Function() { + @Override + public Short apply(String n) { + ApprovalType at = approvalTypes.byLabel(n); + return at != null ? at.getCategory().getPosition() : null; + } + }).compound(Ordering.natural()); + } + + private LabelOrdering() { + } +} 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 72ec5c8805..d174ef563c 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 @@ -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()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 07222fa921..ba5faad390 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -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 SORT_APPROVALS = Ordering.natural() + .onResultOf(new Function() { + @Override + public Timestamp apply(PatchSetApproval a) { + return a.getGranted(); + } + }); + + public static List sortApprovals( + Iterable approvals) { + return SORT_APPROVALS.sortedCopy(approvals); + } + public static void ensureChangeLoaded( Provider db, List changes) throws OrmException { Map 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 limitedIds; private Collection patches; - private Multimap approvals; - private Collection currentApprovals; + private ListMultimap approvals; + private List currentApprovals; private String[] currentFiles; private Collection comments; private Collection trackingIds; @@ -254,7 +270,7 @@ public class ChangeData { return currentPatchSet; } - public Collection currentApprovals(Provider db) + public List currentApprovals(Provider 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 approvals(Provider db) + public List approvals(Provider 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 approvalsMap( + public ListMultimap approvalsMap( Provider 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); }