Allow deletion of open and abandoned changes

If a user includes some sensitive information in a change, admins
need to be able to delete it. Previously, only draft changes could be
deleted. Deleting merged changes isn't supported because we would
need to rewrite history.

Feature: Issue 4509
Change-Id: I6a5bf055257b3762271c61fdd82a349e24148be3

Documentation/rest-api-changes.txt

@@ -1767,13 +1767,19 @@ Publishes a draft change.
   HTTP/1.1 204 No Content
 ----
 
-[[delete-draft-change]]
+[[delete-change]]
-=== Delete Draft Change
+=== Delete Change
 --
 'DELETE /changes/link:#change-id[\{change-id\}]'
 --
 
-Deletes a draft change.
+Deletes a change.
+
+New or abandoned changes can only be deleted by administrators. The deletion of
+merged changes isn't supported at the moment. Draft changes can only be deleted
+by their owner or other users who have the permissions to view and delete
+drafts. If the draft workflow is disabled, only administrators with those
+permissions may delete draft changes.
 
 .Request
 ----

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -68,6 +68,7 @@ import com.google.gerrit.extensions.common.MergeInput;
 import com.google.gerrit.extensions.common.MergePatchSetInput;
 import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -398,7 +399,7 @@ public class ChangeIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void delete() throws Exception {
+  public void deleteDraftChange() throws Exception {
     PushOneCommit.Result r = createChange("refs/drafts/master");
     assertThat(query(r.getChangeId())).hasSize(1);
     assertThat(info(r.getChangeId()).status).isEqualTo(ChangeStatus.DRAFT);
@@ -408,6 +409,110 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(query(r.getChangeId())).isEmpty();
   }
 
+  @Test
+  public void deleteNewChangeAsAdmin() throws Exception {
+    PushOneCommit.Result changeResult = createChange();
+    String changeId = changeResult.getChangeId();
+
+    gApi.changes()
+        .id(changeId)
+        .delete();
+
+    assertThat(query(changeId)).isEmpty();
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void deleteNewChangeAsNormalUser() throws Exception {
+    PushOneCommit.Result changeResult =
+        pushFactory.create(db, user.getIdent(), testRepo)
+            .to("refs/for/master");
+    String changeId = changeResult.getChangeId();
+    Change.Id id = changeResult.getChange().getId();
+
+    setApiUser(user);
+    exception.expect(AuthException.class);
+    exception.expectMessage(String.format(
+        "Deleting change %s is not permitted", id));
+    gApi.changes()
+        .id(changeId)
+        .delete();
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void deleteNewChangeOfAnotherUserAsAdmin() throws Exception {
+    PushOneCommit.Result changeResult =
+        pushFactory.create(db, user.getIdent(), testRepo)
+            .to("refs/for/master");
+    changeResult.assertOkStatus();
+    String changeId = changeResult.getChangeId();
+
+    setApiUser(admin);
+    gApi.changes()
+        .id(changeId)
+        .delete();
+
+    assertThat(query(changeId)).isEmpty();
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void deleteAbandonedChangeAsNormalUser() throws Exception {
+    PushOneCommit.Result changeResult =
+        pushFactory.create(db, user.getIdent(), testRepo)
+        .to("refs/for/master");
+    String changeId = changeResult.getChangeId();
+    Change.Id id = changeResult.getChange().getId();
+
+    setApiUser(user);
+    gApi.changes()
+        .id(changeId)
+        .abandon();
+
+    exception.expect(AuthException.class);
+    exception.expectMessage(String.format(
+        "Deleting change %s is not permitted", id));
+    gApi.changes()
+        .id(changeId)
+        .delete();
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void deleteAbandonedChangeOfAnotherUserAsAdmin() throws Exception {
+    PushOneCommit.Result changeResult =
+        pushFactory.create(db, user.getIdent(), testRepo)
+        .to("refs/for/master");
+    String changeId = changeResult.getChangeId();
+
+    gApi.changes()
+        .id(changeId)
+        .abandon();
+
+    gApi.changes()
+        .id(changeId)
+        .delete();
+
+    assertThat(query(changeId)).isEmpty();
+  }
+
+  @Test
+  public void deleteMergedChange() throws Exception {
+    PushOneCommit.Result changeResult = createChange();
+    String changeId = changeResult.getChangeId();
+    Change.Id id = changeResult.getChange().getId();
+
+    merge(changeResult);
+
+    exception.expect(MethodNotAllowedException.class);
+    exception.expectMessage(String.format(
+        "Deleting merged change %s is not allowed", id));
+    gApi.changes()
+        .id(changeId)
+        .delete();
+  }
+
   @Test
   public void rebaseUpToDateChange() throws Exception {
     PushOneCommit.Result r = createChange();

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java

@@ -22,20 +22,38 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.RestResponse;
 import com.google.gerrit.acceptance.RestSession;
+import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
+import com.google.gerrit.extensions.common.RevisionInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.git.BatchUpdate;
+import com.google.gerrit.server.git.UpdateException;
+import com.google.gerrit.server.notedb.PatchSetState;
 import com.google.gerrit.testutil.ConfigSuite;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
 
 import org.eclipse.jgit.lib.Config;
 import org.junit.Test;
 
 import java.util.Collection;
+import java.util.EnumSet;
+import java.util.List;
+import java.util.stream.Collectors;
 
 public class DraftChangeIT extends AbstractDaemonTest {
   @ConfigSuite.Config
@@ -43,20 +61,8 @@ public class DraftChangeIT extends AbstractDaemonTest {
     return allowDraftsDisabledConfig();
   }
 
-  @Test
+  @Inject
-  public void deleteChange() throws Exception {
+  private BatchUpdate.Factory updateFactory;
-    PushOneCommit.Result result = createChange();
-    result.assertOkStatus();
-    String changeId = result.getChangeId();
-    String triplet = project.get() + "~master~" + changeId;
-    ChangeInfo c = get(triplet);
-    assertThat(c.id).isEqualTo(triplet);
-    assertThat(c.status).isEqualTo(ChangeStatus.NEW);
-    RestResponse response = deleteChange(changeId, adminRestSession);
-    assertThat(response.getEntityContent())
-        .isEqualTo("Change is not a draft: " + c._number);
-    response.assertConflict();
-  }
 
   @Test
   public void deleteDraftChange() throws Exception {
@@ -74,6 +80,104 @@ public class DraftChangeIT extends AbstractDaemonTest {
     get(triplet);
   }
 
+  @Test
+  public void deleteDraftChangeOfAnotherUser() throws Exception {
+    assume().that(isAllowDrafts()).isTrue();
+    PushOneCommit.Result changeResult = createDraftChange();
+    changeResult.assertOkStatus();
+    String changeId = changeResult.getChangeId();
+    Change.Id id = changeResult.getChange().getId();
+
+    // The user needs to be able to see the draft change (which reviewers can).
+    gApi.changes()
+        .id(changeId)
+        .addReviewer(user.fullName);
+
+    setApiUser(user);
+    exception.expect(AuthException.class);
+    exception.expectMessage(String.format(
+        "Deleting change %s is not permitted", id));
+    gApi.changes()
+        .id(changeId)
+        .delete();
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void deleteDraftChangeWhenDraftsNotAllowedAsNormalUser()
+      throws Exception {
+    assume().that(isAllowDrafts()).isFalse();
+
+    setApiUser(user);
+    // We can't create a draft change while the draft workflow is disabled.
+    // For this reason, we create a normal change and modify the database.
+    PushOneCommit.Result changeResult =
+        pushFactory.create(db, user.getIdent(), testRepo)
+            .to("refs/for/master");
+    Change.Id id = changeResult.getChange().getId();
+    markChangeAsDraft(id);
+    setDraftStatusOfPatchSetsOfChange(id, true);
+
+    String changeId = changeResult.getChangeId();
+    exception.expect(MethodNotAllowedException.class);
+    exception.expectMessage("Draft workflow is disabled");
+    gApi.changes()
+        .id(changeId)
+        .delete();
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void deleteDraftChangeWhenDraftsNotAllowedAsAdmin() throws Exception {
+    assume().that(isAllowDrafts()).isFalse();
+
+    setApiUser(user);
+    // We can't create a draft change while the draft workflow is disabled.
+    // For this reason, we create a normal change and modify the database.
+    PushOneCommit.Result changeResult =
+        pushFactory.create(db, user.getIdent(), testRepo)
+        .to("refs/for/master");
+    Change.Id id = changeResult.getChange().getId();
+    markChangeAsDraft(id);
+    setDraftStatusOfPatchSetsOfChange(id, true);
+
+    String changeId = changeResult.getChangeId();
+
+    // Grant those permissions to admins.
+    grant(Permission.VIEW_DRAFTS, project, "refs/*");
+    grant(Permission.DELETE_DRAFTS, project, "refs/*");
+
+    try {
+      setApiUser(admin);
+      gApi.changes()
+          .id(changeId)
+          .delete();
+    } finally {
+      removePermission(Permission.DELETE_DRAFTS, project, "refs/*");
+      removePermission(Permission.VIEW_DRAFTS, project, "refs/*");
+    }
+
+    setApiUser(user);
+    assertThat(query(changeId)).isEmpty();
+  }
+
+  @Test
+  public void deleteDraftChangeWithNonDraftPatchSet() throws Exception {
+    assume().that(isAllowDrafts()).isTrue();
+
+    PushOneCommit.Result changeResult = createDraftChange();
+    Change.Id id = changeResult.getChange().getId();
+    setDraftStatusOfPatchSetsOfChange(id, false);
+
+    String changeId = changeResult.getChangeId();
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage(String.format(
+        "Cannot delete draft change %s: patch set 1 is not a draft", id));
+    gApi.changes()
+        .id(changeId)
+        .delete();
+  }
+
   @Test
   public void publishDraftChange() throws Exception {
     assume().that(isAllowDrafts()).isTrue();
@@ -160,4 +264,92 @@ public class DraftChangeIT extends AbstractDaemonTest {
         + patchSet.getRevision().get()
         + "/publish");
   }
+
+  private void markChangeAsDraft(Change.Id id) throws OrmException,
+      RestApiException, UpdateException {
+    try (BatchUpdate batchUpdate = updateFactory
+        .create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
+      batchUpdate.addOp(id, new MarkChangeAsDraftUpdateOp());
+      batchUpdate.execute();
+    }
+
+    ChangeStatus changeStatus = gApi.changes()
+        .id(id.get())
+        .get()
+        .status;
+    assertThat(changeStatus).isEqualTo(ChangeStatus.DRAFT);
+  }
+
+  private void setDraftStatusOfPatchSetsOfChange(Change.Id id,
+      boolean draftStatus) throws OrmException, RestApiException,
+      UpdateException {
+    try (BatchUpdate batchUpdate = updateFactory
+        .create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
+      batchUpdate.addOp(id, new DraftStatusOfPatchSetsUpdateOp(draftStatus));
+      batchUpdate.execute();
+    }
+
+    Boolean expectedDraftStatus = draftStatus ? Boolean.TRUE : null;
+    List<Boolean> patchSetDraftStatuses = getPatchSetDraftStatuses(id);
+    patchSetDraftStatuses.forEach(status ->
+        assertThat(status).isEqualTo(expectedDraftStatus));
+  }
+
+  private List<Boolean> getPatchSetDraftStatuses(Change.Id id)
+      throws RestApiException {
+    Collection<RevisionInfo> revisionInfos = gApi.changes()
+        .id(id.get())
+        .get(EnumSet.of(ListChangesOption.ALL_REVISIONS))
+        .revisions
+        .values();
+    return revisionInfos.stream()
+        .map(revisionInfo -> revisionInfo.draft)
+        .collect(Collectors.toList());
+  }
+
+  private class MarkChangeAsDraftUpdateOp extends BatchUpdate.Op {
+    @Override
+    public boolean updateChange(BatchUpdate.ChangeContext ctx)
+        throws Exception {
+      Change change = ctx.getChange();
+
+      // Change status in database.
+      change.setStatus(Change.Status.DRAFT);
+
+      // Change status in NoteDb.
+      PatchSet.Id currentPatchSetId = change.currentPatchSetId();
+      ctx.getUpdate(currentPatchSetId).setStatus(Change.Status.DRAFT);
+
+      return true;
+    }
+  }
+
+  private class DraftStatusOfPatchSetsUpdateOp extends BatchUpdate.Op {
+    private final boolean draftStatus;
+
+    DraftStatusOfPatchSetsUpdateOp(boolean draftStatus) {
+      this.draftStatus = draftStatus;
+    }
+
+    @Override
+    public boolean updateChange(BatchUpdate.ChangeContext ctx)
+        throws Exception {
+      Collection<PatchSet> patchSets = psUtil.byChange(db, ctx.getNotes());
+
+      // Change status in database.
+      patchSets.forEach(patchSet -> patchSet.setDraft(draftStatus));
+      db.patchSets().update(patchSets);
+
+      // Change status in NoteDb.
+      PatchSetState patchSetState = draftStatus ? PatchSetState.DRAFT
+          : PatchSetState.PUBLISHED;
+      patchSets.stream()
+          .map(PatchSet::getId)
+          .map(ctx::getUpdate)
+          .forEach(changeUpdate ->
+              changeUpdate.setPatchSetState(patchSetState));
+
+      return true;
+    }
+  }
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java

@@ -112,7 +112,7 @@ public interface ChangeApi {
   void publish() throws RestApiException;
 
   /**
-   * Deletes a draft change.
+   * Deletes a change.
    */
   void delete() throws RestApiException;
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java

@@ -51,6 +51,6 @@ public interface ChangeConstants extends Constants {
   String abandoned();
 
   String deleteChangeEdit();
-  String deleteDraftChange();
+  String deleteChange();
   String deleteDraftRevision();
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties

@@ -34,5 +34,5 @@ abandoned = Abandoned
 deleteChangeEdit = Delete Change Edit?\n\
   \n\
   All changes made in the edit revision will be lost.
-deleteDraftChange = Delete Draft Change?
+deleteChange = Delete Change?
 deleteDraftRevision = Delete Draft Revision?

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java

@@ -495,15 +495,13 @@ public class ChangeScreen extends Screen {
   }
 
   private void initChangeAction(ChangeInfo info) {
-    if (info.status() == Status.DRAFT) {
+    NativeMap<ActionInfo> actions = info.hasActions()
-      NativeMap<ActionInfo> actions = info.hasActions()
+        ? info.actions()
-          ? info.actions()
+        : NativeMap.create();
-          : NativeMap.<ActionInfo> create();
+    actions.copyKeysIntoChildren("id");
-      actions.copyKeysIntoChildren("id");
+    if (actions.containsKey("/")) {
-      if (actions.containsKey("/")) {
+      deleteChange.setVisible(true);
-        deleteChange.setVisible(true);
+      deleteChange.setTitle(actions.get("/").title());
-        deleteChange.setTitle(actions.get("/").title());
-      }
     }
   }
 
@@ -679,7 +677,7 @@ public class ChangeScreen extends Screen {
 
   @UiHandler("deleteChange")
   void onDeleteChange(@SuppressWarnings("unused") ClickEvent e) {
-    if (Window.confirm(Resources.C.deleteDraftChange())) {
+    if (Window.confirm(Resources.C.deleteChange())) {
       DraftActions.delete(changeId, publish, deleteRevision, deleteChange);
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -45,7 +45,7 @@ import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.Check;
 import com.google.gerrit.server.change.CreateMergePatchSet;
 import com.google.gerrit.server.change.DeleteAssignee;
-import com.google.gerrit.server.change.DeleteDraftChange;
+import com.google.gerrit.server.change.DeleteChange;
 import com.google.gerrit.server.change.GetAssignee;
 import com.google.gerrit.server.change.GetHashtags;
 import com.google.gerrit.server.change.GetPastAssignees;
@@ -98,7 +98,7 @@ class ChangeApiImpl implements ChangeApi {
   private final Provider<SubmittedTogether> submittedTogether;
   private final PublishDraftPatchSet.CurrentRevision
     publishDraftChange;
-  private final DeleteDraftChange deleteDraftChange;
+  private final DeleteChange deleteChange;
   private final GetTopic getTopic;
   private final PutTopic putTopic;
   private final PostReviewers postReviewers;
@@ -129,7 +129,7 @@ class ChangeApiImpl implements ChangeApi {
       CreateMergePatchSet updateByMerge,
       Provider<SubmittedTogether> submittedTogether,
       PublishDraftPatchSet.CurrentRevision publishDraftChange,
-      DeleteDraftChange deleteDraftChange,
+      DeleteChange deleteChange,
       GetTopic getTopic,
       PutTopic putTopic,
       PostReviewers postReviewers,
@@ -159,7 +159,7 @@ class ChangeApiImpl implements ChangeApi {
     this.updateByMerge = updateByMerge;
     this.submittedTogether = submittedTogether;
     this.publishDraftChange = publishDraftChange;
-    this.deleteDraftChange = deleteDraftChange;
+    this.deleteChange = deleteChange;
     this.getTopic = getTopic;
     this.putTopic = putTopic;
     this.postReviewers = postReviewers;
@@ -324,7 +324,7 @@ class ChangeApiImpl implements ChangeApi {
   @Override
   public void delete() throws RestApiException {
     try {
-      deleteDraftChange.apply(change, null);
+      deleteChange.apply(change, null);
     } catch (UpdateException e) {
       throw new RestApiException("Cannot delete change", e);
     }

gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -658,7 +658,7 @@ public class ConsistencyChecker {
     public boolean updateChange(ChangeContext ctx)
         throws OrmException, PatchSetInfoNotAvailableException {
       // Delete dangling key references.
-      ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb());
+      ReviewDb db = DeleteChangeOp.unwrap(ctx.getDb());
       accountPatchReviewStore.get().clearReviewed(psId);
       db.changeMessages().delete(
           db.changeMessages().byChange(psId.getParentKey()));

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java

@@ -22,10 +22,11 @@ import com.google.gerrit.extensions.webui.UiAction;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Change.Status;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.change.DeleteDraftChange.Input;
+import com.google.gerrit.server.change.DeleteChange.Input;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.UpdateException;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -34,25 +35,25 @@ import com.google.inject.Singleton;
 import org.eclipse.jgit.lib.Config;
 
 @Singleton
-public class DeleteDraftChange implements
+public class DeleteChange implements
     RestModifyView<ChangeResource, Input>, UiAction<ChangeResource> {
   public static class Input {
   }
 
   private final Provider<ReviewDb> db;
   private final BatchUpdate.Factory updateFactory;
-  private final Provider<DeleteDraftChangeOp> opProvider;
+  private final Provider<DeleteChangeOp> opProvider;
   private final boolean allowDrafts;
 
   @Inject
-  public DeleteDraftChange(Provider<ReviewDb> db,
+  public DeleteChange(Provider<ReviewDb> db,
       BatchUpdate.Factory updateFactory,
-      Provider<DeleteDraftChangeOp> opProvider,
+      Provider<DeleteChangeOp> opProvider,
       @GerritServerConfig Config cfg) {
     this.db = db;
     this.updateFactory = updateFactory;
     this.opProvider = opProvider;
-    this.allowDrafts = DeleteDraftChangeOp.allowDrafts(cfg);
+    this.allowDrafts = DeleteChangeOp.allowDrafts(cfg);
   }
 
   @Override
@@ -71,14 +72,21 @@ public class DeleteDraftChange implements
   @Override
   public UiAction.Description getDescription(ChangeResource rsrc) {
     try {
+      Change.Status status = rsrc.getChange().getStatus();
+      ChangeControl changeControl = rsrc.getControl();
+      boolean visible = isActionAllowed(changeControl, status)
+          && changeControl.canDelete(db.get(), status);
       return new UiAction.Description()
         .setLabel("Delete")
-        .setTitle("Delete draft change " + rsrc.getId())
+        .setTitle("Delete change " + rsrc.getId())
-        .setVisible(allowDrafts
+        .setVisible(visible);
-            && rsrc.getChange().getStatus() == Status.DRAFT
-            && rsrc.getControl().canDeleteDraft(db.get()));
     } catch (OrmException e) {
       throw new IllegalStateException(e);
     }
   }
+
+  private boolean isActionAllowed(ChangeControl changeControl,
+      Status status) {
+    return status != Status.DRAFT || allowDrafts || changeControl.isAdmin();
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java

@@ -44,7 +44,7 @@ import org.eclipse.jgit.transport.ReceiveCommand;
 import java.io.IOException;
 import java.util.Collection;
 
-class DeleteDraftChangeOp extends BatchUpdate.Op {
+class DeleteChangeOp extends BatchUpdate.Op {
   static boolean allowDrafts(Config cfg) {
     return cfg.getBoolean("change", "allowDrafts", true);
   }
@@ -68,7 +68,7 @@ class DeleteDraftChangeOp extends BatchUpdate.Op {
   private Change.Id id;
 
   @Inject
-  DeleteDraftChangeOp(PatchSetUtil psUtil,
+  DeleteChangeOp(PatchSetUtil psUtil,
       StarredChangesUtil starredChangesUtil,
       DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
       @GerritServerConfig Config cfg) {
@@ -82,16 +82,18 @@ class DeleteDraftChangeOp extends BatchUpdate.Op {
   public boolean updateChange(ChangeContext ctx) throws RestApiException,
       OrmException, IOException, NoSuchChangeException {
     checkState(ctx.getOrder() == BatchUpdate.Order.DB_BEFORE_REPO,
-        "must use DeleteDraftChangeOp with DB_BEFORE_REPO");
+        "must use DeleteChangeOp with DB_BEFORE_REPO");
-    checkState(id == null, "cannot reuse DeleteDraftChangeOp");
+    checkState(id == null, "cannot reuse DeleteChangeOp");
 
     id = ctx.getChange().getId();
     Collection<PatchSet> patchSets = psUtil.byChange(ctx.getDb(),
         ctx.getNotes());
 
     ensureDeletable(ctx, id, patchSets);
-    deleteChangeElementsFromDb(ctx, id);
+    // Cleaning up is only possible as long as the change and its elements are
+    // still part of the database.
     cleanUpReferences(ctx, id, patchSets);
+    deleteChangeElementsFromDb(ctx, id);
 
     ctx.deleteChange();
     return true;
@@ -100,19 +102,25 @@ class DeleteDraftChangeOp extends BatchUpdate.Op {
   private void ensureDeletable(ChangeContext ctx, Change.Id id,
       Collection<PatchSet> patchSets) throws ResourceConflictException,
       MethodNotAllowedException, OrmException, AuthException {
-    if (ctx.getChange().getStatus() != Change.Status.DRAFT) {
+    Change.Status status = ctx.getChange().getStatus();
-      throw new ResourceConflictException("Change is not a draft: " + id);
+    if (status == Change.Status.MERGED) {
+      throw new MethodNotAllowedException("Deleting merged change " + id
+          + " is not allowed");
     }
-    if (!allowDrafts) {
+
-      throw new MethodNotAllowedException("Draft workflow is disabled");
+    if (!ctx.getControl().canDelete(ctx.getDb(), status)) {
+      throw new AuthException("Deleting change " + id + " is not permitted");
     }
-    if (!ctx.getControl().canDeleteDraft(ctx.getDb())) {
+
-      throw new AuthException("Not permitted to delete this draft change");
+    if (status == Change.Status.DRAFT) {
-    }
+      if (!allowDrafts && !ctx.getControl().isAdmin()) {
-    for (PatchSet ps : patchSets) {
+        throw new MethodNotAllowedException("Draft workflow is disabled");
-      if (!ps.isDraft()) {
+      }
-        throw new ResourceConflictException("Cannot delete draft change " + id
+      for (PatchSet ps : patchSets) {
-            + ": patch set " + ps.getPatchSetId() + " is not a draft");
+        if (!ps.isDraft()) {
+          throw new ResourceConflictException("Cannot delete draft change " + id
+              + ": patch set " + ps.getPatchSetId() + " is not a draft");
+        }
       }
     }
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java

@@ -59,7 +59,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
   private final BatchUpdate.Factory updateFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final PatchSetUtil psUtil;
-  private final Provider<DeleteDraftChangeOp> deleteChangeOpProvider;
+  private final Provider<DeleteChangeOp> deleteChangeOpProvider;
   private final DynamicItem<AccountPatchReviewStore> accountPatchReviewStore;
   private final boolean allowDrafts;
 
@@ -68,7 +68,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       BatchUpdate.Factory updateFactory,
       PatchSetInfoFactory patchSetInfoFactory,
       PatchSetUtil psUtil,
-      Provider<DeleteDraftChangeOp> deleteChangeOpProvider,
+      Provider<DeleteChangeOp> deleteChangeOpProvider,
       DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
       @GerritServerConfig Config cfg) {
     this.db = db;
@@ -97,7 +97,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
 
     private Collection<PatchSet> patchSetsBeforeDeletion;
     private PatchSet patchSet;
-    private DeleteDraftChangeOp deleteChangeOp;
+    private DeleteChangeOp deleteChangeOp;
 
     private Op(PatchSet.Id psId) {
       this.psId = psId;
@@ -116,7 +116,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       if (!allowDrafts) {
         throw new MethodNotAllowedException("Draft workflow is disabled");
       }
-      if (!ctx.getControl().canDeleteDraft(ctx.getDb())) {
+      if (!ctx.getControl().canDelete(ctx.getDb(), Change.Status.DRAFT)) {
         throw new AuthException("Not permitted to delete this draft patch set");
       }
 
@@ -146,8 +146,8 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       psUtil.delete(ctx.getDb(), ctx.getUpdate(patchSet.getId()), patchSet);
 
       accountPatchReviewStore.get().clearReviewed(psId);
-      // Use the unwrap from DeleteDraftChangeOp to handle BatchUpdateReviewDb.
+      // Use the unwrap from DeleteChangeOp to handle BatchUpdateReviewDb.
-      ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb());
+      ReviewDb db = DeleteChangeOp.unwrap(ctx.getDb());
       db.changeMessages().delete(db.changeMessages().byPatchSet(psId));
       db.patchComments().delete(db.patchComments().byPatchSet(psId));
       db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(psId));
@@ -195,7 +195,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
             rsrc.getPatchSet().getPatchSetId()))
         .setVisible(allowDrafts
             && rsrc.getPatchSet().isDraft()
-            && rsrc.getControl().canDeleteDraft(db.get())
+            && rsrc.getControl().canDelete(db.get(), Change.Status.DRAFT)
             && psCount > 1);
     } catch (OrmException e) {
       throw new IllegalStateException(e);

gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java

@@ -68,7 +68,7 @@ public class Module extends RestApiModule {
     post(CHANGE_KIND, "check").to(Check.class);
     put(CHANGE_KIND, "topic").to(PutTopic.class);
     delete(CHANGE_KIND, "topic").to(PutTopic.class);
-    delete(CHANGE_KIND).to(DeleteDraftChange.class);
+    delete(CHANGE_KIND).to(DeleteChange.class);
     post(CHANGE_KIND, "abandon").to(Abandon.class);
     post(CHANGE_KIND, "hashtags").to(PostHashtags.class);
     post(CHANGE_KIND, "publish").to(PublishDraftPatchSet.CurrentRevision.class);

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -261,10 +261,23 @@ public class ChangeControl {
         && isVisible(db);
   }
 
-  /** Can this user delete this draft change or any draft patch set of this change? */
+  /** Can this user delete this change or any patch set of this change? */
-  public boolean canDeleteDraft(final ReviewDb db) throws OrmException {
+  public boolean canDelete(ReviewDb db, Change.Status status)
-    return (isOwner() || getRefControl().canDeleteDrafts())
+      throws OrmException {
-        && isVisible(db);
+    if (!isVisible(db)) {
+      return false;
+    }
+
+    switch (status) {
+      case DRAFT:
+        return (isOwner() || getRefControl().canDeleteDrafts());
+      case NEW:
+      case ABANDONED:
+        return isAdmin();
+      case MERGED:
+      default:
+        return false;
+    }
   }
 
   /** Can this user rebase this change? */
@@ -377,6 +390,10 @@ public class ChangeControl {
     return false;
   }
 
+  public boolean isAdmin() {
+    return getUser().getCapabilities().canAdministrateServer();
+  }
+
   /** @return true if the user is allowed to remove this reviewer. */
   public boolean canRemoveReviewer(PatchSetApproval approval) {
     return canRemoveReviewer(approval.getAccountId(), approval.getValue());