commit 1cfd4d17728bd6b8d6d6af644caa64f9792cbee8
author Patrick Hiesel <hiesel@google.com> Mon Jan 15 17:30:39 2018 +0100
committer Patrick Hiesel <hiesel@google.com> Tue Jan 16 09:03:42 2018 +0000
tree a83ff63c4bab38039acc8c6260c29d6a559dce63
parent 1b619e140e209503f441e6edbbdfdf26a3378b2d

Make project state check in RESTORE explict

The majority of code in {Project,Ref,Change}Control is now about
permissions, but not all. Exceptions include checks for a project's
state. This is confusing, because users are presented with an exception
telling them that they lack some kind of permission while the real
reason for the failed operation is that the project's current state
doesn't permit the operation.

This is part of a series of commits to remove all project state checks
from *Control classes and make explicit checks instead.

This commit also adds intermediary checks for project state to
ChangeControl to keep the individual changes small. These will be
removed in subsequent commits.

Change-Id: I888d82af952041000e97f4fee6995864be36eb49

java/com/google/gerrit/server/project/ChangeControl.java

diff --git a/java/com/google/gerrit/server/project/ChangeControl.java b/java/com/google/gerrit/server/project/ChangeControl.java
index ac9bfe7..8cd12f8 100644
--- a/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/java/com/google/gerrit/server/project/ChangeControl.java
@@ -166,9 +166,7 @@
   /** Can this user restore this change? */
   private boolean canRestore(ReviewDb db) throws OrmException {
     // Anyone who can abandon the change can restore it, as long as they can create changes.
-    return canAbandon(db)
-        && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)
-        && getProjectControl().getProjectState().statePermitsWrite();
+    return canAbandon(db) && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
   }
 
   /** The range of permitted values associated with a label permission. */

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

diff --git a/java/com/google/gerrit/server/restapi/change/Restore.java b/java/com/google/gerrit/server/restapi/change/Restore.java
index 4bf1254..642c35a 100644
--- a/java/com/google/gerrit/server/restapi/change/Restore.java
+++ b/java/com/google/gerrit/server/restapi/change/Restore.java
@@ -39,6 +39,7 @@
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.permissions.ChangePermission;
 import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
@@ -50,6 +51,7 @@
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
+import java.io.IOException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -64,6 +66,7 @@
   private final ChangeMessagesUtil cmUtil;
   private final PatchSetUtil psUtil;
   private final ChangeRestored changeRestored;
+  private final ProjectCache projectCache;
 
   @Inject
   Restore(
@@ -73,7 +76,8 @@
       ChangeMessagesUtil cmUtil,
       PatchSetUtil psUtil,
       RetryHelper retryHelper,
-      ChangeRestored changeRestored) {
+      ChangeRestored changeRestored,
+      ProjectCache projectCache) {
     super(retryHelper);
     this.restoredSenderFactory = restoredSenderFactory;
     this.dbProvider = dbProvider;
@@ -81,13 +85,16 @@
     this.cmUtil = cmUtil;
     this.psUtil = psUtil;
     this.changeRestored = changeRestored;
+    this.projectCache = projectCache;
   }
 
   @Override
   protected ChangeInfo applyImpl(
       BatchUpdate.Factory updateFactory, ChangeResource req, RestoreInput input)
-      throws RestApiException, UpdateException, OrmException, PermissionBackendException {
+      throws RestApiException, UpdateException, OrmException, PermissionBackendException,
+          IOException {
     req.permissions().database(dbProvider).check(ChangePermission.RESTORE);
+    projectCache.checkedGet(req.getProject()).checkStatePermitsWrite();
 
     Op op = new Op(input);
     try (BatchUpdate u =
@@ -154,12 +161,18 @@
 
   @Override
   public UiAction.Description getDescription(ChangeResource rsrc) {
+    boolean projectStatePermitsWrite = false;
+    try {
+      projectStatePermitsWrite = projectCache.checkedGet(rsrc.getProject()).statePermitsWrite();
+    } catch (IOException e) {
+      log.error("Failed to check if project state permits write: " + rsrc.getProject(), e);
+    }
     return new UiAction.Description()
         .setLabel("Restore")
         .setTitle("Restore the change")
         .setVisible(
             and(
-                rsrc.getChange().getStatus() == Status.ABANDONED,
+                rsrc.getChange().getStatus() == Status.ABANDONED && projectStatePermitsWrite,
                 rsrc.permissions().database(dbProvider).testCond(ChangePermission.RESTORE)));
   }
 }