Allow EditEngine forms to be marked as "edit" forms

Summary:
Ref T9132. Ref T9908. This attempts to move us forward on answering this question:

> Which form gets used when a user clicks "Edit Task"?

One answer is "the same form that was used to create the task". There are several problems with that:

  - The form might not exist anymore.
  - The user might not have permission to see it.
  - Some of the fields might be hidden, essentially preventing them from being edited.
  - We have to store the value somewhere and old tasks won't have a value.
  - Any instructions on the form probably don't apply to edits.

One answer is "force the default, full form". That's not as problematic, but it means we have no ability to create limited access users who see fewer fields.

The answer in this diff is:

  - Forms can be marked as "edit forms".
  - We take the user to the first edit form they have permission to see, from a master list.

This allows you to create several forms like:

  - Advanced Edit Form (say, all fields -- visible to administrators).
  - Basic Edit Form (say, no policies -- visible to trusted users).
  - Noob Edit Form (say, no policies, priorities, or status -- visible to everyone).

Then you can give everyone access to "noob", some people access to "basic", and a few people access to "advanced".

This might only be part of the answer. In particular, you can still //use// any edit form you can see, so we could do these things in the future:

  - Give you an option to switch to a different form if you want.
  - Save the form the task was created with, and use that form by default.

If we do pursue those, we can fall back to this behavior if there's a problem with them (e.g., original form doesn't exist or wasn't recorded).

There's also no "reorder" UI yet, that'll be coming in the next diff.

I'm also going to try to probably make the "create" and "edit" stuff a little more consistent / less weird in a bit.

Test Plan: Marked various forms as edit forms or not edit forms, made edits, hit permissions errors, etc.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9132, T9908

Differential Revision: https://secure.phabricator.com/D14702

src/__phutil_library_map__.php

