Fix member edit transaction validation so it works for both implicit and explicit account creation

Summary:
Ref T12451. Ref T12484. This should deal with all the `+` / `-` / `=` cases correctly, I think.

Also makes sure that members are real users, not commits or tokens or whatever. And expands the creation test case to make some other basic sanity checks.

Test Plan:
  - Went through implicit first-time creation flow.
  - Went through explicit second-time creation flow.
  - Unit test now passes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12484, T12451

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

src/applications/phortune/__tests__/PhabricatorPhortuneTestCase.php

@@ -21,6 +21,23 @@ final class PhabricatorPhortuneTestCase
       1,
       count($accounts),
       pht('Creation of default account for users with no accounts.'));
+
+    // Reload the account. The user should be able to view and edit it, and
+    // should be a member.
+
+    $account = head($accounts);
+    $account = id(new PhortuneAccountQuery())
+      ->setViewer($user)
+      ->withPHIDs(array($account->getPHID()))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+
+    $this->assertEqual(true, ($account instanceof PhortuneAccount));
+    $this->assertEqual(array($user->getPHID()), $account->getMemberPHIDs());
   }
 
 }

src/applications/phortune/editor/PhortuneAccountEditor.php

@@ -28,47 +28,48 @@ final class PhortuneAccountEditor
 
     $errors = parent::validateTransaction($object, $type, $xactions);
 
+    $viewer = $this->requireActor();
+
     switch ($type) {
       case PhabricatorTransactions::TYPE_EDGE:
         foreach ($xactions as $xaction) {
           switch ($xaction->getMetadataValue('edge:type')) {
             case PhortuneAccountHasMemberEdgeType::EDGECONST:
-              $actor_phid = $this->requireActor()->getPHID();
-              $new = $xaction->getNewValue();
               $old = $object->getMemberPHIDs();
+              $new = $this->getPHIDTransactionNewValue($xaction, $old);
 
-              // Check if user is trying to not set themselves on creation
+              $old = array_fuse($old);
-              if (!$old) {
+              $new = array_fuse($new);
-                $set = idx($new, '+', array());
+
-                $actor_set = false;
+              foreach ($new as $new_phid) {
-                foreach ($set as $phid) {
+                if (isset($old[$new_phid])) {
-                  if ($actor_phid == $phid) {
+                  continue;
-                    $actor_set = true;
+                }
+
+                $user = id(new PhabricatorPeopleQuery())
+                  ->setViewer($viewer)
+                  ->withPHIDs(array($new_phid))
+                  ->executeOne();
+                if (!$user) {
+                  $error = new PhabricatorApplicationTransactionValidationError(
+                    $type,
+                    pht('Invalid'),
+                    pht(
+                      'Account managers must be valid users, "%s" is not.',
+                      $new_phid));
+                  $errors[] = $error;
+                  continue;
                 }
               }
-                if (!$actor_set) {
+
+              $actor_phid = $this->getActingAsPHID();
+              if (!isset($new[$actor_phid])) {
                 $error = new PhabricatorApplicationTransactionValidationError(
                   $type,
                   pht('Invalid'),
                   pht('You can not remove yourself as an account manager.'),
                   $xaction);
                 $errors[] = $error;
-
-                }
-              }
-
-              // Check if user is trying to remove themselves on edit
-              $set = idx($new, '-', array());
-              foreach ($set as $phid) {
-                if ($actor_phid == $phid) {
-                  $error = new PhabricatorApplicationTransactionValidationError(
-                    $type,
-                    pht('Invalid'),
-                    pht('You can not remove yourself as an account manager.'),
-                    $xaction);
-                  $errors[] = $error;
-
-                }
               }
             break;
           }