opendev/gerrit
Code Issues Proposed changes

Add a /changes option to include detailed label info

Browse Source 
The intent of this option is to be able to render the ApprovalTable
using the new REST API. As such, we need to display detailed approval
info for all relevant accounts, not just the output of the submit rule
evaluator. The submit rule evaluator only outputs at most one account
for approved/recommended/disliked/rejected, and omits the numeric
value.

Detailed label information is included in the "labels" field, which is
all per-change information. The permitted ranges for the current user,
which is of course per-user, is included in the top-level
"permitted_labels" field.

Example JSON output for a change:

  {
    "kind": "gerritcodereview#change",
    "id": "repo1~master~I190d6f46480fbc160561bea7e46d1c0905d75a80",
    "project": "repo1",
    "branch": "master",
    "change_id": "I190d6f46480fbc160561bea7e46d1c0905d75a80",
    "subject": "Add bar/baz",
    "status": "NEW",
    "created": "2013-01-29 21:37:03.870000000",
    "updated": "2013-02-06 00:17:26.751000000",
    "reviewed": true,
    "_sortkey": "0022ea3100000002",
    "_number": 2,
    "owner": {
      "_account_id": 1000000,
      "name": "Dave Borowitz",
      "email": "dborowitz@google.com"
    },
    "labels": {
      "Verified": {
        "all": [
          {
            "value": 1,
            "_account_id": 1000000,
            "name": "Dave Borowitz",
            "email": "dborowitz@google.com"
          }
        ],
        "values": {
          "-1": "Fails",
          " 0": "No score",
          "+1": "Verified"
        }
      },
      "Code-Review": {
        "all": [
          {
            "value": 1,
            "_account_id": 1000000,
            "name": "Dave Borowitz",
            "email": "dborowitz@google.com"
          },
          {
            "value": -1,
            "_account_id": 1000001,
            "name": "David Borowitz",
            "email": "ddborowitz@gmail.com"
          }
        ],
        "values": {
          "-2": "Do not submit",
          "-1": "I would prefer that you didn\u0027t submit this",
          " 0": "No score",
          "+1": "Looks good to me, but someone else must approve",
          "+2": "Looks good to me, approved"
        }
      }
    },
    "permitted_labels": {
      "Code-Review": [
        "-1",
        " 0",
        "+1"
      ]
    }
  }

Change-Id: I97e97fa0cb27cc3dbdc8496d5115dc353dafc9ae
This commit is contained in:
Dave Borowitz
2013-01-30 16:18:59 -08:00
parent a5d6eadbf2
commit 4c7231a910
5 changed files with 227 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -140,6 +140,10 @@ default. Optional fields are:
* `LABELS`: a summary of each label required for submit, and
approvers that have granted (or rejected) with that label.
* `DETAILED_LABELS`: detailed label information, including numeric
values of all existing approvals, recognized label values, and
values permitted to be set by the current user.
* `CURRENT_REVISION`: describe the current revision (patch set)
of the change, including the commit SHA-1 and URLs to fetch from.

1
gerrit-common/src/main/java/com/google/gerrit/common/changes/ListChangesOption.java
View File

