diff --git a/resources/sql/autopatches/20141210.maniphestsubscribersmig.1.sql b/resources/sql/autopatches/20141210.maniphestsubscribersmig.1.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20141210.maniphestsubscribersmig.1.sql @@ -0,0 +1,9 @@ +INSERT IGNORE INTO {$NAMESPACE}_maniphest.edge (src, type, dst) + SELECT taskPHID, 21, subscriberPHID + FROM {$NAMESPACE}_maniphest.maniphest_tasksubscriber + WHERE subscriberPHID != ''; + +INSERT IGNORE INTO {$NAMESPACE}_maniphest.edge (src, type, dst) + SELECT subscriberPHID, 22, taskPHID + FROM {$NAMESPACE}_maniphest.maniphest_tasksubscriber + WHERE subscriberPHID != ''; diff --git a/resources/sql/autopatches/20141210.maniphestsubscribersmig.2.sql b/resources/sql/autopatches/20141210.maniphestsubscribersmig.2.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20141210.maniphestsubscribersmig.2.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task + DROP ccPHIDs; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1001,7 +1001,6 @@ 'ManiphestSearchIndexer' => 'applications/maniphest/search/ManiphestSearchIndexer.php', 'ManiphestStatusConfigOptionType' => 'applications/maniphest/config/ManiphestStatusConfigOptionType.php', 'ManiphestSubpriorityController' => 'applications/maniphest/controller/ManiphestSubpriorityController.php', - 'ManiphestSubscribeController' => 'applications/maniphest/controller/ManiphestSubscribeController.php', 'ManiphestTask' => 'applications/maniphest/storage/ManiphestTask.php', 'ManiphestTaskDescriptionPreviewController' => 'applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php', 'ManiphestTaskDetailController' => 'applications/maniphest/controller/ManiphestTaskDetailController.php', @@ -4072,9 +4071,9 @@ 'ManiphestSearchIndexer' => 'PhabricatorSearchDocumentIndexer', 'ManiphestStatusConfigOptionType' => 'PhabricatorConfigJSONOptionType', 'ManiphestSubpriorityController' => 'ManiphestController', - 'ManiphestSubscribeController' => 'ManiphestController', 'ManiphestTask' => array( 'ManiphestDAO', + 'PhabricatorSubscribableInterface', 'PhabricatorMarkupInterface', 'PhabricatorPolicyInterface', 'PhabricatorTokenReceiverInterface', diff --git a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php --- a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php +++ b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php @@ -139,7 +139,7 @@ case self::FIELD_ASSIGNEE: return $this->getTask()->getOwnerPHID(); case self::FIELD_CC: - return $this->getTask()->getCCPHIDs(); + return $this->getTask()->getSubscriberPHIDs(); case self::FIELD_PROJECTS: return PhabricatorEdgeQuery::loadDestinationPHIDs( $this->getTask()->getPHID(), diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -66,8 +66,6 @@ ), 'export/(?P[^/]+)/' => 'ManiphestExportController', 'subpriority/' => 'ManiphestSubpriorityController', - 'subscribe/(?Padd|rem)/(?P[1-9]\d*)/' - => 'ManiphestSubscribeController', ), ); } diff --git a/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php --- a/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php @@ -62,6 +62,8 @@ $task->setDescription((string)$request->getValue('description')); $changes[ManiphestTransaction::TYPE_STATUS] = ManiphestTaskStatus::getDefaultStatus(); + $changes[PhabricatorTransactions::TYPE_SUBSCRIBERS] = + array('+' => array($request->getUser()->getPHID())); } else { $comments = $request->getValue('comments'); @@ -111,7 +113,8 @@ $ccs = $request->getValue('ccPHIDs'); if ($ccs !== null) { - $changes[ManiphestTransaction::TYPE_CCS] = $ccs; + $changes[PhabricatorTransactions::TYPE_SUBSCRIBERS] = + array('=' => array_fuse($ccs)); } $transactions = array(); @@ -228,6 +231,13 @@ $event->setUser($request->getUser()); $event->setConduitRequest($request); PhutilEventEngine::dispatchEvent($event); + + // reload the task now that we've done all the fun stuff + return id(new ManiphestTaskQuery()) + ->setViewer($request->getUser()) + ->withPHIDs(array($task->getPHID())) + ->needSubscriberPHIDs(true) + ->executeOne(); } protected function buildTaskInfoDictionaries(array $tasks) { @@ -265,7 +275,7 @@ 'phid' => $task->getPHID(), 'authorPHID' => $task->getAuthorPHID(), 'ownerPHID' => $task->getOwnerPHID(), - 'ccPHIDs' => $task->getCCPHIDs(), + 'ccPHIDs' => $task->getSubscriberPHIDs(), 'status' => $task->getStatus(), 'statusName' => ManiphestTaskStatus::getTaskStatusName( $task->getStatus()), diff --git a/src/applications/maniphest/conduit/ManiphestCreateTaskConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestCreateTaskConduitAPIMethod.php --- a/src/applications/maniphest/conduit/ManiphestCreateTaskConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestCreateTaskConduitAPIMethod.php @@ -28,7 +28,7 @@ protected function execute(ConduitAPIRequest $request) { $task = ManiphestTask::initializeNewTask($request->getUser()); - $this->applyRequest($task, $request, $is_new = true); + $task = $this->applyRequest($task, $request, $is_new = true); return $this->buildTaskInfoDictionary($task); } diff --git a/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php --- a/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php @@ -32,6 +32,7 @@ $task = id(new ManiphestTaskQuery()) ->setViewer($request->getUser()) ->withIDs(array($task_id)) + ->needSubscriberPHIDs(true) ->executeOne(); if (!$task) { throw new ConduitException('ERR_BAD_TASK'); diff --git a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php --- a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php @@ -64,6 +64,7 @@ $query = new ManiphestTaskQuery(); $query->setViewer($request->getUser()); + $query->needSubscriberPHIDs(true); $task_ids = $request->getValue('ids'); if ($task_ids) { diff --git a/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php --- a/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php @@ -38,11 +38,13 @@ $task = id(new ManiphestTaskQuery()) ->setViewer($request->getUser()) ->withIDs(array($id)) + ->needSubscriberPHIDs(true) ->executeOne(); } else { $task = id(new ManiphestTaskQuery()) ->setViewer($request->getUser()) ->withPHIDs(array($phid)) + ->needSubscriberPHIDs(true) ->executeOne(); } @@ -58,7 +60,7 @@ throw new ConduitException('ERR-BAD-TASK'); } - $this->applyRequest($task, $request, $is_new = false); + $task = $this->applyRequest($task, $request, $is_new = false); return $this->buildTaskInfoDictionary($task); } diff --git a/src/applications/maniphest/controller/ManiphestBatchEditController.php b/src/applications/maniphest/controller/ManiphestBatchEditController.php --- a/src/applications/maniphest/controller/ManiphestBatchEditController.php +++ b/src/applications/maniphest/controller/ManiphestBatchEditController.php @@ -18,6 +18,7 @@ PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_EDIT, )) + ->needSubscriberPHIDs(true) ->execute(); $actions = $request->getStr('actions'); @@ -171,8 +172,8 @@ 'priority' => ManiphestTransaction::TYPE_PRIORITY, 'add_project' => ManiphestTransaction::TYPE_PROJECTS, 'remove_project' => ManiphestTransaction::TYPE_PROJECTS, - 'add_ccs' => ManiphestTransaction::TYPE_CCS, - 'remove_ccs' => ManiphestTransaction::TYPE_CCS, + 'add_ccs' => PhabricatorTransactions::TYPE_SUBSCRIBERS, + 'remove_ccs' => PhabricatorTransactions::TYPE_SUBSCRIBERS, ); $edge_edit_types = array( @@ -215,8 +216,8 @@ case ManiphestTransaction::TYPE_PROJECTS: $current = $task->getProjectPHIDs(); break; - case ManiphestTransaction::TYPE_CCS: - $current = $task->getCCPHIDs(); + case PhabricatorTransactions::TYPE_SUBSCRIBERS: + $current = $task->getSubscriberPHIDs(); break; } } @@ -246,7 +247,7 @@ continue 2; } break; - case ManiphestTransaction::TYPE_CCS: + case PhabricatorTransactions::TYPE_SUBSCRIBERS: if (empty($value)) { continue 2; } @@ -274,12 +275,7 @@ } break; case ManiphestTransaction::TYPE_PROJECTS: - case ManiphestTransaction::TYPE_CCS: - $remove_actions = array( - 'remove_project' => true, - 'remove_ccs' => true, - ); - $is_remove = isset($remove_actions[$action['action']]); + $is_remove = $action['action'] == 'remove_project'; $current = array_fill_keys($current, true); $value = array_fill_keys($value, true); @@ -309,6 +305,39 @@ $value = array_keys($new); break; + case PhabricatorTransactions::TYPE_SUBSCRIBERS: + $is_remove = $action['action'] == 'remove_ccs'; + + $current = array_fill_keys($current, true); + + $new = array(); + $did_something = false; + + if ($is_remove) { + foreach ($value as $phid) { + if (isset($current[$phid])) { + $new[$phid] = true; + $did_something = true; + } + } + if ($new) { + $value = array('-' => array_keys($new)); + } + } else { + $new = array(); + foreach ($value as $phid) { + $new[$phid] = true; + $did_something = true; + } + if ($new) { + $value = array('+' => array_keys($new)); + } + } + if (!$did_something) { + continue 2; + } + + break; } $value_map[$type] = $value; diff --git a/src/applications/maniphest/controller/ManiphestSubscribeController.php b/src/applications/maniphest/controller/ManiphestSubscribeController.php deleted file mode 100644 --- a/src/applications/maniphest/controller/ManiphestSubscribeController.php +++ /dev/null @@ -1,51 +0,0 @@ -id = $data['id']; - $this->action = $data['action']; - } - - public function processRequest() { - - $request = $this->getRequest(); - $user = $request->getUser(); - - $task = id(new ManiphestTaskQuery()) - ->setViewer($user) - ->withIDs(array($this->id)) - ->executeOne(); - if (!$task) { - return new Aphront404Response(); - } - - $ccs = $task->getCCPHIDs(); - switch ($this->action) { - case 'add': - $ccs[] = $user->getPHID(); - break; - case 'rem': - $ccs = array_diff($ccs, array($user->getPHID())); - break; - default: - return new Aphront400Response(); - } - - $xaction = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_CCS) - ->setNewValue($ccs); - - $editor = id(new ManiphestTransactionEditor()) - ->setActor($user) - ->setContentSourceFromRequest($request) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true) - ->applyTransactions($task, array($xaction)); - - return id(new AphrontRedirectResponse())->setURI('/T'.$task->getID()); - } -} diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -23,6 +23,7 @@ $task = id(new ManiphestTaskQuery()) ->setViewer($user) ->withIDs(array($this->id)) + ->needSubscriberPHIDs(true) ->executeOne(); if (!$task) { return new Aphront404Response(); @@ -65,12 +66,6 @@ $edges = idx($query->execute(), $phid); $phids = array_fill_keys($query->getDestinationPHIDs(), true); - foreach ($task->getCCPHIDs() as $phid) { - $phids[$phid] = true; - } - foreach ($task->getProjectPHIDs() as $phid) { - $phids[$phid] = true; - } if ($task->getOwnerPHID()) { $phids[$task->getOwnerPHID()] = true; } @@ -139,12 +134,12 @@ $resolution_types = ManiphestTaskStatus::getTaskStatusMap(); $transaction_types = array( - PhabricatorTransactions::TYPE_COMMENT => pht('Comment'), - ManiphestTransaction::TYPE_STATUS => pht('Change Status'), - ManiphestTransaction::TYPE_OWNER => pht('Reassign / Claim'), - ManiphestTransaction::TYPE_CCS => pht('Add CCs'), - ManiphestTransaction::TYPE_PRIORITY => pht('Change Priority'), - ManiphestTransaction::TYPE_PROJECTS => pht('Associate Projects'), + PhabricatorTransactions::TYPE_COMMENT => pht('Comment'), + ManiphestTransaction::TYPE_STATUS => pht('Change Status'), + ManiphestTransaction::TYPE_OWNER => pht('Reassign / Claim'), + PhabricatorTransactions::TYPE_SUBSCRIBERS => pht('Add CCs'), + ManiphestTransaction::TYPE_PRIORITY => pht('Change Priority'), + ManiphestTransaction::TYPE_PROJECTS => pht('Associate Projects'), ); // Remove actions the user doesn't have permission to take. @@ -265,11 +260,11 @@ ->setValue(pht('Submit'))); $control_map = array( - ManiphestTransaction::TYPE_STATUS => 'resolution', - ManiphestTransaction::TYPE_OWNER => 'assign_to', - ManiphestTransaction::TYPE_CCS => 'ccs', - ManiphestTransaction::TYPE_PRIORITY => 'priority', - ManiphestTransaction::TYPE_PROJECTS => 'projects', + ManiphestTransaction::TYPE_STATUS => 'resolution', + ManiphestTransaction::TYPE_OWNER => 'assign_to', + PhabricatorTransactions::TYPE_SUBSCRIBERS => 'ccs', + ManiphestTransaction::TYPE_PRIORITY => 'priority', + ManiphestTransaction::TYPE_PROJECTS => 'projects', ); $projects_source = new PhabricatorProjectDatasource(); @@ -289,7 +284,7 @@ 'limit' => 1, 'placeholder' => $users_source->getPlaceholderText(), ), - ManiphestTransaction::TYPE_CCS => array( + PhabricatorTransactions::TYPE_SUBSCRIBERS => array( 'id' => 'cc-tokenizer', 'src' => $mailable_source->getDatasourceURI(), 'placeholder' => $mailable_source->getPlaceholderText(), @@ -397,7 +392,6 @@ private function buildActionView(ManiphestTask $task) { $viewer = $this->getRequest()->getUser(); $viewer_phid = $viewer->getPHID(); - $viewer_is_cc = in_array($viewer_phid, $task->getCCPHIDs()); $id = $task->getID(); $phid = $task->getPHID(); @@ -420,26 +414,6 @@ ->setDisabled(!$can_edit) ->setWorkflow(!$can_edit)); - if ($task->getOwnerPHID() === $viewer_phid) { - $view->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Automatically Subscribed')) - ->setDisabled(true) - ->setIcon('fa-check-circle')); - } else { - $action = $viewer_is_cc ? 'rem' : 'add'; - $name = $viewer_is_cc ? pht('Unsubscribe') : pht('Subscribe'); - $icon = $viewer_is_cc ? 'fa-minus-circle' : 'fa-plus-circle'; - - $view->addAction( - id(new PhabricatorActionView()) - ->setName($name) - ->setHref("/maniphest/subscribe/{$action}/{$id}/") - ->setRenderAsForm(true) - ->setUser($viewer) - ->setIcon($icon)); - } - $view->addAction( id(new PhabricatorActionView()) ->setName(pht('Merge Duplicates In')) @@ -490,14 +464,6 @@ pht('Priority'), ManiphestTaskPriority::getTaskPriorityName($task->getPriority())); - $handles = $this->getLoadedHandles(); - $cc_handles = array_select_keys($handles, $task->getCCPHIDs()); - $subscriber_html = id(new SubscriptionListStringBuilder()) - ->setObjectPHID($task->getPHID()) - ->setHandles($cc_handles) - ->buildPropertyString(); - $view->addProperty(pht('Subscribers'), $subscriber_html); - $view->addProperty( pht('Author'), $this->getHandle($task->getAuthorPHID())->renderLink()); diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -38,6 +38,7 @@ PhabricatorPolicyCapability::CAN_EDIT, )) ->withIDs(array($this->id)) + ->needSubscriberPHIDs(true) ->executeOne(); if (!$task) { return new Aphront404Response(); @@ -215,7 +216,7 @@ $task->setDescription($new_desc); $task->setPriority($request->getInt('priority')); $task->setOwnerPHID($owner_phid); - $task->setCCPHIDs($request->getArr('cc')); + $task->attachSubscriberPHIDs($request->getArr('cc')); $task->attachProjectPHIDs($request->getArr('projects')); } else { @@ -227,7 +228,8 @@ $changes[ManiphestTransaction::TYPE_OWNER] = $owner_phid; } - $changes[ManiphestTransaction::TYPE_CCS] = $request->getArr('cc'); + $changes[PhabricatorTransactions::TYPE_SUBSCRIBERS] = + array('=' => $request->getArr('cc')); if ($can_edit_projects) { $projects = $request->getArr('projects'); @@ -436,19 +438,20 @@ } } else { if (!$task->getID()) { - $task->setCCPHIDs(array( + $task->attachSubscriberPHIDs(array( $user->getPHID(), )); if ($template_id) { $template_task = id(new ManiphestTaskQuery()) ->setViewer($user) ->withIDs(array($template_id)) + ->needSubscriberPHIDs(true) ->executeOne(); if ($template_task) { $cc_phids = array_unique(array_merge( - $template_task->getCCPHIDs(), + $template_task->getSubscriberPHIDs(), array($user->getPHID()))); - $task->setCCPHIDs($cc_phids); + $task->attachSubscriberPHIDs($cc_phids); $task->attachProjectPHIDs($template_task->getProjectPHIDs()); $task->setOwnerPHID($template_task->getOwnerPHID()); $task->setPriority($template_task->getPriority()); @@ -486,7 +489,7 @@ $phids = array_merge( array($task->getOwnerPHID()), - $task->getCCPHIDs(), + $task->getSubscriberPHIDs(), $task->getProjectPHIDs()); if ($parent_task) { @@ -512,8 +515,8 @@ $assigned_value = array(); } - if ($task->getCCPHIDs()) { - $cc_value = array_select_keys($handles, $task->getCCPHIDs()); + if ($task->getSubscriberPHIDs()) { + $cc_value = array_select_keys($handles, $task->getSubscriberPHIDs()); } else { $cc_value = array(); } diff --git a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php --- a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php @@ -58,22 +58,19 @@ } $transaction->setNewValue($value); break; - case ManiphestTransaction::TYPE_CCS: + case PhabricatorTransactions::TYPE_SUBSCRIBERS: if ($value) { $value = json_decode($value); } if (!$value) { $value = array(); } - $phids = $value; - - foreach ($task->getCCPHIDs() as $cc_phid) { + $phids = array(); + foreach ($value as $cc_phid) { $phids[] = $cc_phid; - $value[] = $cc_phid; } - - $transaction->setOldValue($task->getCCPHIDs()); - $transaction->setNewValue($value); + $transaction->setOldValue(array()); + $transaction->setNewValue($phids); break; case ManiphestTransaction::TYPE_PROJECTS: if ($value) { diff --git a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php --- a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php @@ -9,6 +9,7 @@ $task = id(new ManiphestTaskQuery()) ->setViewer($user) ->withIDs(array($request->getStr('taskID'))) + ->needSubscriberPHIDs(true) ->executeOne(); if (!$task) { return new Aphront404Response(); @@ -33,7 +34,7 @@ $cc_transaction = new ManiphestTransaction(); $cc_transaction - ->setTransactionType(ManiphestTransaction::TYPE_CCS); + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); $transaction = new ManiphestTransaction(); $transaction @@ -64,7 +65,7 @@ '+' => array_fuse($projects), )); break; - case ManiphestTransaction::TYPE_CCS: + case PhabricatorTransactions::TYPE_SUBSCRIBERS: // Accumulate the new explicit CCs into the array that we'll add in // the CC transaction later. $added_ccs = array_merge($added_ccs, $request->getArr('ccs')); @@ -135,19 +136,21 @@ if (!$user_owns_task) { // If we aren't making the user the new task owner and they aren't the // existing task owner, add them to CC unless they're aleady CC'd. - if (!in_array($user->getPHID(), $task->getCCPHIDs())) { + if (!in_array($user->getPHID(), $task->getSubscriberPHIDs())) { $added_ccs[] = $user->getPHID(); } } // Evade no-effect detection in the new editor stuff until we can switch // to subscriptions. - $added_ccs = array_filter(array_diff($added_ccs, $task->getCCPHIDs())); + $added_ccs = array_filter(array_diff( + $added_ccs, + $task->getSubscriberPHIDs())); if ($added_ccs) { // We've added CCs, so include a CC transaction. - $all_ccs = array_merge($task->getCCPHIDs(), $added_ccs); - $cc_transaction->setNewValue($all_ccs); + $all_ccs = array_merge($task->getSubscriberPHIDs(), $added_ccs); + $cc_transaction->setNewValue(array('=' => $all_ccs)); $transactions[] = $cc_transaction; } diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -23,7 +23,6 @@ $types[] = ManiphestTransaction::TYPE_TITLE; $types[] = ManiphestTransaction::TYPE_DESCRIPTION; $types[] = ManiphestTransaction::TYPE_OWNER; - $types[] = ManiphestTransaction::TYPE_CCS; $types[] = ManiphestTransaction::TYPE_SUBPRIORITY; $types[] = ManiphestTransaction::TYPE_PROJECT_COLUMN; $types[] = ManiphestTransaction::TYPE_MERGED_INTO; @@ -62,8 +61,6 @@ return $object->getDescription(); case ManiphestTransaction::TYPE_OWNER: return nonempty($object->getOwnerPHID(), null); - case ManiphestTransaction::TYPE_CCS: - return array_values(array_unique($object->getCCPHIDs())); case ManiphestTransaction::TYPE_PROJECT_COLUMN: // These are pre-populated. return $xaction->getOldValue(); @@ -82,8 +79,6 @@ switch ($xaction->getTransactionType()) { case ManiphestTransaction::TYPE_PRIORITY: return (int)$xaction->getNewValue(); - case ManiphestTransaction::TYPE_CCS: - return array_values(array_unique($xaction->getNewValue())); case ManiphestTransaction::TYPE_OWNER: return nonempty($xaction->getNewValue(), null); case ManiphestTransaction::TYPE_STATUS: @@ -106,10 +101,6 @@ $new = $xaction->getNewValue(); switch ($xaction->getTransactionType()) { - case ManiphestTransaction::TYPE_CCS: - sort($old); - sort($new); - return ($old !== $new); case ManiphestTransaction::TYPE_PROJECT_COLUMN: $new_column_phids = $new['columnPHIDs']; $old_column_phids = $old['columnPHIDs']; @@ -155,8 +146,6 @@ } return $object->setOwnerPHID($phid); - case ManiphestTransaction::TYPE_CCS: - return $object->setCCPHIDs($xaction->getNewValue()); case ManiphestTransaction::TYPE_SUBPRIORITY: $data = $xaction->getNewValue(); $new_sub = $this->getNextSubpriority( @@ -433,10 +422,6 @@ protected function getMailCC(PhabricatorLiskDAO $object) { $phids = array(); - foreach ($object->getCCPHIDs() as $phid) { - $phids[] = $phid; - } - foreach (parent::getMailCC($object) as $phid) { $phids[] = $phid; } @@ -561,22 +546,16 @@ HeraldAdapter $adapter, HeraldTranscript $transcript) { - // TODO: Convert these to transactions. The way Maniphest deals with these - // transactions is currently unconventional and messy. + $this->heraldEmailPHIDs = $adapter->getEmailPHIDs(); + $xactions = array(); - $save_again = false; $cc_phids = $adapter->getCcPHIDs(); if ($cc_phids) { - $existing_cc = $object->getCCPHIDs(); - $new_cc = array_unique(array_merge($cc_phids, $existing_cc)); - $object->setCCPHIDs($new_cc); - $object->save(); + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue(array('+' => $cc_phids)); } - $this->heraldEmailPHIDs = $adapter->getEmailPHIDs(); - - $xactions = array(); - $assign_phid = $adapter->getAssignPHID(); if ($assign_phid) { $xactions[] = id(new ManiphestTransaction()) diff --git a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php --- a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php +++ b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php @@ -28,8 +28,8 @@ $this->generateTaskStatus(); $changes[ManiphestTransaction::TYPE_PRIORITY] = $this->generateTaskPriority(); - $changes[ManiphestTransaction::TYPE_CCS] = - $this->getCCPHIDs(); + $changes[PhabricatorTransactions::TYPE_SUBSCRIBERS] = + array('=' => $this->getCCPHIDs()); $transactions = array(); foreach ($changes as $type => $value) { $transaction = clone $template; diff --git a/src/applications/maniphest/mail/ManiphestReplyHandler.php b/src/applications/maniphest/mail/ManiphestReplyHandler.php --- a/src/applications/maniphest/mail/ManiphestReplyHandler.php +++ b/src/applications/maniphest/mail/ManiphestReplyHandler.php @@ -106,14 +106,14 @@ break; case 'unsubscribe': $is_unsub = true; - $ttype = ManiphestTransaction::TYPE_CCS; - $ccs = $task->getCCPHIDs(); + $ttype = PhabricatorTransactions::TYPE_SUBSCRIBERS; + $ccs = $task->getSubscriberPHIDs(); foreach ($ccs as $k => $phid) { if ($phid == $user->getPHID()) { unset($ccs[$k]); } } - $new_value = array_values($ccs); + $new_value = array('=' => array_values($ccs)); break; } @@ -135,7 +135,7 @@ } $ccs = $mail->loadCCPHIDs(); - $old_ccs = $task->getCCPHIDs(); + $old_ccs = $task->getSubscriberPHIDs(); $new_ccs = array_merge($old_ccs, $ccs); if (!$is_unsub) { $new_ccs[] = $user->getPHID(); @@ -144,8 +144,9 @@ if (array_diff($new_ccs, $old_ccs)) { $cc_xaction = clone $template; - $cc_xaction->setTransactionType(ManiphestTransaction::TYPE_CCS); - $cc_xaction->setNewValue($new_ccs); + $cc_xaction->setTransactionType( + PhabricatorTransactions::TYPE_SUBSCRIBERS); + $cc_xaction->setNewValue(array('=' => $new_ccs)); $xactions[] = $cc_xaction; } diff --git a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php --- a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php @@ -17,6 +17,7 @@ $results = id(new ManiphestTaskQuery()) ->setViewer($viewer) ->withIDs(array($id)) + ->needSubscriberPHIDs(true) ->execute(); return head($results); diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -50,6 +50,8 @@ const ORDER_MODIFIED = 'order-modified'; const ORDER_TITLE = 'order-title'; + private $needSubscriberPHIDs; + const DEFAULT_PAGE_SIZE = 1000; public function withAuthors(array $authors) { @@ -178,6 +180,11 @@ return $this; } + public function needSubscriberPHIDs($bool) { + $this->needSubscriberPHIDs = $bool; + return $this; + } + public function loadPage() { // TODO: (T603) It is possible for a user to find the PHID of a project // they can't see, then query for tasks in that project and deduce the @@ -196,7 +203,6 @@ $where[] = $this->buildPrioritiesWhereClause($conn); $where[] = $this->buildAuthorWhereClause($conn); $where[] = $this->buildOwnerWhereClause($conn); - $where[] = $this->buildSubscriberWhereClause($conn); $where[] = $this->buildProjectWhereClause($conn); $where[] = $this->buildAnyProjectWhereClause($conn); $where[] = $this->buildAnyUserProjectWhereClause($conn); @@ -332,12 +338,14 @@ } protected function didFilterPage(array $tasks) { + $phids = mpull($tasks, 'getPHID'); + // TODO: Eventually, we should make this optional and introduce a // needProjectPHIDs() method, but for now there's a lot of code which // assumes the data is always populated. $edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(mpull($tasks, 'getPHID')) + ->withSourcePHIDs($phids) ->withEdgeTypes( array( PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, @@ -345,8 +353,19 @@ $edge_query->execute(); foreach ($tasks as $task) { - $phids = $edge_query->getDestinationPHIDs(array($task->getPHID())); - $task->attachProjectPHIDs($phids); + $project_phids = $edge_query->getDestinationPHIDs( + array($task->getPHID())); + $task->attachProjectPHIDs($project_phids); + } + + if ($this->needSubscriberPHIDs) { + $subscriber_sets = id(new PhabricatorSubscribersQuery()) + ->withObjectPHIDs($phids) + ->execute(); + foreach ($tasks as $task) { + $subscribers = idx($subscriber_sets, $task->getPHID(), array()); + $task->attachSubscriberPHIDs($subscribers); + } } return $tasks; @@ -497,17 +516,6 @@ $fulltext_results); } - private function buildSubscriberWhereClause(AphrontDatabaseConnection $conn) { - if (!$this->subscriberPHIDs) { - return null; - } - - return qsprintf( - $conn, - 'subscriber.subscriberPHID IN (%Ls)', - $this->subscriberPHIDs); - } - private function buildProjectWhereClause(AphrontDatabaseConnection $conn) { if (!$this->projectPHIDs && !$this->includeNoProject) { return null; @@ -708,11 +716,14 @@ } if ($this->subscriberPHIDs) { - $subscriber_dao = new ManiphestTaskSubscriber(); $joins[] = qsprintf( $conn_r, - 'JOIN %T subscriber ON subscriber.taskPHID = task.phid', - $subscriber_dao->getTableName()); + 'JOIN %T e_ccs ON e_ccs.src = task.phid '. + 'AND e_ccs.type = %s '. + 'AND e_ccs.dst in (%Ls)', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + PhabricatorEdgeConfig::TYPE_OBJECT_HAS_SUBSCRIBER, + $this->subscriberPHIDs); } switch ($this->groupBy) { diff --git a/src/applications/maniphest/search/ManiphestSearchIndexer.php b/src/applications/maniphest/search/ManiphestSearchIndexer.php --- a/src/applications/maniphest/search/ManiphestSearchIndexer.php +++ b/src/applications/maniphest/search/ManiphestSearchIndexer.php @@ -54,21 +54,6 @@ $task->getDateCreated()); } - // We need to load handles here since non-users may subscribe (mailing - // lists, e.g.) - $ccs = $task->getCCPHIDs(); - $handles = id(new PhabricatorHandleQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs($ccs) - ->execute(); - foreach ($ccs as $cc) { - $doc->addRelationship( - PhabricatorSearchRelationship::RELATIONSHIP_SUBSCRIBER, - $handles[$cc]->getPHID(), - $handles[$cc]->getType(), - time()); - } - return $doc; } diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -2,6 +2,7 @@ final class ManiphestTask extends ManiphestDAO implements + PhabricatorSubscribableInterface, PhabricatorMarkupInterface, PhabricatorPolicyInterface, PhabricatorTokenReceiverInterface, @@ -17,7 +18,6 @@ protected $authorPHID; protected $ownerPHID; - protected $ccPHIDs = array(); protected $status; protected $priority; @@ -33,10 +33,10 @@ protected $attached = array(); protected $projectPHIDs = array(); - private $subscribersNeedUpdate; protected $ownerOrdering; + private $subscriberPHIDs = self::ATTACHABLE; private $groupByProjectPHID = self::ATTACHABLE; private $customFields = self::ATTACHABLE; private $edgeProjectPHIDs = self::ATTACHABLE; @@ -56,7 +56,8 @@ ->setAuthorPHID($actor->getPHID()) ->setViewPolicy($view_policy) ->setEditPolicy($edit_policy) - ->attachProjectPHIDs(array()); + ->attachProjectPHIDs(array()) + ->attachSubscriberPHIDs(array()); } public function getConfiguration() { @@ -141,8 +142,8 @@ return PhabricatorPHID::generateNewPHID(ManiphestTaskPHIDType::TYPECONST); } - public function getCCPHIDs() { - return array_values(nonempty($this->ccPHIDs, array())); + public function getSubscriberPHIDs() { + return $this->assertAttached($this->subscriberPHIDs); } public function getProjectPHIDs() { @@ -154,15 +155,13 @@ return $this; } - public function setCCPHIDs(array $phids) { - $this->ccPHIDs = array_values($phids); - $this->subscribersNeedUpdate = true; + public function attachSubscriberPHIDs(array $phids) { + $this->subscriberPHIDs = $phids; return $this; } public function setOwnerPHID($phid) { $this->ownerPHID = nonempty($phid, null); - $this->subscribersNeedUpdate = true; return $this; } @@ -194,13 +193,6 @@ $result = parent::save(); - if ($this->subscribersNeedUpdate) { - // If we've changed the subscriber PHIDs for this task, update the link - // table. - ManiphestTaskSubscriber::updateTaskSubscribers($this); - $this->subscribersNeedUpdate = false; - } - return $result; } @@ -217,6 +209,22 @@ } +/* -( PhabricatorSubscribableInterface )----------------------------------- */ + + + public function isAutomaticallySubscribed($phid) { + return ($phid == $this->getOwnerPHID()); + } + + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + + /* -( Markup Interface )--------------------------------------------------- */ diff --git a/src/applications/maniphest/storage/ManiphestTaskSubscriber.php b/src/applications/maniphest/storage/ManiphestTaskSubscriber.php --- a/src/applications/maniphest/storage/ManiphestTaskSubscriber.php +++ b/src/applications/maniphest/storage/ManiphestTaskSubscriber.php @@ -1,5 +1,8 @@ setViewer($user) ->withPHIDs(array_keys($phids)) + ->needSubscriberPHIDs(true) ->execute(); if (empty($targets)) { @@ -156,9 +157,13 @@ ->setContinueOnMissingFields(true); $cc_vector = array(); - $cc_vector[] = $task->getCCPHIDs(); + // since we loaded this via a generic object query, go ahead and get the + // attach the cc phids now + $task->attachSubscriberPHIDs( + PhabricatorSubscribersQuery::loadSubscribersForPHID($task->getPHID())); + $cc_vector[] = $task->getSubscriberPHIDs(); foreach ($targets as $target) { - $cc_vector[] = $target->getCCPHIDs(); + $cc_vector[] = $target->getSubscriberPHIDs(); $cc_vector[] = array( $target->getAuthorPHID(), $target->getOwnerPHID(), @@ -178,8 +183,8 @@ $all_ccs = array_unique($all_ccs); $add_ccs = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_CCS) - ->setNewValue($all_ccs); + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue(array('=' => $all_ccs)); $merged_from_txn = id(new ManiphestTransaction()) ->setTransactionType(ManiphestTransaction::TYPE_MERGED_FROM) diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsListController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsListController.php --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsListController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsListController.php @@ -23,8 +23,6 @@ if ($object instanceof PhabricatorSubscribableInterface) { $subscriber_phids = PhabricatorSubscribersQuery::loadSubscribersForPHID( $phid); - } else if ($object instanceof ManiphestTask) { - $subscriber_phids = $object->getCCPHIDs(); } $handle_phids = $subscriber_phids;