@@ -2135,6 +2135,7 @@ phutil_register_library_map(array(
     'PhabricatorEditEngineConfigurationEditController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationEditController.php',
     'PhabricatorEditEngineConfigurationEditEngine' => 'applications/transactions/editor/PhabricatorEditEngineConfigurationEditEngine.php',
     'PhabricatorEditEngineConfigurationEditor' => 'applications/transactions/editor/PhabricatorEditEngineConfigurationEditor.php',
+    'PhabricatorEditEngineConfigurationIsEditController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationIsEditController.php',
     'PhabricatorEditEngineConfigurationListController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationListController.php',
     'PhabricatorEditEngineConfigurationLockController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationLockController.php',
     'PhabricatorEditEngineConfigurationPHIDType' => 'applications/transactions/phid/PhabricatorEditEngineConfigurationPHIDType.php',
@@ -6263,6 +6264,7 @@ phutil_register_library_map(array(
     'PhabricatorEditEngineConfigurationEditController' => 'PhabricatorEditEngineController',
     'PhabricatorEditEngineConfigurationEditEngine' => 'PhabricatorEditEngine',
     'PhabricatorEditEngineConfigurationEditor' => 'PhabricatorApplicationTransactionEditor',
+    'PhabricatorEditEngineConfigurationIsEditController' => 'PhabricatorEditEngineController',
     'PhabricatorEditEngineConfigurationListController' => 'PhabricatorEditEngineController',
     'PhabricatorEditEngineConfigurationLockController' => 'PhabricatorEditEngineController',
     'PhabricatorEditEngineConfigurationPHIDType' => 'PhabricatorPHIDType',

src/applications/transactions/application/PhabricatorTransactionsApplication.php

@@ -53,6 +53,8 @@ final class PhabricatorTransactionsApplication extends PhabricatorApplication {
               'PhabricatorEditEngineConfigurationLockController',
             'defaultcreate/(?P<key>[^/]+)/' =>
               'PhabricatorEditEngineConfigurationDefaultCreateController',
+            'defaultedit/(?P<key>[^/]+)/' =>
+              'PhabricatorEditEngineConfigurationIsEditController',
             'disable/(?P<key>[^/]+)/' =>
               'PhabricatorEditEngineConfigurationDisableController',
           ),

src/applications/transactions/controller/PhabricatorEditEngineConfigurationIsEditController.php

@@ -0,0 +1,60 @@
+<?php
+
+final class PhabricatorEditEngineConfigurationIsEditController
+  extends PhabricatorEditEngineController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+
+    $config = $this->loadConfigForEdit();
+    if (!$config) {
+      return id(new Aphront404Response());
+    }
+
+    $engine_key = $config->getEngineKey();
+    $key = $config->getIdentifier();
+    $cancel_uri = "/transactions/editengine/{$engine_key}/view/{$key}/";
+
+    $type = PhabricatorEditEngineConfigurationTransaction::TYPE_ISEDIT;
+
+    if ($request->isFormPost()) {
+      $xactions = array();
+
+      $xactions[] = id(new PhabricatorEditEngineConfigurationTransaction())
+        ->setTransactionType($type)
+        ->setNewValue(!$config->getIsEdit());
+
+      $editor = id(new PhabricatorEditEngineConfigurationEditor())
+        ->setActor($viewer)
+        ->setContentSourceFromRequest($request)
+        ->setContinueOnMissingFields(true)
+        ->setContinueOnNoEffect(true);
+
+      $editor->applyTransactions($config, $xactions);
+
+      return id(new AphrontRedirectResponse())
+        ->setURI($cancel_uri);
+    }
+
+    if ($config->getIsEdit()) {
+      $title = pht('Unmark as Edit Form');
+      $body = pht(
+        'Unmark this form as an edit form? It will no longer be able to be '.
+        'used to edit objects.');
+      $button = pht('Unmark Form');
+    } else {
+      $title = pht('Mark as Edit Form');
+      $body = pht(
+        'Mark this form as an edit form? Users who can view it will be able '.
+        'to use it to edit objects.');
+      $button = pht('Mark Form');
+    }
+
+    return $this->newDialog()
+      ->setTitle($title)
+      ->appendParagraph($body)
+      ->addSubmitButton($button)
+      ->addCancelbutton($cancel_uri);
+  }
+
+}

src/applications/transactions/controller/PhabricatorEditEngineConfigurationViewController.php

@@ -177,6 +177,24 @@ final class PhabricatorEditEngineConfigurationViewController
         ->setWorkflow(true)
         ->setDisabled(!$can_edit));
 
+    if ($config->getIsEdit()) {
+      $isedit_name = pht('Unmark as "Edit" Form');
+      $isedit_icon = 'fa-minus';
+    } else {
+      $isedit_name = pht('Mark as "Edit" Form');
+      $isedit_icon = 'fa-plus';
+    }
+
+    $isedit_uri = "{$base_uri}/defaultedit/{$form_key}/";
+
+    $view->addAction(
+      id(new PhabricatorActionView())
+        ->setName($isedit_name)
+        ->setIcon($isedit_icon)
+        ->setHref($isedit_uri)
+        ->setWorkflow(true)
+        ->setDisabled(!$can_edit));
+
     return $view;
   }
 

src/applications/transactions/editengine/PhabricatorEditEngine.php

@@ -198,59 +198,68 @@ abstract class PhabricatorEditEngine
     return $this->editEngineConfiguration;
   }
 
