Refactor AbandonChange code into gerrit-server

While most of the logic for AbandonChange was already in gerrit-server
in ChangeUtil, this change moves it into it's own class.

Using a ReviewResult will allow us to perform one of several related
review actions when responding to an ssh/rpc request and then handle
the result (or more specifically, the errors associated with it) in
a single block of code.

Change-Id: Icb70f2edbb4968a2cc43e78eba71f16f7f93d6e4

gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java

@@ -0,0 +1,73 @@
+// 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.Change;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Result from performing a review (comment, abandon, etc.)
+ */
+public class ReviewResult {
+  protected List<Error> errors;
+  protected Change.Id changeId;
+
+  public ReviewResult() {
+    errors = new ArrayList<Error>();
+  }
+
+  public void addError(final Error e) {
+    errors.add(e);
+  }
+
+  public List<Error> getErrors() {
+    return errors;
+  }
+
+  public Change.Id getChangeId() {
+    return changeId;
+  }
+
+  public void setChangeId(Change.Id changeId) {
+    this.changeId = changeId;
+  }
+
+  public static class Error {
+    public static enum Type {
+      /** Not permitted to abandon this change. */
+      ABANDON_NOT_PERMITTED
+    }
+
+    protected Type type;
+
+    protected Error() {
+    }
+
+    public Error(final Type type) {
+      this.type = type;
+    }
+
+    public Type getType() {
+      return type;
+    }
+
+    @Override
+    public String toString() {
+      return type + "";
+    }
+  }
+}

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChangeHandler.java

@@ -16,17 +16,15 @@ package com.google.gerrit.httpd.rpc.changedetail;
 
 import com.google.gerrit.common.ChangeHookRunner;
 import com.google.gerrit.common.data.ChangeDetail;
+import com.google.gerrit.common.data.ReviewResult;
 import com.google.gerrit.common.errors.NoSuchEntityException;
 import com.google.gerrit.httpd.rpc.Handler;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSet;
-import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.changedetail.AbandonChange;
-import com.google.gerrit.server.mail.AbandonedSender;
 import com.google.gerrit.server.mail.EmailException;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
-import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.client.OrmException;
@@ -35,55 +33,40 @@ import com.google.inject.assistedinject.Assisted;
 
 import javax.annotation.Nullable;
 
