opendev/gerrit
Code Issues Proposed changes

Merge "Add ApprovalInfo.permittedVotingRange and updated reviewers tooltip"

Browse Source
This commit is contained in:
ekempin 2016-12-02 09:47:40 +00:00 committed by Gerrit Code Review
commit 422b453fb5
8 changed files with 181 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -4829,14 +4829,19 @@ In addition `ApprovalInfo` has the following fields:
[options="header",cols="1,^1,5"]
|===========================
|Field Name ||Description
|`value` |optional|
|Field Name ||Description
|`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.
|`date` |optional|
|`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|
|`tag` |optional|
Value of the `tag` field from link:#review-input[ReviewInput] set
while posting the review.
NOTE: To apply different tags on on different votes/comments multiple
@ -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;
}