diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java new file mode 100644 index 0000000000..068293d54f --- /dev/null +++ b/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 errors; + protected Change.Id changeId; + + public ReviewResult() { + errors = new ArrayList(); + } + + public void addError(final Error e) { + errors.add(e); + } + + public List 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 + ""; + } + } +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChange.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChangeHandler.java similarity index 60% rename from gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChange.java rename to gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChangeHandler.java index b4f595c416..b78af5e21f 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChange.java +++ b/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.mail.AbandonedSender; +import com.google.gerrit.server.changedetail.AbandonChange; 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 { +class AbandonChangeHandler extends Handler { + interface Factory { - AbandonChange create(PatchSet.Id patchSetId, String message); + AbandonChangeHandler create(PatchSet.Id patchSetId, String message); } - private final ChangeControl.Factory changeControlFactory; - private final ReviewDb db; - private final IdentifiedUser currentUser; - private final AbandonedSender.Factory senderFactory; + private final AbandonChange.Factory abandonChangeFactory; 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, - final ReviewDb db, final IdentifiedUser currentUser, - final AbandonedSender.Factory senderFactory, + AbandonChangeHandler(final AbandonChange.Factory abandonChangeFactory, final ChangeDetailFactory.Factory changeDetailFactory, @Assisted final PatchSet.Id patchSetId, - @Assisted @Nullable final String message, final ChangeHookRunner hooks) { - this.changeControlFactory = changeControlFactory; - this.db = db; - this.currentUser = currentUser; - this.senderFactory = senderFactory; + @Assisted @Nullable final String message) { + this.abandonChangeFactory = abandonChangeFactory; this.changeDetailFactory = changeDetailFactory; this.patchSetId = patchSetId; this.message = message; - this.hooks = hooks; } @Override public ChangeDetail call() throws NoSuchChangeException, OrmException, EmailException, NoSuchEntityException, InvalidChangeOperationException, PatchSetInfoNotAvailableException { - - final Change.Id changeId = patchSetId.getParentKey(); - final ChangeControl control = changeControlFactory.validateFor(changeId); - if (!control.canAbandon()) { - throw new NoSuchChangeException(changeId); + final ReviewResult result = + abandonChangeFactory.create(patchSetId, message).call(); + if (result.getErrors().size() > 0) { + throw new NoSuchChangeException(result.getChangeId()); } - - ChangeUtil.abandon(patchSetId, currentUser, message, db, senderFactory, - hooks); - - return changeDetailFactory.create(changeId).call(); + return changeDetailFactory.create(result.getChangeId()).call(); } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeManageServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeManageServiceImpl.java index 11434a7c06..88945cb02c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeManageServiceImpl.java +++ b/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 callback) { - abandonChangeFactory.create(patchSetId, message).to(callback); + abandonChangeHandlerFactory.create(patchSetId, message).to(callback); } public void revertChange(final PatchSet.Id patchSetId, final String message, @@ -73,4 +73,4 @@ class ChangeManageServiceImpl implements ChangeManageService { final AsyncCallback callback) { deleteDraftChangeFactory.create(patchSetId).to(callback); } -} \ No newline at end of file +} diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeModule.java index 34685ec19a..d91b8e826b 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeModule.java +++ b/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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 7aca649578..9e2d54f23f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/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() { - @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 void updatedChange( + public static void updatedChange( final ReviewDb db, final IdentifiedUser user, final Change change, final ChangeMessage cmsg, ReplyToChangeSender.Factory senderFactory, final String err) throws NoSuchChangeException, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AbandonChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AbandonChange.java new file mode 100644 index 0000000000..775e738bcf --- /dev/null +++ b/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 { + + 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() { + @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; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java index 3cd17d0b86..179660e41a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java +++ b/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); 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 7f7f290570..d6306ba717 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 @@ -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()) { - ChangeUtil.abandon(patchSetId, currentUser, changeComment, db, - abandonedSenderFactory, hooks); - } else { - throw error("Not permitted to abandon change"); - } + result = abandonChangeFactory.create(patchSetId, changeComment).call(); } if (restoreChange) { @@ -294,11 +296,11 @@ public class ReviewCommand extends BaseCommand { } if (submitChange) { - List result = changeControl.canSubmit(db, patchSetId); - if (result.isEmpty()) { + List submitResult = changeControl.canSubmit(db, patchSetId); + 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) { - throw error("change " + changeId + ": " + result.get(0).errorMessage); + if (submitResult.get(0).errorMessage != null) { + 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 parsePatchSetId(final String patchIdentity)