Differential - allow setting viewPolicy from web ui during diff creation process

Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.

Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6237, T6152

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

resources/sql/autopatches/20141119.differential.diff.policy.sql

@@ -0,0 +1,5 @@
+ALTER TABLE {$NAMESPACE}_differential.differential_diff
+  ADD viewPolicy VARBINARY(64) NOT NULL;
+
+UPDATE {$NAMESPACE}_differential.differential_diff
+  SET viewPolicy = 'users' WHERE viewPolicy = '';

src/applications/differential/__tests__/DifferentialParseRenderTestCase.php

@@ -35,7 +35,9 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase {
     $parser = new ArcanistDiffParser();
     $changes = $parser->parseDiff($data);
 
-    $diff = DifferentialDiff::newFromRawChanges($changes);
+    $diff = DifferentialDiff::newFromRawChanges(
+      PhabricatorUser::getOmnipotentUser(),
+      $changes);
     if (count($diff->getChangesets()) !== 1) {
       throw new Exception("Expected one changeset: {$file}");
     }

src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php

@@ -69,7 +69,7 @@ final class DifferentialCreateDiffConduitAPIMethod
       $changes[] = ArcanistDiffChange::newFromDictionary($dict);
     }
 
-    $diff = DifferentialDiff::newFromRawChanges($changes);
+    $diff = DifferentialDiff::newFromRawChanges($viewer, $changes);
 
     // TODO: Remove repository UUID eventually; for now continue writing
     // the UUID. Note that we'll overwrite it below if we identify a

src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php

@@ -15,6 +15,7 @@ final class DifferentialCreateRawDiffConduitAPIMethod
     return array(
       'diff' => 'required string',
       'repositoryPHID' => 'optional string',
+      'viewPolicy' => 'optional string',
     );
   }
 
@@ -45,7 +46,7 @@ final class DifferentialCreateRawDiffConduitAPIMethod
 
     $parser = new ArcanistDiffParser();
     $changes = $parser->parseDiff($raw_diff);
-    $diff = DifferentialDiff::newFromRawChanges($changes);
+    $diff = DifferentialDiff::newFromRawChanges($viewer, $changes);
 
     $diff_data_dict = array(
       'creationMethod' => 'web',
@@ -58,6 +59,12 @@ final class DifferentialCreateRawDiffConduitAPIMethod
       ->setTransactionType(DifferentialDiffTransaction::TYPE_DIFF_CREATE)
       ->setNewValue($diff_data_dict),);
 
+    if ($request->getValue('viewPolicy')) {
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
+        ->setNewValue($request->getValue('viewPolicy'));
+    }
+
     id(new DifferentialDiffEditor())
       ->setActor($viewer)
       ->setContentSourceFromConduitRequest($request)

src/applications/differential/controller/DifferentialDiffCreateController.php

@@ -5,8 +5,11 @@ final class DifferentialDiffCreateController extends DifferentialController {
   public function processRequest() {
 
     $request = $this->getRequest();
+    $viewer = $request->getUser();
 
     $diff = null;
+    // This object is just for policy stuff
+    $diff_object = DifferentialDiff::initializeNewDiff($viewer);
     $repository_phid = null;
     $repository_value = array();
     $errors = array();
@@ -41,8 +44,9 @@ final class DifferentialDiffCreateController extends DifferentialController {
             'differential.createrawdiff',
             array(
               'diff' => $diff,
-              'repositoryPHID' => $repository_phid,));
+              'repositoryPHID' => $repository_phid,
-          $call->setUser($request->getUser());
+              'viewPolicy' => $request->getStr('viewPolicy'),));
+          $call->setUser($viewer);
           $result = $call->execute();
           $path = id(new PhutilURI($result['uri']))->getPath();
           return id(new AphrontRedirectResponse())->setURI($path);
@@ -68,10 +72,15 @@ final class DifferentialDiffCreateController extends DifferentialController {
       $repository_value = $this->loadViewerHandles(array($repository_phid));
     }
 
+    $policies = id(new PhabricatorPolicyQuery())
+      ->setViewer($viewer)
+      ->setObject($diff_object)
+      ->execute();
+
     $form
       ->setAction('/differential/diff/create/')
       ->setEncType('multipart/form-data')
