Separate editing of package data and paths in Owners

Summary:
Ref T8320. There's currently one enormous form; split it into a general information form (name, description, owners) and a paths form.

I think this is a little more manageable from both a UX point of view and from an "I have to convert this to use ApplicationTransactions" point of view.

Test Plan:
  - Edited paths.
  - Edited non-path information.
  - Created new packages.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8320

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

src/__phutil_library_map__.php

@@ -2181,6 +2181,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnersPackageSearchEngine' => 'applications/owners/query/PhabricatorOwnersPackageSearchEngine.php',
     'PhabricatorOwnersPackageTestCase' => 'applications/owners/storage/__tests__/PhabricatorOwnersPackageTestCase.php',
     'PhabricatorOwnersPath' => 'applications/owners/storage/PhabricatorOwnersPath.php',
+    'PhabricatorOwnersPathsController' => 'applications/owners/controller/PhabricatorOwnersPathsController.php',
     'PhabricatorPHDConfigOptions' => 'applications/config/option/PhabricatorPHDConfigOptions.php',
     'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php',
     'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php',
@@ -5588,6 +5589,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnersPackageSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'PhabricatorOwnersPackageTestCase' => 'PhabricatorTestCase',
     'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO',
+    'PhabricatorOwnersPathsController' => 'PhabricatorOwnersController',
     'PhabricatorPHDConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorPHPASTApplication' => 'PhabricatorApplication',
     'PhabricatorPHPConfigSetupCheck' => 'PhabricatorSetupCheck',

src/applications/owners/application/PhabricatorOwnersApplication.php

@@ -47,6 +47,7 @@ final class PhabricatorOwnersApplication extends PhabricatorApplication {
         'new/' => 'PhabricatorOwnersEditController',
         'package/(?P<id>[1-9]\d*)/' => 'PhabricatorOwnersDetailController',
         'delete/(?P<id>[1-9]\d*)/' => 'PhabricatorOwnersDeleteController',
+        'paths/(?P<id>[1-9]\d*)/' => 'PhabricatorOwnersPathsController',
       ),
     );
   }

src/applications/owners/controller/PhabricatorOwnersDetailController.php

@@ -188,6 +188,7 @@ final class PhabricatorOwnersDetailController
 
     $id = $package->getID();
     $edit_uri = $this->getApplicationURI("/edit/{$id}/");
+    $paths_uri = $this->getApplicationURI("/paths/{$id}/");
     $delete_uri = $this->getApplicationURI("/delete/{$id}/");
 
     $view = id(new PhabricatorActionListView())
@@ -200,6 +201,13 @@ final class PhabricatorOwnersDetailController
           ->setDisabled(!$can_edit)
           ->setWorkflow(!$can_edit)
           ->setHref($edit_uri))
+      ->addAction(
+        id(new PhabricatorActionView())
+          ->setName(pht('Edit Paths'))
+          ->setIcon('fa-folder-open')
+          ->setDisabled(!$can_edit)
+          ->setWorkflow(!$can_edit)
+          ->setHref($paths_uri))
       ->addAction(
         id(new PhabricatorActionView())
           ->setName(pht('Delete Package'))
@@ -242,6 +250,18 @@ final class PhabricatorOwnersDetailController
       );
     }
 
+    $info = null;
+    if (!$paths) {
+      $info = id(new PHUIInfoView())
+        ->setSeverity(PHUIInfoView::SEVERITY_WARNING)
+        ->setErrors(
+          array(
+            pht(
+              'This package does not contain any paths yet. Use '.
+              '"Edit Paths" to add some.'),
+          ));
+    }
+
     $table = id(new AphrontTableView($rows))
       ->setHeaders(
         array(
@@ -256,10 +276,15 @@ final class PhabricatorOwnersDetailController
           'wide',
         ));
 
-    return id(new PHUIObjectBoxView())
+    $box = id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Paths'))
       ->appendChild($table);
 
+    if ($info) {
+      $box->setInfoView($info);
+    }
+
+    return $box;
   }
 
 }

src/applications/owners/controller/PhabricatorOwnersEditController.php

@@ -21,9 +21,13 @@ final class PhabricatorOwnersEditController
       if (!$package) {
         return new Aphront404Response();
       }
+
+      $is_new = false;
     } else {
       $package = new PhabricatorOwnersPackage();
       $package->setPrimaryOwnerPHID($viewer->getPHID());
+
+      $is_new = true;
     }
 
     $e_name = true;
@@ -51,22 +55,6 @@ final class PhabricatorOwnersEditController
       }
       $owners = array_unique($owners);
 
-      $paths = $request->getArr('path');
-      $repos = $request->getArr('repo');
-      $excludes = $request->getArr('exclude');
-
-      $path_refs = array();
-      for ($ii = 0; $ii < count($paths); $ii++) {
-        if (empty($paths[$ii]) || empty($repos[$ii])) {
-          continue;
-        }
-        $path_refs[] = array(
-          'repositoryPHID'  => $repos[$ii],
-          'path'            => $paths[$ii],
-          'excluded'        => $excludes[$ii],
-        );
-      }
-
       if (!strlen($package->getName())) {
         $e_name = pht('Required');
         $errors[] = pht('Package name is required.');
@@ -81,13 +69,9 @@ final class PhabricatorOwnersEditController
         $e_primary = null;
       }
 
