From 02630e708c52a9403e1371ec7d7561d92496dc30 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 20 Jun 2011 15:03:31 -0700 Subject: [PATCH] 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 --- .../gerrit/common/data/ApprovalDetail.java | 44 +++- .../gerrit/common/data/ChangeDetail.java | 12 +- .../gerrit/common/data/SubmitRecord.java | 82 +++++++ .../gerrit/client/changes/ApprovalTable.java | 201 +++++++++++----- .../gerrit/client/changes/ChangeMessages.java | 2 +- .../client/changes/ChangeMessages.properties | 2 +- .../gerrit/client/changes/ChangeScreen.java | 3 +- .../client/changes/PublishCommentScreen.java | 18 +- .../rpc/changedetail/ChangeDetailFactory.java | 27 ++- .../PatchSetPublishDetailFactory.java | 97 ++++++-- .../httpd/rpc/changedetail/SubmitAction.java | 55 ++++- .../server/project/CanSubmitResult.java | 43 ---- .../gerrit/server/project/ChangeControl.java | 226 ++++++++++++------ .../gerrit/sshd/commands/ReviewCommand.java | 64 ++++- 14 files changed, 621 insertions(+), 255 deletions(-) create mode 100644 gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitRecord.java delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/project/CanSubmitResult.java 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 98346932cd..318c60b8c2 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 com.google.gerrit.reviewdb.PatchSetApproval; 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 SORT = @@ -43,6 +43,8 @@ public class ApprovalDetail { protected List approvals; protected boolean canRemove; + private transient Set approved; + private transient Set rejected; private transient int hasNonZero; private transient Timestamp sortOrder = EG_D; @@ -66,13 +68,17 @@ public class ApprovalDetail { canRemove = removeable; } - public Map getApprovalMap() { - final HashMap r; - r = new HashMap(); - for (final PatchSetApproval ca : approvals) { - r.put(ca.getCategoryId(), ca); + public List 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 @@ public class ApprovalDetail { hasNonZero = 1; } } + + public void approved(String label) { + if (approved == null) { + approved = new HashSet(); + } + approved.add(label); + } + + public void rejected(String label) { + if (rejected == null) { + rejected = new HashSet(); + } + 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 f196c058f8..c4c56d80e4 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.ArrayList; 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 @@ public class ChangeDetail { protected List neededBy; protected List patchSets; protected List approvals; - protected Set missingApprovals; + protected List submitRecords; protected boolean canSubmit; protected List messages; protected PatchSet.Id currentPatchSetId; @@ -153,12 +151,12 @@ public class ChangeDetail { Collections.sort(approvals, ApprovalDetail.SORT); } - public Set getMissingApprovals() { - return missingApprovals; + public void setSubmitRecords(List all) { + submitRecords = all; } - public void setMissingApprovals(Set a) { - missingApprovals = a; + public List 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 0000000000..953ae99205 --- /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. + *

+ * Additional detail may be available in {@link SubmitRecord#errorMessage}. + */ + RULE_ERROR; + } + + public Status status; + public List

+ * 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. + *

+ * 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 4a5c1ced97..9081e38d52 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.client.ui.AddMemberBox; 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.gwt.user.client.ui.HTMLTable.CellFormatter; 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 types; + private final ApprovalTypes types; private final Grid table; private final Widget missing; private final Panel addReviewer; @@ -61,10 +63,9 @@ public class ApprovalTable extends Composite { 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 @@ public class ApprovalTable extends Composite { setStyleName(Gerrit.RESOURCES.css().approvalTable()); } - private void displayHeader() { + private void displayHeader(List labels) { + table.resizeColumns(2 + labels.size()); + final CellFormatter fmt = table.getCellFormatter(); int col = 0; @@ -107,16 +110,12 @@ public class ApprovalTable extends Composite { 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 @@ public class ApprovalTable extends Composite { return AccountDashboardLink.link(accountCache, id); } - public void display(final Change change, final Set need, - final List rows) { - changeId = change.getId(); + void display(ChangeDetail detail) { + List columns = new ArrayList(); + List 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 reportedMissing = new HashSet(); + + HashMap byUser = + new HashMap(); + 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() { + @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 @@ public class ApprovalTable extends Composite { 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 @@ public class ApprovalTable extends Composite { } private void displayRow(final int row, final ApprovalDetail ad, - final Change change) { + final Change change, List columns) { final CellFormatter fmt = table.getCellFormatter(); - final Map am = ad.getApprovalMap(); - final StringBuilder hint = new StringBuilder(); int col = 0; table.setWidget(row, col++, link(ad.getAccount())); @@ -250,31 +323,30 @@ public class ApprovalTable extends Composite { } 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 @@ public class ApprovalTable extends Composite { 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 class ApprovalTable extends Composite { 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 cbf4a6bd23..53d1c18256 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 @@ public interface ChangeMessages extends Messages { 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 5982ad5d06..6f6e34e229 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 @@ renamedFrom = renamed from {0} 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 edb25e4b60..6bd4ebfe6a 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 @@ public class ChangeScreen extends Screen { .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 f7f8ae16a5..274f8f5a24 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.ApprovalType; 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 @@ public class PublishCommentScreen extends AccountScreen implements 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 fc3fb8f683..2705991362 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.ApprovalType; 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.AnonymousUser; 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 @@ public class ChangeDetailFactory extends Handler { if (patch == null) { throw new NoSuchEntityException(); } - final CanSubmitResult canSubmitResult = control.canSubmit(patch.getId()); aic.want(change.getOwner()); @@ -109,12 +107,26 @@ public class ChangeDetailFactory extends Handler { 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 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 @@ public class ChangeDetailFactory extends Handler { final FunctionState fs = functionState.create(detail.getChange(), psId, allApprovals); - final Set missingApprovals = - new HashSet(); - 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 58daa09cd5..ded92e06f0 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.IdentifiedUser; 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 com.google.inject.assistedinject.Assisted; 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 { interface Factory { @@ -47,12 +50,12 @@ final class PatchSetPublishDetailFactory extends Handler 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 drafts; @@ -62,10 +65,12 @@ final class PatchSetPublishDetailFactory extends Handler 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 @@ final class PatchSetPublishDetailFactory extends Handler 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 allowed = Collections.emptyList(); List given = Collections.emptyList(); if (change.getStatus().isOpen() && patchSetId.equals(change.currentPatchSetId())) { - allowed = new ArrayList(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 rangeByName = + new HashMap(); + for (PermissionRange r : control.getLabelRanges()) { + if (r.isLabel()) { + rangeByName.put(r.getLabel(), r); + } + } + allowed = new ArrayList(); given = db.patchSetApprovals() // .byPatchSetUser(patchSetId, user.getAccountId()) // .toList(); + + boolean couldSubmit = false; + List 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 cbefae6e20..909e59af01 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.IdentifiedUser; 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 { interface Factory { SubmitAction create(PatchSet.Id patchSetId); @@ -72,12 +74,51 @@ class SubmitAction extends Handler { 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 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 0c14b80b35..0000000000 --- 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 81807593ab..72db7340c1 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.Logger; import org.slf4j.LoggerFactory; import java.util.ArrayList; +import java.util.Collections; import java.util.List; @@ -224,36 +227,29 @@ public class ChangeControl { return false; } - /** @return {@link CanSubmitResult#OK}, or a result with an error message. */ - public CanSubmitResult canSubmit(final PatchSet.Id patchSetId) { + public List 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 @@ public class ChangeControl { "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 results = new ArrayList(); @@ -278,8 +277,19 @@ public class ChangeControl { 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 @@ public class ChangeControl { // 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 out = new ArrayList(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.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 93b2ba5982..6404898e09 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 @@ package com.google.gerrit.sshd.commands; 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.git.MergeQueue; 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 @@ public class ReviewCommand extends BaseCommand { } 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 @@ public class ReviewCommand extends BaseCommand { } if (submitChange) { - CanSubmitResult result = changeControl.canSubmit(db, patchSetId); - if (result == CanSubmitResult.OK) { - toSubmit.add(patchSetId); - } else { - throw error(result.getMessage()); + List 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); } } }