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); + } + }