-
-  /**
-   * Load the default configuration, ignoring customization in the database
-   * (which means we implicitly ignore policies).
-   *
-   * This is used from places like Conduit, where the fields available in the
-   * API should not be affected by configuration changes.
-   *
-   * @return PhabricatorEditEngineConfiguration Default configuration, ignoring
-   *   customization.
-   */
-  private function loadDefaultEditEngineConfiguration() {
-    return $this->loadEditEngineConfigurationWithOptions(
-      self::EDITENGINECONFIG_DEFAULT,
-      true);
+  private function newConfigurationQuery() {
+    return id(new PhabricatorEditEngineConfigurationQuery())
+      ->setViewer($this->getViewer())
+      ->withEngineKeys(array($this->getEngineKey()));
   }
 
+  private function loadEditEngineConfigurationWithQuery(
+    PhabricatorEditEngineConfigurationQuery $query,
+    $sort_method) {
 
-  /**
-   * Load a named configuration, respecting database customization and policies.
-   *
-   * @param string Configuration key, or null to load the default.
-   * @return PhabricatorEditEngineConfiguration Default configuration,
-   *   respecting customization.
-   */
-  private function loadEditEngineConfiguration($key) {
-    if (!strlen($key)) {
-      $key = self::EDITENGINECONFIG_DEFAULT;
+    if ($sort_method) {
+      $results = $query->execute();
+      $results = msort($results, $sort_method);
+      $result = head($results);
+    } else {
+      $result = $query->executeOne();
     }
 
-    return $this->loadEditEngineConfigurationWithOptions(
-      $key,
-      false);
-  }
-
-  private function loadEditEngineConfigurationWithOptions(
-    $key,
-    $ignore_database) {
-    $viewer = $this->getViewer();
-
-    $config = id(new PhabricatorEditEngineConfigurationQuery())
-      ->setViewer($viewer)
-      ->withEngineKeys(array($this->getEngineKey()))
-      ->withIdentifiers(array($key))
-      ->withIgnoreDatabaseConfigurations($ignore_database)
-      ->executeOne();
-    if (!$config) {
+    if (!$result) {
       return null;
     }
 
-    $this->editEngineConfiguration = $config;
+    $this->editEngineConfiguration = $result;
+    return $result;
+  }
 
-    return $config;
+  private function loadEditEngineConfigurationWithIdentifier($identifier) {
+    $query = $this->newConfigurationQuery()
+      ->withIdentifiers(array($identifier));
+
+    return $this->loadEditEngineConfigurationWithQuery($query, null);
+  }
+
+  private function loadDefaultConfiguration() {
+    $query = $this->newConfigurationQuery()
+      ->withIdentifiers(
+        array(
+          self::EDITENGINECONFIG_DEFAULT,
+        ))
+      ->withIgnoreDatabaseConfigurations(true);
+
+    return $this->loadEditEngineConfigurationWithQuery($query, null);
+  }
+
+  private function loadDefaultCreateConfiguration() {
+    $query = $this->newConfigurationQuery()
+      ->withIsDefault(true)
+      ->withIsDisabled(false);
+
+    return $this->loadEditEngineConfigurationWithQuery(
+      $query,
+      'getCreateSortKey');
+  }
+
+  private function loadDefaultEditConfiguration() {
+    $query = $this->newConfigurationQuery()
+      ->withIsEdit(true)
+      ->withIsDisabled(false);
+
+    return $this->loadEditEngineConfigurationWithQuery(
+      $query,
+      'getEditSortKey');
   }
 
   final public function getBuiltinEngineConfigurations() {
@@ -278,7 +287,8 @@ abstract class PhabricatorEditEngine
       if (!$first->getBuiltinKey()) {
         $first
           ->setBuiltinKey(self::EDITENGINECONFIG_DEFAULT)
-          ->setIsDefault(true);
+          ->setIsDefault(true)
+          ->setIsEdit(true);
 
         if (!strlen($first->getName())) {
           $first->setName($this->getObjectCreateShortText());
@@ -648,18 +658,8 @@ abstract class PhabricatorEditEngine
         break;
     }
 
-    if ($use_default) {
-      $config = $this->loadDefaultEditEngineConfiguration();
-    } else {
-      $form_key = $request->getURIData('formKey');
-      $config = $this->loadEditEngineConfiguration($form_key);
-    }
-
-    if (!$config) {
-      return new Aphront404Response();
-    }
-
     $id = $request->getURIData('id');
+
     if ($id) {
       $this->setIsCreate(false);
       $object = $this->newObjectFromID($id, $capabilities);
@@ -679,6 +679,42 @@ abstract class PhabricatorEditEngine
 
     $this->validateObject($object);
 
+    if ($use_default) {
+      $config = $this->loadDefaultConfiguration();
+      if (!$config) {
+        return new Aphront404Response();
+      }
+    } else {
+      $form_key = $request->getURIData('formKey');
+      if (strlen($form_key)) {
+        $config = $this->loadEditEngineConfigurationWithIdentifier($form_key);
+
+        if (!$config) {
+          return new Aphront404Response();
+        }
+
+        if ($id && !$config->getIsEdit()) {
+          return $this->buildNotEditFormRespose($object, $config);
+        }
+      } else {
+        if ($id) {
+          $config = $this->loadDefaultEditConfiguration();
+          if (!$config) {
+            return $this->buildNoEditResponse($object);
+          }
+        } else {
+          $config = $this->loadDefaultCreateConfiguration();
+          if (!$config) {
+            return $this->buildNoCreateResponse($object);
+          }
+        }
+      }
+    }
+
+    if ($config->getIsDisabled()) {
+      return $this->buildFormDisabledResponse($object, $config);
+    }
+
     switch ($action) {
       case 'parameters':
         return $this->buildParametersResponse($object);
@@ -1032,7 +1068,7 @@ abstract class PhabricatorEditEngine
   }
 
   final public function buildEditEngineCommentView($object) {
-    $config = $this->loadDefaultEditEngineConfiguration();
+    $config = $this->loadDefaultConfiguration();
 
     $viewer = $this->getViewer();
     $object_phid = $object->getPHID();
@@ -1146,42 +1182,68 @@ abstract class PhabricatorEditEngine
   }
 
 
-  private function buildNoDefaultResponse($object) {
+  private function buildError($object, $title, $body) {
     $cancel_uri = $this->getObjectCreateCancelURI($object);
 
     return $this->getController()
       ->newDialog()
-      ->setTitle(pht('No Default Create Forms'))
-      ->appendParagraph(
-        pht(
-          'This application is not configured with any visible, enabled '.
-          'forms for creating objects.'))
+      ->setTitle($title)
+      ->appendParagraph($body)
       ->addCancelButton($cancel_uri);
   }
 
+
+  private function buildNoDefaultResponse($object) {
+    return $this->buildError(
+      $object,
+      pht('No Default Create Forms'),
+      pht(
+        'This application is not configured with any forms for creating '.
+        'objects that are visible to you and enabled.'));
+  }
+
   private function buildNoCreateResponse($object) {
-    $cancel_uri = $this->getObjectCreateCancelURI($object);
-
-    return $this->getController()
-      ->newDialog()
-      ->setTitle(pht('No Create Permission'))
-      ->appendParagraph(
-        pht(
-          'You do not have permission to create these objects.'))
-      ->addCancelButton($cancel_uri);
+    return $this->buildError(
+      $object,
+      pht('No Create Permission'),
+      pht('You do not have permission to create these objects.'));
   }
 
   private function buildNoManageResponse($object) {
-    $cancel_uri = $this->getObjectCreateCancelURI($object);
+    return $this->buildError(
+      $object,
+      pht('No Manage Permission'),
+      pht(
+        'You do not have permission to configure forms for this '.
+        'application.'));
+  }
 
-    return $this->getController()
-      ->newDialog()
-      ->setTitle(pht('No Manage Permission'))
-      ->appendParagraph(
-        pht(
-          'You do not have permission to configure forms for this '.
-          'application.'))
-      ->addCancelButton($cancel_uri);
+  private function buildNoEditResponse($object) {
+    return $this->buildError(
+      $object,
+      pht('No Edit Forms'),
+      pht(
+        'You do not have access to any forms which are enabled and marked '.
+        'as edit forms.'));
+  }
+
+  private function buildNotEditFormRespose($object, $config) {
+    return $this->buildError(
+      $object,
+      pht('Not an Edit Form'),
+      pht(
+        'This form ("%s") is not marked as an edit form, so '.
+        'it can not be used to edit objects.',
+        $config->getName()));
+  }
+
+  private function buildDisabledFormResponse($object, $config) {
+    return $this->buildError(
+      $object,
+      pht('Form Disabled'),
+      pht(
+        'This form ("%s") has been disabled, so it can not be used.',
+        $config->getName()));
   }
 
   private function buildCommentResponse($object) {
@@ -1198,7 +1260,7 @@ abstract class PhabricatorEditEngine
       return new Aphront400Response();
     }
 
-    $config = $this->loadDefaultEditEngineConfiguration();
+    $config = $this->loadDefaultConfiguration();
     $fields = $this->buildEditFields($object);
 
     $is_preview = $request->isPreviewRequest();
@@ -1328,7 +1390,7 @@ abstract class PhabricatorEditEngine
   final public function buildConduitResponse(ConduitAPIRequest $request) {
     $viewer = $this->getViewer();
 
-    $config = $this->loadDefaultEditEngineConfiguration();
+    $config = $this->loadDefaultConfiguration();
     if (!$config) {
       throw new Exception(
         pht(
@@ -1476,7 +1538,7 @@ abstract class PhabricatorEditEngine
   }
 
   public function getConduitEditTypes() {
-    $config = $this->loadDefaultEditEngineConfiguration();
+    $config = $this->loadDefaultConfiguration();
     if (!$config) {
       return array();
     }

src/applications/transactions/editor/PhabricatorEditEngineConfigurationEditor.php

@@ -23,6 +23,7 @@ final class PhabricatorEditEngineConfigurationEditor
     $types[] = PhabricatorEditEngineConfigurationTransaction::TYPE_LOCKS;
     $types[] =
       PhabricatorEditEngineConfigurationTransaction::TYPE_DEFAULTCREATE;
+    $types[] = PhabricatorEditEngineConfigurationTransaction::TYPE_ISEDIT;
     $types[] = PhabricatorEditEngineConfigurationTransaction::TYPE_DISABLE;
 
 
@@ -75,6 +76,8 @@ final class PhabricatorEditEngineConfigurationEditor
         return $object->getFieldLocks();
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DEFAULTCREATE:
         return (int)$object->getIsDefault();
+      case PhabricatorEditEngineConfigurationTransaction::TYPE_ISEDIT:
+        return (int)$object->getIsEdit();
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DISABLE:
         return (int)$object->getIsDisabled();
     }
@@ -92,6 +95,7 @@ final class PhabricatorEditEngineConfigurationEditor
       case PhabricatorEditEngineConfigurationTransaction::TYPE_LOCKS:
         return $xaction->getNewValue();
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DEFAULTCREATE:
+      case PhabricatorEditEngineConfigurationTransaction::TYPE_ISEDIT:
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DISABLE:
         return (int)$xaction->getNewValue();
     }
@@ -121,6 +125,9 @@ final class PhabricatorEditEngineConfigurationEditor
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DEFAULTCREATE:
         $object->setIsDefault($xaction->getNewValue());
         return;
+      case PhabricatorEditEngineConfigurationTransaction::TYPE_ISEDIT:
+        $object->setIsEdit($xaction->getNewValue());
+        return;
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DISABLE:
         $object->setIsDisabled($xaction->getNewValue());
         return;
@@ -138,6 +145,7 @@ final class PhabricatorEditEngineConfigurationEditor
       case PhabricatorEditEngineConfigurationTransaction::TYPE_PREAMBLE;
       case PhabricatorEditEngineConfigurationTransaction::TYPE_ORDER;
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DEFAULT:
+      case PhabricatorEditEngineConfigurationTransaction::TYPE_ISEDIT:
       case PhabricatorEditEngineConfigurationTransaction::TYPE_LOCKS:
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DEFAULTCREATE:
       case PhabricatorEditEngineConfigurationTransaction::TYPE_DISABLE:

src/applications/transactions/query/PhabricatorEditEngineConfigurationQuery.php

@@ -9,6 +9,7 @@ final class PhabricatorEditEngineConfigurationQuery
   private $builtinKeys;
   private $identifiers;
   private $default;
+  private $isEdit;
   private $disabled;
   private $ignoreDatabaseConfigurations;
 
@@ -42,6 +43,11 @@ final class PhabricatorEditEngineConfigurationQuery
     return $this;
   }
 
+  public function withIsEdit($edit) {
+    $this->isEdit = $edit;
+    return $this;
+  }
+
   public function withIsDisabled($disabled) {
     $this->disabled = $disabled;
     return $this;
@@ -148,6 +154,14 @@ final class PhabricatorEditEngineConfigurationQuery
       }
     }
 
+    if ($this->isEdit !== null) {
+      foreach ($page as $key => $config) {
+        if ($config->getIsEdit() != $this->isEdit) {
+          unset($page[$key]);
+        }
+      }
+    }
+
     if ($this->disabled !== null) {
       foreach ($page as $key => $config) {
         if ($config->getIsDisabled() != $this->disabled) {

src/applications/transactions/query/PhabricatorEditEngineConfigurationSearchEngine.php

@@ -33,11 +33,37 @@ final class PhabricatorEditEngineConfigurationSearchEngine
 
   protected function buildQueryFromParameters(array $map) {
     $query = $this->newQuery();
+
+    $is_create = $map['isCreate'];
+    if ($is_create !== null) {
+      $query->withIsDefault($is_create);
+    }
+
+    $is_edit = $map['isEdit'];
+    if ($is_edit !== null) {
+      $query->withIsEdit($is_edit);
+    }
+
     return $query;
   }
 
   protected function buildCustomSearchFields() {
-    return array();
+    return array(
+      id(new PhabricatorSearchThreeStateField())
+        ->setLabel(pht('Create'))
+        ->setKey('isCreate')
+        ->setOptions(
+          pht('Show All'),
+          pht('Hide Create Forms'),
+          pht('Show Only Create Forms')),
+      id(new PhabricatorSearchThreeStateField())
+        ->setLabel(pht('Edit'))
+        ->setKey('isEdit')
+        ->setOptions(
+          pht('Show All'),
+          pht('Hide Edit Forms'),
+          pht('Show Only Edit Forms')),
+    );
   }
 
   protected function getDefaultFieldOrder() {
@@ -51,6 +77,8 @@ final class PhabricatorEditEngineConfigurationSearchEngine
   protected function getBuiltinQueryNames() {
     $names = array(
       'all' => pht('All Forms'),
+      'create' => pht('Create Forms'),
+      'modify' => pht('Edit Forms'),
     );
 
     return $names;
@@ -63,6 +91,10 @@ final class PhabricatorEditEngineConfigurationSearchEngine
     switch ($query_key) {
       case 'all':
         return $query;
+      case 'create':
+        return $query->setParameter('isCreate', true);
+      case 'modify':
+        return $query->setParameter('isEdit', true);
     }
 
     return parent::buildSavedQueryFromBuiltin($query_key);
@@ -96,6 +128,10 @@ final class PhabricatorEditEngineConfigurationSearchEngine
         $item->addIcon('fa-plus', pht('Default'));
       }
 
+      if ($config->getIsEdit()) {
+        $item->addIcon('fa-pencil', pht('Edit Form'));
+      }
+
       if ($config->getIsDisabled()) {
         $item->addIcon('fa-ban', pht('Disabled'));
       }

src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php

@@ -9,6 +9,7 @@ final class PhabricatorEditEngineConfigurationTransaction
   const TYPE_DEFAULT = 'editengine.config.default';
   const TYPE_LOCKS = 'editengine.config.locks';
   const TYPE_DEFAULTCREATE = 'editengine.config.default.create';
+  const TYPE_ISEDIT = 'editengine.config.isedit';
   const TYPE_DISABLE = 'editengine.config.disable';
 
   public function getApplicationName() {
@@ -72,6 +73,16 @@ final class PhabricatorEditEngineConfigurationTransaction
             '%s removed this form from the "Create" menu.',
             $this->renderHandleLink($author_phid));
         }
+      case self::TYPE_ISEDIT:
+        if ($new) {
+          return pht(
+            '%s marked this form as an edit form.',
+            $this->renderHandleLink($author_phid));
+        } else {
+          return pht(
+            '%s unmarked this form as an edit form.',
+            $this->renderHandleLink($author_phid));
+        }
       case self::TYPE_DISABLE:
         if ($new) {
           return pht(