opendev/gerrit
Code Issues Proposed changes

Make NoBlock and NoOp approval category functions work

Browse Source 
The approval category functions NoBlock and NoOp have not worked
since the integration of Prolog, and this commit makes a number of
changes to remedy this.

MAY is being introduced as a new submit record status to complement
OK, REJECT, NEED, and IMPOSSIBLE. This allows the expression of
approval categories (labels) that are optional, i.e. could either be
set or unset without ever influencing whether the change could be
submitted. Previously there was no way to express this property in
the submit record.

This enables the NoBlock and NoOp approval category functions to work
as they now emit may() terms from the Prolog rules. Previously they
returned ok() terms lacking a nested user term, leading to exceptions
in code that expected a user context if the label was OK.

A side-effect is the addition of an "optional" boolean property of
the LabelInfo JSON objects, indicating whether the label in question
is may be omitted and still submit the change.

Change-Id: I02bedf892564f759418fa7427ac226970f9a6ef5
This commit is contained in:
Magnus Bäck
2012-05-10 11:33:47 -07:00
committed by Shawn O. Pearce
parent 277ce33c33
commit a6ce960fa1
9 changed files with 47 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitRecord.java
View File

@@ -67,6 +67,12 @@ public class SubmitRecord {
*/
NEED,
/**
* The label may be set, but it's neither necessary for submission
* nor does it block submission if set.
*/
MAY,
/**
* The label is required for submission, but is impossible to complete.
* The likely cause is access has not been granted correctly by the

3
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
View File

@@ -182,6 +182,9 @@ public class ApprovalTable extends Composite {
break;
}
case MAY:
break;
case NEED:
case IMPOSSIBLE:
if (reportedMissing.add(lbl.label)) {

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

@@ -81,6 +81,8 @@ public class ChangeInfo extends JavaScriptObject {
return SubmitRecord.Label.Status.OK;
} else if (rejected() != null) {
return SubmitRecord.Label.Status.REJECT;
} else if (optional()) {
return SubmitRecord.Label.Status.MAY;
} else {
return SubmitRecord.Label.Status.NEED;
}
@@ -92,6 +94,7 @@ public class ChangeInfo extends JavaScriptObject {
public final native AccountInfo recommended() /*-{ return this.recommended; }-*/;
public final native AccountInfo disliked() /*-{ return this.disliked; }-*/;
public final native boolean optional() /*-{ return this.optional ? true : false; }-*/;
final native short _value()
/*-{
if (this.value) return this.value;

8
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
View File

@@ -143,20 +143,20 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
detail.setCanEdit(control.getRefControl().canWrite());
if (detail.getChange().getStatus().isOpen()) {
List<SubmitRecord> submitRecords = control.canSubmit(db, patch);
List<SubmitRecord> submitRecords = control.getSubmitRecords(db, patch);
for (SubmitRecord rec : submitRecords) {
if (rec.labels != null) {
for (SubmitRecord.Label lbl : rec.labels) {
aic.want(lbl.appliedBy);
}
}
if (rec.status == SubmitRecord.Status.OK && control.getRefControl().canSubmit()) {
if (detail.getChange().getStatus().isOpen()
&& rec.status == SubmitRecord.Status.OK
&& control.getRefControl().canSubmit()) {
detail.setCanSubmit(true);
}
}
detail.setSubmitRecords(submitRecords);
}
patchsetsById = new HashMap<PatchSet.Id, PatchSet>();
loadPatchSets();

1
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
View File

@@ -145,6 +145,7 @@ final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
switch (lbl.status) {
case OK:
case MAY:
ok++;
break;

4
gerrit-server/src/main/java/com/google/gerrit/server/changedetail/Submit.java
View File

@@ -113,6 +113,10 @@ public class Submit implements Callable<ReviewResult> {
errMsg.append("change " + changeId + ": needs " + lbl.label);
break;
case MAY:
// The MAY label didn't cause the NOT_READY status
break;
case IMPOSSIBLE:
if (errMsg.length() > 0) errMsg.append("; ");
errMsg.append("change " + changeId + ": needs " + lbl.label

13
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -292,13 +292,17 @@ public class ChangeControl {
return false;
}
public List<SubmitRecord> getSubmitRecords(ReviewDb db, PatchSet patchSet) {
return canSubmit(db, patchSet, null, false, true);
}
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet) {
return canSubmit(db, patchSet, null, false);
return canSubmit(db, patchSet, null, false, false);
}
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet,
@Nullable ChangeData cd, boolean fastEvalLabels) {
if (change.getStatus().isClosed()) {
@Nullable ChangeData cd, boolean fastEvalLabels, boolean allowClosed) {
if (!allowClosed && change.getStatus().isClosed()) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.CLOSED;
return Collections.singletonList(rec);
@@ -495,6 +499,9 @@ public class ChangeControl {
} else if ("need".equals(status.name())) {
lbl.status = SubmitRecord.Label.Status.NEED;
} else if ("may".equals(status.name())) {
lbl.status = SubmitRecord.Label.Status.MAY;
} else if ("impossible".equals(status.name())) {
lbl.status = SubmitRecord.Label.Status.IMPOSSIBLE;

7
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ListChanges.java
View File

@@ -205,6 +205,9 @@ public class ListChanges {
}
private AccountAttribute asAccountAttribute(Account.Id user) {
if (user == null) {
return null;
}
AccountAttribute a = accounts.get(user);
if (a == null) {
a = new AccountAttribute();
@@ -226,7 +229,7 @@ public class ListChanges {
PatchSet ps = cd.currentPatchSet(db);
Map<String, LabelInfo> labels = Maps.newLinkedHashMap();
for (SubmitRecord rec : ctl.canSubmit(db.get(), ps, cd, true)) {
for (SubmitRecord rec : ctl.canSubmit(db.get(), ps, cd, true, false)) {
if (rec.labels == null) {
continue;
}
@@ -243,6 +246,7 @@ public class ListChanges {
n.rejected = asAccountAttribute(r.appliedBy);
break;
}
n.optional = n._status == SubmitRecord.Label.Status.MAY ? true : null;
labels.put(r.label, n);
}
}
@@ -314,5 +318,6 @@ public class ListChanges {
AccountAttribute recommended;
AccountAttribute disliked;
Short value;
Boolean optional;
}
}

5
gerrit-server/src/main/prolog/gerrit_common.pl
View File

@@ -149,6 +149,7 @@ call_submit_rule(X, Arg) :- !, F =.. [X, Arg], F.
is_all_ok([]).
is_all_ok([label(_, ok(__)) | Ls]) :- is_all_ok(Ls).
is_all_ok([label(_, may(__)) | Ls]) :- is_all_ok(Ls).
is_all_ok(_) :- fail.
@@ -209,8 +210,8 @@ default_submit([Type | Types], Tmp, Out) :-
%%
legacy_submit_rule('MaxWithBlock', Label, Id, Min, Max, T) :- !, max_with_block(Label, Min, Max, T).
legacy_submit_rule('MaxNoBlock', Label, Id, Min, Max, T) :- !, max_no_block(Label, Max, T).
legacy_submit_rule('NoBlock', Label, Id, Min, Max, T) :- !, T = ok(_).
legacy_submit_rule('NoOp', Label, Id, Min, Max, T) :- !, T = ok(_).
legacy_submit_rule('NoBlock', Label, Id, Min, Max, T) :- !, T = may(_).
legacy_submit_rule('NoOp', Label, Id, Min, Max, T) :- !, T = may(_).
legacy_submit_rule(Fun, Label, Id, Min, Max, T) :- T = impossible(unsupported(Fun)).