opendev/gerrit
Code Issues Proposed changes

Add ApprovalInfo.permittedVotingRange and updated reviewers tooltip

Browse Source 
Add new API object VotingRangeInfo to represent continuous voting range
from min to max values.

Add VotingRangeInfo as field to ApprovalInfo, the new
field represents the range of values the approver is permitted
to vote on the label.

Updated reviewers UI tooltip to indicate the reviewers
who have maximum vote permission on the label. For example
the tooltip will show "Votable: Code-Review (+2)" if the
reviewer is permitted to vote with maximum range value for
Code-Review label. If the reviewer isn't permitted to vote
with maximum range value, but still votable for Code-Review
label the "Votable: Code-Review" will be shown.

This change will allow authors to easily verify (by API and UI)
reviewers with +2 Code-Review permission assigned for the change.

Change-Id: I47e692b82691b87eefbc2143e10b99bcf49c1d29
This commit is contained in:
Oleg Aravin
2016-10-24 12:28:56 -07:00
committed by Edwin Kempin
parent d1091dfc81
commit bf313bbbba
8 changed files with 181 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -4834,6 +4834,11 @@ In addition `ApprovalInfo` has the following fields:
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.
|`permitted_voting_range` |optional|
The link:#voting-range-info[VotingRangeInfo] the user is authorized to vote
on that label. If present, the user is permitted to vote on the label
regarding the range values. If absent, the user is not permitted to vote
on that label.
|`date` |optional|
The time and date describing when the approval was made.
|`tag` |optional|
@@ -6118,6 +6123,18 @@ The `TopicInput` entity contains information for setting a topic.
The topic will be deleted if not set.
|===========================
[[voting-range-info]]
=== VotingRangeInfo
The `VotingRangeInfo` entity describes the continuous voting range from min
to max values.
[options="header",cols="1,6"]
|======================
|Field Name|Description
|`min` |The minimum voting value.
|`max` |The maximum voting value.
|======================
[[web-link-info]]
=== WebLinkInfo
The `WebLinkInfo` entity describes a link to an external site.

