From bf313bbbba31a777a8e268008c6f1f29a20ec8b6 Mon Sep 17 00:00:00 2001 From: Oleg Aravin Date: Mon, 24 Oct 2016 12:28:56 -0700 Subject: [PATCH] Add ApprovalInfo.permittedVotingRange and updated reviewers tooltip 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 --- Documentation/rest-api-changes.txt | 25 +++++-- .../acceptance/api/change/ChangeIT.java | 62 +++++++++++++++++ .../extensions/common/ApprovalInfo.java | 1 + .../extensions/common/VotingRangeInfo.java | 25 +++++++ .../google/gerrit/client/info/ChangeInfo.java | 10 +++ .../gerrit/client/change/Reviewers.java | 6 +- .../gerrit/server/change/ChangeJson.java | 67 ++++++++++++++++--- .../server/extensions/events/EventUtil.java | 2 +- 8 files changed, 181 insertions(+), 17 deletions(-) create mode 100644 gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/VotingRangeInfo.java diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 0f643c91db..fb1a49ff7c 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -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. diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index be05ea69e3..c52989d093 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -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 getReviewers( Collection r) { return Iterables.transform(r, a -> new Account.Id(a._accountId)); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ApprovalInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ApprovalInfo.java index d59e813574..9125bfd69c 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ApprovalInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ApprovalInfo.java @@ -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); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/VotingRangeInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/VotingRangeInfo.java new file mode 100644 index 0000000000..5c35a49e36 --- /dev/null +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/VotingRangeInfo.java @@ -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; + } +} diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java index ff060b8051..054bfdd33e 100644 --- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java +++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/ChangeInfo.java @@ -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; }-*/; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java index e0c252c029..6b4cf36228 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java @@ -255,6 +255,7 @@ public class Reviewers extends Composite { Map 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); } } 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 d480df9860..d924b33331 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 @@ -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 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 pvr = + getPermittedVotingRanges(permittedLabels(ctl, cd)); for (Map.Entry 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 getPermittedVotingRanges( + Map> permittedLabels) { + Map permittedVotingRanges = + Maps.newHashMapWithExpectedSize(permittedLabels.size()); + for (String label : permittedLabels.keySet()) { + List 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 s = cd.getSubmitApproval(); return s.isPresent() ? s.get().getGranted() : null; } - private Map labelsForClosedChange(ChangeData cd, - LabelTypes labelTypes, boolean standard, boolean detailed) + private Map labelsForClosedChange( + ChangeControl baseCtrl, ChangeData cd, LabelTypes labelTypes, + boolean standard, boolean detailed) throws OrmException { Set allUsers = new HashSet<>(); if (detailed) { @@ -831,10 +872,12 @@ public class ChangeJson { for (Account.Id accountId : allUsers) { Map byLabel = Maps.newHashMapWithExpectedSize(labels.size()); - + Map pvr = Collections.emptyMap(); if (detailed) { + ChangeControl ctl = baseCtrl.forUser(userFactory.create(accountId)); + pvr = getPermittedVotingRanges(permittedLabels(ctl, cd)); for (Map.Entry 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; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java index 5e4eb6f8d5..162a6b17ec 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java @@ -93,7 +93,7 @@ public class EventUtil { for (Map.Entry 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; }