Update approvals in web UI to adapt to rules.pl submit_rule
The UI now shows whatever the results of the submit_rule are, which
permits the submit_rule to make an ApprovalCategory optional, or to
make a new label required. Currently making a new label required is
also going to make the change unsubmittable, as we do not yet have
a way to store the new label.
Change-Id: I9c6600c181e9f22ff980539d535caa8458d9a654
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java
index 9834693..318c60b 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java
@@ -21,9 +21,9 @@
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Comparator;
-import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
-import java.util.Map;
+import java.util.Set;
public class ApprovalDetail {
public static final Comparator<ApprovalDetail> SORT =
@@ -43,6 +43,8 @@
protected List<PatchSetApproval> approvals;
protected boolean canRemove;
+ private transient Set<String> approved;
+ private transient Set<String> rejected;
private transient int hasNonZero;
private transient Timestamp sortOrder = EG_D;
@@ -66,13 +68,17 @@
canRemove = removeable;
}
- public Map<ApprovalCategory.Id, PatchSetApproval> getApprovalMap() {
- final HashMap<ApprovalCategory.Id, PatchSetApproval> r;
- r = new HashMap<ApprovalCategory.Id, PatchSetApproval>();
- for (final PatchSetApproval ca : approvals) {
- r.put(ca.getCategoryId(), ca);
+ public List<PatchSetApproval> getPatchSetApprovals() {
+ return approvals;
+ }
+
+ public PatchSetApproval getPatchSetApproval(ApprovalCategory.Id category) {
+ for (PatchSetApproval psa : approvals) {
+ if (psa.getCategoryId().equals(category)) {
+ return psa;
+ }
}
- return r;
+ return null;
}
public void sortFirst() {
@@ -91,4 +97,26 @@
hasNonZero = 1;
}
}
+
+ public void approved(String label) {
+ if (approved == null) {
+ approved = new HashSet<String>();
+ }
+ approved.add(label);
+ }
+
+ public void rejected(String label) {
+ if (rejected == null) {
+ rejected = new HashSet<String>();
+ }
+ rejected.add(label);
+ }
+
+ public boolean isApproved(String label) {
+ return approved != null && approved.contains(label);
+ }
+
+ public boolean isRejected(String label) {
+ return rejected != null && rejected.contains(label);
+ }
}
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetail.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetail.java
index f196c05..c4c56d8 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetail.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetail.java
@@ -14,7 +14,6 @@
package com.google.gerrit.common.data;
-import com.google.gerrit.reviewdb.ApprovalCategory;
import com.google.gerrit.reviewdb.Change;
import com.google.gerrit.reviewdb.ChangeMessage;
import com.google.gerrit.reviewdb.PatchSet;
@@ -23,7 +22,6 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
-import java.util.Set;
/** Detail necessary to display a change. */
public class ChangeDetail {
@@ -38,7 +36,7 @@
protected List<ChangeInfo> neededBy;
protected List<PatchSet> patchSets;
protected List<ApprovalDetail> approvals;
- protected Set<ApprovalCategory.Id> missingApprovals;
+ protected List<SubmitRecord> submitRecords;
protected boolean canSubmit;
protected List<ChangeMessage> messages;
protected PatchSet.Id currentPatchSetId;
@@ -153,12 +151,12 @@
Collections.sort(approvals, ApprovalDetail.SORT);
}
- public Set<ApprovalCategory.Id> getMissingApprovals() {
- return missingApprovals;
+ public void setSubmitRecords(List<SubmitRecord> all) {
+ submitRecords = all;
}
- public void setMissingApprovals(Set<ApprovalCategory.Id> a) {
- missingApprovals = a;
+ public List<SubmitRecord> getSubmitRecords() {
+ return submitRecords;
}
public boolean isCurrentPatchSet(final PatchSetDetail detail) {
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitRecord.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitRecord.java
new file mode 100644
index 0000000..953ae99
--- /dev/null
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitRecord.java
@@ -0,0 +1,82 @@
+// Copyright (C) 2011 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.common.data;
+
+import com.google.gerrit.reviewdb.Account;
+
+import java.util.List;
+
+/**
+ * Describes the state required to submit a change.
+ */
+public class SubmitRecord {
+ public static enum Status {
+ /** The change is ready for submission. */
+ OK,
+
+ /** The change is missing a required label. */
+ NOT_READY,
+
+ /** The change has been closed. */
+ CLOSED,
+
+ /**
+ * An internal server error occurred preventing computation.
+ * <p>
+ * Additional detail may be available in {@link SubmitRecord#errorMessage}.
+ */
+ RULE_ERROR;
+ }
+
+ public Status status;
+ public List<Label> labels;
+ public String errorMessage;
+
+ public static class Label {
+ public static enum Status {
+ /**
+ * This label provides what is necessary for submission.
+ * <p>
+ * If provided, {@link Label#appliedBy} describes the user account
+ * that applied this label to the change.
+ */
+ OK,
+
+ /**
+ * This label prevents the change from being submitted.
+ * <p>
+ * If provided, {@link Label#appliedBy} describes the user account
+ * that applied this label to the change.
+ */
+ REJECT,
+
+ /**
+ * The label is required for submission, but has not been satisfied.
+ */
+ NEED,
+
+ /**
+ * The label is required for submission, but is impossible to complete.
+ * The likely cause is access has not been granted correctly by the
+ * project owner or site administrator.
+ */
+ IMPOSSIBLE;
+ }
+
+ public String label;
+ public Status status;
+ public Account.Id appliedBy;
+ }
+}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
index 4a5c1ce..9081e38 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
@@ -24,11 +24,11 @@
import com.google.gerrit.common.data.AccountInfoCache;
import com.google.gerrit.common.data.ApprovalDetail;
import com.google.gerrit.common.data.ApprovalType;
+import com.google.gerrit.common.data.ApprovalTypes;
import com.google.gerrit.common.data.ChangeDetail;
import com.google.gerrit.common.data.ReviewerResult;
+import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.Account;
-import com.google.gerrit.reviewdb.ApprovalCategory;
-import com.google.gerrit.reviewdb.ApprovalCategoryValue;
import com.google.gerrit.reviewdb.Change;
import com.google.gerrit.reviewdb.PatchSetApproval;
import com.google.gwt.event.dom.client.ClickEvent;
@@ -46,13 +46,15 @@
import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
-import java.util.Map;
-import java.util.Set;
/** Displays a table of {@link ApprovalDetail} objects for a change record. */
public class ApprovalTable extends Composite {
- private final List<ApprovalType> types;
+ private final ApprovalTypes types;
private final Grid table;
private final Widget missing;
private final Panel addReviewer;
@@ -61,10 +63,9 @@
private AccountInfoCache accountCache = AccountInfoCache.empty();
public ApprovalTable() {
- types = Gerrit.getConfig().getApprovalTypes().getApprovalTypes();
- table = new Grid(1, 3 + types.size());
+ types = Gerrit.getConfig().getApprovalTypes();
+ table = new Grid(1, 3);
table.addStyleName(Gerrit.RESOURCES.css().infoTable());
- displayHeader();
missing = new Widget() {
{
@@ -95,7 +96,9 @@
setStyleName(Gerrit.RESOURCES.css().approvalTable());
}
- private void displayHeader() {
+ private void displayHeader(List<String> labels) {
+ table.resizeColumns(2 + labels.size());
+
final CellFormatter fmt = table.getCellFormatter();
int col = 0;
@@ -107,16 +110,12 @@
fmt.setStyleName(0, col, Gerrit.RESOURCES.css().header());
col++;
- for (final ApprovalType t : types) {
- table.setText(0, col, t.getCategory().getName());
+ for (String name : labels) {
+ table.setText(0, col, name);
fmt.setStyleName(0, col, Gerrit.RESOURCES.css().header());
col++;
}
-
- table.clearCell(0, col);
- fmt.setStyleName(0, col, Gerrit.RESOURCES.css().header());
- fmt.addStyleName(0, col, Gerrit.RESOURCES.css().rightmost());
- col++;
+ fmt.addStyleName(0, col - 1, Gerrit.RESOURCES.css().rightmost());
}
public void setAccountInfoCache(final AccountInfoCache aic) {
@@ -128,40 +127,115 @@
return AccountDashboardLink.link(accountCache, id);
}
- public void display(final Change change, final Set<ApprovalCategory.Id> need,
- final List<ApprovalDetail> rows) {
- changeId = change.getId();
+ void display(ChangeDetail detail) {
+ List<String> columns = new ArrayList<String>();
+ List<ApprovalDetail> rows = detail.getApprovals();
- if (rows.isEmpty()) {
- table.setVisible(false);
- } else {
- table.resizeRows(1 + rows.size());
- for (int i = 0; i < rows.size(); i++) {
- displayRow(i + 1, rows.get(i), change);
- }
- table.setVisible(true);
- }
+ changeId = detail.getChange().getId();
final Element missingList = missing.getElement();
while (DOM.getChildCount(missingList) > 0) {
DOM.removeChild(missingList, DOM.getChild(missingList, 0));
}
-
missing.setVisible(false);
- if (need != null) {
- for (final ApprovalType at : types) {
- if (need.contains(at.getCategory().getId())) {
- final Element li = DOM.createElement("li");
- li.setClassName(Gerrit.RESOURCES.css().missingApproval());
- DOM.setInnerText(li, Util.M.needApproval(at.getCategory().getName(),
- at.getMax().formatValue(), at.getMax().getName()));
- DOM.appendChild(missingList, li);
- missing.setVisible(true);
+
+ if (detail.getSubmitRecords() != null) {
+ HashSet<String> reportedMissing = new HashSet<String>();
+
+ HashMap<Account.Id, ApprovalDetail> byUser =
+ new HashMap<Account.Id, ApprovalDetail>();
+ for (ApprovalDetail ad : detail.getApprovals()) {
+ byUser.put(ad.getAccount(), ad);
+ }
+
+ for (SubmitRecord rec : detail.getSubmitRecords()) {
+ if (rec.labels == null) {
+ continue;
}
+
+ for (SubmitRecord.Label lbl : rec.labels) {
+ if (!columns.contains(lbl.label)) {
+ columns.add(lbl.label);
+ }
+
+ switch (lbl.status) {
+ case OK: {
+ ApprovalDetail ad = byUser.get(lbl.appliedBy);
+ if (ad != null) {
+ ad.approved(lbl.label);
+ }
+ break;
+ }
+
+ case REJECT: {
+ ApprovalDetail ad = byUser.get(lbl.appliedBy);
+ if (ad != null) {
+ ad.rejected(lbl.label);
+ }
+ break;
+ }
+
+ case NEED:
+ case IMPOSSIBLE:
+ if (reportedMissing.add(lbl.label)) {
+ Element li = DOM.createElement("li");
+ li.setClassName(Gerrit.RESOURCES.css().missingApproval());
+ DOM.setInnerText(li, Util.M.needApproval(lbl.label));
+ DOM.appendChild(missingList, li);
+ }
+ break;
+ }
+ }
+ }
+ missing.setVisible(!reportedMissing.isEmpty());
+
+ } else {
+ for (ApprovalDetail ad : rows) {
+ for (PatchSetApproval psa : ad.getPatchSetApprovals()) {
+ ApprovalType legacyType = types.byId(psa.getCategoryId());
+ if (legacyType == null) {
+ continue;
+ }
+ String labelName = legacyType.getCategory().getLabelName();
+ if (psa.getValue() == legacyType.getMax().getValue()) {
+ ad.approved(labelName);
+ } else if (psa.getValue() == legacyType.getMin().getValue()) {
+ ad.rejected(labelName);
+ }
+ if (!columns.contains(labelName)) {
+ columns.add(labelName);
+ }
+ }
+ Collections.sort(columns, new Comparator<String>() {
+ @Override
+ public int compare(String o1, String o2) {
+ ApprovalType a = types.byLabel(o1);
+ ApprovalType b = types.byLabel(o2);
+ int cmp = 0;
+ if (a != null && b != null) {
+ cmp = a.getCategory().getPosition() - b.getCategory().getPosition();
+ }
+ if (cmp == 0) {
+ cmp = o1.compareTo(o2);
+ }
+ return cmp;
+ }
+ });
}
}
- addReviewer.setVisible(Gerrit.isSignedIn() && change.getStatus().isOpen());
+ if (rows.isEmpty()) {
+ table.setVisible(false);
+ } else {
+ displayHeader(columns);
+ table.resizeRows(1 + rows.size());
+ for (int i = 0; i < rows.size(); i++) {
+ displayRow(i + 1, rows.get(i), detail.getChange(), columns);
+ }
+ table.setVisible(true);
+ }
+
+ addReviewer.setVisible(Gerrit.isSignedIn() && detail.getChange().getStatus().isOpen());
}
private void doAddReviewer() {
@@ -210,10 +284,11 @@
final ChangeDetail r = result.getChange();
if (r != null) {
setAccountInfoCache(r.getAccounts());
- display(r.getChange(), r.getMissingApprovals(), r.getApprovals());
+ display(r);
}
}
+
@Override
public void onFailure(final Throwable caught) {
addMemberBox.setEnabled(true);
@@ -223,10 +298,8 @@
}
private void displayRow(final int row, final ApprovalDetail ad,
- final Change change) {
+ final Change change, List<String> columns) {
final CellFormatter fmt = table.getCellFormatter();
- final Map<ApprovalCategory.Id, PatchSetApproval> am = ad.getApprovalMap();
- final StringBuilder hint = new StringBuilder();
int col = 0;
table.setWidget(row, col++, link(ad.getAccount()));
@@ -250,31 +323,30 @@
}
fmt.setStyleName(row, col++, Gerrit.RESOURCES.css().removeReviewerCell());
- for (final ApprovalType type : types) {
+ for (String labelName : columns) {
fmt.setStyleName(row, col, Gerrit.RESOURCES.css().approvalscore());
- final PatchSetApproval ca = am.get(type.getCategory().getId());
- if (ca == null || ca.getValue() == 0) {
- table.clearCell(row, col);
- col++;
- continue;
- }
-
- final ApprovalCategoryValue acv = type.getValue(ca);
- if (acv != null) {
- if (hint.length() > 0) {
- hint.append("; ");
- }
- hint.append(acv.getName());
- }
-
- if (type.isMaxNegative(ca)) {
+ if (ad.isRejected(labelName)) {
table.setWidget(row, col, new Image(Gerrit.RESOURCES.redNot()));
- } else if (type.isMaxPositive(ca)) {
+ } else if (ad.isApproved(labelName)) {
table.setWidget(row, col, new Image(Gerrit.RESOURCES.greenCheck()));
} else {
+ ApprovalType legacyType = types.byLabel(labelName);
+ if (legacyType == null) {
+ table.clearCell(row, col);
+ col++;
+ continue;
+ }
+
+ PatchSetApproval ca = ad.getPatchSetApproval(legacyType.getCategory().getId());
+ if (ca == null || ca.getValue() == 0) {
+ table.clearCell(row, col);
+ col++;
+ continue;
+ }
+
String vstr = String.valueOf(ca.getValue());
if (ca.getValue() > 0) {
vstr = "+" + vstr;
@@ -288,10 +360,7 @@
col++;
}
- table.setText(row, col, hint.toString());
- fmt.setStyleName(row, col, Gerrit.RESOURCES.css().rightmost());
- fmt.addStyleName(row, col, Gerrit.RESOURCES.css().approvalhint());
- col++;
+ fmt.addStyleName(row, col - 1, Gerrit.RESOURCES.css().rightmost());
}
private void doRemove(final ApprovalDetail ad, final PushButton remove) {
@@ -302,7 +371,7 @@
public void onSuccess(ReviewerResult result) {
if (result.getErrors().isEmpty()) {
final ChangeDetail r = result.getChange();
- display(r.getChange(), r.getMissingApprovals(), r.getApprovals());
+ display(r);
} else {
final ReviewerResult.Error resultError =
result.getErrors().get(0);
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
index cbf4a6b..53d1c18 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
@@ -43,7 +43,7 @@
String copiedFrom(String sourcePath);
String otherFrom(String sourcePath);
- String needApproval(String categoryName, String value, String valueName);
+ String needApproval(String labelName);
String publishComments(String changeId, int ps);
String lineHeader(int line);
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
index 5982ad5..6f6e34e2 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
@@ -24,7 +24,7 @@
copiedFrom = copied from {0}
otherFrom = from {0}
-needApproval = Need {0} {1} ({2})
+needApproval = Need {0}
publishComments = Change {0} - Patch Set {1}: Publish Comments
lineHeader = Line {0}:
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java
index edb25e4..6bd4ebf 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java
@@ -306,8 +306,7 @@
.getCurrentPatchSetDetail().getInfo(), detail.getAccounts());
dependsOn.display(detail.getDependsOn());
neededBy.display(detail.getNeededBy());
- approvals.display(detail.getChange(), detail.getMissingApprovals(), detail
- .getApprovals());
+ approvals.display(detail);
for (PatchSet pId : detail.getPatchSets()) {
if (patchesList != null) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
index f7f8ae1..274f8f5 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
@@ -28,7 +28,6 @@
import com.google.gerrit.common.data.ApprovalTypes;
import com.google.gerrit.common.data.ChangeDetail;
import com.google.gerrit.common.data.PatchSetPublishDetail;
-import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.reviewdb.ApprovalCategory;
import com.google.gerrit.reviewdb.ApprovalCategoryValue;
@@ -221,18 +220,13 @@
private void initApprovals(final PatchSetPublishDetail r, final Panel body) {
ApprovalTypes types = Gerrit.getConfig().getApprovalTypes();
-
- for (ApprovalType type : types.getApprovalTypes()) {
- String permission = Permission.forLabel(type.getCategory().getLabelName());
- PermissionRange range = r.getRange(permission);
- if (range != null && !range.isEmpty()) {
- initApprovalType(r, body, type, range);
- }
- }
-
for (PermissionRange range : r.getLabels()) {
- if (!range.isEmpty() && types.byLabel(range.getLabel()) == null) {
- // TODO: this is a non-standard label. Offer it without the type.
+ ApprovalType type = types.byLabel(range.getLabel());
+ if (type != null) {
+ // Legacy type, use radio buttons.
+ initApprovalType(r, body, type, range);
+ } else {
+ // TODO Newer style label.
}
}
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
index fc3fb8f..2705991 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
@@ -19,10 +19,10 @@
import com.google.gerrit.common.data.ApprovalTypes;
import com.google.gerrit.common.data.ChangeDetail;
import com.google.gerrit.common.data.ChangeInfo;
+import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.errors.NoSuchEntityException;
import com.google.gerrit.httpd.rpc.Handler;
import com.google.gerrit.reviewdb.Account;
-import com.google.gerrit.reviewdb.ApprovalCategory;
import com.google.gerrit.reviewdb.Change;
import com.google.gerrit.reviewdb.ChangeMessage;
import com.google.gerrit.reviewdb.PatchSet;
@@ -34,7 +34,6 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountInfoCacheFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
-import com.google.gerrit.server.project.CanSubmitResult;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.workflow.CategoryFunction;
@@ -99,7 +98,6 @@
if (patch == null) {
throw new NoSuchEntityException();
}
- final CanSubmitResult canSubmitResult = control.canSubmit(patch.getId());
aic.want(change.getOwner());
@@ -109,12 +107,26 @@
detail.setCanAbandon(change.getStatus().isOpen() && control.canAbandon());
detail.setCanRestore(change.getStatus() == Change.Status.ABANDONED && control.canRestore());
- detail.setCanSubmit(canSubmitResult == CanSubmitResult.OK);
detail.setStarred(control.getCurrentUser().getStarredChanges().contains(
changeId));
detail.setCanRevert(change.getStatus() == Change.Status.MERGED && control.canAddPatchSet());
+ if (detail.getChange().getStatus().isOpen()) {
+ List<SubmitRecord> submitRecords = control.canSubmit(db, patch.getId());
+ 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()) {
+ detail.setCanSubmit(true);
+ }
+ }
+ detail.setSubmitRecords(submitRecords);
+ }
+
loadPatchSets();
loadMessages();
if (change.currentPatchSetId() != null) {
@@ -145,16 +157,9 @@
final FunctionState fs =
functionState.create(detail.getChange(), psId, allApprovals);
- final Set<ApprovalCategory.Id> missingApprovals =
- new HashSet<ApprovalCategory.Id>();
-
for (final ApprovalType at : approvalTypes.getApprovalTypes()) {
CategoryFunction.forCategory(at.getCategory()).run(at, fs);
- if (!fs.isValid(at)) {
- missingApprovals.add(at.getCategory().getId());
- }
}
- detail.setMissingApprovals(missingApprovals);
}
final boolean canRemoveReviewers = detail.getChange().getStatus().isOpen() //
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
index 58daa09..ded92e0 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
@@ -14,9 +14,11 @@
package com.google.gerrit.httpd.rpc.changedetail;
-import com.google.gerrit.common.data.AccountInfoCache;
+import com.google.gerrit.common.data.ApprovalType;
+import com.google.gerrit.common.data.ApprovalTypes;
import com.google.gerrit.common.data.PatchSetPublishDetail;
import com.google.gerrit.common.data.PermissionRange;
+import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.httpd.rpc.Handler;
import com.google.gerrit.reviewdb.Change;
import com.google.gerrit.reviewdb.PatchLineComment;
@@ -28,7 +30,6 @@
import com.google.gerrit.server.account.AccountInfoCacheFactory;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
-import com.google.gerrit.server.project.CanSubmitResult;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.client.OrmException;
@@ -37,7 +38,9 @@
import java.util.ArrayList;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail> {
interface Factory {
@@ -47,12 +50,12 @@
private final PatchSetInfoFactory infoFactory;
private final ReviewDb db;
private final ChangeControl.Factory changeControlFactory;
+ private final ApprovalTypes approvalTypes;
private final AccountInfoCacheFactory aic;
private final IdentifiedUser user;
private final PatchSet.Id patchSetId;
- private AccountInfoCache accounts;
private PatchSetInfo patchSetInfo;
private Change change;
private List<PatchLineComment> drafts;
@@ -62,10 +65,12 @@
final ReviewDb db,
final AccountInfoCacheFactory.Factory accountInfoCacheFactory,
final ChangeControl.Factory changeControlFactory,
+ final ApprovalTypes approvalTypes,
final IdentifiedUser user, @Assisted final PatchSet.Id patchSetId) {
this.infoFactory = infoFactory;
this.db = db;
this.changeControlFactory = changeControlFactory;
+ this.approvalTypes = approvalTypes;
this.aic = accountInfoCacheFactory.create();
this.user = user;
@@ -81,32 +86,92 @@
patchSetInfo = infoFactory.get(patchSetId);
drafts = db.patchComments().draft(patchSetId, user.getAccountId()).toList();
+ aic.want(change.getOwner());
+
+ PatchSetPublishDetail detail = new PatchSetPublishDetail();
+ detail.setPatchSetInfo(patchSetInfo);
+ detail.setChange(change);
+ detail.setDrafts(drafts);
+
List<PermissionRange> allowed = Collections.emptyList();
List<PatchSetApproval> given = Collections.emptyList();
if (change.getStatus().isOpen()
&& patchSetId.equals(change.currentPatchSetId())) {
- allowed = new ArrayList<PermissionRange>(control.getLabelRanges());
- Collections.sort(allowed);
+ // TODO Push this selection of labels down into the Prolog interpreter.
+ // Ideally we discover the labels the user can apply here based on doing
+ // a findall() over the space of labels they can apply combined against
+ // the submit rule, thereby skipping any mutually exclusive cases. However
+ // those are not common, so it might just be reasonable to take this
+ // simple approach.
+
+ Map<String, PermissionRange> rangeByName =
+ new HashMap<String, PermissionRange>();
+ for (PermissionRange r : control.getLabelRanges()) {
+ if (r.isLabel()) {
+ rangeByName.put(r.getLabel(), r);
+ }
+ }
+ allowed = new ArrayList<PermissionRange>();
given = db.patchSetApprovals() //
.byPatchSetUser(patchSetId, user.getAccountId()) //
.toList();
+
+ boolean couldSubmit = false;
+ List<SubmitRecord> submitRecords = control.canSubmit(db, patchSetId);
+ for (SubmitRecord rec : submitRecords) {
+ if (rec.status == SubmitRecord.Status.OK) {
+ couldSubmit = true;
+ }
+
+ if (rec.labels != null) {
+ int ok = 0;
+
+ for (SubmitRecord.Label lbl : rec.labels) {
+ boolean canMakeOk = false;
+ PermissionRange range = rangeByName.get(lbl.label);
+ if (range != null) {
+ if (!allowed.contains(range)) {
+ allowed.add(range);
+ }
+
+ ApprovalType at = approvalTypes.byLabel(lbl.label);
+ if (at != null && at.getMax().getValue() == range.getMax()) {
+ canMakeOk = true;
+ } else if (at == null) {
+ canMakeOk = true;
+ }
+ }
+
+ switch (lbl.status) {
+ case OK:
+ ok++;
+ break;
+
+ case NEED:
+ if (canMakeOk) {
+ ok++;
+ }
+ break;
+ }
+ }
+
+ if (rec.status == SubmitRecord.Status.NOT_READY
+ && ok == rec.labels.size()) {
+ couldSubmit = true;
+ }
+ }
+ }
+
+ if (couldSubmit && control.getRefControl().canSubmit()) {
+ detail.setCanSubmit(true);
+ }
}
- aic.want(change.getOwner());
- accounts = aic.create();
-
- PatchSetPublishDetail detail = new PatchSetPublishDetail();
- detail.setAccounts(accounts);
- detail.setPatchSetInfo(patchSetInfo);
- detail.setChange(change);
- detail.setDrafts(drafts);
detail.setLabels(allowed);
detail.setGiven(given);
-
- final CanSubmitResult canSubmitResult = control.canSubmit(patchSetId);
- detail.setCanSubmit(canSubmitResult == CanSubmitResult.OK);
+ detail.setAccounts(aic.create());
return detail;
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/SubmitAction.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/SubmitAction.java
index cbefae6..909e59a 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/SubmitAction.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/SubmitAction.java
@@ -15,6 +15,7 @@
package com.google.gerrit.httpd.rpc.changedetail;
import com.google.gerrit.common.data.ChangeDetail;
+import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.errors.NoSuchEntityException;
import com.google.gerrit.httpd.rpc.Handler;
import com.google.gerrit.reviewdb.Change;
@@ -25,13 +26,14 @@
import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.git.MergeQueue;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
-import com.google.gerrit.server.project.CanSubmitResult;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.client.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.List;
+
class SubmitAction extends Handler<ChangeDetail> {
interface Factory {
SubmitAction create(PatchSet.Id patchSetId);
@@ -72,12 +74,51 @@
final ChangeControl changeControl =
changeControlFactory.validateFor(changeId);
- CanSubmitResult err = changeControl.canSubmit(db, patchSetId);
- if (err == CanSubmitResult.OK) {
- ChangeUtil.submit(patchSetId, user, db, opFactory, merger);
- return changeDetailFactory.create(changeId).call();
- } else {
- throw new IllegalStateException(err.getMessage());
+ List<SubmitRecord> result = changeControl.canSubmit(db, patchSetId);
+ if (result.isEmpty()) {
+ throw new IllegalStateException("Cannot submit");
+ }
+
+ switch (result.get(0).status) {
+ case OK:
+ ChangeUtil.submit(patchSetId, user, db, opFactory, merger);
+ return changeDetailFactory.create(changeId).call();
+
+ case NOT_READY: {
+ StringBuilder msg = new StringBuilder();
+ for (SubmitRecord.Label lbl : result.get(0).labels) {
+ switch (lbl.status) {
+ case OK:
+ break;
+
+ case REJECT:
+ throw new IllegalStateException("Blocked by " + lbl.label);
+
+ case NEED:
+ throw new IllegalStateException("Needs " + lbl.label);
+
+ case IMPOSSIBLE:
+ throw new IllegalStateException("Cannnot submit, check project access");
+
+ default:
+ throw new IllegalArgumentException("Unknown status " + lbl.status);
+ }
+ }
+ throw new IllegalStateException("Cannot submit");
+ }
+
+ case CLOSED:
+ throw new IllegalStateException("Change is closed");
+
+ case RULE_ERROR:
+ if (result.get(0).errorMessage != null) {
+ throw new IllegalStateException(result.get(0).errorMessage);
+ } else {
+ throw new IllegalStateException("Internal rule error");
+ }
+
+ default:
+ throw new IllegalStateException("Uknown status " + result.get(0).status);
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CanSubmitResult.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CanSubmitResult.java
deleted file mode 100644
index 0c14b80..0000000
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CanSubmitResult.java
+++ /dev/null
@@ -1,43 +0,0 @@
-// Copyright (C) 2010 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.project;
-
-/**
- * Result from {@code ChangeControl.canSubmit()}.
- *
- * @see ChangeControl#canSubmit(com.google.gerrit.reviewdb.PatchSet.Id,
- * com.google.gerrit.reviewdb.ReviewDb,
- * com.google.gerrit.common.data.ApprovalTypes,
- * com.google.gerrit.server.workflow.FunctionState.Factory)
- */
-public class CanSubmitResult {
- /** Magic constant meaning submitting is possible. */
- public static final CanSubmitResult OK = new CanSubmitResult("OK");
-
- private final String errorMessage;
-
- CanSubmitResult(String error) {
- this.errorMessage = error;
- }
-
- public String getMessage() {
- return errorMessage;
- }
-
- @Override
- public String toString() {
- return "CanSubmitResult[" + getMessage() + "]";
- }
-}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 8180759..72db734 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -15,6 +15,8 @@
package com.google.gerrit.server.project;
import com.google.gerrit.common.data.PermissionRange;
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.reviewdb.Account;
import com.google.gerrit.reviewdb.Change;
import com.google.gerrit.reviewdb.PatchSet;
import com.google.gerrit.reviewdb.PatchSetApproval;
@@ -39,6 +41,7 @@
import org.slf4j.LoggerFactory;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
@@ -224,36 +227,29 @@
return false;
}
- /** @return {@link CanSubmitResult#OK}, or a result with an error message. */
- public CanSubmitResult canSubmit(final PatchSet.Id patchSetId) {
+ public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
if (change.getStatus().isClosed()) {
- return new CanSubmitResult("Change " + change.getId() + " is closed");
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.CLOSED;
+ return Collections.singletonList(rec);
}
- if (!patchSetId.equals(change.currentPatchSetId())) {
- return new CanSubmitResult("Patch set " + patchSetId + " is not current");
- }
- if (!getRefControl().canSubmit()) {
- return new CanSubmitResult("User does not have permission to submit");
- }
- if (!(getCurrentUser() instanceof IdentifiedUser)) {
- return new CanSubmitResult("User is not signed-in");
- }
- return CanSubmitResult.OK;
- }
- /** @return {@link CanSubmitResult#OK}, or a result with an error message. */
- public CanSubmitResult canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
- CanSubmitResult result = canSubmit(patchSetId);
- if (result != CanSubmitResult.OK) {
- return result;
+ if (!patchSetId.equals(change.currentPatchSetId())) {
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Patch set " + patchSetId + " is not current";
+ return Collections.singletonList(rec);
}
PrologEnvironment env;
try {
env = getProjectControl().getProjectState().newPrologEnvironment();
} catch (CompileException err) {
- log.error("cannot consult rules.pl", err);
- return new CanSubmitResult("Error reading submit rule");
+ log.error("Cannot consult rules.pl for " + getProject().getName(), err);
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error loading project rules, check server log";
+ return Collections.singletonList(rec);
}
env.set(StoredValues.REVIEW_DB, db);
@@ -265,8 +261,11 @@
"gerrit", "locate_submit_rule",
new VariableTerm());
if (submitRule == null) {
- log.error("Error in locate_submit_rule: no submit_rule found");
- return new CanSubmitResult("Error in finding submit rule");
+ log.error("No user:submit_rule found for " + getProject().getName());
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error loading project rules, check server log";
+ return Collections.singletonList(rec);
}
List<Term> results = new ArrayList<Term>();
@@ -278,8 +277,19 @@
results.add(template[1]);
}
} catch (PrologException err) {
- log.error("PrologException calling " + submitRule, err);
- return new CanSubmitResult("Error in submit rule");
+ log.error("Exception calling " + submitRule + " on change "
+ + change.getId() + " of " + getProject().getName(), err);
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error evaluating project rules, check server log";
+ return Collections.singletonList(rec);
+ } catch (RuntimeException err) {
+ log.error("Exception calling " + submitRule + " on change "
+ + change.getId() + " of " + getProject().getName(), err);
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error evaluating project rules, check server log";
+ return Collections.singletonList(rec);
}
if (results.isEmpty()) {
@@ -287,65 +297,133 @@
// at least one result informing the caller of the labels that are
// required for this change to be submittable. Each label will indicate
// whether or not that is actually possible given the permissions.
- log.error("Submit rule has no solution: " + submitRule);
- return new CanSubmitResult("Error in submit rule (no solution possible)");
+ log.error("Submit rule " + submitRule + " for change " + change.getId()
+ + " of " + getProject().getName() + " has no solution.");
+ SubmitRecord rec = new SubmitRecord();
+ rec.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Project submit rule has no solution";
+ return Collections.singletonList(rec);
}
- // The last result produced will be an "ok(P)" format if submit is possible.
- // This is always true because can_submit (called above) will cut away all
- // choice points once a solution is found.
- Term last = results.get(results.size() - 1);
- if (last.isStructure() && 1 == last.arity() && "ok".equals(last.name())) {
- // Term solution = last.arg(0);
- return CanSubmitResult.OK;
- }
+ // Convert the results from Prolog Cafe's format to Gerrit's common format.
+ // can_submit/1 terminates when an ok(P) record is found. Therefore walk
+ // the results backwards, using only that ok(P) record if it exists. This
+ // skips partial results that occur early in the output. Later after the loop
+ // the out collection is reversed to restore it to the original ordering.
+ //
+ List<SubmitRecord> out = new ArrayList<SubmitRecord>(results.size());
+ for (int resultIdx = results.size() - 1; 0 <= resultIdx; resultIdx--) {
+ Term submitRecord = results.get(resultIdx);
+ SubmitRecord rec = new SubmitRecord();
+ out.add(rec);
- // For now only process the first result. Later we can examine all of the
- // results and proposes different alternative paths to a submit solution.
- Term first = results.get(0);
- if (!first.isStructure() || 1 != first.arity() || !"not_ready".equals(first.name())) {
- log.error("Unexpected result from can_submit: " + first);
- return new CanSubmitResult("Error in submit rule");
- }
-
- Term submitRecord = first.arg(0);
- if (!submitRecord.isStructure()) {
- log.error("Invalid result from submit rule " + submitRule + ": " + submitRecord);
- return new CanSubmitResult("Error in submit rule");
- }
-
- for (Term state : ((StructureTerm) submitRecord).args()) {
- if (!state.isStructure() || 2 != state.arity() || !"label".equals(state.name())) {
- log.error("Invalid result from submit rule " + submitRule + ": " + submitRecord);
- return new CanSubmitResult("Invalid submit rule result");
+ if (!submitRecord.isStructure() || 1 != submitRecord.arity()) {
+ log.error("Submit rule " + submitRule + " for change " + change.getId()
+ + " of " + getProject().getName() + " output invalid result: "
+ + submitRecord);
+ SubmitRecord err = new SubmitRecord();
+ err.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error evaluating project rules, check server log";
+ return Collections.singletonList(err);
}
- String label = state.arg(0).name();
- Term status = state.arg(1);
+ if ("ok".equals(submitRecord.name())) {
+ rec.status = SubmitRecord.Status.OK;
- if ("ok".equals(status.name())) {
- continue;
-
- } else if ("reject".equals(status.name())) {
- return new CanSubmitResult("Submit blocked by " + label);
-
- } else if ("need".equals(status.name())) {
- if (status.isStructure() && status.arg(0).isInteger()) {
- IntegerTerm val = (IntegerTerm) status.arg(0);
- if (1 < val.intValue()) {
- label += "+" + val.intValue();
- }
- }
- return new CanSubmitResult("Requires " + label);
-
- } else if ("impossble".equals(status.name())) {
- return new CanSubmitResult("Requires " + label + " (check permissions)");
+ } else if ("not_ready".equals(submitRecord.name())) {
+ rec.status = SubmitRecord.Status.NOT_READY;
} else {
- return new CanSubmitResult("Invalid submit rule result");
+ log.error("Submit rule " + submitRule + " for change " + change.getId()
+ + " of " + getProject().getName() + " output invalid result: "
+ + submitRecord);
+ SubmitRecord err = new SubmitRecord();
+ err.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error evaluating project rules, check server log";
+ return Collections.singletonList(err);
+ }
+
+ // Unpack the one argument. This should also be a structure with one
+ // argument per label that needs to be reported on to the caller.
+ //
+ submitRecord = submitRecord.arg(0);
+
+ if (!submitRecord.isStructure()) {
+ log.error("Submit rule " + submitRule + " for change " + change.getId()
+ + " of " + getProject().getName() + " output invalid result: "
+ + submitRecord);
+ SubmitRecord err = new SubmitRecord();
+ err.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error evaluating project rules, check server log";
+ return Collections.singletonList(err);
+ }
+
+ rec.labels = new ArrayList<SubmitRecord.Label> (submitRecord.arity());
+
+ for (Term state : ((StructureTerm) submitRecord).args()) {
+ if (!state.isStructure() || 2 != state.arity() || !"label".equals(state.name())) {
+ log.error("Submit rule " + submitRule + " for change " + change.getId()
+ + " of " + getProject().getName() + " output invalid result: "
+ + submitRecord);
+ SubmitRecord err = new SubmitRecord();
+ err.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error evaluating project rules, check server log";
+ return Collections.singletonList(err);
+ }
+
+ SubmitRecord.Label lbl = new SubmitRecord.Label();
+ rec.labels.add(lbl);
+
+ lbl.label = state.arg(0).name();
+ Term status = state.arg(1);
+
+ if ("ok".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.OK;
+ appliedBy(lbl, status);
+
+ } else if ("reject".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.REJECT;
+ appliedBy(lbl, status);
+
+ } else if ("need".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.NEED;
+
+ } else if ("impossible".equals(status.name())) {
+ lbl.status = SubmitRecord.Label.Status.IMPOSSIBLE;
+
+ } else {
+ log.error("Submit rule " + submitRule + " for change " + change.getId()
+ + " of " + getProject().getName() + " output invalid result: "
+ + submitRecord);
+ SubmitRecord err = new SubmitRecord();
+ err.status = SubmitRecord.Status.RULE_ERROR;
+ rec.errorMessage = "Error evaluating project rules, check server log";
+ return Collections.singletonList(err);
+ }
+ }
+
+ if (rec.status == SubmitRecord.Status.OK) {
+ break;
}
}
+ Collections.reverse(out);
- return CanSubmitResult.OK;
+ return out;
+ }
+
+ private void appliedBy(SubmitRecord.Label label, Term status) {
+ if (status.isStructure() && status.arity() == 1) {
+ Term who = status.arg(0);
+ if (isUser(who)) {
+ label.appliedBy = new Account.Id(((IntegerTerm) who.arg(0)).intValue());
+ }
+ }
+ }
+
+ private static boolean isUser(Term who) {
+ return who.isStructure()
+ && who.arity() == 1
+ && who.name().equals("user")
+ && who.arg(0).isInteger();
}
}
\ No newline at end of file
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index 93b2ba5..6404898 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -17,6 +17,7 @@
import com.google.gerrit.common.ChangeHookRunner;
import com.google.gerrit.common.data.ApprovalType;
import com.google.gerrit.common.data.ApprovalTypes;
+import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.reviewdb.ApprovalCategory;
import com.google.gerrit.reviewdb.ApprovalCategoryValue;
import com.google.gerrit.reviewdb.Branch;
@@ -32,7 +33,6 @@
import com.google.gerrit.server.mail.AbandonedSender;
import com.google.gerrit.server.mail.EmailException;
import com.google.gerrit.server.patch.PublishComments;
-import com.google.gerrit.server.project.CanSubmitResult;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
@@ -204,7 +204,7 @@
}
private void approveOne(final PatchSet.Id patchSetId) throws
- NoSuchChangeException, UnloggedFailure, OrmException, EmailException {
+ NoSuchChangeException, OrmException, EmailException, Failure {
final Change.Id changeId = patchSetId.getParentKey();
ChangeControl changeControl = changeControlFactory.validateFor(changeId);
@@ -250,11 +250,61 @@
}
if (submitChange) {
- CanSubmitResult result = changeControl.canSubmit(db, patchSetId);
- if (result == CanSubmitResult.OK) {
- toSubmit.add(patchSetId);
- } else {
- throw error(result.getMessage());
+ List<SubmitRecord> result = changeControl.canSubmit(db, patchSetId);
+ if (result.isEmpty()) {
+ throw new Failure(1, "ChangeControl.canSubmit returned empty list");
+ }
+ switch (result.get(0).status) {
+ case OK:
+ if (changeControl.getRefControl().canSubmit()) {
+ toSubmit.add(patchSetId);
+ } else {
+ throw error("change " + changeId + ": you do not have submit permission");
+ }
+ break;
+
+ case NOT_READY: {
+ StringBuilder msg = new StringBuilder();
+ for (SubmitRecord.Label lbl : result.get(0).labels) {
+ switch (lbl.status) {
+ case OK:
+ break;
+
+ case REJECT:
+ if (msg.length() > 0) msg.append("\n");
+ msg.append("change " + changeId + ": blocked by " + lbl.label);
+ break;
+
+ case NEED:
+ if (msg.length() > 0) msg.append("\n");
+ msg.append("change " + changeId + ": needs " + lbl.label);
+ break;
+
+ case IMPOSSIBLE:
+ if (msg.length() > 0) msg.append("\n");
+ msg.append("change " + changeId + ": needs " + lbl.label
+ + " (check project access)");
+ break;
+
+ default:
+ throw new Failure(1, "Unsupported label status " + lbl.status);
+ }
+ }
+ throw error(msg.toString());
+ }
+
+ case CLOSED:
+ throw error("change " + changeId + " is closed");
+
+ case RULE_ERROR:
+ if (result.get(0).errorMessage != null) {
+ throw error("change " + changeId + ": " + result.get(0).errorMessage);
+ } else {
+ throw error("change " + changeId + ": internal rule error");
+ }
+
+ default:
+ throw new Failure(1, "Unsupported status " + result.get(0).status);
}
}
}
