Merge branch 'master' into redesign-2015

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -771,19 +771,23 @@ final class DifferentialRevisionQuery
         $this->updatedEpochMax);
     }
 
+    // NOTE: Although the status constants are integers in PHP, the column is a
+    // string column in MySQL, and MySQL won't use keys on string columns if
+    // you put integers in the query.
+
     switch ($this->status) {
       case self::STATUS_ANY:
         break;
       case self::STATUS_OPEN:
         $where[] = qsprintf(
           $conn_r,
-          'r.status IN (%Ld)',
+          'r.status IN (%Ls)',
           DifferentialRevisionStatus::getOpenStatuses());
         break;
       case self::STATUS_NEEDS_REVIEW:
         $where[] = qsprintf(
           $conn_r,
-          'r.status IN (%Ld)',
+          'r.status IN (%Ls)',
           array(
             ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
           ));
@@ -791,7 +795,7 @@ final class DifferentialRevisionQuery
       case self::STATUS_NEEDS_REVISION:
         $where[] = qsprintf(
           $conn_r,
-          'r.status IN (%Ld)',
+          'r.status IN (%Ls)',
           array(
             ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
           ));
@@ -799,7 +803,7 @@ final class DifferentialRevisionQuery
       case self::STATUS_ACCEPTED:
         $where[] = qsprintf(
           $conn_r,
-          'r.status IN (%Ld)',
+          'r.status IN (%Ls)',
           array(
             ArcanistDifferentialRevisionStatus::ACCEPTED,
           ));
@@ -807,13 +811,13 @@ final class DifferentialRevisionQuery
       case self::STATUS_CLOSED:
         $where[] = qsprintf(
           $conn_r,
-          'r.status IN (%Ld)',
+          'r.status IN (%Ls)',
           DifferentialRevisionStatus::getClosedStatuses());
         break;
       case self::STATUS_ABANDONED:
         $where[] = qsprintf(
           $conn_r,
-          'r.status IN (%Ld)',
+          'r.status IN (%Ls)',
           array(
             ArcanistDifferentialRevisionStatus::ABANDONED,
           ));

src/applications/differential/storage/DifferentialRevision.php

@@ -102,6 +102,14 @@ final class DifferentialRevision extends DifferentialDAO
         'repositoryPHID' => array(
           'columns' => array('repositoryPHID'),
         ),
+        // If you (or a project you are a member of) is reviewing a significant
+        // fraction of the revisions on an install, the result set of open
+        // revisions may be smaller than the result set of revisions where you
+        // are a reviewer. In these cases, this key is better than keys on the
+        // edge table.
+        'key_status' => array(
+          'columns' => array('status', 'phid'),
+        ),
       ),
     ) + parent::getConfiguration();
   }

src/applications/policy/__tests__/PhabricatorPolicyTestCase.php

@@ -353,6 +353,37 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase {
     $this->assertEqual(array(), $result);
   }
 
