From 9e18a83e88efc60e724a6c5b4dbc0fa4a5ab6d3a Mon Sep 17 00:00:00 2001
From: Conley Owens <cco3@android.com>
Date: Wed, 29 Feb 2012 11:38:23 -0800
Subject: [PATCH] Refactor out approval checking from ReviewCommand

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

Change-Id: Iaf4fb7b6c073b3fbaed64a34263f71d8d2d57507
---
 .../gerrit/server/patch/PublishComments.java  |  8 ++++-
 .../gerrit/sshd/commands/ReviewCommand.java   | 34 -------------------
 2 files changed, 7 insertions(+), 35 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
index 0186142209..34d9d0ea75 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
@@ -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.
         //
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 010f82c822..f888dce0c2 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
@@ -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>();