From 435085862892ac92dc30fbfdb1ccf54dbfd9974c Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 19 Nov 2014 12:16:07 -0800 Subject: [PATCH] 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 --- .../20141119.differential.diff.policy.sql | 5 +++ .../DifferentialParseRenderTestCase.php | 4 +- ...DifferentialCreateDiffConduitAPIMethod.php | 2 +- ...ferentialCreateRawDiffConduitAPIMethod.php | 9 +++- .../DifferentialDiffCreateController.php | 22 ++++++++-- .../DifferentialRevisionEditController.php | 7 ++++ .../editor/DifferentialDiffEditor.php | 5 +++ .../DifferentialHunkParserTestCase.php | 4 +- .../query/DifferentialDiffQuery.php | 1 - .../differential/storage/DifferentialDiff.php | 41 +++++++++++++++---- .../__tests__/DifferentialDiffTestCase.php | 5 ++- .../controller/DiffusionChangeController.php | 4 +- .../controller/DiffusionCommitController.php | 1 + .../controller/DiffusionDiffController.php | 4 +- .../diffusion/data/DiffusionPathChange.php | 6 ++- .../engine/DiffusionCommitHookEngine.php | 4 +- .../herald/adapter/HeraldCommitAdapter.php | 4 +- ...torRepositoryCommitMessageParserWorker.php | 2 +- .../diff/PhabricatorDifferenceEngine.php | 4 +- 19 files changed, 108 insertions(+), 26 deletions(-) create mode 100644 resources/sql/autopatches/20141119.differential.diff.policy.sql diff --git a/resources/sql/autopatches/20141119.differential.diff.policy.sql b/resources/sql/autopatches/20141119.differential.diff.policy.sql new file mode 100644 index 0000000000..b6c19c0dea --- /dev/null +++ b/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 = ''; diff --git a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php index 41165bf23e..9d9f47315d 100644 --- a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php +++ b/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}"); } diff --git a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php index 3ea3c1768f..26b1c04ddd 100644 --- a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php +++ b/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 diff --git a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php index 68531ea6bc..29638b5396 100644 --- a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php +++ b/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) diff --git a/src/applications/differential/controller/DifferentialDiffCreateController.php b/src/applications/differential/controller/DifferentialDiffCreateController.php index c092597fbb..9c3698e3f2 100644 --- a/src/applications/differential/controller/DifferentialDiffCreateController.php +++ b/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,)); - $call->setUser($request->getUser()); + 'repositoryPHID' => $repository_phid, + '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) diff --git a/src/applications/differential/controller/DifferentialRevisionEditController.php b/src/applications/differential/controller/DifferentialRevisionEditController.php index 14bcdf0476..ea26693454 100644 --- a/src/applications/differential/controller/DifferentialRevisionEditController.php +++ b/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; diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php index c2513dab7a..69125da1e1 100644 --- a/src/applications/differential/editor/DifferentialDiffEditor.php +++ b/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; } diff --git a/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php index dc4acb383b..111d6ba91f 100644 --- a/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php +++ b/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(); } diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php index f9666400f1..4fc3ffcdfe 100644 --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/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; } diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index d52df5c2ec..c16a9f85fd 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/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) { - assert_instances_of($changes, 'ArcanistDiffChange'); - $diff = new DifferentialDiff(); + public static function initializeNewDiff(PhabricatorUser $actor) { + $app = id(new PhabricatorApplicationQuery()) + ->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.'); } diff --git a/src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php b/src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php index e9e21784e7..3f851d767c 100644 --- a/src/applications/differential/storage/__tests__/DifferentialDiffTestCase.php +++ b/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); } diff --git a/src/applications/diffusion/controller/DiffusionChangeController.php b/src/applications/diffusion/controller/DiffusionChangeController.php index 37e1dfb02d..c510b403cd 100644 --- a/src/applications/diffusion/controller/DiffusionChangeController.php +++ b/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); diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 3e7308a56a..f4006f82c7 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/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(); diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php index 92b7b64cd4..81460ea67a 100644 --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/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); diff --git a/src/applications/diffusion/data/DiffusionPathChange.php b/src/applications/diffusion/data/DiffusionPathChange.php index 188669126e..e4f6fa182a 100644 --- a/src/applications/diffusion/data/DiffusionPathChange.php +++ b/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(); } diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 09a65f3c3d..9e7d5ccef9 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/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(); } diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index e52ad18fa6..35e93f5916 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/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; } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 7c56251dea..f97fefc4c6 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/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') diff --git a/src/infrastructure/diff/PhabricatorDifferenceEngine.php b/src/infrastructure/diff/PhabricatorDifferenceEngine.php index 8c2bf08c7b..b7b22d7da0 100644 --- a/src/infrastructure/diff/PhabricatorDifferenceEngine.php +++ b/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()); }