opendev/gerrit
Code Issues Proposed changes

Allow configuration of a default value for a label.

Browse Source 
This change allows a project admin or owner to define a default value
(or score) for a label. The default value is set in the project configurations
file with a 'defaultValue' key.  The defaultValue must be within the range of
valid label values.  It is an optional label setting, if not defined the
defaultValue for the label will be 0.  When a defaultValue is defined, that
value will get set in the Reply dialog by default.  A defaultValue can be set
to a score that is outside of the permissible range for a user.  In that case
the score that will get set in the Reply box will be the next closes score
to the defaultValue.

feature: Issue 2041
Change-Id: I12dece29b043e09eaab62cdcf56146a1380041bc
This commit is contained in:
Khai Do
2014-04-06 23:27:43 -07:00
parent 2b3a5badc9
commit 4c91b000fa
12 changed files with 153 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
Documentation/config-labels.txt
View File

@@ -169,6 +169,20 @@ text"`. The `<#>` may be any positive or negative number with an
optional leading `+`.
[[label_defaultValue]]
=== `label.Label-Name.defaultValue`
The default value (or score) for the label. The defaultValue must be
within the range of valid label values. It is an optional label setting,
if not defined the defaultValue for the label will be 0. When a
defaultValue is defined, that value will get set in the Reply dialog
by default.
A defaultValue can be set to a score that is outside of the permissible
range for a user. In that case the score that will get set in the Reply
box will be either the lowest or highest score in the permissible range.
[[label_abbreviation]]
=== `label.Label-Name.abbreviation`
@@ -297,6 +311,32 @@ The new column will appear at the end of the table, and `-1 Do not have
copyright` will block submit, while `+1 Copyright clear` is required to
enable submit.
=== Default Value Example
This example attempts to describe how a label default value works with the
user permissions. Assume the configuration below.
====
[access "refs/heads/*"]
label-Snarky-Review = -3..+3 group Administrators
label-Snarky-Review = -2..+2 group Project Owners
label-Snarky-Review = -1..+1 group Registered Users
[label "Snarky-Review"]
value = -3 Ohh, hell no!
value = -2 Hmm, I'm not a fan
value = -1 I'm not sure I like this
value = 0 No score
value = +1 I like, but need another to like it as well
value = +2 Hmm, this is pretty nice
value = +3 Ohh, hell yes!
defaultValue = -3
====
Upon clicking the Reply button:
* Administrators have all scores (-3..+3) available, -3 is set as the default.
* Project Owners have limited scores (-2..+2) available, -2 is set as the default.
* Registered Users have limited scores (-1..+1) available, -1 is set as the default.
GERRIT
------
Part of link:index.html[Gerrit Code Review]

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

@@ -3093,6 +3093,8 @@ If not set, the default is false.
|`value` |optional|The voting value of the user who
recommended/disliked this label on the change if it is not
"`+1`"/"`-1`".
|`default_value`|optional|The default voting value for the label.
This value may be outside the range specified in permitted_labels.
|===========================
==== Fields set by `DETAILED_LABELS`

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java
View File