@@ -19,6 +19,7 @@ import java.util.EnumSet;
/** Output options available when using {@code /changes/} RPCs. */
public enum ListChangesOption {
LABELS(0),
DETAILED_LABELS(8),
/** Return information on the current patch set of the change. */
CURRENT_REVISION(1),

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

@@ -14,17 +14,27 @@
package com.google.gerrit.client.changes;
import com.google.gerrit.client.rpc.NativeMap;
import com.google.gerrit.client.rpc.NativeString;
import com.google.gerrit.client.rpc.Natives;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.core.client.JsArray;
import com.google.gwt.core.client.JsArrayString;
import com.google.gwtjsonrpc.client.impl.ser.JavaSqlTimestamp_JsonSerializer;
import java.sql.Timestamp;
import java.util.Set;
public class ChangeInfo extends JavaScriptObject {
public final void init() {
if (labels0() != null) {
labels0().copyKeysIntoChildren("_name");
}
}
public final Project.NameKey project_name_key() {
return new Project.NameKey(project());
}
@@ -74,8 +84,17 @@ public class ChangeInfo extends JavaScriptObject {
public final native boolean starred() /*-{ return this.starred ? true : false; }-*/;
public final native boolean reviewed() /*-{ return this.reviewed ? true : false; }-*/;
public final native String _sortkey() /*-{ return this._sortkey; }-*/;
private final native JavaScriptObject labels0() /*-{ return this.labels; }-*/;
private final native NativeMap<LabelInfo> labels0() /*-{ return this.labels; }-*/;
public final native LabelInfo label(String n) /*-{ return this.labels[n]; }-*/;
private final native NativeMap<JavaScriptObject> _permitted_labels()
/*-{ return this.permitted_labels; }-*/;
public final Set<String> permitted_labels() {
return Natives.keys(_permitted_labels());
}
public final native JsArrayString permitted_values(String n)
/*-{ return this.permitted_labels[n]; }-*/;
final native int _number() /*-{ return this._number; }-*/;
final native boolean _more_changes()
/*-{ return this._more_changes ? true : false; }-*/;
@@ -109,6 +128,15 @@ public class ChangeInfo extends JavaScriptObject {
public final native AccountInfo recommended() /*-{ return this.recommended; }-*/;
public final native AccountInfo disliked() /*-{ return this.disliked; }-*/;
public final native JsArray<ApprovalInfo> all() /*-{ return this.all; }-*/;
private final native NativeMap<NativeString> _values() /*-{ return this.values; }-*/;
public final Set<String> values() {
return Natives.keys(_values());
}
public final native String value_text(String n) /*-{ return this.values[n]; }-*/;
public final native boolean optional() /*-{ return this.optional ? true : false; }-*/;
final native short _value()
/*-{
@@ -121,4 +149,14 @@ public class ChangeInfo extends JavaScriptObject {
protected LabelInfo() {
}
}
public static class ApprovalInfo extends JavaScriptObject {
public final native int _account_id() /*-{ return this._account_id || 0; }-*/;
public final native String name() /*-{ return this.name; }-*/;
public final native String email() /*-{ return this.email; }-*/;
public final native short value() /*-{ return this.value; }-*/;
protected ApprovalInfo() {
}
}
}

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

@@ -21,19 +21,25 @@ import static com.google.gerrit.common.changes.ListChangesOption.CURRENT_COMMIT;
import static com.google.gerrit.common.changes.ListChangesOption.CURRENT_FILES;
import static com.google.gerrit.common.changes.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.common.changes.ListChangesOption.DETAILED_ACCOUNTS;
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.Strings;
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.gerrit.common.changes.ListChangesOption;
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.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Patch;
@@ -219,7 +225,11 @@ public class ChangeJson {
out._sortkey = in.getSortKey();
out.starred = user.getStarredChanges().contains(in.getId()) ? true : null;
out.reviewed = in.getStatus().isOpen() && isChangeReviewed(cd) ? true : null;
out.labels = options.contains(LABELS) ? labelsFor(cd) : null;
out.labels = labelsFor(cd, options.contains(LABELS),
options.contains(DETAILED_LABELS));
if (options.contains(DETAILED_LABELS)) {
out.permitted_labels = permittedLabels(cd);
}
out.finish();
if (options.contains(ALL_REVISIONS) || options.contains(CURRENT_REVISION)) {
@@ -259,7 +269,24 @@ public class ChangeJson {
return ctrl;
}
private Map<String, LabelInfo> labelsFor(ChangeData cd) throws OrmException {
private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException {
if (cd.getSubmitRecords() != null) {
return cd.getSubmitRecords();
}
ChangeControl ctl = control(cd);
if (ctl == null) {
return ImmutableList.of();
}
PatchSet ps = cd.currentPatchSet(db);
if (ps == null) {
return ImmutableList.of();
}
cd.setSubmitRecords(ctl.canSubmit(db.get(), ps, cd, true, false));
return cd.getSubmitRecords();
}
private Map<String, LabelInfo> labelsFor(ChangeData cd, boolean standard,
boolean detailed) throws OrmException {
ChangeControl ctl = control(cd);
if (ctl == null) {
return Collections.emptyMap();
@@ -271,7 +298,31 @@ public class ChangeJson {
}
Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
for (SubmitRecord rec : ctl.canSubmit(db.get(), ps, cd, true, false)) {
initLabels(cd, labels, standard);
Collection<PatchSetApproval> approvals = null;
if (detailed) {
approvals = cd.currentApprovals(db);
setAllApprovals(labels, approvals);
}
for (Map.Entry<String, LabelInfo> e : labels.entrySet()) {
ApprovalType type = approvalTypes.byLabel(e.getKey());
if (type == null) {
continue; // TODO: Support arbitrary labels.
}
if (standard) {
approvals = setRecommendedAndDisliked(cd, approvals, type, e.getValue());
}
if (detailed) {
setLabelValues(type, e.getValue());
}
}
return labels;
}
private void initLabels(ChangeData cd, Map<String, LabelInfo> labels,
boolean standard) throws OrmException {
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels == null) {
continue;
}
@@ -280,6 +331,7 @@ public class ChangeJson {
if (p == null || p._status.compareTo(r.status) < 0) {
LabelInfo n = new LabelInfo();
n._status = r.status;
if (standard) {
switch (r.status) {
case OK:
n.approved = accountLoader.get(r.appliedBy);
@@ -290,29 +342,32 @@ public class ChangeJson {
default:
break;
}
}
n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
labels.put(r.label, n);
}
}
}
Collection<PatchSetApproval> approvals = null;
for (Map.Entry<String, LabelInfo> e : labels.entrySet()) {
if (e.getValue().approved != null || e.getValue().rejected != null) {
continue;
}
ApprovalType type = approvalTypes.byLabel(e.getKey());
if (type == null || type.getMin() == null || type.getMax() == null) {
private Collection<PatchSetApproval> setRecommendedAndDisliked(ChangeData cd,
Collection<PatchSetApproval> approvals, ApprovalType type,
LabelInfo label) throws OrmException {
if (label.approved != null || label.rejected != null) {
return approvals;
}
if (type.getMin() == null || type.getMax() == null) {
// Unknown or misconfigured type can't have intermediate scores.
continue;
return approvals;
}
short min = type.getMin().getValue();
short max = type.getMax().getValue();
if (-1 <= min && max <= 1) {
// Types with a range of -1..+1 can't have intermediate scores.
continue;
return approvals;
}
if (approvals == null) {
@@ -323,16 +378,80 @@ public class ChangeJson {
if (val != 0 && min < val && val < max
&& psa.getCategoryId().equals(type.getCategory().getId())) {
if (0 < val) {
e.getValue().recommended = accountLoader.get(psa.getAccountId());
e.getValue().value = val != 1 ? val : null;
label.recommended = accountLoader.get(psa.getAccountId());
label.value = val != 1 ? val : null;
} else {
e.getValue().disliked = accountLoader.get(psa.getAccountId());
e.getValue().value = val != -1 ? val : null;
label.disliked = accountLoader.get(psa.getAccountId());
label.value = val != -1 ? val : null;
}
}
}
return approvals;
}
private void setAllApprovals(Map<String, LabelInfo> labels,
Collection<PatchSetApproval> approvals) {
for (PatchSetApproval psa : approvals) {
ApprovalType at = approvalTypes.byId(psa.getCategoryId());
if (at == null) {
continue;
}
LabelInfo p = labels.get(at.getCategory().getLabelName());
if (p == null) {
continue; // TODO: support arbitrary labels.
}
if (p.all == null) {
p.all = Lists.newArrayList();
}
ApprovalInfo ai = new ApprovalInfo(psa.getAccountId());
accountLoader.put(ai);
ai.value = psa.getValue();
p.all.add(ai);
}
}
private static boolean isOnlyZero(Collection<String> values) {
return values.isEmpty() || (values.size() == 1 && values.contains(" 0"));
}
private void setLabelValues(ApprovalType type, LabelInfo label) {
label.values = Maps.newLinkedHashMap();
for (ApprovalCategoryValue acv : type.getValues()) {
label.values.put(acv.formatValue(), acv.getName());
}
if (isOnlyZero(label.values.keySet())) {
label.values = null;
}
}
private Map<String, Collection<String>> permittedLabels(ChangeData cd)
throws OrmException {
ChangeControl ctl = control(cd);
ListMultimap<String, String> permitted = LinkedListMultimap.create();
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels == null) {
continue;
}
for (SubmitRecord.Label r : rec.labels) {
ApprovalType type = approvalTypes.byLabel(r.label);
if (type == null) {
continue; // TODO: Support arbitrary labels.
}
PermissionRange range = ctl.getRange(Permission.forLabel(r.label));
for (ApprovalCategoryValue acv : type.getValues()) {
if (range.contains(acv.getValue())) {
permitted.put(r.label, acv.formatValue());
}
}
}
}
return labels;
for (Collection<String> values : permitted.asMap().values()) {
if (isOnlyZero(values)) {
values.clear();
}
}
return permitted.asMap();
}
private boolean isChangeReviewed(ChangeData cd) throws OrmException {
@@ -531,6 +650,7 @@ public class ChangeJson {
AccountInfo owner;
Map<String, LabelInfo> labels;
Map<String, Collection<String>> permitted_labels;
String current_revision;
Map<String, RevisionInfo> revisions;
public Boolean _moreChanges;
@@ -588,12 +708,24 @@ public class ChangeJson {
static class LabelInfo {
transient SubmitRecord.Label.Status _status;
AccountInfo approved;
AccountInfo rejected;
AccountInfo recommended;
AccountInfo disliked;
List<ApprovalInfo> all;
Map<String, String> values;
Short value;
Boolean optional;
}
static class ApprovalInfo extends AccountInfo {
short value;
ApprovalInfo(Account.Id id) {
super(id);
}
}
}

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.query.change;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Patch;
@@ -117,6 +118,7 @@ public class ChangeData {
private CurrentUser visibleTo;
private ChangeControl changeControl;
private List<ChangeMessage> messages;
private List<SubmitRecord> submitRecords;
public ChangeData(final Change.Id id) {
legacyId = id;
@@ -332,4 +334,12 @@ public class ChangeData {
}
return messages;
}
public void setSubmitRecords(List<SubmitRecord> records) {
submitRecords = records;
}
public List<SubmitRecord> getSubmitRecords() {
return submitRecords;
}
}