-      if (!$path_refs) {
-        $errors[] = pht('Package must include at least one path.');
-      }
-
       if (!$errors) {
         $package->attachUnsavedOwners($owners);
-        $package->attachUnsavedPaths($path_refs);
+        $package->attachUnsavedPaths(array());
         $package->attachOldAuditingEnabled($old_auditing_enabled);
         $package->attachOldPrimaryOwnerPHID($old_primary);
         try {
@@ -95,8 +79,15 @@ final class PhabricatorOwnersEditController
             ->setActor($viewer)
             ->setPackage($package)
             ->save();
-          return id(new AphrontRedirectResponse())
-            ->setURI('/owners/package/'.$package->getID().'/');
+
+          $id = $package->getID();
+          if ($is_new) {
+            $next_uri = '/owners/paths/'.$id.'/';
+          } else {
+            $next_uri = '/owners/package/'.$id.'/';
+          }
+
+          return id(new AphrontRedirectResponse())->setURI($next_uri);
         } catch (AphrontDuplicateKeyQueryException $ex) {
           $e_name = pht('Duplicate');
           $errors[] = pht('Package name must be unique.');
@@ -105,16 +96,6 @@ final class PhabricatorOwnersEditController
     } else {
       $owners = $package->loadOwners();
       $owners = mpull($owners, 'getUserPHID');
-
-      $paths = $package->loadPaths();
-      $path_refs = array();
-      foreach ($paths as $path) {
-        $path_refs[] = array(
-          'repositoryPHID' => $path->getRepositoryPHID(),
-          'path' => $path->getPath(),
-          'excluded' => $path->getExcluded(),
-        );
-      }
     }
 
     $primary = $package->getPrimaryOwnerPHID();
@@ -124,52 +105,16 @@ final class PhabricatorOwnersEditController
       $value_primary_owner = array();
     }
 
-    if ($package->getID()) {
-      $title = pht('Edit Package');
-    } else {
+    if ($is_new) {
+      $cancel_uri = '/owners/';
       $title = pht('New Package');
+      $button_text = pht('Continue');
+    } else {
+      $cancel_uri = '/owners/package/'.$package->getID().'/';
+      $title = pht('Edit Package');
+      $button_text = pht('Save Package');
     }
 
-    $repos = id(new PhabricatorRepositoryQuery())
-      ->setViewer($viewer)
-      ->execute();
-
-    $default_paths = array();
-    foreach ($repos as $repo) {
-      $default_path = $repo->getDetail('default-owners-path');
-      if ($default_path) {
-        $default_paths[$repo->getPHID()] = $default_path;
-      }
-    }
-
-    $repos = mpull($repos, 'getCallsign', 'getPHID');
-    asort($repos);
-
-    $template = new AphrontTypeaheadTemplateView();
-    $template = $template->render();
-
-    Javelin::initBehavior(
-      'owners-path-editor',
-      array(
-        'root'                => 'path-editor',
-        'table'               => 'paths',
-        'add_button'          => 'addpath',
-        'repositories'        => $repos,
-        'input_template'      => $template,
-        'pathRefs'            => $path_refs,
-
-        'completeURI'         => '/diffusion/services/path/complete/',
-        'validateURI'         => '/diffusion/services/path/validate/',
-
-        'repositoryDefaultPaths' => $default_paths,
-      ));
-
-    require_celerity_resource('owners-path-editor-css');
-
-    $cancel_uri = $package->getID()
-      ? '/owners/package/'.$package->getID().'/'
-      : '/owners/';
-
     $form = id(new AphrontFormView())
       ->setUser($viewer)
       ->appendChild(
@@ -210,30 +155,6 @@ final class PhabricatorOwnersEditController
             $package->getAuditingEnabled()
               ? 'enabled'
               : 'disabled'))
-      ->appendChild(
-        id(new PHUIFormInsetView())
-          ->setTitle(pht('Paths'))
-          ->addDivAttributes(array('id' => 'path-editor'))
-          ->setRightButton(javelin_tag(
-              'a',
-              array(
-                'href' => '#',
-                'class' => 'button green',
-                'sigil' => 'addpath',
-                'mustcapture' => true,
-              ),
-              pht('Add New Path')))
-          ->setDescription(
-            pht(
-              'Specify the files and directories which comprise '.
-              'this package.'))
-          ->setContent(javelin_tag(
-              'table',
-              array(
-                'class' => 'owners-path-editor-table',
-                'sigil' => 'paths',
-              ),
-              '')))
       ->appendChild(
         id(new AphrontFormTextAreaControl())
           ->setLabel(pht('Description'))
@@ -242,7 +163,7 @@ final class PhabricatorOwnersEditController
       ->appendChild(
         id(new AphrontFormSubmitControl())
           ->addCancelButton($cancel_uri)
-          ->setValue(pht('Save Package')));
+          ->setValue($button_text));
 
     $form_box = id(new PHUIObjectBoxView())
       ->setHeaderText($title)