-class AbandonChange extends Handler<ChangeDetail> {
+class AbandonChangeHandler extends Handler<ChangeDetail> {
+
   interface Factory {
-    AbandonChange create(PatchSet.Id patchSetId, String message);
+    AbandonChangeHandler create(PatchSet.Id patchSetId, String message);
   }
 
-  private final ChangeControl.Factory changeControlFactory;
+  private final AbandonChange.Factory abandonChangeFactory;
-  private final ReviewDb db;
-  private final IdentifiedUser currentUser;
-  private final AbandonedSender.Factory senderFactory;
   private final ChangeDetailFactory.Factory changeDetailFactory;
 
   private final PatchSet.Id patchSetId;
   @Nullable
   private final String message;
 
-  private final ChangeHookRunner hooks;
-
   @Inject
-  AbandonChange(final ChangeControl.Factory changeControlFactory,
+  AbandonChangeHandler(final AbandonChange.Factory abandonChangeFactory,
-      final ReviewDb db, final IdentifiedUser currentUser,
-      final AbandonedSender.Factory senderFactory,
       final ChangeDetailFactory.Factory changeDetailFactory,
       @Assisted final PatchSet.Id patchSetId,
-      @Assisted @Nullable final String message, final ChangeHookRunner hooks) {
+      @Assisted @Nullable final String message) {
-    this.changeControlFactory = changeControlFactory;
+    this.abandonChangeFactory = abandonChangeFactory;
-    this.db = db;
-    this.currentUser = currentUser;
-    this.senderFactory = senderFactory;
     this.changeDetailFactory = changeDetailFactory;
 
     this.patchSetId = patchSetId;
     this.message = message;
-    this.hooks = hooks;
   }
 
   @Override
   public ChangeDetail call() throws NoSuchChangeException, OrmException,
       EmailException, NoSuchEntityException, InvalidChangeOperationException,
       PatchSetInfoNotAvailableException {
-
+    final ReviewResult result =
-    final Change.Id changeId = patchSetId.getParentKey();
+        abandonChangeFactory.create(patchSetId, message).call();
-    final ChangeControl control = changeControlFactory.validateFor(changeId);
+    if (result.getErrors().size() > 0) {
-    if (!control.canAbandon()) {
+      throw new NoSuchChangeException(result.getChangeId());
-      throw new NoSuchChangeException(changeId);
     }
-
+    return changeDetailFactory.create(result.getChangeId()).call();
-    ChangeUtil.abandon(patchSetId, currentUser, message, db, senderFactory,
-        hooks);
-
-    return changeDetailFactory.create(changeId).call();
   }
 }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeManageServiceImpl.java

@@ -23,7 +23,7 @@ import com.google.inject.Inject;
 
 class ChangeManageServiceImpl implements ChangeManageService {
   private final SubmitAction.Factory submitAction;
-  private final AbandonChange.Factory abandonChangeFactory;
+  private final AbandonChangeHandler.Factory abandonChangeHandlerFactory;
   private final RestoreChange.Factory restoreChangeFactory;
   private final RevertChange.Factory revertChangeFactory;
   private final PublishAction.Factory publishAction;
@@ -31,13 +31,13 @@ class ChangeManageServiceImpl implements ChangeManageService {
 
   @Inject
   ChangeManageServiceImpl(final SubmitAction.Factory patchSetAction,
-      final AbandonChange.Factory abandonChangeFactory,
+      final AbandonChangeHandler.Factory abandonChangeHandlerFactory,
       final RestoreChange.Factory restoreChangeFactory,
       final RevertChange.Factory revertChangeFactory,
       final PublishAction.Factory publishAction,
       final DeleteDraftChange.Factory deleteDraftChangeFactory) {
     this.submitAction = patchSetAction;
-    this.abandonChangeFactory = abandonChangeFactory;
+    this.abandonChangeHandlerFactory = abandonChangeHandlerFactory;
     this.restoreChangeFactory = restoreChangeFactory;
     this.revertChangeFactory = revertChangeFactory;
     this.publishAction = publishAction;
@@ -51,7 +51,7 @@ class ChangeManageServiceImpl implements ChangeManageService {
 
   public void abandonChange(final PatchSet.Id patchSetId, final String message,
       final AsyncCallback<ChangeDetail> callback) {
-    abandonChangeFactory.create(patchSetId, message).to(callback);
+    abandonChangeHandlerFactory.create(patchSetId, message).to(callback);
   }
 
   public void revertChange(final PatchSet.Id patchSetId, final String message,

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeModule.java

@@ -28,7 +28,7 @@ public class ChangeModule extends RpcServletModule {
     install(new FactoryModule() {
       @Override
       protected void configure() {
-        factory(AbandonChange.Factory.class);
+        factory(AbandonChangeHandler.Factory.class);
         factory(RestoreChange.Factory.class);
         factory(RevertChange.Factory.class);
         factory(ChangeDetailFactory.Factory.class);

gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java

@@ -31,7 +31,6 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.MergeQueue;
 import com.google.gerrit.server.git.ReplicationQueue;
-import com.google.gerrit.server.mail.AbandonedSender;
 import com.google.gerrit.server.mail.EmailException;
 import com.google.gerrit.server.mail.ReplyToChangeSender;
 import com.google.gerrit.server.mail.RestoredSender;
@@ -214,49 +213,6 @@ public class ChangeUtil {
     return new PatchSetApproval(akey, (short) 1);
   }
 
-  public static void abandon(final PatchSet.Id patchSetId,
-      final IdentifiedUser user, final String message, final ReviewDb db,
-      final AbandonedSender.Factory senderFactory,
-      final ChangeHookRunner hooks) throws NoSuchChangeException,
-      InvalidChangeOperationException, EmailException, OrmException {
-    final Change.Id changeId = patchSetId.getParentKey();
-    final PatchSet patch = db.patchSets().get(patchSetId);
-    if (patch == null) {
-      throw new NoSuchChangeException(changeId);
-    }
-
-    final ChangeMessage cmsg =
-        new ChangeMessage(new ChangeMessage.Key(changeId, ChangeUtil
-            .messageUUID(db)), user.getAccountId(), patchSetId);
-    final StringBuilder msgBuf =
-        new StringBuilder("Patch Set " + patchSetId.get() + ": Abandoned");
-    if (message != null && message.length() > 0) {
-      msgBuf.append("\n\n");
-      msgBuf.append(message);
-    }
-    cmsg.setMessage(msgBuf.toString());
-
-    final Change updatedChange = db.changes().atomicUpdate(changeId,
-        new AtomicUpdate<Change>() {
-      @Override
-      public Change update(Change change) {
-        if (change.getStatus().isOpen()
-            && change.currentPatchSetId().equals(patchSetId)) {
-          change.setStatus(Change.Status.ABANDONED);
-          ChangeUtil.updated(change);
-          return change;
-        } else {
-          return null;
-        }
-      }
-    });
-
-    updatedChange(db, user, updatedChange, cmsg, senderFactory,
-        "Change is no longer open or patchset is not latest");
-
-    hooks.doChangeAbandonedHook(updatedChange, user.getAccount(), message, db);
-  }
-
   public static Change.Id revert(final PatchSet.Id patchSetId,
       final IdentifiedUser user, final String message, final ReviewDb db,
       final RevertedSender.Factory revertedSenderFactory,
@@ -531,7 +487,7 @@ public class ChangeUtil {
     db.patchSets().delete(Collections.singleton(patch));
   }
 
-  private static <T extends ReplyToChangeSender> void updatedChange(
+  public static <T extends ReplyToChangeSender> void updatedChange(
       final ReviewDb db, final IdentifiedUser user, final Change change,
       final ChangeMessage cmsg, ReplyToChangeSender.Factory<T> senderFactory,
       final String err) throws NoSuchChangeException,

gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AbandonChange.java

@@ -0,0 +1,121 @@
+// Copyright (C) 2009 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.changedetail;
+
+import com.google.gerrit.common.ChangeHookRunner;
+import com.google.gerrit.common.data.ReviewResult;
+import com.google.gerrit.reviewdb.Change;
+import com.google.gerrit.reviewdb.ChangeMessage;
+import com.google.gerrit.reviewdb.PatchSet;
+import com.google.gerrit.reviewdb.ReviewDb;
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.mail.AbandonedSender;
+import com.google.gerrit.server.mail.EmailException;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gwtorm.client.AtomicUpdate;
+import com.google.gwtorm.client.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+
+import java.util.concurrent.Callable;
+
+public class AbandonChange implements Callable<ReviewResult> {
+
+  public interface Factory {
+    AbandonChange create(PatchSet.Id patchSetId, String changeComment);
+  }
+
+  private final AbandonedSender.Factory abandonedSenderFactory;
+  private final ChangeControl.Factory changeControlFactory;
+  private final ReviewDb db;
+  private final IdentifiedUser currentUser;
+  private final ChangeHookRunner hooks;
+
+  private final PatchSet.Id patchSetId;
+  private final String changeComment;
+
+  @Inject
+  AbandonChange(final AbandonedSender.Factory abandonedSenderFactory,
+      final ChangeControl.Factory changeControlFactory, final ReviewDb db,
+      final IdentifiedUser currentUser, final ChangeHookRunner hooks,
+      @Assisted final PatchSet.Id patchSetId,
+      @Assisted final String changeComment) {
+    this.abandonedSenderFactory = abandonedSenderFactory;
+    this.changeControlFactory = changeControlFactory;
+    this.db = db;
+    this.currentUser = currentUser;
+    this.hooks = hooks;
+
+    this.patchSetId = patchSetId;
+    this.changeComment = changeComment;
+  }
+
+  @Override
+  public ReviewResult call() throws EmailException,
+      InvalidChangeOperationException, NoSuchChangeException, OrmException {
+    final ReviewResult result = new ReviewResult();
+
+    final Change.Id changeId = patchSetId.getParentKey();
+    result.setChangeId(changeId);
+    final ChangeControl control = changeControlFactory.validateFor(changeId);
+    final PatchSet patch = db.patchSets().get(patchSetId);
+    if (!control.canAbandon()) {
+      result.addError(new ReviewResult.Error(
+          ReviewResult.Error.Type.ABANDON_NOT_PERMITTED));
+    } else if (patch == null) {
+      throw new NoSuchChangeException(changeId);
+    } else {
+
+      // Create a message to accompany the abandoned change
+      final ChangeMessage cmsg = new ChangeMessage(
+          new ChangeMessage.Key(changeId, ChangeUtil.messageUUID(db)),
+          currentUser.getAccountId(), patchSetId);
+      final StringBuilder msgBuf =
+          new StringBuilder("Patch Set " + patchSetId.get() + ": Abandoned");
+      if (changeComment != null && changeComment.length() > 0) {
+        msgBuf.append("\n\n");
+        msgBuf.append(changeComment);
+      }
+      cmsg.setMessage(msgBuf.toString());
+
+      // Abandon the change
+      final Change updatedChange = db.changes().atomicUpdate(changeId,
+          new AtomicUpdate<Change>() {
+        @Override
+        public Change update(Change change) {
+          if (change.getStatus().isOpen()
+              && change.currentPatchSetId().equals(patchSetId)) {
+            change.setStatus(Change.Status.ABANDONED);
+            ChangeUtil.updated(change);
+            return change;
+          } else {
+            return null;
+          }
+        }
+      });
+      ChangeUtil.updatedChange(
+          db, currentUser, updatedChange, cmsg, abandonedSenderFactory,
+          "Change is no longer open or patchset is not latest");
+      hooks.doChangeAbandonedHook(updatedChange, currentUser.getAccount(),
+                                  changeComment, db);
+    }
+
+    return result;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java

@@ -27,6 +27,7 @@ import com.google.gerrit.server.account.GroupMembers;
 import com.google.gerrit.server.account.PerformCreateGroup;
 import com.google.gerrit.server.account.PerformRenameGroup;
 import com.google.gerrit.server.account.VisibleGroups;
+import com.google.gerrit.server.changedetail.AbandonChange;
 import com.google.gerrit.server.git.CreateCodeReviewNotes;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.MetaDataUpdate;
@@ -80,6 +81,7 @@ public class GerritRequestModule extends FactoryModule {
     // Not really per-request, but dammit, I don't know where else to
     // easily park this stuff.
     //
+    factory(AbandonChange.Factory.class);
     factory(AddReviewer.Factory.class);
     factory(AddReviewerSender.Factory.class);
     factory(CreateChangeSender.Factory.class);

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.ReviewResult;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.reviewdb.ApprovalCategory;
 import com.google.gerrit.reviewdb.ApprovalCategoryValue;
@@ -28,6 +29,7 @@ import com.google.gerrit.reviewdb.RevId;
 import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.changedetail.AbandonChange;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.MergeQueue;
@@ -132,6 +134,9 @@ public class ReviewCommand extends BaseCommand {
   @Inject
   private ChangeControl.Factory changeControlFactory;
 
+  @Inject
+  private AbandonChange.Factory abandonChangeFactory;
+
   @Inject
   private AbandonedSender.Factory abandonedSenderFactory;
 
@@ -251,6 +256,8 @@ public class ReviewCommand extends BaseCommand {
       NoSuchChangeException, OrmException, EmailException, Failure {
 
     final Change.Id changeId = patchSetId.getParentKey();
+
+    ReviewResult result = null;
     ChangeControl changeControl = changeControlFactory.validateFor(changeId);
 
     if (changeComment == null) {
@@ -270,12 +277,7 @@ public class ReviewCommand extends BaseCommand {
       publishCommentsFactory.create(patchSetId, changeComment, aps, forceMessage).call();
 
       if (abandonChange) {
-        if (changeControl.canAbandon()) {
+        result = abandonChangeFactory.create(patchSetId, changeComment).call();
-          ChangeUtil.abandon(patchSetId, currentUser, changeComment, db,
-              abandonedSenderFactory, hooks);
-        } else {
-          throw error("Not permitted to abandon change");
-        }
       }
 
       if (restoreChange) {
@@ -294,11 +296,11 @@ public class ReviewCommand extends BaseCommand {
     }
 
     if (submitChange) {
-      List<SubmitRecord> result = changeControl.canSubmit(db, patchSetId);
+      List<SubmitRecord> submitResult = changeControl.canSubmit(db, patchSetId);
-      if (result.isEmpty()) {
+      if (submitResult.isEmpty()) {
         throw new Failure(1, "ChangeControl.canSubmit returned empty list");
       }
-      switch (result.get(0).status) {
+      switch (submitResult.get(0).status) {
         case OK:
           if (changeControl.getRefControl().canSubmit()) {
             toSubmit.add(patchSetId);
@@ -309,7 +311,7 @@ public class ReviewCommand extends BaseCommand {
 
         case NOT_READY: {
           StringBuilder msg = new StringBuilder();
-          for (SubmitRecord.Label lbl : result.get(0).labels) {
+          for (SubmitRecord.Label lbl : submitResult.get(0).labels) {
             switch (lbl.status) {
               case OK:
                 break;
@@ -341,14 +343,14 @@ public class ReviewCommand extends BaseCommand {
           throw error("change " + changeId + " is closed");
 
         case RULE_ERROR:
-          if (result.get(0).errorMessage != null) {
+          if (submitResult.get(0).errorMessage != null) {
-            throw error("change " + changeId + ": " + result.get(0).errorMessage);
+            throw error("change " + changeId + ": " + submitResult.get(0).errorMessage);
           } else {
             throw error("change " + changeId + ": internal rule error");
           }
 
         default:
-          throw new Failure(1, "Unsupported status " + result.get(0).status);
+          throw new Failure(1, "Unsupported status " + submitResult.get(0).status);
       }
     }
     if (publishPatchSet) {
@@ -371,6 +373,17 @@ public class ReviewCommand extends BaseCommand {
         throw error("Not permitted to delete draft patchset");
       }
     }
+
+    if (result != null) {
+      for (ReviewResult.Error resultError : result.getErrors()) {
+        switch (resultError.getType()) {
+          case ABANDON_NOT_PERMITTED:
+            writeError("error: not permitted to abandon change");
+          default:
+            writeError("error: failure in review");
+        }
+      }
+    }
   }
 
   private Set<PatchSet.Id> parsePatchSetId(final String patchIdentity)