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/AbandonChange.java index d2f59f5d79..8eb1bbe672 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/AbandonChange.java @@ -27,6 +27,7 @@ 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; import com.google.inject.Inject; @@ -71,7 +72,8 @@ class AbandonChange extends Handler { @Override public ChangeDetail call() throws NoSuchChangeException, OrmException, - EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException { + EmailException, NoSuchEntityException, InvalidChangeOperationException, + PatchSetInfoNotAvailableException { final Change.Id changeId = patchSetId.getParentKey(); final ChangeControl control = changeControlFactory.validateFor(changeId); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChange.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChange.java index f951439b83..fa8785a134 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChange.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChange.java @@ -25,6 +25,7 @@ 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; import com.google.inject.Inject; @@ -69,7 +70,8 @@ class RestoreChange extends Handler { @Override public ChangeDetail call() throws NoSuchChangeException, OrmException, - EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException { + EmailException, NoSuchEntityException, InvalidChangeOperationException, + PatchSetInfoNotAvailableException { final Change.Id changeId = patchSetId.getParentKey(); final ChangeControl control = changeControlFactory.validateFor(changeId); 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 6886d2c85d..87cded02d0 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 @@ -34,6 +34,7 @@ import com.google.gerrit.server.git.MergeQueue; import com.google.gerrit.server.git.ReplicationQueue; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; +import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.mail.AbandonedSender; import com.google.gerrit.server.mail.EmailException; @@ -212,7 +213,7 @@ public class ChangeUtil { final IdentifiedUser user, final String message, final ReviewDb db, final AbandonedSender.Factory abandonedSenderFactory, final ChangeHookRunner hooks) throws NoSuchChangeException, - EmailException, OrmException { + InvalidChangeOperationException, EmailException, OrmException { final Change.Id changeId = patchSetId.getParentKey(); final PatchSet patch = db.patchSets().get(patchSetId); if (patch == null) { @@ -245,23 +246,26 @@ public class ChangeUtil { } }); - if (updatedChange != null) { - db.changeMessages().insert(Collections.singleton(cmsg)); - - final List approvals = - db.patchSetApprovals().byChange(changeId).toList(); - for (PatchSetApproval a : approvals) { - a.cache(updatedChange); - } - db.patchSetApprovals().update(approvals); - - // Email the reviewers - final AbandonedSender cm = abandonedSenderFactory.create(updatedChange); - cm.setFrom(user.getAccountId()); - cm.setChangeMessage(cmsg); - cm.send(); + if (updatedChange == null) { + throw new InvalidChangeOperationException( + "Change is no longer open or patchset is not latest"); } + db.changeMessages().insert(Collections.singleton(cmsg)); + + final List approvals = + db.patchSetApprovals().byChange(changeId).toList(); + for (PatchSetApproval a : approvals) { + a.cache(updatedChange); + } + db.patchSetApprovals().update(approvals); + + // Email the reviewers + final AbandonedSender cm = abandonedSenderFactory.create(updatedChange); + cm.setFrom(user.getAccountId()); + cm.setChangeMessage(cmsg); + cm.send(); + hooks.doChangeAbandonedHook(updatedChange, user.getAccount(), message); } @@ -371,7 +375,7 @@ public class ChangeUtil { final IdentifiedUser user, final String message, final ReviewDb db, final AbandonedSender.Factory abandonedSenderFactory, final ChangeHookRunner hooks) throws NoSuchChangeException, - EmailException, OrmException { + InvalidChangeOperationException, EmailException, OrmException { final Change.Id changeId = patchSetId.getParentKey(); final PatchSet patch = db.patchSets().get(patchSetId); if (patch == null) { @@ -404,23 +408,26 @@ public class ChangeUtil { } }); - if (updatedChange != null) { - db.changeMessages().insert(Collections.singleton(cmsg)); - - final List approvals = - db.patchSetApprovals().byChange(changeId).toList(); - for (PatchSetApproval a : approvals) { - a.cache(updatedChange); - } - db.patchSetApprovals().update(approvals); - - // Email the reviewers - final AbandonedSender cm = abandonedSenderFactory.create(updatedChange); - cm.setFrom(user.getAccountId()); - cm.setChangeMessage(cmsg); - cm.send(); + if (updatedChange == null) { + throw new InvalidChangeOperationException( + "Change is not abandoned or patchset is not latest"); } + db.changeMessages().insert(Collections.singleton(cmsg)); + + final List approvals = + db.patchSetApprovals().byChange(changeId).toList(); + for (PatchSetApproval a : approvals) { + a.cache(updatedChange); + } + db.patchSetApprovals().update(approvals); + + // Email the reviewers + final AbandonedSender cm = abandonedSenderFactory.create(updatedChange); + cm.setFrom(user.getAccountId()); + cm.setChangeMessage(cmsg); + cm.send(); + hooks.doChangeRestoreHook(updatedChange, user.getAccount(), message); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/InvalidChangeOperationException.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/InvalidChangeOperationException.java new file mode 100644 index 0000000000..b288b100eb --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/InvalidChangeOperationException.java @@ -0,0 +1,26 @@ +// 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.server.project; + +import com.google.gerrit.reviewdb.Change; + +/** Indicates the change operation is not currently valid. */ +public class InvalidChangeOperationException extends Exception { + private static final long serialVersionUID = 1L; + + public InvalidChangeOperationException(String msg) { + super(msg); + } +} 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 0a6033bc0f..a8561a535b 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 @@ -34,6 +34,7 @@ import com.google.gerrit.server.mail.EmailException; import com.google.gerrit.server.patch.PublishComments; import com.google.gerrit.server.project.CanSubmitResult; 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; @@ -202,9 +203,8 @@ public class ReviewCommand extends BaseCommand { }); } - private void approveOne(final PatchSet.Id patchSetId) - throws NoSuchChangeException, UnloggedFailure, OrmException, - EmailException { + private void approveOne(final PatchSet.Id patchSetId) throws + NoSuchChangeException, UnloggedFailure, OrmException, EmailException { final Change.Id changeId = patchSetId.getParentKey(); ChangeControl changeControl = changeControlFactory.validateFor(changeId); @@ -224,25 +224,29 @@ public class ReviewCommand extends BaseCommand { publishCommentsFactory.create(patchSetId, changeComment, aps).call(); - if (abandonChange) { - if (changeControl.canAbandon()) { - ChangeUtil.abandon(patchSetId, currentUser, changeComment, db, - abandonedSenderFactory, hooks); - } else { - throw error("Not permitted to abandon change"); + try { + if (abandonChange) { + if (changeControl.canAbandon()) { + ChangeUtil.abandon(patchSetId, currentUser, changeComment, db, + abandonedSenderFactory, hooks); + } else { + throw error("Not permitted to abandon change"); + } } - } - if (restoreChange) { - if (changeControl.canRestore()) { - ChangeUtil.restore(patchSetId, currentUser, changeComment, db, - abandonedSenderFactory, hooks); - } else { - throw error("Not permitted to restore change"); - } - if (submitChange) { - changeControl = changeControlFactory.validateFor(changeId); + if (restoreChange) { + if (changeControl.canRestore()) { + ChangeUtil.restore(patchSetId, currentUser, changeComment, db, + abandonedSenderFactory, hooks); + } else { + throw error("Not permitted to restore change"); + } + if (submitChange) { + changeControl = changeControlFactory.validateFor(changeId); + } } + } catch (InvalidChangeOperationException e) { + throw error(e.getMessage()); } if (submitChange) {