-      ->setUser($request->getUser())
+      ->setUser($viewer)
       ->appendInstructions(
         pht(
           'The best way to create a Differential diff is by using %s, but you '.
@@ -100,6 +109,13 @@ final class DifferentialDiffCreateController extends DifferentialController {
         ->setDatasource(new DiffusionRepositoryDatasource())
         ->setValue($repository_value)
         ->setLimit(1))
+      ->appendChild(
+        id(new AphrontFormPolicyControl())
+        ->setUser($viewer)
+        ->setName('viewPolicy')
+        ->setPolicyObject($diff_object)
+        ->setPolicies($policies)
+        ->setCapability(PhabricatorPolicyCapability::CAN_VIEW))
       ->appendChild(
         id(new AphrontFormSubmitControl())
           ->addCancelButton($cancel_uri)

src/applications/differential/controller/DifferentialRevisionEditController.php

@@ -79,6 +79,13 @@ final class DifferentialRevisionEditController
       if ($repository_field) {
         $repository_field->setValue($request->getStr($repo_key));
       }
+      $view_policy_key = id(new DifferentialViewPolicyField())->getFieldKey();
+      $view_policy_field = idx(
+        $field_list->getFields(),
+        $view_policy_key);
+      if ($view_policy_field) {
+        $view_policy_field->setValue($diff->getViewPolicy());
+      }
     }
 
     $validation_exception = null;

src/applications/differential/editor/DifferentialDiffEditor.php

@@ -22,6 +22,7 @@ final class DifferentialDiffEditor
   public function getTransactionTypes() {
     $types = parent::getTransactionTypes();
 
+    $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
     $types[] = DifferentialDiffTransaction::TYPE_DIFF_CREATE;
 
     return $types;
@@ -61,6 +62,9 @@ final class DifferentialDiffEditor
         $dict = $this->diffDataDict;
         $this->updateDiffFromDict($object, $dict);
         return;
+      case PhabricatorTransactions::TYPE_VIEW_POLICY:
+        $object->setViewPolicy($xaction->getNewValue());
+        return;
     }
 
     return parent::applyCustomInternalTransaction($object, $xaction);
@@ -72,6 +76,7 @@ final class DifferentialDiffEditor
 
     switch ($xaction->getTransactionType()) {
       case DifferentialDiffTransaction::TYPE_DIFF_CREATE:
+      case PhabricatorTransactions::TYPE_VIEW_POLICY:
         return;
     }
 

src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php

@@ -40,7 +40,9 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
       throw new Exception("Expected 1 changeset for '{$name}'!");
     }
 
-    $diff = DifferentialDiff::newFromRawChanges($changes);
+    $diff = DifferentialDiff::newFromRawChanges(
+      PhabricatorUser::getOmnipotentUser(),
+      $changes);
     return head($diff->getChangesets())->getHunks();
   }
 

src/applications/differential/query/DifferentialDiffQuery.php

@@ -62,7 +62,6 @@ final class DifferentialDiffQuery
 
     foreach ($diffs as $key => $diff) {
       if (!$diff->getRevisionID()) {
-        $diff->attachRevision(null);
         continue;
       }
 

src/applications/differential/storage/DifferentialDiff.php

@@ -33,6 +33,8 @@ final class DifferentialDiff
 
   protected $description;
 
+  protected $viewPolicy;
+
   private $unsavedChangesets = array();
   private $changesets = self::ATTACHABLE;
   private $arcanistProject = self::ATTACHABLE;
@@ -136,10 +138,27 @@ final class DifferentialDiff
     return $ret;
   }
 
