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

@@ -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',

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());

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

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

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -0,0 +1,113 @@
+<?php
+
+final class PhabricatorProjectTransactionEditor
+  extends PhabricatorApplicationTransactionEditor {
+
+  public function getTransactionTypes() {
+    $types = parent::getTransactionTypes();
+
+    $types[] = PhabricatorTransactions::TYPE_EDGE;
+
+    return $types;
+  }
+
+  protected function getCustomTransactionOldValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->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();
+  }
+
+}

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