Fully support voting with arbitrary labels

This is mostly TODO cleanup where we originally ignored unknown
labels without knowing for sure that would be the right long-term
approach.

The only semantic change is that a change can no longer possibly be
considered submittable from the publish comment screen if it contains
some completely undefined labels.

Change-Id: I7a722c3e7485b681a7fe9aff8e8da1413f70771e
This commit is contained in:
Dave Borowitz
2013-02-28 15:25:19 -08:00
parent 8e5de82e56
commit 0e0a41a281
5 changed files with 11 additions and 12 deletions

View File

@@ -121,7 +121,7 @@ final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
PermissionRange range = rangeByName.get(lbl.label); PermissionRange range = rangeByName.get(lbl.label);
if (range != null) { if (range != null) {
LabelType lt = control.getLabelTypes().byLabel(lbl.label); LabelType lt = control.getLabelTypes().byLabel(lbl.label);
if (lt == null || lt.getMax().getValue() == range.getMax()) { if (lt != null && lt.getMax().getValue() == range.getMax()) {
canMakeOk = true; canMakeOk = true;
} }
} }

View File

@@ -335,7 +335,7 @@ public class ChangeJson {
for (Map.Entry<String, LabelInfo> e : labels.entrySet()) { for (Map.Entry<String, LabelInfo> e : labels.entrySet()) {
LabelType type = labelTypes.byLabel(e.getKey()); LabelType type = labelTypes.byLabel(e.getKey());
if (type == null) { if (type == null) {
continue; // TODO: Support arbitrary labels. continue;
} }
if (standard) { if (standard) {
setRecommendedAndDisliked(cd, type, e.getValue()); setRecommendedAndDisliked(cd, type, e.getValue());
@@ -444,7 +444,6 @@ public class ChangeJson {
labels.get(name).addApproval(ai); labels.get(name).addApproval(ai);
} }
for (PatchSetApproval psa : current.get(accountId)) { for (PatchSetApproval psa : current.get(accountId)) {
// TODO Support arbitrary labels placed by a reviewer.
LabelType lt = ctl.getLabelTypes().byLabel(psa.getLabelId()); LabelType lt = ctl.getLabelTypes().byLabel(psa.getLabelId());
if (lt == null) { if (lt == null) {
continue; continue;
@@ -576,7 +575,7 @@ public class ChangeJson {
for (SubmitRecord.Label r : rec.labels) { for (SubmitRecord.Label r : rec.labels) {
LabelType type = labelTypes.byLabel(r.label); LabelType type = labelTypes.byLabel(r.label);
if (type == null) { if (type == null) {
continue; // TODO: Support arbitrary labels. continue;
} }
PermissionRange range = ctl.getRange(Permission.forLabel(r.label)); PermissionRange range = ctl.getRange(Permission.forLabel(r.label));
for (LabelValue v : type.getValues()) { for (LabelValue v : type.getValues()) {

View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.change; package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.common.base.Objects; import com.google.common.base.Objects;
import com.google.common.base.Strings; import com.google.common.base.Strings;
import com.google.common.collect.Lists; import com.google.common.collect.Lists;
@@ -65,7 +67,8 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
* on access controls, attempting to use a label not granted to the user * on access controls, attempting to use a label not granted to the user
* will fail the entire modify operation early. If false the operation will * will fail the entire modify operation early. If false the operation will
* execute anyway, but the proposed labels given by the user will be * execute anyway, but the proposed labels given by the user will be
* modified to be the "best" value allowed by the access controls. * modified to be the "best" value allowed by the access controls, or
* ignored if the label does not exist.
*/ */
public boolean strictLabels = true; public boolean strictLabels = true;
@@ -173,7 +176,6 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
while (itr.hasNext()) { while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next(); Map.Entry<String, Short> ent = itr.next();
// TODO Support more generic label assignments.
LabelType lt = revision.getControl().getLabelTypes() LabelType lt = revision.getControl().getLabelTypes()
.byLabel(ent.getKey()); .byLabel(ent.getKey());
if (lt == null) { if (lt == null) {
@@ -339,9 +341,8 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
LabelTypes labelTypes = rsrc.getControl().getLabelTypes(); LabelTypes labelTypes = rsrc.getControl().getLabelTypes();
for (Map.Entry<String, Short> ent : labels.entrySet()) { for (Map.Entry<String, Short> ent : labels.entrySet()) {
// TODO Support arbitrary label names. String name = ent.getKey();
LabelType lt = labelTypes.byLabel(ent.getKey()); LabelType lt = checkNotNull(labelTypes.byLabel(name), name);
String name = lt.getName();
if (change.getStatus().isClosed()) { if (change.getStatus().isClosed()) {
// TODO Allow updating some labels even when closed. // TODO Allow updating some labels even when closed.
continue; continue;

View File

@@ -92,10 +92,10 @@ public class ReviewerJson {
for (PatchSetApproval ca : approvals) { for (PatchSetApproval ca : approvals) {
for (PermissionRange pr : ctl.getLabelRanges()) { for (PermissionRange pr : ctl.getLabelRanges()) {
if (!pr.isEmpty()) { if (!pr.isEmpty()) {
// TODO: Support arbitrary labels.
LabelType at = labelTypes.byLabel(ca.getLabelId()); LabelType at = labelTypes.byLabel(ca.getLabelId());
if (at != null) { if (at != null) {
out.approvals.put(at.getName(), formatValue(ca.getValue())); } out.approvals.put(at.getName(), formatValue(ca.getValue()));
}
} }
} }
} }

View File

@@ -298,7 +298,6 @@ public class MergeUtil {
} else { } else {
final LabelType lt = project.getLabelTypes().byLabel(a.getLabelId()); final LabelType lt = project.getLabelTypes().byLabel(a.getLabelId());
if (lt == null) { if (lt == null) {
// TODO: Support arbitrary labels.
continue; continue;
} }
tag = lt.getName(); tag = lt.getName();