opendev/gerrit
Code Issues Proposed changes

Refactor out approval checking from ReviewCommand

Browse Source 
PublishComments already does some approval verification.  This change
places the burden of this verification there instead of on
ReviewCommand.

Change-Id: Iaf4fb7b6c073b3fbaed64a34263f71d8d2d57507
This commit is contained in:
Conley Owens 2012-02-29 11:38:23 -08:00
parent bc87753e7e
commit 9e18a83e88
2 changed files with 7 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
View File

@ -164,7 +164,8 @@ public class PublishComments implements Callable<VoidResult> {
db.patchComments().update(drafts);
}
private void publishApprovals(ChangeControl ctl) throws OrmException {
private void publishApprovals(ChangeControl ctl)
throws InvalidChangeOperationException, OrmException {
ChangeUtil.updated(change);
final Set<ApprovalCategory.Id> dirty = new HashSet<ApprovalCategory.Id>();
@ -201,6 +202,11 @@ public class PublishComments implements Callable<VoidResult> {
if (!ApprovalCategory.SUBMIT.equals(a.getCategoryId())) {
functionState.normalize(types.byId(a.getCategoryId()), a);
}
if (want.get() != a.getValue()) {
throw new InvalidChangeOperationException(
types.byId(a.getCategoryId()).getCategory().getLabelName()
+ "=" + want.get() + " not permitted");
}
if (o != a.getValue()) {
// Value changed, ensure we update the database.
//

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

@ -21,10 +21,8 @@ import com.google.gerrit.reviewdb.client.ApprovalCategory;
import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.changedetail.AbandonChange;
import com.google.gerrit.server.changedetail.DeleteDraftPatchSet;
import com.google.gerrit.server.changedetail.PublishDraft;
@ -32,11 +30,9 @@ import com.google.gerrit.server.changedetail.RestoreChange;
import com.google.gerrit.server.changedetail.Submit;
import com.google.gerrit.server.mail.EmailException;
import com.google.gerrit.server.patch.PublishComments;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.workflow.FunctionState;
import com.google.gerrit.sshd.BaseCommand;
import com.google.gerrit.util.cli.CmdLineParser;
import com.google.gwtorm.server.OrmException;
@ -110,24 +106,15 @@ public class ReviewCommand extends BaseCommand {
@Inject
private ReviewDb db;
@Inject
private IdentifiedUser currentUser;
@Inject
private ApprovalTypes approvalTypes;
@Inject
private ChangeControl.Factory changeControlFactory;
@Inject
private DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory;
@Inject
private AbandonChange.Factory abandonChangeFactory;
@Inject
private FunctionState.Factory functionStateFactory;
@Inject
private PublishComments.Factory publishCommentsFactory;
@ -205,10 +192,6 @@ public class ReviewCommand extends BaseCommand {
private void approveOne(final PatchSet.Id patchSetId) throws
NoSuchChangeException, OrmException, EmailException, Failure {
final Change.Id changeId = patchSetId.getParentKey();
ChangeControl changeControl = changeControlFactory.validateFor(changeId);
if (changeComment == null) {
changeComment = "";
}
@ -217,7 +200,6 @@ public class ReviewCommand extends BaseCommand {
for (ApproveOption ao : optionList) {
Short v = ao.value();
if (v != null) {
assertScoreIsAllowed(patchSetId, changeControl, ao, v);
aps.add(new ApprovalCategoryValue.Id(ao.getCategoryId(), v));
}
}
@ -362,22 +344,6 @@ public class ReviewCommand extends BaseCommand {
return projectControl.getProject().getNameKey().equals(change.getProject());
}
private void assertScoreIsAllowed(final PatchSet.Id patchSetId,
final ChangeControl changeControl, ApproveOption ao, Short v)
throws UnloggedFailure {
final PatchSetApproval psa =
new PatchSetApproval(new PatchSetApproval.Key(patchSetId, currentUser
.getAccountId(), ao.getCategoryId()), v);
final FunctionState fs =
functionStateFactory.create(changeControl, patchSetId,
Collections.<PatchSetApproval> emptyList());
psa.setValue(v);
fs.normalize(approvalTypes.byId(psa.getCategoryId()), psa);
if (v != psa.getValue()) {
throw error(ao.name() + "=" + ao.value() + " not permitted");
}
}
private void initOptionList() {
optionList = new ArrayList<ApproveOption>();