From 8544d0d00f0834cbcb687cecfbfd539e67a0dfd3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Feb 2014 14:30:00 -0800 Subject: [PATCH] Implement "Edit Members" and "Join/Leave" with real ApplicationTransactions Summary: Ref T4379. Projects has been partially converted to ApplicationTransactions, but the rough state of the world is that all the //storage// is modern, but most of the stuff on top isn't yet. Particularly, there's a `PhabricatorProjectEditor` which is //not// a subclass of `PhabricatorApplicationTransactionEditor`, but which fakes its way through writing reasonable data into modern storage. This introduces a real transaction editor, `PhabricatorProjectTransactionEditor`, with the eventual goal of moving all of the old functionality into it and deleting the old class. This diff only moves the membership transaction into new code (it doesn't even move all of it -- when we create a project, we add the author as a member, and that can't move quite yet since there are other transactions at the same time). Test Plan: - Created a new project. - Edited members. - Joined / left project. - This already has a pile of unit test coverage. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4379 Differential Revision: https://secure.phabricator.com/D8167 --- src/__phutil_library_map__.php | 2 + ...habricatorProjectMembersEditController.php | 47 +++--- .../PhabricatorProjectUpdateController.php | 25 ++- .../editor/PhabricatorProjectEditor.php | 144 +----------------- .../PhabricatorProjectTransactionEditor.php | 113 ++++++++++++++ .../PhabricatorProjectEditorTestCase.php | 54 +++++-- 6 files changed, 206 insertions(+), 179 deletions(-) create mode 100644 src/applications/project/editor/PhabricatorProjectTransactionEditor.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d6a31cf88c..fdd94bceeb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1855,6 +1855,7 @@ phutil_register_library_map(array( 'PhabricatorProjectStatus' => 'applications/project/constants/PhabricatorProjectStatus.php', 'PhabricatorProjectTestDataGenerator' => 'applications/project/lipsum/PhabricatorProjectTestDataGenerator.php', 'PhabricatorProjectTransaction' => 'applications/project/storage/PhabricatorProjectTransaction.php', + 'PhabricatorProjectTransactionEditor' => 'applications/project/editor/PhabricatorProjectTransactionEditor.php', 'PhabricatorProjectTransactionQuery' => 'applications/project/query/PhabricatorProjectTransactionQuery.php', 'PhabricatorProjectUpdateController' => 'applications/project/controller/PhabricatorProjectUpdateController.php', 'PhabricatorQuery' => 'infrastructure/query/PhabricatorQuery.php', @@ -4593,6 +4594,7 @@ phutil_register_library_map(array( 'PhabricatorProjectSearchIndexer' => 'PhabricatorSearchDocumentIndexer', 'PhabricatorProjectTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorProjectTransaction' => 'PhabricatorApplicationTransaction', + 'PhabricatorProjectTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorProjectTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorProjectUpdateController' => 'PhabricatorProjectController', 'PhabricatorRecaptchaConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/project/controller/PhabricatorProjectMembersEditController.php b/src/applications/project/controller/PhabricatorProjectMembersEditController.php index 129f53ded3..95453e5edb 100644 --- a/src/applications/project/controller/PhabricatorProjectMembersEditController.php +++ b/src/applications/project/controller/PhabricatorProjectMembersEditController.php @@ -29,41 +29,34 @@ final class PhabricatorProjectMembersEditController $member_phids = $project->getMemberPHIDs(); - $errors = array(); if ($request->isFormPost()) { - $changed_something = false; - $member_map = array_fill_keys($member_phids, true); + $member_spec = array(); $remove = $request->getStr('remove'); if ($remove) { - if (isset($member_map[$remove])) { - unset($member_map[$remove]); - $changed_something = true; - } - } else { - $new_members = $request->getArr('phids'); - foreach ($new_members as $member) { - if (empty($member_map[$member])) { - $member_map[$member] = true; - $changed_something = true; - } - } + $member_spec['-'] = array_fuse(array($remove)); } + $add_members = $request->getArr('phids'); + if ($add_members) { + $member_spec['+'] = array_fuse($add_members); + } + + $type_member = PhabricatorEdgeConfig::TYPE_PROJ_MEMBER; + $xactions = array(); - if ($changed_something) { - $xaction = new PhabricatorProjectTransaction(); - $xaction->setTransactionType( - PhabricatorProjectTransaction::TYPE_MEMBERS); - $xaction->setNewValue(array_keys($member_map)); - $xactions[] = $xaction; - } - if ($xactions) { - $editor = new PhabricatorProjectEditor($project); - $editor->setActor($user); - $editor->applyTransactions($xactions); - } + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $type_member) + ->setNewValue($member_spec); + + $editor = id(new PhabricatorProjectTransactionEditor($project)) + ->setActor($user) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->applyTransactions($project, $xactions); return id(new AphrontRedirectResponse()) ->setURI($request->getRequestURI()); diff --git a/src/applications/project/controller/PhabricatorProjectUpdateController.php b/src/applications/project/controller/PhabricatorProjectUpdateController.php index 2c27daf760..209c8de18b 100644 --- a/src/applications/project/controller/PhabricatorProjectUpdateController.php +++ b/src/applications/project/controller/PhabricatorProjectUpdateController.php @@ -45,14 +45,35 @@ final class PhabricatorProjectUpdateController $project_uri = '/project/view/'.$project->getID().'/'; if ($process_action) { + + $edge_action = null; switch ($this->action) { case 'join': - PhabricatorProjectEditor::applyJoinProject($project, $user); + $edge_action = '+'; break; case 'leave': - PhabricatorProjectEditor::applyLeaveProject($project, $user); + $edge_action = '-'; break; } + + $type_member = PhabricatorEdgeConfig::TYPE_PROJ_MEMBER; + $member_spec = array( + $edge_action => array($user->getPHID() => $user->getPHID()), + ); + + $xactions = array(); + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $type_member) + ->setNewValue($member_spec); + + $editor = id(new PhabricatorProjectTransactionEditor($project)) + ->setActor($user) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->applyTransactions($project, $xactions); + return id(new AphrontRedirectResponse())->setURI($project_uri); } diff --git a/src/applications/project/editor/PhabricatorProjectEditor.php b/src/applications/project/editor/PhabricatorProjectEditor.php index 05f01db9e4..981beeaa2a 100644 --- a/src/applications/project/editor/PhabricatorProjectEditor.php +++ b/src/applications/project/editor/PhabricatorProjectEditor.php @@ -18,51 +18,6 @@ final class PhabricatorProjectEditor extends PhabricatorEditor { return $this->shouldArchive; } - public static function applyJoinProject( - PhabricatorProject $project, - PhabricatorUser $user) { - - $members = $project->getMemberPHIDs(); - $members[] = $user->getPHID(); - - self::applyOneTransaction( - $project, - $user, - PhabricatorProjectTransaction::TYPE_MEMBERS, - $members); - } - - public static function applyLeaveProject( - PhabricatorProject $project, - PhabricatorUser $user) { - - $members = array_fill_keys($project->getMemberPHIDs(), true); - unset($members[$user->getPHID()]); - $members = array_keys($members); - - self::applyOneTransaction( - $project, - $user, - PhabricatorProjectTransaction::TYPE_MEMBERS, - $members); - } - - private static function applyOneTransaction( - PhabricatorProject $project, - PhabricatorUser $user, - $type, - $new_value) { - - $xaction = new PhabricatorProjectTransaction(); - $xaction->setTransactionType($type); - $xaction->setNewValue($new_value); - - $editor = new PhabricatorProjectEditor($project); - $editor->setActor($user); - $editor->applyTransactions(array($xaction)); - } - - public function __construct(PhabricatorProject $project) { $this->project = $project; } @@ -94,30 +49,10 @@ final class PhabricatorProjectEditor extends PhabricatorEditor { $project, PhabricatorPolicyCapability::CAN_VIEW); - $need_edit = false; - $need_join = false; - foreach ($transactions as $key => $xaction) { - if ($this->getTransactionRequiresEditCapability($xaction)) { - $need_edit = true; - } - if ($this->getTransactionRequiresJoinCapability($xaction)) { - $need_join = true; - } - } - - if ($need_edit) { - PhabricatorPolicyFilter::requireCapability( - $actor, - $project, - PhabricatorPolicyCapability::CAN_EDIT); - } - - if ($need_join) { - PhabricatorPolicyFilter::requireCapability( - $actor, - $project, - PhabricatorPolicyCapability::CAN_JOIN); - } + PhabricatorPolicyFilter::requireCapability( + $actor, + $project, + PhabricatorPolicyCapability::CAN_EDIT); } if (!$transactions) { @@ -317,75 +252,4 @@ final class PhabricatorProjectEditor extends PhabricatorEditor { return ($xaction->getOldValue() !== $xaction->getNewValue()); } - - /** - * All transactions except joining or leaving a project require edit - * capability. - */ - private function getTransactionRequiresEditCapability( - PhabricatorProjectTransaction $xaction) { - return ($this->isJoinOrLeaveTransaction($xaction) === null); - } - - - /** - * Joining a project requires the join capability. Anyone leave a project. - */ - private function getTransactionRequiresJoinCapability( - PhabricatorProjectTransaction $xaction) { - $type = $this->isJoinOrLeaveTransaction($xaction); - return ($type == 'join'); - } - - - /** - * Returns 'join' if this transaction causes the acting user ONLY to join the - * project. - * - * Returns 'leave' if this transaction causes the acting user ONLY to leave - * the project. - * - * Returns null in all other cases. - */ - private function isJoinOrLeaveTransaction( - PhabricatorProjectTransaction $xaction) { - - $type = $xaction->getTransactionType(); - if ($type != PhabricatorProjectTransaction::TYPE_MEMBERS) { - return null; - } - - switch ($type) { - case PhabricatorProjectTransaction::TYPE_MEMBERS: - $old = $xaction->getOldValue(); - $new = $xaction->getNewValue(); - - $add = array_diff($new, $old); - $rem = array_diff($old, $new); - - if (count($add) > 1) { - return null; - } else if (count($add) == 1) { - if (reset($add) != $this->getActor()->getPHID()) { - return null; - } else { - return 'join'; - } - } - - if (count($rem) > 1) { - return null; - } else if (count($rem) == 1) { - if (reset($rem) != $this->getActor()->getPHID()) { - return null; - } else { - return 'leave'; - } - } - break; - } - - return true; - } - } diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php new file mode 100644 index 0000000000..bfc621119a --- /dev/null +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -0,0 +1,113 @@ +getTransactionType()) { + } + + return parent::getCustomTransactionOldValue($object, $xaction); + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + } + + return parent::getCustomTransactionNewValue($object, $xaction); + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_EDGE: + return; + } + + return parent::applyCustomInternalTransaction($object, $xaction); + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_EDGE: + return; + } + + return parent::applyCustomExternalTransaction($object, $xaction); + } + + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + switch ($type) { + } + + return $errors; + } + + protected function requireCapabilities( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_EDGE: + switch ($xaction->getMetadataValue('edge:type')) { + case PhabricatorEdgeConfig::TYPE_PROJ_MEMBER: + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + $add = array_keys(array_diff_key($new, $old)); + $rem = array_keys(array_diff_key($old, $new)); + + $actor_phid = $this->requireActor()->getPHID(); + + $is_join = (($add === array($actor_phid)) && !$rem); + $is_leave = (($rem === array($actor_phid)) && !$add); + + if ($is_join) { + // You need CAN_JOIN to join a project. + PhabricatorPolicyFilter::requireCapability( + $this->requireActor(), + $object, + PhabricatorPolicyCapability::CAN_JOIN); + } else if ($is_leave) { + // You don't need any capabilities to leave a project. + } else { + // You need CAN_EDIT to change members other than yourself. + PhabricatorPolicyFilter::requireCapability( + $this->requireActor(), + $object, + PhabricatorPolicyCapability::CAN_EDIT); + } + return; + } + break; + } + + return parent::requireCapabilities(); + } + +} diff --git a/src/applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php b/src/applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php index 3631cf31d3..6748477969 100644 --- a/src/applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php +++ b/src/applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php @@ -21,7 +21,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { $proj = $this->refreshProject($proj, $user, true); - PhabricatorProjectEditor::applyJoinProject($proj, $user); + $this->joinProject($proj, $user); $proj->setViewPolicy(PhabricatorPolicies::POLICY_USER); $proj->save(); @@ -123,7 +123,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { 'Arbitrary user not member of project.'); // Join the project. - PhabricatorProjectEditor::applyJoinProject($proj, $user); + $this->joinProject($proj, $user); $proj = $this->refreshProject($proj, $user, true); $this->assertEqual(true, (bool)$proj); @@ -135,7 +135,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { // Join the project again. - PhabricatorProjectEditor::applyJoinProject($proj, $user); + $this->joinProject($proj, $user); $proj = $this->refreshProject($proj, $user, true); $this->assertEqual(true, (bool)$proj); @@ -147,7 +147,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { // Leave the project. - PhabricatorProjectEditor::applyLeaveProject($proj, $user); + $this->leaveProject($proj, $user); $proj = $this->refreshProject($proj, $user, true); $this->assertEqual(true, (bool)$proj); @@ -159,7 +159,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { // Leave the project again. - PhabricatorProjectEditor::applyLeaveProject($proj, $user); + $this->leaveProject($proj, $user); $proj = $this->refreshProject($proj, $user, true); $this->assertEqual(true, (bool)$proj); @@ -178,7 +178,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { $proj = $this->refreshProject($proj, $user, true); $caught = null; try { - PhabricatorProjectEditor::applyJoinProject($proj, $user); + $this->joinProject($proj, $user); } catch (Exception $ex) { $caught = $ex; } @@ -191,13 +191,13 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { $proj->save(); $proj = $this->refreshProject($proj, $user, true); - PhabricatorProjectEditor::applyJoinProject($proj, $user); + $this->joinProject($proj, $user); $proj = $this->refreshProject($proj, $user, true); $this->assertEqual( true, $proj->isUserMember($user->getPHID()), 'Join allowed with edit permission.'); - PhabricatorProjectEditor::applyLeaveProject($proj, $user); + $this->leaveProject($proj, $user); // If a user can join a project, they can join, even if they can't edit. @@ -206,7 +206,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { $proj->save(); $proj = $this->refreshProject($proj, $user, true); - PhabricatorProjectEditor::applyJoinProject($proj, $user); + $this->joinProject($proj, $user); $proj = $this->refreshProject($proj, $user, true); $this->assertEqual( true, @@ -220,7 +220,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { $proj->save(); $proj = $this->refreshProject($proj, $user, true); - PhabricatorProjectEditor::applyLeaveProject($proj, $user); + $this->leaveProject($proj, $user); $proj = $this->refreshProject($proj, $user, true); $this->assertEqual( false, @@ -273,4 +273,38 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase { return $user; } + private function joinProject( + PhabricatorProject $project, + PhabricatorUser $user) { + $this->joinOrLeaveProject($project, $user, '+'); + } + + private function leaveProject( + PhabricatorProject $project, + PhabricatorUser $user) { + $this->joinOrLeaveProject($project, $user, '-'); + } + + private function joinOrLeaveProject( + PhabricatorProject $project, + PhabricatorUser $user, + $operation) { + + $spec = array( + $operation => array($user->getPHID() => $user->getPHID()), + ); + + $xactions = array(); + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', PhabricatorEdgeConfig::TYPE_PROJ_MEMBER) + ->setNewValue($spec); + + $editor = id(new PhabricatorProjectTransactionEditor()) + ->setActor($user) + ->setContentSource(PhabricatorContentSource::newConsoleSource()) + ->setContinueOnNoEffect(true) + ->applyTransactions($project, $xactions); + } + }