@@ -69,6 +69,7 @@ public class LabelTypeIT extends AbstractDaemonTest {
codeReview.setCopyMaxScore(false);
codeReview.setCopyAllScoresOnTrivialRebase(false);
codeReview.setCopyAllScoresIfNoCodeChange(false);
codeReview.setDefaultValue((short)-1);
saveProjectConfig(cfg);
}
@@ -264,6 +265,7 @@ public class LabelTypeIT extends AbstractDaemonTest {
// through JSON instead of querying the DB directly.
ChangeInfo c = get(r.getChangeId());
LabelInfo cr = c.labels.get("Code-Review");
assertEquals(-1, (int) cr.defaultValue);
assertEquals(1, cr.all.size());
assertEquals("Administrator", cr.all.get(0).name);
assertEquals(expected, cr.all.get(0).value.intValue());

10
gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
View File

@@ -112,6 +112,7 @@ public class LabelType {
protected boolean copyMaxScore;
protected boolean copyAllScoresOnTrivialRebase;
protected boolean copyAllScoresIfNoCodeChange;
protected short defaultValue;
protected List<LabelValue> values;
protected short maxNegative;
@@ -129,6 +130,7 @@ public class LabelType {
this.name = checkName(name);
canOverride = true;
values = sortValues(valueList);
defaultValue = 0;
abbreviation = defaultAbbreviation(name);
functionName = "MaxWithBlock";
@@ -204,6 +206,14 @@ public class LabelType {
return v.getValue() > 0 ? v : null;
}
public short getDefaultValue() {
return defaultValue;
}
public void setDefaultValue(short defaultValue) {
this.defaultValue = defaultValue;
}
public boolean isCopyMinScore() {
return copyMinScore;
}

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

@@ -25,6 +25,7 @@ public class LabelInfo {
public List<ApprovalInfo> all;
public Map<String, String> values;
public Short value;
public Short defaultValue;
public Boolean optional;
public Boolean blocking;
}

16
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java
View File

@@ -322,10 +322,23 @@ class ReplyBox extends Composite {
}
}
private Short normalizeDefaultValue(Short defaultValue, Set<Short> permittedValues) {
Short pmin = Collections.min(permittedValues);
Short pmax = Collections.max(permittedValues);
Short dv = defaultValue;
if (dv > pmax) {
dv = pmax;
} else if (dv < pmin) {
dv = pmin;
}
return dv;
}
private void renderRadio(int row,
List<Short> columns,
LabelAndValues lv) {
String id = lv.info.name();
Short dv = normalizeDefaultValue(lv.info.defaultValue(), lv.permitted);
labelHelpColumn = 1 + columns.size();
labelsTable.setText(row, 0, id);
@@ -345,9 +358,10 @@ class ReplyBox extends Composite {
if (lv.permitted.contains(v)) {
String text = lv.info.value_text(LabelValue.formatValue(v));
LabelRadioButton b = new LabelRadioButton(group, text, v);
if ((self != null && v == self.value()) || (self == null && v == 0)) {
if ((self != null && v == self.value()) || (self == null && v.equals(dv))) {
b.setValue(true);
group.select(b);
in.label(group.label, v);
labelsTable.setText(row, labelHelpColumn, b.text);
}
group.buttons.add(b);

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeInfo.java
View File

@@ -161,6 +161,7 @@ public class ChangeInfo extends JavaScriptObject {
public final native boolean optional() /*-{ return this.optional ? true : false; }-*/;
public final native boolean blocking() /*-{ return this.blocking ? true : false; }-*/;
public final native short defaultValue() /*-{ return this.default_value; }-*/;
final native short _value()
/*-{
if (this.value) return this.value;

1
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeInfoMapper.java
View File

@@ -116,6 +116,7 @@ class ChangeInfoMapper {
lo.recommended = fromAcountInfo(li.recommended);
lo.disliked = fromAcountInfo(li.disliked);
lo.value = li.value;
lo.defaultValue = li.defaultValue;
lo.optional = li.optional;
lo.blocking = li.blocking;
lo.values = li.values;

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

@@ -631,6 +631,7 @@ public class ChangeJson {
}
private void setLabelValues(LabelType type, LabelInfo label) {
label.defaultValue = type.getDefaultValue();
label.values = Maps.newLinkedHashMap();
for (LabelValue v : type.getValues()) {
label.values.put(v.formatValue(), v.getText());
@@ -1007,6 +1008,7 @@ public class ChangeJson {
public Map<String, String> values;
public Short value;
public Short defaultValue;
public Boolean optional;
public Boolean blocking;

21
gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
View File

@@ -129,6 +129,7 @@ public class ProjectConfig extends VersionedMetaData {
private static final String LABEL = "label";
private static final String KEY_ABBREVIATION = "abbreviation";
private static final String KEY_FUNCTION = "function";
private static final String KEY_DEFAULT_VALUE = "defaultValue";
private static final String KEY_COPY_MIN_SCORE = "copyMinScore";
private static final String KEY_COPY_MAX_SCORE = "copyMaxScore";
private static final String KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = "copyAllScoresOnTrivialRebase";
@@ -665,6 +666,15 @@ public class ProjectConfig extends VersionedMetaData {
KEY_FUNCTION, name, Joiner.on(", ").join(LABEL_FUNCTIONS))));
label.setFunctionName(null);
}
short dv = (short) rc.getInt(LABEL, name, KEY_DEFAULT_VALUE, 0);
if (isInRange(dv, values)) {
label.setDefaultValue(dv);
} else {
error(new ValidationError(PROJECT_CONFIG, String.format(
"Invalid %s \"%s\" for label \"%s\"",
KEY_DEFAULT_VALUE, dv, name)));
}
label.setCopyMinScore(
rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, false));
label.setCopyMaxScore(
@@ -680,6 +690,15 @@ public class ProjectConfig extends VersionedMetaData {
}
}
private boolean isInRange(short value, List<LabelValue> labelValues) {
for (LabelValue lv : labelValues) {
if (lv.getValue() == value) {
return true;
}
}
return false;
}
private List<String> getStringListOrNull(Config rc, String section,
String subSection, String name) {
String[] ac = rc.getStringList(section, subSection, name);
@@ -1018,6 +1037,8 @@ public class ProjectConfig extends VersionedMetaData {
} else {
rc.unset(LABEL, name, KEY_ABBREVIATION);
}
rc.setInt(LABEL, name, KEY_DEFAULT_VALUE, label.getDefaultValue());
if (label.isCopyMinScore()) {
rc.setBoolean(LABEL, name, KEY_COPY_MIN_SCORE, true);
} else {

57
gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java
View File

@@ -21,9 +21,11 @@ import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import com.google.common.collect.Iterables;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -47,6 +49,7 @@ import org.junit.Test;
import java.io.IOException;
import java.util.Collections;
import java.util.Map;
public class ProjectConfigTest extends LocalDiskRepositoryTestCase {
private final GroupReference developers = new GroupReference(
@@ -114,6 +117,60 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase {
assertFalse(push.getExclusiveGroup());
}
@Test
public void testReadConfigLabelDefaultValue() throws Exception {
RevCommit rev = util.commit(util.tree( //
util.file("groups", util.blob(group(developers))), //
util.file("project.config", util.blob(""//
+ "[label \"CustomLabel\"]\n" //
+ " value = -1 Negative\n" //
+ " value = 0 No Score\n" //
+ " value = 1 Positive\n")) //
));
ProjectConfig cfg = read(rev);
Map<String, LabelType> labels = cfg.getLabelSections();
Short dv = labels.entrySet().iterator().next().getValue().getDefaultValue();
assertEquals(0, (int) dv);
}
@Test
public void testReadConfigLabelDefaultValueInRange() throws Exception {
RevCommit rev = util.commit(util.tree( //
util.file("groups", util.blob(group(developers))), //
util.file("project.config", util.blob(""//
+ "[label \"CustomLabel\"]\n" //
+ " value = -1 Negative\n" //
+ " value = 0 No Score\n" //
+ " value = 1 Positive\n" //
+ " defaultValue = -1\n")) //
));
ProjectConfig cfg = read(rev);
Map<String, LabelType> labels = cfg.getLabelSections();
Short dv = labels.entrySet().iterator().next().getValue().getDefaultValue();
assertEquals(-1, (int) dv);
}
@Test
public void testReadConfigLabelDefaultValueNotInRange() throws Exception {
RevCommit rev = util.commit(util.tree( //
util.file("groups", util.blob(group(developers))), //
util.file("project.config", util.blob(""//
+ "[label \"CustomLabel\"]\n" //
+ " value = -1 Negative\n" //
+ " value = 0 No Score\n" //
+ " value = 1 Positive\n" //
+ " defaultValue = -2\n")) //
));
ProjectConfig cfg = read(rev);
assertEquals(1, cfg.getValidationErrors().size());
assertEquals("project.config: Invalid defaultValue \"-2\" "
+ "for label \"CustomLabel\"",
Iterables.getOnlyElement(cfg.getValidationErrors()).getMessage());
}
@Test
public void testEditConfig() throws Exception {
RevCommit rev = util.commit(util.tree( //

1
gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
View File

@@ -129,6 +129,7 @@ public class SchemaCreatorTest {
assertNotNull(codeReview);
assertEquals("Code-Review", codeReview.getName());
assertEquals("CR", codeReview.getAbbreviation());
assertEquals(0, codeReview.getDefaultValue());
assertEquals("MaxWithBlock", codeReview.getFunctionName());
assertTrue(codeReview.isCopyMinScore());
assertValueRange(codeReview, 2, 1, 0, -1, -2);