Improve some Spaces behaviors

Summary:
Ref T8449. Try out some more subtle behaviors:

  - Make the "Space" control part of the policy control, so the UI shows "Visible To: [Space][Policy]". I think this helps make the role of spaces more clear. It also makes them easier to implement.
  - Don't show the default space in headers: instead, show nothing.
  - If the user has access to only one space, pretend spaces don't exist (no edit controls, no header stuff).

This might be confusing, but I think most of the time it will all align fairly well with user expectation.

Test Plan:
  - Viewed a list of pastes (saw Space with non-default space, no space with default space, no space with user in only one space).
  - Viewed a paste (saw Space with non-default space, saw no space with default space, saw no space with user in only one space).
  - Edited spaces on objects (control as privileged user, no control as locked user).
  - Created a new paste in a space (got space select as privileged user, no select as locked user).

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8449

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

src/applications/paste/controller/PhabricatorPasteEditController.php

@@ -167,12 +167,6 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController {
       ->setObject($paste)
       ->execute();
 
-    $form->appendControl(
-      id(new PhabricatorSpacesControl())
-        ->setObject($paste)
-        ->setValue($v_space)
-        ->setName('spacePHID'));
-
     $form->appendChild(
       id(new AphrontFormPolicyControl())
         ->setUser($user)
@@ -180,6 +174,7 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController {
         ->setPolicyObject($paste)
         ->setPolicies($policies)
         ->setValue($v_view_policy)
+        ->setSpacePHID($v_space)
         ->setName('can_view'));
 
     $form->appendChild(

src/applications/people/storage/PhabricatorUser.php

@@ -758,6 +758,7 @@ final class PhabricatorUser
     // TODO: We might let the user switch which space they're "in" later on;
     // for now just use the global space if one exists.
 
+    // If the viewer has access to the default space, use that.
     $spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($this);
     foreach ($spaces as $space) {
       if ($space->getIsDefaultNamespace()) {
@@ -765,6 +766,14 @@ final class PhabricatorUser
       }
     }
 
+    // Otherwise, use the space with the lowest ID that they have access to.
+    // This just tends to keep the default stable and predictable over time,
+    // so adding a new space won't change behavior for users.
+    if ($spaces) {
+      $spaces = msort($spaces, 'getID');
+      return head($spaces)->getPHID();
+    }
+
     return null;
   }
 

src/applications/spaces/query/PhabricatorSpacesNamespaceQuery.php

@@ -90,6 +90,17 @@ final class PhabricatorSpacesNamespaceQuery
     return (bool)self::getAllSpaces();
   }
 
+  public static function getViewerSpacesExist(PhabricatorUser $viewer) {
+    if (!self::getSpacesExist()) {
+      return false;
+    }
+
+    // If the viewer has access to only one space, pretend spaces simply don't
+    // exist.
+    $spaces = self::getViewerSpaces($viewer);
+    return (count($spaces) > 1);
+  }
+
   public static function getAllSpaces() {
     $cache = PhabricatorCaches::getRequestCache();
     $cache_key = self::KEY_ALL;

src/applications/spaces/view/PHUISpacesNamespaceContextView.php

@@ -21,7 +21,20 @@ final class PHUISpacesNamespaceContextView extends AphrontView {
       return null;
     }
 
+    // If the viewer can't see spaces, pretend they don't exist.
     $viewer = $this->getUser();
+    if (!PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) {
+      return null;
+    }
+
+    // If this is the default space, don't show a space label.
+    $default = PhabricatorSpacesNamespaceQuery::getDefaultSpace();
+    if ($default) {
+      if ($default->getPHID() == $space_phid) {
+        return null;
+      }
+    }
+
     return phutil_tag(
       'span',
       array(

src/applications/spaces/view/PhabricatorSpacesControl.php

@@ -1,49 +0,0 @@
-<?php
-
-final class PhabricatorSpacesControl extends AphrontFormControl {
-
-  private $object;
-
-  protected function shouldRender() {
-    // Render this control only if some Spaces exist.
-    return PhabricatorSpacesNamespaceQuery::getAllSpaces();
-  }
-
-  public function setObject(PhabricatorSpacesInterface $object) {
-    $this->object = $object;
-    return $this;
-  }
-
-  protected function getCustomControlClass() {
-    return '';
-  }
-
-  protected function getOptions() {
-    $viewer = $this->getUser();
-    $viewer_spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer);
-
-    $map = mpull($viewer_spaces, 'getNamespaceName', 'getPHID');
-    asort($map);
-
-    return $map;
-  }
-
-  protected function renderInput() {
-    $viewer = $this->getUser();
-
-    $this->setLabel(pht('Space'));
-
-    $value = $this->getValue();
-    if ($value === null) {
-      $value = $viewer->getDefaultSpacePHID();
-    }
-
-    return AphrontFormSelectControl::renderSelectTag(
-      $value,
-      $this->getOptions(),
-      array(
-        'name' => $this->getName(),
-      ));
-  }
-
-}

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -360,10 +360,13 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_SPACE:
         $space_phid = $xaction->getNewValue();
         if (!strlen($space_phid)) {
-          // If an install has no Spaces, we might end up with the empty string
-          // here instead of a strict `null`. Just make this work like callers
-          // might reasonably expect.
-          return null;
+          // If an install has no Spaces or the Spaces controls are not visible
+          // to the viewer, we might end up with the empty string here instead
+          // of a strict `null`, because some controller just used `getStr()`
+          // to read the space PHID from the request.
+          // Just make this work like callers might reasonably expect so we
+          // don't need to handle this specially in every EditController.
+          return $this->getActor()->getDefaultSpacePHID();
         } else {
           return $space_phid;
         }
@@ -2002,14 +2005,15 @@ abstract class PhabricatorApplicationTransactionEditor
     $transaction_type) {
     $errors = array();
 
-    $all_spaces = PhabricatorSpacesNamespaceQuery::getAllSpaces();
-    $viewer_spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces(
-      $this->getActor());
+    $actor = $this->getActor();
+
+    $has_spaces = PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($actor);
+    $actor_spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($actor);
     foreach ($xactions as $xaction) {
       $space_phid = $xaction->getNewValue();
 
       if ($space_phid === null) {
-        if (!$all_spaces) {
+        if (!$has_spaces) {
           // The install doesn't have any spaces, so this is fine.
           continue;
         }
@@ -2026,7 +2030,7 @@ abstract class PhabricatorApplicationTransactionEditor
 
       // If the PHID isn't `null`, it needs to be a valid space that the
       // viewer can see.
-      if (empty($viewer_spaces[$space_phid])) {
+      if (empty($actor_spaces[$space_phid])) {
         $errors[] = new PhabricatorApplicationTransactionValidationError(
           $transaction_type,
           pht('Invalid'),
@@ -2045,7 +2049,18 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorLiskDAO $object,
     array $xactions) {
 
-    return clone $object;
+    $copy = clone $object;
+
+    foreach ($xactions as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case PhabricatorTransactions::TYPE_SPACE:
+          $space_phid = $this->getTransactionNewValue($object, $xaction);
+          $copy->setSpacePHID($space_phid);
+          break;
+      }
+    }
+
+    return $copy;
   }
 
   protected function validateAllTransactions(

src/view/form/control/AphrontFormPolicyControl.php

@@ -5,6 +5,7 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
   private $object;
   private $capability;
   private $policies;
+  private $spacePHID;
 
   public function setPolicyObject(PhabricatorPolicyInterface $object) {
     $this->object = $object;
@@ -17,6 +18,15 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
     return $this;
   }
 
+  public function setSpacePHID($space_phid) {
+    $this->spacePHID = $space_phid;
+    return $this;
+  }
+
+  public function getSpacePHID() {
+    return $this->spacePHID;
+  }
+
   public function setCapability($capability) {
     $this->capability = $capability;
 
@@ -187,11 +197,14 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
     $selected_icon = idx($selected, 'icon');
     $selected_name = idx($selected, 'name');
 
+    $spaces_control = $this->buildSpacesControl();
+
     return phutil_tag(
       'div',
       array(
       ),
       array(
+        $spaces_control,
         javelin_tag(
           'a',
           array(
@@ -231,4 +244,48 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
     return 'custom:placeholder';
   }
 
+  private function buildSpacesControl() {
+    if ($this->capability != PhabricatorPolicyCapability::CAN_VIEW) {
+      return null;
+    }
+
+    if (!($this->object instanceof PhabricatorSpacesInterface)) {
+      return null;
+    }
+
+    $viewer = $this->getUser();
+    if (!PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) {
+      return null;
+    }
+
+    $space_phid = $this->getSpacePHID();
+    if ($space_phid === null) {
+      $space_phid = $viewer->getDefaultSpacePHID();
+    }
+
+    $select = AphrontFormSelectControl::renderSelectTag(
+      $space_phid,
+      $this->getSpaceOptions(),
+      array(
+        'name' => 'spacePHID',
+      ));
+
+    return $select;
+  }
+
+  protected function getSpaceOptions() {
+    $viewer = $this->getUser();
+    $viewer_spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer);
+
+    $map = array();
+    foreach ($viewer_spaces as $space) {
+      $map[$space->getPHID()] = pht(
+        'Space %s: %s',
+        $space->getMonogram(),
+        $space->getNamespaceName());
+    }
+    asort($map);
+
+    return $map;
+  }
 }