Disallow marking merged/abandoned changes private

We want to improve the UX of private changes by limiting the number of
merged private changes. As a first step, we stop allowing users to mark
merged (or abandoned) changes private.

A subsequent commit will auto-publish private changes on submission and
include a UI dialog that tells the user about this process.

Change-Id: Ia4039099b5b23b9db6c88183be51687168e59580

Documentation/rest-api-changes.txt

@@ -2213,8 +2213,8 @@ if no review comment is added.
 'POST /changes/link:#change-id[\{change-id\}]/private'
 --
 
-Marks the change to be private. Changes may only be marked private by the
-owner or site administrators.
+Marks the change to be private. Only open changes can be marked private.
+Changes may only be marked private by the owner or site administrators.
 
 A message can be specified in the request body inside a
 link:#private-input[PrivateInput] entity.

java/com/google/gerrit/server/change/SetPrivateOp.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.change;
 
 import com.google.common.base.Strings;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -71,8 +72,12 @@ public class SetPrivateOp implements BatchUpdateOp {
   }
 
   @Override
-  public boolean updateChange(ChangeContext ctx) throws ResourceConflictException, OrmException {
+  public boolean updateChange(ChangeContext ctx)
+      throws ResourceConflictException, OrmException, BadRequestException {
     change = ctx.getChange();
+    if (isPrivate && !change.isNew()) {
+      throw new BadRequestException("cannot set a non-open change to private");
+    }
     ChangeNotes notes = ctx.getNotes();
     ps = psUtil.get(notes, change.currentPatchSetId());
     ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());

java/com/google/gerrit/server/restapi/change/PostPrivate.java

@@ -93,7 +93,10 @@ public class PostPrivate
     return new UiAction.Description()
         .setLabel("Mark private")
         .setTitle("Mark change as private")
-        .setVisible(and(!disablePrivateChanges && !change.isPrivate(), canSetPrivate(rsrc)));
+        .setVisible(
+            and(
+                !disablePrivateChanges && !change.isPrivate() && change.isNew(),
+                canSetPrivate(rsrc)));
   }
 
   private BooleanCondition canSetPrivate(ChangeResource rsrc) {

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -299,6 +299,20 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_UNSET_PRIVATE);
   }
 
+  @Test
+  public void setMergedChangePrivate() throws Exception {
+    PushOneCommit.Result result = createChange();
+    approve(result.getChangeId());
+    merge(result);
+
+    String changeId = result.getChangeId();
+    assertThat(gApi.changes().id(changeId).get().isPrivate).isNull();
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("cannot set a non-open change to private");
+    gApi.changes().id(changeId).setPrivate(true);
+  }
+
   @Test
   public void administratorCanSetUserChangePrivate() throws Exception {
     TestRepository<InMemoryRepository> userRepo = cloneProject(project, user);
@@ -371,16 +385,6 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(gApi.changes().id(changeId).get().isPrivate).isNull();
   }
 
-  @Test
-  public void administratorCanMarkPrivateAfterMerging() throws Exception {
-    PushOneCommit.Result result = createChange();
-    String changeId = result.getChangeId();
-    assertThat(gApi.changes().id(changeId).get().isPrivate).isNull();
-    merge(result);
-    gApi.changes().id(changeId).setPrivate(true, null);
-    assertThat(gApi.changes().id(changeId).get().isPrivate).isTrue();
-  }
-
   @Test
   public void ownerCannotMarkPrivateAfterMerging() throws Exception {
     TestRepository<InMemoryRepository> userRepo = cloneProject(project, user);