@@ -251,7 +172,10 @@ final class PhabricatorOwnersEditController
 
     $crumbs = $this->buildApplicationCrumbs();
     if ($package->getID()) {
-      $crumbs->addTextCrumb(pht('Edit %s', $package->getName()));
+      $crumbs->addTextCrumb(
+        $package->getName(),
+        $this->getApplicationURI('package/'.$package->getID().'/'));
+      $crumbs->addTextCrumb(pht('Edit'));
     } else {
       $crumbs->addTextCrumb(pht('New Package'));
     }

src/applications/owners/controller/PhabricatorOwnersPathsController.php

@@ -0,0 +1,156 @@
+<?php
+
+final class PhabricatorOwnersPathsController
+  extends PhabricatorOwnersController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $request->getUser();
+
+    $package = id(new PhabricatorOwnersPackageQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($request->getURIData('id')))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          // TODO: Support this capability.
+          // PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+    if (!$package) {
+      return new Aphront404Response();
+    }
+
+    if ($request->isFormPost()) {
+      $paths = $request->getArr('path');
+      $repos = $request->getArr('repo');
+      $excludes = $request->getArr('exclude');
+
+      $path_refs = array();
+      for ($ii = 0; $ii < count($paths); $ii++) {
+        if (empty($paths[$ii]) || empty($repos[$ii])) {
+          continue;
+        }
+        $path_refs[] = array(
+          'repositoryPHID' => $repos[$ii],
+          'path' => $paths[$ii],
+          'excluded' => $excludes[$ii],
+        );
+      }
+
+      $package->attachUnsavedOwners(array());
+      $package->attachUnsavedPaths($path_refs);
+      $package->attachOldAuditingEnabled($package->getAuditingEnabled());
+      $package->attachOldPrimaryOwnerPHID($package->getPrimaryOwnerPHID());
+
+      id(new PhabricatorOwnersPackageEditor())
+        ->setActor($viewer)
+        ->setPackage($package)
+        ->save();
+
+      return id(new AphrontRedirectResponse())
+        ->setURI('/owners/package/'.$package->getID().'/');
+    } else {
+      $paths = $package->loadPaths();
+      $path_refs = array();
+      foreach ($paths as $path) {
+        $path_refs[] = array(
+          'repositoryPHID' => $path->getRepositoryPHID(),
+          'path' => $path->getPath(),
+          'excluded' => $path->getExcluded(),
+        );
+      }
+    }
+
+    $repos = id(new PhabricatorRepositoryQuery())
+      ->setViewer($viewer)
+      ->execute();
+
+    $default_paths = array();
+    foreach ($repos as $repo) {
+      $default_path = $repo->getDetail('default-owners-path');
+      if ($default_path) {
+        $default_paths[$repo->getPHID()] = $default_path;
+      }
+    }
+
+    $repos = mpull($repos, 'getCallsign', 'getPHID');
+    asort($repos);
+
+    $template = new AphrontTypeaheadTemplateView();
+    $template = $template->render();
+
+    Javelin::initBehavior(
+      'owners-path-editor',
+      array(
+        'root'                => 'path-editor',
+        'table'               => 'paths',
+        'add_button'          => 'addpath',
+        'repositories'        => $repos,
+        'input_template'      => $template,
+        'pathRefs'            => $path_refs,
+
+        'completeURI'         => '/diffusion/services/path/complete/',
+        'validateURI'         => '/diffusion/services/path/validate/',
+
+        'repositoryDefaultPaths' => $default_paths,
+      ));
+
+    require_celerity_resource('owners-path-editor-css');
+
+    $cancel_uri = '/owners/package/'.$package->getID().'/';
+
+    $form = id(new AphrontFormView())
+      ->setUser($viewer)
+      ->appendChild(
+        id(new PHUIFormInsetView())
+          ->setTitle(pht('Paths'))
+          ->addDivAttributes(array('id' => 'path-editor'))
+          ->setRightButton(javelin_tag(
+              'a',
+              array(
+                'href' => '#',
+                'class' => 'button green',
+                'sigil' => 'addpath',
+                'mustcapture' => true,
+              ),
+              pht('Add New Path')))
+          ->setDescription(
+            pht(
+              'Specify the files and directories which comprise '.
+              'this package.'))
+          ->setContent(javelin_tag(
+              'table',
+              array(
+                'class' => 'owners-path-editor-table',
+                'sigil' => 'paths',
+              ),
+              '')))
+      ->appendChild(
+        id(new AphrontFormSubmitControl())
+          ->addCancelButton($cancel_uri)
+          ->setValue(pht('Save Paths')));
+
+    $form_box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Edit Paths'))
+      ->setForm($form);
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $crumbs->addTextCrumb(
+      $package->getName(),
+      $this->getApplicationURI('package/'.$package->getID().'/'));
+    $crumbs->addTextCrumb(pht('Edit Paths'));
+
+    return $this->buildApplicationPage(
+      array(
+        $crumbs,
+        $form_box,
+      ),
+      array(
+        'title' => array(
+          $package->getName(),
+          pht('Edit Paths'),
+        ),
+      ));
+  }
+
+}