opendev/gerrit
Code Issues Proposed changes

Label config: Allow to configure individual votes as sticky

Browse Source 
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 <ekempin@google.com>
Change-Id: Ia8d1b634bc76123db9fc1ec0667e17c3e9e27204
This commit is contained in:
Edwin Kempin 2020-01-10 11:38:06 +01:00
parent a767f08db3
commit 33c13a444e
16 changed files with 152 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
Documentation/config-labels.txt
View File

@ -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`

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

@ -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.

14
java/com/google/gerrit/common/data/LabelType.java
View File

@ -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<Short> 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<Short> 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<Short> getCopyValues() {
return copyValues;
}
public void setCopyValues(Collection<Short> copyValues) {
this.copyValues = copyValues.stream().sorted().collect(toImmutableList());
}
public boolean isMaxNegative(PatchSetApproval ca) {
return maxNegative == ca.value();
}

1
java/com/google/gerrit/extensions/common/LabelDefinitionInfo.java
View File

@ -32,6 +32,7 @@ public class LabelDefinitionInfo {
public Boolean copyAllScoresIfNoCodeChange;
public Boolean copyAllScoresOnTrivialRebase;
public Boolean copyAllScoresOnMergeFirstParentUpdate;
public List<Short> copyValues;
public Boolean allowPostSubmit;
public Boolean ignoreSelfApproval;
}

1
java/com/google/gerrit/extensions/common/LabelDefinitionInput.java
View File

@ -31,6 +31,7 @@ public class LabelDefinitionInput extends InputWithCommitMessage {
public Boolean copyAllScoresIfNoCodeChange;
public Boolean copyAllScoresOnTrivialRebase;
public Boolean copyAllScoresOnMergeFirstParentUpdate;
public List<Short> copyValues;
public Boolean allowPostSubmit;
public Boolean ignoreSelfApproval;
}

2
java/com/google/gerrit/server/ApprovalInference.java
View File

@ -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:

1
java/com/google/gerrit/server/project/LabelDefinitionJson.java
View File

@ -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;

27
java/com/google/gerrit/server/project/ProjectConfig.java
View File

@ -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<Short> 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<String> values = new ArrayList<>(label.getValues().size());

4
java/com/google/gerrit/server/restapi/project/CreateLabel.java
View File

@ -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);
}

5
java/com/google/gerrit/server/restapi/project/SetLabel.java
View File

@ -205,6 +205,11 @@ public class SetLabel implements RestModifyView<LabelResource, LabelDefinitionIn
dirty = true;
}
if (input.copyValues != null) {
labelType.setCopyValues(input.copyValues);
dirty = true;
}
if (input.allowPostSubmit != null) {
labelType.setAllowPostSubmit(input.allowPostSubmit);
dirty = true;

29
javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
View File

@ -184,6 +184,35 @@ public class StickyApprovalsIT extends AbstractDaemonTest {
}
}
@Test
public void stickyOnCopyValues() throws Exception {
TestAccount user2 = accountCreator.user2();
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.getLabelSections()
.get("Code-Review")
.setCopyValues(ImmutableList.of((short) -1, (short) 1));
u.save();
}
for (ChangeKind changeKind :
EnumSet.of(REWORK, TRIVIAL_REBASE, NO_CODE_CHANGE, MERGE_FIRST_PARENT_UPDATE, NO_CHANGE)) {
testRepo.reset(projectOperations.project(project).getHead("master"));
String changeId = createChange(changeKind);
vote(admin, changeId, -1, 1);
vote(user, changeId, -2, -1);
vote(user2, changeId, 1, -1);
updateChange(changeId, changeKind);
ChangeInfo c = detailedChange(changeId);
assertVotes(c, admin, -1, 0, changeKind);
assertVotes(c, user, 0, 0, changeKind);
assertVotes(c, user2, 1, 0, changeKind);
}
}
@Test
public void stickyOnTrivialRebase() throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {

12
javatests/com/google/gerrit/acceptance/rest/project/CreateLabelIT.java
View File

@ -244,6 +244,7 @@ public class CreateLabelIT extends AbstractDaemonTest {
assertThat(createdLabel.copyAllScoresIfNoCodeChange).isNull();
assertThat(createdLabel.copyAllScoresOnTrivialRebase).isNull();
assertThat(createdLabel.copyAllScoresOnMergeFirstParentUpdate).isNull();
assertThat(createdLabel.copyValues).isNull();
assertThat(createdLabel.allowPostSubmit).isTrue();
assertThat(createdLabel.ignoreSelfApproval).isNull();
}
@ -536,6 +537,17 @@ public class CreateLabelIT extends AbstractDaemonTest {
assertThat(createdLabel.copyAllScoresOnMergeFirstParentUpdate).isNull();
}
@Test
public void createWithCopyValues() throws Exception {
LabelDefinitionInput input = new LabelDefinitionInput();
input.values = ImmutableMap.of("+1", "Looks Good", " 0", "Don't Know", "-1", "Looks Bad");
input.copyValues = ImmutableList.of((short) -1, (short) 1);
LabelDefinitionInfo createdLabel =
gApi.projects().name(project.get()).label("foo").create(input).get();
assertThat(createdLabel.copyValues).containsExactly((short) -1, (short) 1).inOrder();
}
@Test
public void createWithAllowPostSubmit() throws Exception {
LabelDefinitionInput input = new LabelDefinitionInput();

3
javatests/com/google/gerrit/acceptance/rest/project/GetLabelIT.java
View File

@ -117,6 +117,7 @@ public class GetLabelIT extends AbstractDaemonTest {
assertThat(fooLabel.copyAllScoresIfNoCodeChange).isNull();
assertThat(fooLabel.copyAllScoresOnTrivialRebase).isNull();
assertThat(fooLabel.copyAllScoresOnMergeFirstParentUpdate).isNull();
assertThat(fooLabel.copyValues).isNull();
assertThat(fooLabel.allowPostSubmit).isNull();
assertThat(fooLabel.ignoreSelfApproval).isNull();
}
@ -134,6 +135,7 @@ public class GetLabelIT extends AbstractDaemonTest {
labelType.setCopyAllScoresIfNoCodeChange(true);
labelType.setCopyAllScoresOnTrivialRebase(true);
labelType.setCopyAllScoresOnMergeFirstParentUpdate(true);
labelType.setCopyValues(ImmutableList.of((short) -1, (short) 1));
labelType.setIgnoreSelfApproval(true);
u.getConfig().getLabelSections().put(labelType.getName(), labelType);
u.save();
@ -148,6 +150,7 @@ public class GetLabelIT extends AbstractDaemonTest {
assertThat(fooLabel.copyAllScoresIfNoCodeChange).isTrue();
assertThat(fooLabel.copyAllScoresOnTrivialRebase).isTrue();
assertThat(fooLabel.copyAllScoresOnMergeFirstParentUpdate).isTrue();
assertThat(fooLabel.copyValues).containsExactly((short) -1, (short) 1).inOrder();
assertThat(fooLabel.allowPostSubmit).isTrue();
assertThat(fooLabel.ignoreSelfApproval).isTrue();
}

1
javatests/com/google/gerrit/acceptance/rest/project/LabelAssert.java
View File

@ -47,6 +47,7 @@ public class LabelAssert {
assertThat(codeReviewLabel.copyAllScoresIfNoCodeChange).isNull();
assertThat(codeReviewLabel.copyAllScoresOnTrivialRebase).isTrue();
assertThat(codeReviewLabel.copyAllScoresOnMergeFirstParentUpdate).isNull();
assertThat(codeReviewLabel.copyValues).isNull();
assertThat(codeReviewLabel.allowPostSubmit).isTrue();
assertThat(codeReviewLabel.ignoreSelfApproval).isNull();
}

3
javatests/com/google/gerrit/acceptance/rest/project/ListLabelsIT.java
View File

@ -139,6 +139,7 @@ public class ListLabelsIT extends AbstractDaemonTest {
assertThat(fooLabel.copyAllScoresIfNoCodeChange).isNull();
assertThat(fooLabel.copyAllScoresOnTrivialRebase).isNull();
assertThat(fooLabel.copyAllScoresOnMergeFirstParentUpdate).isNull();
assertThat(fooLabel.copyValues).isNull();
assertThat(fooLabel.allowPostSubmit).isNull();
assertThat(fooLabel.ignoreSelfApproval).isNull();
}
@ -156,6 +157,7 @@ public class ListLabelsIT extends AbstractDaemonTest {
labelType.setCopyAllScoresIfNoCodeChange(true);
labelType.setCopyAllScoresOnTrivialRebase(true);
labelType.setCopyAllScoresOnMergeFirstParentUpdate(true);
labelType.setCopyValues(ImmutableList.of((short) -1, (short) 1));
labelType.setIgnoreSelfApproval(true);
u.getConfig().getLabelSections().put(labelType.getName(), labelType);
u.save();
@ -173,6 +175,7 @@ public class ListLabelsIT extends AbstractDaemonTest {
assertThat(fooLabel.copyAllScoresIfNoCodeChange).isTrue();
assertThat(fooLabel.copyAllScoresOnTrivialRebase).isTrue();
assertThat(fooLabel.copyAllScoresOnMergeFirstParentUpdate).isTrue();
assertThat(fooLabel.copyValues).containsExactly((short) -1, (short) 1).inOrder();
assertThat(fooLabel.allowPostSubmit).isTrue();
assertThat(fooLabel.ignoreSelfApproval).isTrue();
}

38
javatests/com/google/gerrit/acceptance/rest/project/SetLabelIT.java
View File

@ -770,6 +770,44 @@ public class SetLabelIT extends AbstractDaemonTest {
.isNull();
}
@Test
public void setCopyValues() throws Exception {
configLabel("foo", LabelFunction.NO_OP);
assertThat(gApi.projects().name(project.get()).label("foo").get().copyValues).isNull();
LabelDefinitionInput input = new LabelDefinitionInput();
input.copyValues = ImmutableList.of((short) -1, (short) 1);
LabelDefinitionInfo updatedLabel =
gApi.projects().name(project.get()).label("foo").update(input);
assertThat(updatedLabel.copyValues).containsExactly((short) -1, (short) 1).inOrder();
assertThat(gApi.projects().name(project.get()).label("foo").get().copyValues)
.containsExactly((short) -1, (short) 1)
.inOrder();
}
@Test
public void unsetCopyValues() throws Exception {
configLabel("foo", LabelFunction.NO_OP);
try (ProjectConfigUpdate u = updateProject(project)) {
LabelType labelType = u.getConfig().getLabelSections().get("foo");
labelType.setCopyValues(ImmutableList.of((short) -1, (short) 1));
u.getConfig().getLabelSections().put(labelType.getName(), labelType);
u.save();
}
assertThat(gApi.projects().name(project.get()).label("foo").get().copyValues).isNotEmpty();
LabelDefinitionInput input = new LabelDefinitionInput();
input.copyValues = ImmutableList.of();
LabelDefinitionInfo updatedLabel =
gApi.projects().name(project.get()).label("foo").update(input);
assertThat(updatedLabel.copyValues).isNull();
assertThat(gApi.projects().name(project.get()).label("foo").get().copyValues).isNull();
}
@Test
public void setAllowPostSubmit() throws Exception {
configLabel("foo", LabelFunction.NO_OP);