-  public static function newFromRawChanges(array $changes) {
+  public static function initializeNewDiff(PhabricatorUser $actor) {
-    assert_instances_of($changes, 'ArcanistDiffChange');
+    $app = id(new PhabricatorApplicationQuery())
-    $diff = new DifferentialDiff();
+      ->setViewer($actor)
+      ->withClasses(array('PhabricatorDifferentialApplication'))
+      ->executeOne();
+    $view_policy = $app->getPolicy(
+      DifferentialDefaultViewCapability::CAPABILITY);
 
+    $diff = id(new DifferentialDiff())
+      ->setViewPolicy($view_policy);
+
+    return $diff;
+  }
+
+  public static function newFromRawChanges(
+    PhabricatorUser $actor,
+    array $changes) {
+
+    assert_instances_of($changes, 'ArcanistDiffChange');
+
+    $diff = self::initializeNewDiff($actor);
     // There may not be any changes; initialize the changesets list so that
     // we don't throw later when accessing it.
     $diff->attachChangesets(array());
@@ -289,6 +308,10 @@ final class DifferentialDiff
     return $changes;
   }
 
+  public function hasRevision() {
+    return $this->revision !== self::ATTACHABLE;
+  }
+
   public function getRevision() {
     return $this->assertAttached($this->revision);
   }
@@ -318,27 +341,27 @@ final class DifferentialDiff
   }
 
   public function getPolicy($capability) {
-    if ($this->getRevision()) {
+    if ($this->hasRevision()) {
       return $this->getRevision()->getPolicy($capability);
     }
 
-    return PhabricatorPolicies::POLICY_USER;
+    return $this->viewPolicy;
   }
 
   public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    if ($this->getRevision()) {
+    if ($this->hasRevision()) {
       return $this->getRevision()->hasAutomaticCapability($capability, $viewer);
     }
 
-    return false;
+    return ($this->getAuthorPHID() == $viewer->getPhid());
   }
 
   public function describeAutomaticCapability($capability) {
-    if ($this->getRevision()) {
+    if ($this->hasRevision()) {
       return pht(
         'This diff is attached to a revision, and inherits its policies.');
     }
-    return null;
+    return pht('The author of a diff can see it.');
   }
 
 

src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php

@@ -7,6 +7,7 @@ final class DifferentialDiffTestCase extends ArcanistPhutilTestCase {
     $parser = new ArcanistDiffParser();
 
     $diff = DifferentialDiff::newFromRawChanges(
+      PhabricatorUser::getOmnipotentUser(),
       $parser->parseDiff(Filesystem::readFile($root.'lint_engine.diff')));
     $copies = idx(head($diff->getChangesets())->getMetadata(), 'copy:lines');
 
@@ -46,7 +47,9 @@ index 123457..0000000
 {$oblock}
 EODIFF;
 
-    $diff = DifferentialDiff::newFromRawChanges($parser->parseDiff($raw_diff));
+    $diff = DifferentialDiff::newFromRawChanges(
+      PhabricatorUser::getOmnipotentUser(),
+      $parser->parseDiff($raw_diff));
 
     $this->assertTrue(true);
   }

src/applications/diffusion/controller/DiffusionChangeController.php

@@ -22,7 +22,9 @@ final class DiffusionChangeController extends DiffusionController {
     $drequest->updateSymbolicCommit($data['effectiveCommit']);
 
     $raw_changes = ArcanistDiffChange::newFromConduit($data['changes']);
-    $diff = DifferentialDiff::newFromRawChanges($raw_changes);
+    $diff = DifferentialDiff::newFromRawChanges(
+      $viewer,
+      $raw_changes);
     $changesets = $diff->getChangesets();
     $changeset = reset($changesets);
 

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -276,6 +276,7 @@ final class DiffusionCommitController extends DiffusionController {
       $content[] = $change_panel;
 
       $changesets = DiffusionPathChange::convertToDifferentialChangesets(
+        $user,
         $changes);
 
       $vcs = $repository->getVersionControlSystem();

src/applications/diffusion/controller/DiffusionDiffController.php

@@ -54,7 +54,9 @@ final class DiffusionDiffController extends DiffusionController {
       ));
     $drequest->updateSymbolicCommit($data['effectiveCommit']);
     $raw_changes = ArcanistDiffChange::newFromConduit($data['changes']);
-    $diff = DifferentialDiff::newFromRawChanges($raw_changes);
+    $diff = DifferentialDiff::newFromRawChanges(
+      $user,
+      $raw_changes);
     $changesets = $diff->getChangesets();
     $changeset = reset($changesets);
 

src/applications/diffusion/data/DiffusionPathChange.php

@@ -142,10 +142,12 @@ final class DiffusionPathChange {
     return array_select_keys($result, $direct);
   }
 
-  final public static function convertToDifferentialChangesets(array $changes) {
+  final public static function convertToDifferentialChangesets(
+    PhabricatorUser $user,
+    array $changes) {
     assert_instances_of($changes, 'DiffusionPathChange');
     $arcanist_changes = self::convertToArcanistChanges($changes);
-    $diff = DifferentialDiff::newFromRawChanges($arcanist_changes);
+    $diff = DifferentialDiff::newFromRawChanges($user, $arcanist_changes);
     return $diff->getChangesets();
   }
 

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

@@ -1115,7 +1115,9 @@ final class DiffusionCommitHookEngine extends Phobject {
 
     $parser = new ArcanistDiffParser();
     $changes = $parser->parseDiff($raw_diff);
-    $diff = DifferentialDiff::newFromRawChanges($changes);
+    $diff = DifferentialDiff::newFromRawChanges(
+      $this->getViewer(),
+      $changes);
     return $diff->getChangesets();
   }
 

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -346,7 +346,9 @@ final class HeraldCommitAdapter extends HeraldAdapter {
     $parser = new ArcanistDiffParser();
     $changes = $parser->parseDiff($raw);
 
-    $diff = DifferentialDiff::newFromRawChanges($changes);
+    $diff = DifferentialDiff::newFromRawChanges(
+      PhabricatorUser::getOmnipotentUser(),
+      $changes);
     return $diff;
   }
 

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -265,7 +265,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       $changes = array();
     }
 
-    $diff = DifferentialDiff::newFromRawChanges($changes)
+    $diff = DifferentialDiff::newFromRawChanges($viewer, $changes)
       ->setRepositoryPHID($this->repository->getPHID())
       ->setAuthorPHID($actor_phid)
       ->setCreationMethod('commit')

src/infrastructure/diff/PhabricatorDifferenceEngine.php

@@ -163,7 +163,9 @@ final class PhabricatorDifferenceEngine {
     $diff = $this->generateRawDiffFromFileContent($old, $new);
 
     $changes = id(new ArcanistDiffParser())->parseDiff($diff);
-    $diff = DifferentialDiff::newFromRawChanges($changes);
+    $diff = DifferentialDiff::newFromRawChanges(
+      PhabricatorUser::getOmnipotentUser(),
+      $changes);
     return head($diff->getChangesets());
   }