62
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -2550,6 +2550,68 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(change.permittedLabels).isEmpty();
}
@Test
public void maxPermittedValueAllowed() throws Exception {
final int minPermittedValue = -2;
final int maxPermittedValue = +2;
String heads = "refs/heads/*";
PushOneCommit.Result r = createChange();
String triplet = project.get() + "~master~" + r.getChangeId();
gApi.changes().id(triplet).addReviewer(user.username);
ChangeInfo c = gApi.changes()
.id(triplet)
.get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
LabelInfo codeReview = c.labels.get("Code-Review");
assertThat(codeReview.all).hasSize(1);
ApprovalInfo approval = codeReview.all.get(0);
assertThat(approval._accountId).isEqualTo(user.id.get());
assertThat(approval.permittedVotingRange).isNotNull();
// default values
assertThat(approval.permittedVotingRange.min).isEqualTo(-1);
assertThat(approval.permittedVotingRange.max).isEqualTo(1);
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
Util.allow(cfg,
Permission.forLabel("Code-Review"), minPermittedValue, maxPermittedValue,
REGISTERED_USERS, heads);
saveProjectConfig(project, cfg);
c = gApi.changes()
.id(triplet)
.get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
codeReview = c.labels.get("Code-Review");
assertThat(codeReview.all).hasSize(1);
approval = codeReview.all.get(0);
assertThat(approval._accountId).isEqualTo(user.id.get());
assertThat(approval.permittedVotingRange).isNotNull();
assertThat(approval.permittedVotingRange.min).isEqualTo(minPermittedValue);
assertThat(approval.permittedVotingRange.max).isEqualTo(maxPermittedValue);
}
@Test
public void maxPermittedValueBlocked() throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
blockLabel(cfg, "Code-Review", REGISTERED_USERS, "refs/heads/*");
saveProjectConfig(project, cfg);
PushOneCommit.Result r = createChange();
String triplet = project.get() + "~master~" + r.getChangeId();
gApi.changes().id(triplet).addReviewer(user.username);
ChangeInfo c = gApi.changes()
.id(triplet)
.get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
LabelInfo codeReview = c.labels.get("Code-Review");
assertThat(codeReview.all).hasSize(1);
ApprovalInfo approval = codeReview.all.get(0);
assertThat(approval._accountId).isEqualTo(user.id.get());
assertThat(approval.permittedVotingRange).isNull();
}
private static Iterable<Account.Id> getReviewers(
Collection<AccountInfo> r) {
return Iterables.transform(r, a -> new Account.Id(a._accountId));

1
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ApprovalInfo.java
View File

@@ -21,6 +21,7 @@ public class ApprovalInfo extends AccountInfo {
public Integer value;
public Timestamp date;
public Boolean postSubmit;
public VotingRangeInfo permittedVotingRange;
public ApprovalInfo(Integer id) {
super(id);

25
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/VotingRangeInfo.java Normal file
View File

@@ -0,0 +1,25 @@
// Copyright (C) 2016 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.extensions.common;
public class VotingRangeInfo {
public int min;
public int max;
public VotingRangeInfo(int min, int max) {
this.min = min;
this.max = max;
}
}

10
gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java
View File

@@ -305,10 +305,20 @@ public class ChangeInfo extends JavaScriptObject {
public final native boolean hasValue() /*-{ return this.hasOwnProperty('value'); }-*/;
public final native short value() /*-{ return this.value || 0; }-*/;
public final native VotingRangeInfo permittedVotingRange() /*-{ return this.permitted_voting_range; }-*/;
protected ApprovalInfo() {
}
}
public static class VotingRangeInfo extends AccountInfo {
public final native short min() /*-{ return this.min || 0; }-*/;
public final native short max() /*-{ return this.max || 0; }-*/;
protected VotingRangeInfo() {
}
}
public static class EditInfo extends JavaScriptObject {
public final native String name() /*-{ return this.name; }-*/;
public final native String setName(String n) /*-{ this.name = n; }-*/;

6
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java
View File

@@ -255,6 +255,7 @@ public class Reviewers extends Composite {
Map<Integer, VotableInfo> d = new HashMap<>();
for (String name : change.labels()) {
LabelInfo label = change.label(name);
short labelMaxValue = LabelInfo.parseValue(label.maxValue());
if (label.all() != null) {
for (ApprovalInfo ai : Natives.asList(label.all())) {
int id = ai._accountId();
@@ -263,7 +264,10 @@ public class Reviewers extends Composite {
ad = new VotableInfo();
d.put(id, ad);
}
if (ai.hasValue()) {
if (ai.permittedVotingRange() != null
&& ai.permittedVotingRange().max() == labelMaxValue) {
ad.votable(name + " (" + label.maxValue() + ") ");
} else if (ai.hasValue()) {
ad.votable(name);
}
}

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

@@ -45,6 +45,7 @@ import com.google.common.base.Throwables;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.LinkedHashMultimap;
@@ -54,6 +55,7 @@ import com.google.common.collect.Multimap;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.common.collect.Table;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
@@ -75,6 +77,7 @@ import com.google.gerrit.extensions.common.ProblemInfo;
import com.google.gerrit.extensions.common.PushCertificateInfo;
import com.google.gerrit.extensions.common.ReviewerUpdateInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.common.VotingRangeInfo;
import com.google.gerrit.extensions.common.WebLinkInfo;
import com.google.gerrit.extensions.config.DownloadCommand;
import com.google.gerrit.extensions.config.DownloadScheme;
@@ -602,7 +605,7 @@ public class ChangeJson {
LabelTypes labelTypes = ctl.getLabelTypes();
Map<String, LabelWithStatus> withStatus = cd.change().getStatus().isOpen()
? labelsForOpenChange(ctl, cd, labelTypes, standard, detailed)
: labelsForClosedChange(cd, labelTypes, standard, detailed);
: labelsForClosedChange(ctl, cd, labelTypes, standard, detailed);
return ImmutableMap.copyOf(
Maps.transformValues(withStatus, LabelWithStatus::label));
}
@@ -722,6 +725,8 @@ public class ChangeJson {
for (Account.Id accountId : allUsers) {
IdentifiedUser user = userFactory.create(accountId);
ChangeControl ctl = baseCtrl.forUser(user);
Map<String, VotingRangeInfo> pvr =
getPermittedVotingRanges(permittedLabels(ctl, cd));
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
LabelType lt = ctl.getLabelTypes().byLabel(e.getKey());
if (lt == null) {
@@ -730,6 +735,8 @@ public class ChangeJson {
continue;
}
Integer value;
VotingRangeInfo permittedVotingRange =
pvr.getOrDefault(lt.getName(), null);
String tag = null;
Timestamp date = null;
PatchSetApproval psa = current.get(accountId, lt.getName());
@@ -753,19 +760,53 @@ public class ChangeJson {
value = labelNormalizer.canVote(ctl, lt, accountId) ? 0 : null;
}
addApproval(e.getValue().label(),
approvalInfo(accountId, value, tag, date));
approvalInfo(accountId, value, permittedVotingRange, tag, date));
}
}
}
private Map<String, VotingRangeInfo> getPermittedVotingRanges(
Map<String, Collection<String>> permittedLabels) {
Map<String, VotingRangeInfo> permittedVotingRanges =
Maps.newHashMapWithExpectedSize(permittedLabels.size());
for (String label : permittedLabels.keySet()) {
List<Integer> permittedVotingRange = permittedLabels.get(label)
.stream()
.map(this::parseRangeValue)
.filter(java.util.Objects::nonNull)
.sorted()
.collect(toList());
if (permittedVotingRange.isEmpty()) {
permittedVotingRanges.put(label, null);
} else {
int minPermittedValue = permittedVotingRange.get(0);
int maxPermittedValue = Iterables.getLast(permittedVotingRange);
permittedVotingRanges.put(label,
new VotingRangeInfo(minPermittedValue, maxPermittedValue));
}
}
return permittedVotingRanges;
}
private Integer parseRangeValue(String value) {
if (value.startsWith("+")) {
value = value.substring(1);
} else if (value.startsWith(" ")) {
value = value.trim();
}
return Ints.tryParse(value);
}
private Timestamp getSubmittedOn(ChangeData cd)
throws OrmException {
Optional<PatchSetApproval> s = cd.getSubmitApproval();
return s.isPresent() ? s.get().getGranted() : null;
}
private Map<String, LabelWithStatus> labelsForClosedChange(ChangeData cd,
LabelTypes labelTypes, boolean standard, boolean detailed)
private Map<String, LabelWithStatus> labelsForClosedChange(
ChangeControl baseCtrl, ChangeData cd, LabelTypes labelTypes,
boolean standard, boolean detailed)
throws OrmException {
Set<Account.Id> allUsers = new HashSet<>();
if (detailed) {
@@ -831,10 +872,12 @@ public class ChangeJson {
for (Account.Id accountId : allUsers) {
Map<String, ApprovalInfo> byLabel =
Maps.newHashMapWithExpectedSize(labels.size());
Map<String, VotingRangeInfo> pvr = Collections.emptyMap();
if (detailed) {
ChangeControl ctl = baseCtrl.forUser(userFactory.create(accountId));
pvr = getPermittedVotingRanges(permittedLabels(ctl, cd));
for (Map.Entry<String, LabelWithStatus> entry : labels.entrySet()) {
ApprovalInfo ai = approvalInfo(accountId, 0, null, null);
ApprovalInfo ai = approvalInfo(accountId, 0, null, null, null);
byLabel.put(entry.getKey(), ai);
addApproval(entry.getValue().label(), ai);
}
@@ -849,6 +892,7 @@ public class ChangeJson {
ApprovalInfo info = byLabel.get(type.getName());
if (info != null) {
info.value = Integer.valueOf(val);
info.permittedVotingRange = pvr.getOrDefault(type.getName(), null);
info.date = psa.getGranted();
info.tag = psa.getTag();
if (psa.isPostSubmit()) {
@@ -865,17 +909,18 @@ public class ChangeJson {
return labels;
}
private ApprovalInfo approvalInfo(Account.Id id, Integer value, String tag,
Timestamp date) {
ApprovalInfo ai = getApprovalInfo(id, value, tag, date);
private ApprovalInfo approvalInfo(Account.Id id, Integer value,
VotingRangeInfo permittedVotingRange, String tag, Timestamp date) {
ApprovalInfo ai = getApprovalInfo(id, value, permittedVotingRange, tag, date);
accountLoader.put(ai);
return ai;
}
public static ApprovalInfo getApprovalInfo(
Account.Id id, Integer value, String tag, Timestamp date) {
public static ApprovalInfo getApprovalInfo(Account.Id id, Integer value,
VotingRangeInfo permittedVotingRange, String tag, Timestamp date) {
ApprovalInfo ai = new ApprovalInfo(id.get());
ai.value = value;
ai.permittedVotingRange = permittedVotingRange;
ai.date = date;
ai.tag = tag;
return ai;

2
gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java
View File

@@ -93,7 +93,7 @@ public class EventUtil {
for (Map.Entry<String, Short> e : approvals.entrySet()) {
Integer value = e.getValue() != null ? Integer.valueOf(e.getValue()) : null;
result.put(e.getKey(),
ChangeJson.getApprovalInfo(a.getId(), value, null, ts));
ChangeJson.getApprovalInfo(a.getId(), value, null, null, ts));
}
return result;
}