From 33c13a444eb58321a5a96184fe3ae5427fa8142d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 10 Jan 2020 11:38:06 +0100 Subject: [PATCH] Label config: Allow to configure individual votes as sticky At the moment one can only configure min/max (-2/+2) or all (-2 .. +2) votes as sticky, but sometimes it's e.g. wanted that all negative (-2, -1) / all positive (+1, +2) votes are sticky. Instead of adding sepcific copy rules for this (e.g. copyNegativeScore and copyPositiveScore) allow to configure indiviual votes as sticky. This is more flexible and can cover all possible scenarios. E.g. we could use the new copy rule to make positive votes in the homepage repo sticky which e.g. helps with visualising consent on design docs (see issue 12025). Bug: Issue 12025 Signed-off-by: Edwin Kempin Change-Id: Ia8d1b634bc76123db9fc1ec0667e17c3e9e27204 --- Documentation/config-labels.txt | 7 ++++ Documentation/rest-api-projects.txt | 4 ++ .../google/gerrit/common/data/LabelType.java | 14 +++++++ .../common/LabelDefinitionInfo.java | 1 + .../common/LabelDefinitionInput.java | 1 + .../gerrit/server/ApprovalInference.java | 2 + .../server/project/LabelDefinitionJson.java | 1 + .../gerrit/server/project/ProjectConfig.java | 27 +++++++++++++ .../server/restapi/project/CreateLabel.java | 4 ++ .../server/restapi/project/SetLabel.java | 5 +++ .../api/change/StickyApprovalsIT.java | 29 ++++++++++++++ .../rest/project/CreateLabelIT.java | 12 ++++++ .../acceptance/rest/project/GetLabelIT.java | 3 ++ .../acceptance/rest/project/LabelAssert.java | 1 + .../acceptance/rest/project/ListLabelsIT.java | 3 ++ .../acceptance/rest/project/SetLabelIT.java | 38 +++++++++++++++++++ 16 files changed, 152 insertions(+) diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index 193a96f6b7..a3b9d0b7ec 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -344,6 +344,13 @@ Code-Review labels. Defaults to true. +[[label_copyValue]] +=== `label.Label-Name.copyValue` + +Value that should be copied forward when a new patch set is uploaded. +This can be used to enable sticky votes. Can be specified multiple +times. By default not set. + [[label_canOverride]] === `label.Label-Name.canOverride` diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt index 6fbb338009..74515e6fd2 100644 --- a/Documentation/rest-api-projects.txt +++ b/Documentation/rest-api-projects.txt @@ -3917,6 +3917,8 @@ copyAllScoresOnTrivialRebase] is set on the label. |`copy_all_scores_on_merge_first_parent_update`|`false` if not set| Whether link:config-labels.html#label_copyAllScoresOnMergeFirstParentUpdate[ copyAllScoresOnMergeFirstParentUpdate] is set on the label. +|`copy_values` |optional| +List of values that should be copied forward when a new patch set is uploaded. |`allow_post_submit`|`false` if not set| Whether link:config-labels.html#label_allowPostSubmit[allowPostSubmit] is set on the label. @@ -3982,6 +3984,8 @@ copyAllScoresOnTrivialRebase] is set on the label. |`copy_all_scores_on_merge_first_parent_update`|optional| Whether link:config-labels.html#label_copyAllScoresOnMergeFirstParentUpdate[ copyAllScoresOnMergeFirstParentUpdate] is set on the label. +|`copy_values` |optional| +List of values that should be copied forward when a new patch set is uploaded. |`allow_post_submit`|optional| Whether link:config-labels.html#label_allowPostSubmit[allowPostSubmit] is set on the label. diff --git a/java/com/google/gerrit/common/data/LabelType.java b/java/com/google/gerrit/common/data/LabelType.java index 14b8310741..964cf67188 100644 --- a/java/com/google/gerrit/common/data/LabelType.java +++ b/java/com/google/gerrit/common/data/LabelType.java @@ -14,14 +14,17 @@ package com.google.gerrit.common.data; +import static com.google.common.collect.ImmutableList.toImmutableList; import static java.util.Comparator.comparing; import static java.util.stream.Collectors.collectingAndThen; import static java.util.stream.Collectors.toList; +import com.google.common.collect.ImmutableList; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.LabelId; import com.google.gerrit.entities.PatchSetApproval; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -37,6 +40,7 @@ public class LabelType { public static final boolean DEF_COPY_ANY_SCORE = false; public static final boolean DEF_COPY_MAX_SCORE = false; public static final boolean DEF_COPY_MIN_SCORE = false; + public static final ImmutableList DEF_COPY_VALUES = ImmutableList.of(); public static final boolean DEF_IGNORE_SELF_APPROVAL = false; public static LabelType withDefaultValues(String name) { @@ -104,6 +108,7 @@ public class LabelType { protected boolean copyAllScoresOnTrivialRebase; protected boolean copyAllScoresIfNoCodeChange; protected boolean copyAllScoresIfNoChange; + protected ImmutableList copyValues; protected boolean allowPostSubmit; protected boolean ignoreSelfApproval; protected short defaultValue; @@ -144,6 +149,7 @@ public class LabelType { setCopyAnyScore(DEF_COPY_ANY_SCORE); setCopyMaxScore(DEF_COPY_MAX_SCORE); setCopyMinScore(DEF_COPY_MIN_SCORE); + setCopyValues(DEF_COPY_VALUES); setAllowPostSubmit(DEF_ALLOW_POST_SUBMIT); setIgnoreSelfApproval(DEF_IGNORE_SELF_APPROVAL); @@ -298,6 +304,14 @@ public class LabelType { this.copyAllScoresIfNoChange = copyAllScoresIfNoChange; } + public ImmutableList getCopyValues() { + return copyValues; + } + + public void setCopyValues(Collection copyValues) { + this.copyValues = copyValues.stream().sorted().collect(toImmutableList()); + } + public boolean isMaxNegative(PatchSetApproval ca) { return maxNegative == ca.value(); } diff --git a/java/com/google/gerrit/extensions/common/LabelDefinitionInfo.java b/java/com/google/gerrit/extensions/common/LabelDefinitionInfo.java index 64c3997eab..f552566fb6 100644 --- a/java/com/google/gerrit/extensions/common/LabelDefinitionInfo.java +++ b/java/com/google/gerrit/extensions/common/LabelDefinitionInfo.java @@ -32,6 +32,7 @@ public class LabelDefinitionInfo { public Boolean copyAllScoresIfNoCodeChange; public Boolean copyAllScoresOnTrivialRebase; public Boolean copyAllScoresOnMergeFirstParentUpdate; + public List copyValues; public Boolean allowPostSubmit; public Boolean ignoreSelfApproval; } diff --git a/java/com/google/gerrit/extensions/common/LabelDefinitionInput.java b/java/com/google/gerrit/extensions/common/LabelDefinitionInput.java index 0523f61345..23d5df1dbe 100644 --- a/java/com/google/gerrit/extensions/common/LabelDefinitionInput.java +++ b/java/com/google/gerrit/extensions/common/LabelDefinitionInput.java @@ -31,6 +31,7 @@ public class LabelDefinitionInput extends InputWithCommitMessage { public Boolean copyAllScoresIfNoCodeChange; public Boolean copyAllScoresOnTrivialRebase; public Boolean copyAllScoresOnMergeFirstParentUpdate; + public List copyValues; public Boolean allowPostSubmit; public Boolean ignoreSelfApproval; } diff --git a/java/com/google/gerrit/server/ApprovalInference.java b/java/com/google/gerrit/server/ApprovalInference.java index 44b0529dde..a4c9f2a042 100644 --- a/java/com/google/gerrit/server/ApprovalInference.java +++ b/java/com/google/gerrit/server/ApprovalInference.java @@ -101,6 +101,8 @@ public class ApprovalInference { return true; } else if (type.isCopyAnyScore()) { return true; + } else if (type.getCopyValues().contains(psa.value())) { + return true; } switch (kind) { case MERGE_FIRST_PARENT_UPDATE: diff --git a/java/com/google/gerrit/server/project/LabelDefinitionJson.java b/java/com/google/gerrit/server/project/LabelDefinitionJson.java index 2ecd8c2f36..0452d0b8e9 100644 --- a/java/com/google/gerrit/server/project/LabelDefinitionJson.java +++ b/java/com/google/gerrit/server/project/LabelDefinitionJson.java @@ -40,6 +40,7 @@ public class LabelDefinitionJson { label.copyAllScoresOnTrivialRebase = toBoolean(labelType.isCopyAllScoresOnTrivialRebase()); label.copyAllScoresOnMergeFirstParentUpdate = toBoolean(labelType.isCopyAllScoresOnMergeFirstParentUpdate()); + label.copyValues = labelType.getCopyValues().isEmpty() ? null : labelType.getCopyValues(); label.allowPostSubmit = toBoolean(labelType.allowPostSubmit()); label.ignoreSelfApproval = toBoolean(labelType.ignoreSelfApproval()); return label; diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java index fa877afb08..4ab583d5f4 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -109,6 +109,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. public static final String KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = "copyAllScoresOnTrivialRebase"; public static final String KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE = "copyAllScoresIfNoCodeChange"; public static final String KEY_COPY_ALL_SCORES_IF_NO_CHANGE = "copyAllScoresIfNoChange"; + public static final String KEY_COPY_VALUE = "copyValue"; public static final String KEY_VALUE = "value"; public static final String KEY_CAN_OVERRIDE = "canOverride"; public static final String KEY_BRANCH = "branch"; @@ -1014,6 +1015,27 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, LabelType.DEF_COPY_ALL_SCORES_IF_NO_CHANGE)); + Set copyValues = new HashSet<>(); + for (String value : rc.getStringList(LABEL, name, KEY_COPY_VALUE)) { + try { + short copyValue = Shorts.checkedCast(PermissionRule.parseInt(value)); + if (!copyValues.add(copyValue)) { + error( + new ValidationError( + PROJECT_CONFIG, + String.format( + "Duplicate %s \"%s\" for label \"%s\"", KEY_COPY_VALUE, value, name))); + } + } catch (IllegalArgumentException notValue) { + error( + new ValidationError( + PROJECT_CONFIG, + String.format( + "Invalid %s \"%s\" for label \"%s\": %s", + KEY_COPY_VALUE, value, name, notValue.getMessage()))); + } + } + label.setCopyValues(copyValues); label.setCanOverride( rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, LabelType.DEF_CAN_OVERRIDE)); label.setRefPatterns(getStringListOrNull(rc, LABEL, name, KEY_BRANCH)); @@ -1492,6 +1514,11 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. KEY_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE, label.isCopyAllScoresOnMergeFirstParentUpdate(), LabelType.DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE); + rc.setStringList( + LABEL, + name, + KEY_COPY_VALUE, + label.getCopyValues().stream().map(LabelValue::formatValue).collect(toList())); setBooleanConfigKey( rc, LABEL, name, KEY_CAN_OVERRIDE, label.canOverride(), LabelType.DEF_CAN_OVERRIDE); List values = new ArrayList<>(label.getValues().size()); diff --git a/java/com/google/gerrit/server/restapi/project/CreateLabel.java b/java/com/google/gerrit/server/restapi/project/CreateLabel.java index 5d5152791b..a85ad39bb7 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateLabel.java +++ b/java/com/google/gerrit/server/restapi/project/CreateLabel.java @@ -191,6 +191,10 @@ public class CreateLabel input.copyAllScoresOnMergeFirstParentUpdate); } + if (input.copyValues != null) { + labelType.setCopyValues(input.copyValues); + } + if (input.allowPostSubmit != null) { labelType.setAllowPostSubmit(input.allowPostSubmit); } diff --git a/java/com/google/gerrit/server/restapi/project/SetLabel.java b/java/com/google/gerrit/server/restapi/project/SetLabel.java index 824b4edaa1..0a35865f46 100644 --- a/java/com/google/gerrit/server/restapi/project/SetLabel.java +++ b/java/com/google/gerrit/server/restapi/project/SetLabel.java @@ -205,6 +205,11 @@ public class SetLabel implements RestModifyView