+  public function testPolicyStrength() {
+    $public = PhabricatorPolicyQuery::getGlobalPolicy(
+      PhabricatorPolicies::POLICY_PUBLIC);
+    $user = PhabricatorPolicyQuery::getGlobalPolicy(
+      PhabricatorPolicies::POLICY_USER);
+    $admin = PhabricatorPolicyQuery::getGlobalPolicy(
+      PhabricatorPolicies::POLICY_ADMIN);
+    $noone = PhabricatorPolicyQuery::getGlobalPolicy(
+      PhabricatorPolicies::POLICY_NOONE);
+
+    $this->assertFalse($public->isStrongerThan($public));
+    $this->assertFalse($public->isStrongerThan($user));
+    $this->assertFalse($public->isStrongerThan($admin));
+    $this->assertFalse($public->isStrongerThan($noone));
+
+    $this->assertTrue($user->isStrongerThan($public));
+    $this->assertFalse($user->isStrongerThan($user));
+    $this->assertFalse($user->isStrongerThan($admin));
+    $this->assertFalse($user->isStrongerThan($noone));
+
+    $this->assertTrue($admin->isStrongerThan($public));
+    $this->assertTrue($admin->isStrongerThan($user));
+    $this->assertFalse($admin->isStrongerThan($admin));
+    $this->assertFalse($admin->isStrongerThan($noone));
+
+    $this->assertTrue($noone->isStrongerThan($public));
+    $this->assertTrue($noone->isStrongerThan($user));
+    $this->assertTrue($noone->isStrongerThan($admin));
+    $this->assertFalse($admin->isStrongerThan($noone));
+  }
+
 
   /**
    * Test an object for visibility across multiple user specifications.

src/applications/policy/controller/PhabricatorPolicyExplainController.php

@@ -69,44 +69,115 @@ final class PhabricatorPolicyExplainController
       $capability_name = $capobj->getCapabilityName();
     }
 
-    $space_info = null;
+    $dialog = id(new AphrontDialogView())
-    if ($object instanceof PhabricatorSpacesInterface) {
+      ->setUser($viewer)
-      if (PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) {
+      ->setClass('aphront-access-dialog');
-        $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID(
-          $object);
 
-        $handles = $viewer->loadHandles(array($space_phid));
+    $this->appendSpaceInformation($dialog, $object, $policy, $capability);
 
-        $space_info = array(
+    $intro = pht(
-          pht(
+      'Users with the "%s" capability for this object:',
-            'This object is in %s, and can only be seen by users with '.
+      $capability_name);
-            'access to that space.',
-            $handles[$space_phid]->renderLink()),
-          phutil_tag('br'),
-          phutil_tag('br'),
-        );
-      }
-    }
-
-    $content = array(
-      $space_info,
-      pht('Users with the "%s" capability:', $capability_name),
-      $auto_info,
-    );
 
     $object_name = pht(
       '%s %s',
       $handle->getTypeName(),
       $handle->getObjectName());
 
-    $dialog = id(new AphrontDialogView())
+    return $dialog
-      ->setUser($viewer)
-      ->setClass('aphront-access-dialog')
       ->setTitle(pht('Policy Details: %s', $object_name))
-      ->appendChild($content)
+      ->appendParagraph($intro)
+      ->appendChild($auto_info)
       ->addCancelButton($object_uri, pht('Done'));
+  }
 
-    return id(new AphrontDialogResponse())->setDialog($dialog);
+  private function appendSpaceInformation(
+    AphrontDialogView $dialog,
+    PhabricatorPolicyInterface $object,
+    PhabricatorPolicy $policy,
+    $capability) {
+    $viewer = $this->getViewer();
+
+    if (!($object instanceof PhabricatorSpacesInterface)) {
+      return;
+    }
+
+    if (!PhabricatorSpacesNamespaceQuery::getSpacesExist($viewer)) {
+      return;
+    }
+
+    // NOTE: We're intentionally letting users through here, even if they only
+    // have access to one space. The intent is to help users in "space jail"
+    // understand who objects they create are visible to:
+
+    $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID(
+      $object);
+
+    $handles = $viewer->loadHandles(array($space_phid));
+    $doc_href = PhabricatorEnv::getDoclink('Spaces User Guide');
+
+    $dialog->appendParagraph(
+      array(
+        pht(
+          'This object is in %s, and can only be seen or edited by users with '.
+          'access to view objects in the space.',
+          $handles[$space_phid]->renderLink()),
+        ' ',
+        phutil_tag(
+          'strong',
+          array(),
+          phutil_tag(
+            'a',
+            array(
+              'href' => $doc_href,
+              'target' => '_blank',
+            ),
+            pht('Learn More'))),
+      ));
+
+    $spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer);
+    $space = idx($spaces, $space_phid);
+    if (!$space) {
+      return;
+    }
+
+    $space_policies = PhabricatorPolicyQuery::loadPolicies($viewer, $space);
+    $space_policy = idx($space_policies, PhabricatorPolicyCapability::CAN_VIEW);
+    if (!$space_policy) {
+      return;
+    }
+
+    $space_explanation = PhabricatorPolicy::getPolicyExplanation(
+      $viewer,
+      $space_policy->getPHID());
+    $items = array();
+    $items[] = $space_explanation;
+
+    foreach ($items as $key => $item) {
+      $items[$key] = phutil_tag('li', array(), $item);
+    }
+
+    $dialog->appendParagraph(pht('Users who can see objects in this space:'));
+    $dialog->appendChild(phutil_tag('ul', array(), $items));
+
+    $view_capability = PhabricatorPolicyCapability::CAN_VIEW;
+    if ($capability == $view_capability) {
+      $stronger = $space_policy->isStrongerThan($policy);
+      if ($stronger) {
+        $dialog->appendParagraph(
+          pht(
+            'The space this object is in has a more restrictive view '.
+            'policy ("%s") than the object does ("%s"), so the space\'s '.
+            'view policy is shown as a hint instead of the object policy.',
+            $space_policy->getShortName(),
+            $policy->getShortName()));
+      }
+    }
+
+    $dialog->appendParagraph(
+      pht(
+        'After a user passes space policy checks, they must still pass '.
+        'object policy checks.'));
   }
 
 }

src/applications/policy/storage/PhabricatorPolicy.php

@@ -362,6 +362,60 @@ final class PhabricatorPolicy
   }
 
 
+  /**
+   * Return `true` if this policy is stronger (more restrictive) than some
+   * other policy.
+   *
+   * Because policies are complicated, determining which policies are
+   * "stronger" is not trivial. This method uses a very coarse working
+   * definition of policy strength which is cheap to compute, unambiguous,
+   * and intuitive in the common cases.
+   *
+   * This method returns `true` if the //class// of this policy is stronger
+   * than the other policy, even if the policies are (or might be) the same in
+   * practice. For example, "Members of Project X" is considered a stronger
+   * policy than "All Users", even though "Project X" might (in some rare
+   * cases) contain every user.
+   *
+   * Generally, the ordering here is:
+   *
+   *   - Public
+   *   - All Users
+   *   - (Everything Else)
+   *   - No One
+   *
+   * In the "everything else" bucket, we can't make any broad claims about
+   * which policy is stronger (and we especially can't make those claims
+   * cheaply).
+   *
+   * Even if we fully evaluated each policy, the two policies might be
+   * "Members of X" and "Members of Y", each of which permits access to some
+   * set of unique users. In this case, neither is strictly stronger than
+   * the other.
+   *
+   * @param PhabricatorPolicy Other policy.
+   * @return bool `true` if this policy is more restrictive than the other
+   *  policy.
+   */
+  public function isStrongerThan(PhabricatorPolicy $other) {
+    $this_policy = $this->getPHID();
+    $other_policy = $other->getPHID();
+
+    $strengths = array(
+      PhabricatorPolicies::POLICY_PUBLIC => -2,
+      PhabricatorPolicies::POLICY_USER => -1,
+      // (Default policies have strength 0.)
+      PhabricatorPolicies::POLICY_NOONE => 1,
+    );
+
+    $this_strength = idx($strengths, $this->getPHID(), 0);
+    $other_strength = idx($strengths, $other->getPHID(), 0);
+
+    return ($this_strength > $other_strength);
+  }
+
+
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 

src/view/phui/PHUIHeaderView.php

@@ -355,6 +355,32 @@ final class PHUIHeaderView extends AphrontTagView {
       return null;
     }
 
+    // If an object is in a Space with a strictly stronger (more restrictive)
+    // policy, we show the more restrictive policy. This better aligns the
+    // UI hint with the actual behavior.
+
+    // NOTE: We'll do this even if the viewer has access to only one space, and
+    // show them information about the existence of spaces if they click
+    // through.
+    if ($object instanceof PhabricatorSpacesInterface) {
+      $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID(
+        $object);
+
+      $spaces = PhabricatorSpacesNamespaceQuery::getViewerSpaces($viewer);
+      $space = idx($spaces, $space_phid);
+      if ($space) {
+        $space_policies = PhabricatorPolicyQuery::loadPolicies(
+          $viewer,
+          $space);
+        $space_policy = idx($space_policies, $view_capability);
+        if ($space_policy) {
+          if ($space_policy->isStrongerThan($policy)) {
+            $policy = $space_policy;
+          }
+        }
+      }
+    }
+
     $phid = $object->getPHID();
 
     $icon = id(new PHUIIconView())