Add participants ModularTransactions to Conpherence

Summary: Moves participants over to ModularTransactions, simplified a lot of the code. Fixes T12550

Test Plan:
Create a new room with just myself and myself + fake accounts.
Remove a person.
Remove myself.
Edit a room, topic.
Type some messages.
???
Profit

Reviewers: chad

Reviewed By: chad

Subscribers: Korvin

Maniphest Tasks: T12550

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

src/__phutil_library_map__.php

@@ -321,6 +321,7 @@ phutil_register_library_map(array(
     'ConpherenceThreadListView' => 'applications/conpherence/view/ConpherenceThreadListView.php',
     'ConpherenceThreadMailReceiver' => 'applications/conpherence/mail/ConpherenceThreadMailReceiver.php',
     'ConpherenceThreadMembersPolicyRule' => 'applications/conpherence/policyrule/ConpherenceThreadMembersPolicyRule.php',
+    'ConpherenceThreadParticipantsTransaction' => 'applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php',
     'ConpherenceThreadPictureTransaction' => 'applications/conpherence/xaction/ConpherenceThreadPictureTransaction.php',
     'ConpherenceThreadQuery' => 'applications/conpherence/query/ConpherenceThreadQuery.php',
     'ConpherenceThreadRemarkupRule' => 'applications/conpherence/remarkup/ConpherenceThreadRemarkupRule.php',
@@ -5121,6 +5122,7 @@ phutil_register_library_map(array(
     'ConpherenceThreadListView' => 'AphrontView',
     'ConpherenceThreadMailReceiver' => 'PhabricatorObjectMailReceiver',
     'ConpherenceThreadMembersPolicyRule' => 'PhabricatorPolicyRule',
+    'ConpherenceThreadParticipantsTransaction' => 'ConpherenceThreadTransactionType',
     'ConpherenceThreadPictureTransaction' => 'ConpherenceThreadTransactionType',
     'ConpherenceThreadQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'ConpherenceThreadRemarkupRule' => 'PhabricatorObjectRemarkupRule',

src/applications/conpherence/__tests__/ConpherenceRoomTestCase.php

@@ -107,7 +107,8 @@ final class ConpherenceRoomTestCase extends ConpherenceTestCase {
 
     $xactions = array();
     $xactions[] = id(new ConpherenceTransaction())
-      ->setTransactionType(ConpherenceTransaction::TYPE_PARTICIPANTS)
+      ->setTransactionType(
+        ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
       ->setNewValue(array('+' => $participant_phids));
     $xactions[] = id(new ConpherenceTransaction())
       ->setTransactionType(

src/applications/conpherence/__tests__/ConpherenceTestCase.php

@@ -9,7 +9,8 @@ abstract class ConpherenceTestCase extends PhabricatorTestCase {
 
     $xactions = array(
       id(new ConpherenceTransaction())
-        ->setTransactionType(ConpherenceTransaction::TYPE_PARTICIPANTS)
+        ->setTransactionType(
+          ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
         ->setNewValue(array('+' => $participant_phids)),
     );
     $editor = id(new ConpherenceEditor())
@@ -26,7 +27,8 @@ abstract class ConpherenceTestCase extends PhabricatorTestCase {
 
     $xactions = array(
       id(new ConpherenceTransaction())
-        ->setTransactionType(ConpherenceTransaction::TYPE_PARTICIPANTS)
+        ->setTransactionType(
+          ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
         ->setNewValue(array('-' => $participant_phids)),
     );
     $editor = id(new ConpherenceEditor())

src/applications/conpherence/conduit/ConpherenceUpdateThreadConduitAPIMethod.php

@@ -69,7 +69,7 @@ final class ConpherenceUpdateThreadConduitAPIMethod
     if ($add_participant_phids) {
       $xactions[] = id(new ConpherenceTransaction())
         ->setTransactionType(
-          ConpherenceTransaction::TYPE_PARTICIPANTS)
+          ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
         ->setNewValue(array('+' => $add_participant_phids));
     }
     if ($remove_participant_phid) {
@@ -78,7 +78,7 @@ final class ConpherenceUpdateThreadConduitAPIMethod
       }
       $xactions[] = id(new ConpherenceTransaction())
         ->setTransactionType(
-          ConpherenceTransaction::TYPE_PARTICIPANTS)
+          ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
         ->setNewValue(array('-' => array($remove_participant_phid)));
     }
     if ($title) {

src/applications/conpherence/controller/ConpherenceNewRoomController.php

@@ -23,7 +23,8 @@ final class ConpherenceNewRoomController extends ConpherenceController {
       $participants[] = $user->getPHID();
       $participants = array_unique($participants);
       $xactions[] = id(new ConpherenceTransaction())
-        ->setTransactionType(ConpherenceTransaction::TYPE_PARTICIPANTS)
+        ->setTransactionType(
+          ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
         ->setNewValue(array('+' => $participants));
       $xactions[] = id(new ConpherenceTransaction())
         ->setTransactionType(ConpherenceThreadTopicTransaction::TRANSACTIONTYPE)

src/applications/conpherence/controller/ConpherenceUpdateController.php

@@ -61,7 +61,7 @@ final class ConpherenceUpdateController
         case ConpherenceUpdateActions::JOIN_ROOM:
           $xactions[] = id(new ConpherenceTransaction())
             ->setTransactionType(
-              ConpherenceTransaction::TYPE_PARTICIPANTS)
+              ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
             ->setNewValue(array('+' => array($user->getPHID())));
           $delete_draft = true;
           $message = $request->getStr('text');
@@ -95,7 +95,7 @@ final class ConpherenceUpdateController
           if (!empty($person_phids)) {
             $xactions[] = id(new ConpherenceTransaction())
               ->setTransactionType(
-                ConpherenceTransaction::TYPE_PARTICIPANTS)
+                ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
               ->setNewValue(array('+' => $person_phids));
           }
           break;
@@ -108,7 +108,7 @@ final class ConpherenceUpdateController
           if ($person_phid) {
             $xactions[] = id(new ConpherenceTransaction())
               ->setTransactionType(
-                ConpherenceTransaction::TYPE_PARTICIPANTS)
+                ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
               ->setNewValue(array('-' => array($person_phid)));
             $response_mode = 'go-home';
           }

src/applications/conpherence/editor/ConpherenceEditor.php

@@ -37,7 +37,8 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     if (!$errors) {
       $xactions = array();
       $xactions[] = id(new ConpherenceTransaction())
-        ->setTransactionType(ConpherenceTransaction::TYPE_PARTICIPANTS)
+        ->setTransactionType(
+          ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
         ->setNewValue(array('+' => $participant_phids));
       if ($title) {
         $xactions[] = id(new ConpherenceTransaction())
@@ -87,8 +88,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
   public function getTransactionTypes() {
     $types = parent::getTransactionTypes();
 
-    $types[] = ConpherenceTransaction::TYPE_PARTICIPANTS;
-
     $types[] = PhabricatorTransactions::TYPE_COMMENT;
     $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
     $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
@@ -100,29 +99,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     return pht('%s created this room.', $author);
   }
 
-  protected function getCustomTransactionOldValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case ConpherenceTransaction::TYPE_PARTICIPANTS:
-        if ($this->getIsNewObject()) {
-          return array();
-        }
-        return $object->getParticipantPHIDs();
-    }
-  }
-
-  protected function getCustomTransactionNewValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case ConpherenceTransaction::TYPE_PARTICIPANTS:
-        return $this->getPHIDTransactionNewValue($xaction);
-    }
-  }
-
   /**
    * We really only need a read lock if we have a comment. In that case, we
    * must update the messagesCount field on the conpherence and
@@ -142,66 +118,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     return $lock;
   }
 
-  /**
-   * We need to apply initial effects IFF the conpherence is new. We must
-   * save the conpherence first thing to make sure we have an id and a phid, as
-   * well as create the initial set of participants so that we pass policy
-   * checks.
-   */
-  protected function shouldApplyInitialEffects(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
-
-    return $this->getIsNewObject();
-  }
-
-  protected function applyInitialEffects(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
-
-    $object->save();
-
-    foreach ($xactions as $xaction) {
-      switch ($xaction->getTransactionType()) {
-        case ConpherenceTransaction::TYPE_PARTICIPANTS:
-          // Since this is a new ConpherenceThread, we have to create the
-          // participation data asap to pass policy checks. For existing
-          // ConpherenceThreads, the existing participation is correct
-          // at this stage. Note that later in applyCustomExternalTransaction
-          // this participation data will be updated.
-          $participants = array();
-          $phids = $this->getPHIDTransactionNewValue($xaction, array());
-          foreach ($phids as $phid) {
-            if ($phid == $this->getActor()->getPHID()) {
-              $message_count = 1;
-            } else {
-              $message_count = 0;
-            }
-            $participants[$phid] =
-              id(new ConpherenceParticipant())
-              ->setConpherencePHID($object->getPHID())
-              ->setParticipantPHID($phid)
-              ->setSeenMessageCount($message_count)
-              ->save();
-            $object->attachParticipants($participants);
-          }
-          break;
-      }
-    }
-  }
-
-  protected function applyCustomInternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case ConpherenceTransaction::TYPE_PARTICIPANTS:
-        if (!$this->getIsNewObject()) {}
-        break;
-    }
-
-  }
-
   protected function applyBuiltinInternalTransaction(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
@@ -215,83 +131,23 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     return parent::applyBuiltinInternalTransaction($object, $xaction);
   }
 
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case ConpherenceTransaction::TYPE_PARTICIPANTS:
-        if ($this->getIsNewObject()) {
-          continue;
-        }
-        $participants = $object->getParticipants();
-
-        $old_map = array_fuse($xaction->getOldValue());
-        $new_map = array_fuse($xaction->getNewValue());
-
-        $remove = array_keys(array_diff_key($old_map, $new_map));
-        foreach ($remove as $phid) {
-          $remove_participant = $participants[$phid];
-          $remove_participant->delete();
-          unset($participants[$phid]);
-        }
-
-        $add = array_keys(array_diff_key($new_map, $old_map));
-        foreach ($add as $phid) {
-          if ($phid == $this->getActor()->getPHID()) {
-            $message_count = $object->getMessageCount();
-          } else {
-            $message_count = 0;
-          }
-          $participants[$phid] =
-            id(new ConpherenceParticipant())
-            ->setConpherencePHID($object->getPHID())
-            ->setParticipantPHID($phid)
-            ->setSeenMessageCount($message_count)
-            ->save();
-        }
-        $object->attachParticipants($participants);
-        break;
-    }
-  }
 
   protected function applyFinalEffects(
     PhabricatorLiskDAO $object,
     array $xactions) {
 
-    if (!$xactions) {
+    $acting_phid = $this->getActingAsPHID();
-      return $xactions;
-    }
-
-    $message_count = 0;
-    foreach ($xactions as $xaction) {
-      switch ($xaction->getTransactionType()) {
-        case PhabricatorTransactions::TYPE_COMMENT:
-          $message_count++;
-
-          // update everyone's participation status on a message -only-
-          $xaction_phid = $xaction->getPHID();
     $participants = $object->getParticipants();
-          $user = $this->getActor();
+    foreach ($participants as $participant) {
-          $time = time();
+      if ($participant->getParticipantPHID() == $acting_phid) {
-          foreach ($participants as $phid => $participant) {
+        $participant->markUpToDate($object);
-            if ($phid != $user->getPHID()) {
-              if ($participant->isUpToDate($object)) {
-                $participant->setSeenMessageCount(
-                  $object->getMessageCount() - $message_count);
       }
-            } else {
-              $participant->setSeenMessageCount($object->getMessageCount());
-            }
-            $participant->save();
     }
 
+    if ($participants) {
       PhabricatorUserCache::clearCaches(
         PhabricatorUserMessageCountCacheType::KEY_COUNT,
         array_keys($participants));
-
-          break;
-      }
     }
 
     if ($xactions) {
@@ -315,7 +171,7 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     parent::requireCapabilities($object, $xaction);
 
     switch ($xaction->getTransactionType()) {
-      case ConpherenceTransaction::TYPE_PARTICIPANTS:
+      case ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE:
         $old_map = array_fuse($xaction->getOldValue());
         $new_map = array_fuse($xaction->getNewValue());
 
@@ -339,8 +195,8 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
             $object,
             PhabricatorPolicyCapability::CAN_EDIT);
         }
-
         break;
+
       case ConpherenceThreadTitleTransaction::TRANSACTIONTYPE:
       case ConpherenceThreadTopicTransaction::TRANSACTIONTYPE:
         PhabricatorPolicyFilter::requireCapability(
@@ -351,19 +207,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     }
   }
 
-  protected function mergeTransactions(
-    PhabricatorApplicationTransaction $u,
-    PhabricatorApplicationTransaction $v) {
-
-    $type = $u->getTransactionType();
-    switch ($type) {
-      case ConpherenceTransaction::TYPE_PARTICIPANTS:
-        return $this->mergePHIDOrEdgeTransactions($u, $v);
-    }
-
-    return parent::mergeTransactions($u, $v);
-  }
-
   protected function shouldSendMail(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -466,43 +309,4 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     return true;
   }
 
-  protected function validateTransaction(
-    PhabricatorLiskDAO $object,
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateTransaction($object, $type, $xactions);
-
-    switch ($type) {
-      case ConpherenceTransaction::TYPE_PARTICIPANTS:
-        foreach ($xactions as $xaction) {
-          $new_phids = $this->getPHIDTransactionNewValue($xaction, array());
-          $old_phids = nonempty($object->getParticipantPHIDs(), array());
-          $phids = array_diff($new_phids, $old_phids);
-
-          if (!$phids) {
-            continue;
-          }
-
-          $users = id(new PhabricatorPeopleQuery())
-            ->setViewer($this->requireActor())
-            ->withPHIDs($phids)
-            ->execute();
-          $users = mpull($users, null, 'getPHID');
-          foreach ($phids as $phid) {
-            if (isset($users[$phid])) {
-              continue;
-            }
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Invalid'),
-              pht('New room participant "%s" is not a valid user.', $phid),
-              $xaction);
-          }
-        }
-        break;
-    }
-
-    return $errors;
-  }
 }

src/applications/conpherence/mail/ConpherenceReplyHandler.php

@@ -55,7 +55,8 @@ final class ConpherenceReplyHandler extends PhabricatorMailReplyHandler {
     $xactions = array();
     if ($this->getMailAddedParticipantPHIDs()) {
       $xactions[] = id(new ConpherenceTransaction())
-        ->setTransactionType(ConpherenceTransaction::TYPE_PARTICIPANTS)
+        ->setTransactionType(
+          ConpherenceThreadParticipantsTransaction::TRANSACTIONTYPE)
         ->setNewValue(array('+' => $this->getMailAddedParticipantPHIDs()));
     }
 

src/applications/conpherence/storage/ConpherenceTransaction.php

@@ -3,8 +3,6 @@
 final class ConpherenceTransaction
   extends PhabricatorModularTransaction {
 
-  const TYPE_PARTICIPANTS    = 'participants';
-
   public function getApplicationName() {
     return 'conpherence';
   }
@@ -21,81 +19,4 @@ final class ConpherenceTransaction
     return 'ConpherenceThreadTransactionType';
   }
 
-  public function getNoEffectDescription() {
-    switch ($this->getTransactionType()) {
-      case self::TYPE_PARTICIPANTS:
-        return pht(
-          'You can not add a participant who has already been added.');
-        break;
-    }
-
-    return parent::getNoEffectDescription();
-  }
-
-  public function shouldHide() {
-    $old = $this->getOldValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_PARTICIPANTS:
-        return ($old === null);
-    }
-
-    return parent::shouldHide();
-  }
-
-  public function getTitle() {
-    $author_phid = $this->getAuthorPHID();
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_PARTICIPANTS:
-        $add = array_diff($new, $old);
-        $rem = array_diff($old, $new);
-
-        if ($add && $rem) {
-          $title = pht(
-            '%s edited participant(s), added %d: %s; removed %d: %s.',
-            $this->renderHandleLink($author_phid),
-            count($add),
-            $this->renderHandleList($add),
-            count($rem),
-            $this->renderHandleList($rem));
-        } else if ($add) {
-          $title = pht(
-            '%s added %d participant(s): %s.',
-            $this->renderHandleLink($author_phid),
-            count($add),
-            $this->renderHandleList($add));
-        } else {
-          $title = pht(
-            '%s removed %d participant(s): %s.',
-            $this->renderHandleLink($author_phid),
-            count($rem),
-            $this->renderHandleList($rem));
-        }
-        return $title;
-        break;
-    }
-
-    return parent::getTitle();
-  }
-
-  public function getRequiredHandlePHIDs() {
-    $phids = parent::getRequiredHandlePHIDs();
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $phids[] = $this->getAuthorPHID();
-    switch ($this->getTransactionType()) {
-      case self::TYPE_PARTICIPANTS:
-        $phids = array_merge($phids, $this->getOldValue());
-        $phids = array_merge($phids, $this->getNewValue());
-        break;
-    }
-
-    return $phids;
-  }
 }

src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php

@@ -0,0 +1,117 @@
+<?php
+
+final class ConpherenceThreadParticipantsTransaction
+  extends ConpherenceThreadTransactionType {
+
+  const TRANSACTIONTYPE = 'participants';
+
+  public function generateOldValue($object) {
+    return $object->getParticipantPHIDs();
+  }
+
+  public function generateNewValue($object, $value) {
+    $old = $this->generateOldValue($object);
+    return $this->getPHIDList($old, $value);
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $participants = $object->getParticipants();
+
+    $old = array_keys($participants);
+    $new = $value;
+
+    $add_map = array_fuse(array_diff($new, $old));
+    $rem_map = array_fuse(array_diff($old, $new));
+
+    foreach ($rem_map as $phid) {
+      $remove_participant = $participants[$phid];
+      $remove_participant->delete();
+      unset($participants[$phid]);
+    }
+
+    foreach ($add_map as $phid) {
+      if (isset($participants[$phid])) {
+        continue;
+      }
+
+      $participants[$phid] = id(new ConpherenceParticipant())
+        ->setConpherencePHID($object->getPHID())
+        ->setParticipantPHID($phid)
+        ->setSeenMessageCount(0)
+        ->save();
+    }
+
+    $object->attachParticipants($participants);
+  }
+
+  public function getTitle() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $add = array_diff($new, $old);
+    $rem = array_diff($old, $new);
+
+    $author_phid = $this->getAuthorPHID();
+
+    if ($add && $rem) {
+      return pht(
+        '%s edited participant(s), added %d: %s; removed %d: %s.',
+        $this->renderAuthor(),
+        count($add),
+        $this->renderHandleList($add),
+        count($rem),
+        $this->renderHandleList($rem));
+    } else if ((in_array($author_phid, $add)) && (count($add) == 1)) {
+      return pht(
+        '%s joined the room.',
+        $this->renderAuthor());
+    } else if ((in_array($author_phid, $rem)) && (count($rem) == 1)) {
+      return pht(
+        '%s left the room.',
+        $this->renderAuthor());
+    } else if ($add) {
+      return pht(
+        '%s added %d participant(s): %s.',
+        $this->renderAuthor(),
+        count($add),
+        $this->renderHandleList($add));
+    } else {
+      return pht(
+        '%s removed %d participant(s): %s.',
+        $this->renderAuthor(),
+        count($rem),
+        $this->renderHandleList($rem));
+    }
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    foreach ($xactions as $xaction) {
+      $old = $object->getParticipantPHIDs();
+
+      $new = $xaction->getNewValue();
+      $new = $this->getPHIDList($old, $new);
+
+      $add_map = array_fuse(array_diff($new, $old));
+      $rem_map = array_fuse(array_diff($old, $new));
+
+      foreach ($add_map as $user_phid) {
+        $user = id(new PhabricatorPeopleQuery())
+          ->setViewer($this->getActor())
+          ->withPHIDs(array($user_phid))
+          ->executeOne();
+        if (!$user) {
+          $errors[] = $this->newInvalidError(
+            pht(
+              'Participant PHID "%s" is not a valid user PHID.',
+              $user_phid));
+          continue;
+        }
+      }
+    }
+
+    return $errors;
+  }
+
+}

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1801,7 +1801,10 @@ abstract class PhabricatorApplicationTransactionEditor
       $old = array_fuse($xaction->getOldValue());
     }
 
-    $new = $xaction->getNewValue();
+    return $this->getPHIDList($old, $xaction->getNewValue());
+  }
+
+  public function getPHIDList(array $old, array $new) {
     $new_add = idx($new, '+', array());
     unset($new['+']);
     $new_rem = idx($new, '-', array());

src/applications/transactions/storage/PhabricatorModularTransactionType.php

@@ -309,4 +309,10 @@ abstract class PhabricatorModularTransactionType
     return $this->getStorage()->getIsCreateTransaction();
   }
 
+  final protected function getPHIDList(array $old, array $new) {
+    $editor = $this->getEditor();
+
+    return $editor->getPHIDList($old, $new);
+  }
+
 }