Move boolean args from canSubmit to SubmitRuleEvaluator Remove the check for the current patch set, since this is not a requirement of SubmitRuleEvaluator. It is also redundant for canSubmit; all existing callers either pass in the current patch set explicitly (e.g. QueryProcessor) or, in the case of Submit, have their own check that the patch set is current. The result of this is that canSubmit is now a thin wrapper around SubmitRuleEvaluator and can be removed in a later change. Change-Id: Ice4fd5f0af089f098eed9feadd43263189959995
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 83f844c..7243e10 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
@@ -14,8 +14,6 @@ package com.google.gerrit.server.project; -import static com.google.gerrit.server.project.SubmitRuleEvaluator.createRuleError; - import com.google.common.collect.Lists; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.LabelType; @@ -48,7 +46,6 @@ import java.io.IOException; import java.util.Collection; -import java.util.Collections; import java.util.List; @@ -425,27 +422,13 @@ public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet, @Nullable ChangeData cd, boolean fastEvalLabels, boolean allowClosed, boolean allowDraft) { - if (!allowClosed && getChange().getStatus().isClosed()) { - SubmitRecord rec = new SubmitRecord(); - rec.status = SubmitRecord.Status.CLOSED; - return Collections.singletonList(rec); - } - - if (!patchSet.getId().equals(getChange().currentPatchSetId())) { - return createRuleError( - "Patch set " + patchSet.getPatchSetId() + " is not current"); - } - cd = changeData(db, cd); - if ((getChange().getStatus() == Change.Status.DRAFT || patchSet.isDraft()) - && !allowDraft) { - return cannotSubmitDraft(db, patchSet, cd); - } - try { return new SubmitRuleEvaluator(cd) .setPatchSet(patchSet) .setFastEvalLabels(fastEvalLabels) + .setAllowClosed(allowClosed) + .setAllowDraft(allowDraft) .canSubmit(); } catch (OrmException e) { log.error("Error evaluating submit rule", e); @@ -458,23 +441,6 @@ this.getRefControl().getCurrentUser().getUserName()); } - private List<SubmitRecord> cannotSubmitDraft(ReviewDb db, PatchSet patchSet, - @Nullable ChangeData cd) { - try { - if (!isDraftVisible(db, cd)) { - return createRuleError("Patch set " + patchSet.getPatchSetId() + " not found"); - } else if (patchSet.isDraft()) { - return createRuleError("Cannot submit draft patch sets"); - } else { - return createRuleError("Cannot submit draft changes"); - } - } catch (OrmException err) { - String msg = "Cannot read patch set " + patchSet.getId(); - log.error(msg, err); - return createRuleError(msg); - } - } - public SubmitTypeRecord getSubmitTypeRecord(ReviewDb db, PatchSet patchSet) { return getSubmitTypeRecord(db, patchSet, null); @@ -512,7 +478,7 @@ return cd != null ? cd : changeDataFactory.create(db, this); } - private boolean isDraftVisible(ReviewDb db, ChangeData cd) + public boolean isDraftVisible(ReviewDb db, ChangeData cd) throws OrmException { return isOwner() || isReviewer(db, cd) || getRefControl().canViewDrafts(); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index a4ef94a..fa6e9be 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
@@ -24,6 +24,7 @@ import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.extensions.common.SubmitType; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.rules.PrologEnvironment; import com.google.gerrit.rules.StoredValues; @@ -100,6 +101,8 @@ private PatchSet patchSet; private boolean fastEvalLabels; + private boolean allowDraft; + private boolean allowClosed; private boolean skipFilters; private String rule; private boolean logErrors = true; @@ -132,6 +135,24 @@ } /** + * @param allow whether to allow {@link #canSubmit()} on closed changes. + * @return this + */ + public SubmitRuleEvaluator setAllowClosed(boolean allow) { + allowClosed = allow; + return this; + } + + /** + * @param allow whether to allow {@link #canSubmit()} on closed changes. + * @return this + */ + public SubmitRuleEvaluator setAllowDraft(boolean allow) { + allowDraft = allow; + return this; + } + + /** * @param skip if true, submit filter will not be applied. * @return this */ @@ -164,7 +185,18 @@ * @return List of {@link SubmitRecord} objects returned from the evaluated * rules, including any errors. */ - public List<SubmitRecord> canSubmit() { + public List<SubmitRecord> canSubmit() throws OrmException { + Change c = control.getChange(); + if (!allowClosed && c.getStatus().isClosed()) { + SubmitRecord rec = new SubmitRecord(); + rec.status = SubmitRecord.Status.CLOSED; + return Collections.singletonList(rec); + } + if ((c.getStatus() == Change.Status.DRAFT || getPatchSet().isDraft()) + && !allowDraft) { + return cannotSubmitDraft(); + } + List<Term> results; try { results = evaluateImpl("locate_submit_rule", "can_submit", @@ -185,6 +217,24 @@ return resultsToSubmitRecord(getSubmitRule(), results); } + private List<SubmitRecord> cannotSubmitDraft() throws OrmException { + PatchSet ps = getPatchSet(); + try { + if (!control.isDraftVisible(cd.db(), cd)) { + return createRuleError( + "Patch set " + ps.getPatchSetId() + " not found"); + } else if (patchSet.isDraft()) { + return createRuleError("Cannot submit draft patch sets"); + } else { + return createRuleError("Cannot submit draft changes"); + } + } catch (OrmException err) { + String msg = "Cannot read patch set " + ps; + log.error(msg, err); + return createRuleError(msg); + } + } + /** * Convert the results from Prolog Cafe's format to Gerrit's common format. * @@ -402,15 +452,6 @@ } private PrologEnvironment getPrologEnvironment() throws RuleEvalException { - if (patchSet == null) { - try { - patchSet = cd.currentPatchSet(); - } catch (OrmException err) { - throw new RuleEvalException("Missing current patch set on change " - + cd.getId() + " of " + getProjectName(), - err); - } - } ProjectState projectState = control.getProjectControl().getProjectState(); PrologEnvironment env; try { @@ -503,6 +544,13 @@ return submitRule; } + private PatchSet getPatchSet() throws OrmException { + if (patchSet == null) { + patchSet = cd.currentPatchSet(); + } + return patchSet; + } + private String getProjectName() { return control.getProjectControl().getProjectState().getProject().getName(); }