opendev/gerrit
Code Issues Proposed changes

Update approvals in web UI to adapt to rules.pl submit_rule

Browse Source 
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
This commit is contained in:
Shawn O. Pearce
2011-06-20 15:03:31 -07:00
parent 849a0a5845
commit 02630e708c
14 changed files with 621 additions and 255 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
gerrit-common/src/main/java/com/google/gerrit/common/data/ApprovalDetail.java
View File

@@ -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<ApprovalDetail> SORT =
@@ -43,6 +43,8 @@ public class ApprovalDetail {
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 @@ public class ApprovalDetail {
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;
}
return r;
public PatchSetApproval getPatchSetApproval(ApprovalCategory.Id category) {
for (PatchSetApproval psa : approvals) {
if (psa.getCategoryId().equals(category)) {
return psa;
}
}
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<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);
}
}

12
gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetail.java
View File

@@ -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<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 @@ public class ChangeDetail {
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) {

82
gerrit-common/src/main/java/com/google/gerrit/common/data/SubmitRecord.java Normal file
View File

@@ -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;
}
}

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

@@ -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<ApprovalType> 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<String> 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<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 @@ 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<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 @@ 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 (ad.isRejected(labelName)) {
table.setWidget(row, col, new Image(Gerrit.RESOURCES.redNot()));
} 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;
}
final ApprovalCategoryValue acv = type.getValue(ca);
if (acv != null) {
if (hint.length() > 0) {
hint.append("; ");
}
hint.append(acv.getName());
}
if (type.isMaxNegative(ca)) {
table.setWidget(row, col, new Image(Gerrit.RESOURCES.redNot()));
} else if (type.isMaxPositive(ca)) {
table.setWidget(row, col, new Image(Gerrit.RESOURCES.greenCheck()));
} else {
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);

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
View File

@@ -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);

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
View File

@@ -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}:

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

@@ -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) {

18
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PublishCommentScreen.java
View File

@@ -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.
}
}
}

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

@@ -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<ChangeDetail> {
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<ChangeDetail> {
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,17 +157,10 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
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() //
&& control.getCurrentUser() instanceof IdentifiedUser;

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

@@ -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<PatchSetPublishDetail> {
interface Factory {
@@ -47,12 +50,12 @@ final class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
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 class PatchSetPublishDetailFactory extends Handler<PatchSetPublishDetail>
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<PatchSetPublishDetail>
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;
}
aic.want(change.getOwner());
accounts = aic.create();
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);
}
}
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;
}

49
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/SubmitAction.java
View File

@@ -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<ChangeDetail> {
interface Factory {
SubmitAction create(PatchSet.Id patchSetId);
@@ -72,12 +74,51 @@ class SubmitAction extends Handler<ChangeDetail> {
final ChangeControl changeControl =
changeControlFactory.validateFor(changeId);
CanSubmitResult err = changeControl.canSubmit(db, patchSetId);
if (err == CanSubmitResult.OK) {
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(err.getMessage());
throw new IllegalStateException("Internal rule error");
}
default:
throw new IllegalStateException("Uknown status " + result.get(0).status);
}
}
}

43
gerrit-server/src/main/java/com/google/gerrit/server/project/CanSubmitResult.java
View File

@@ -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() + "]";
}
}

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

@@ -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<SubmitRecord> canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
if (change.getStatus().isClosed()) {
return new CanSubmitResult("Change " + change.getId() + " is closed");
}
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;
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.CLOSED;
return Collections.singletonList(rec);
}
/** @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<Term> results = new ArrayList<Term>();
@@ -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<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);
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);
}
// 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");
if ("ok".equals(submitRecord.name())) {
rec.status = SubmitRecord.Status.OK;
} else if ("not_ready".equals(submitRecord.name())) {
rec.status = SubmitRecord.Status.NOT_READY;
} 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);
}
Term submitRecord = first.arg(0);
// 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("Invalid result from submit rule " + submitRule + ": " + submitRecord);
return new CanSubmitResult("Error in submit rule");
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("Invalid result from submit rule " + submitRule + ": " + submitRecord);
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);
}
String label = state.arg(0).name();
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())) {
continue;
lbl.status = SubmitRecord.Label.Status.OK;
appliedBy(lbl, status);
} else if ("reject".equals(status.name())) {
return new CanSubmitResult("Submit blocked by " + label);
lbl.status = SubmitRecord.Label.Status.REJECT;
appliedBy(lbl, status);
} 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);
lbl.status = SubmitRecord.Label.Status.NEED;
} else if ("impossble".equals(status.name())) {
return new CanSubmitResult("Requires " + label + " (check permissions)");
} else if ("impossible".equals(status.name())) {
lbl.status = SubmitRecord.Label.Status.IMPOSSIBLE;
} 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);
}
}
return CanSubmitResult.OK;
if (rec.status == SubmitRecord.Status.OK) {
break;
}
}
Collections.reverse(out);
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();
}
}

60
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
View File

@@ -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) {
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(result.getMessage());
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);
}
}
}