revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-03-19 07:40:09 +01:00
Code Issues Releases Wiki Activity

Maniphest - use subscribers framework properly

Browse source 
Summary: Fixes T5604. This should fix some random bugs, lets us move forward more easily, and all that good stuff about killing code debt.

Test Plan:
- Conduit method maniphest.createtask
  - verified creating user subscribed
  - verified subscription transaction
- Conduit method maniphest.update
  - verified subscribers set as specified to ccPHIDs parameter
  - verified subscription transaction
- Herald
  - verified herald rule to add subscriber worked
  - verified no subscribers removed accidentally
- edit controller
  - test create and verify author gets added IFF they put themselves in subscribers control box
  - test update gets set to exactly what user enters
- lipsum generator'd tasks work
- bulk add subscribers works
- bulk remove subscriber works
- detail controller
  - added myself by leaving a comment
  - added another user via explicit action
  - added another user via implicit mention
- task merge via search attach controller
- mail reply handler
  - add subscriber via ./bin/mail receive-test
  - unsubscribe via ./bin/mail receive-test

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T5604

Differential Revision: https://secure.phabricator.com/D10965
This commit is contained in:
Bob Trahan 2014-12-10 16:27:30 -08:00
parent 3297bc2ed9
commit 7d96870570
27 changed files with 195 additions and 230 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
resources/sql/autopatches/20141210.maniphestsubscribersmig.1.sql Normal file
View file

@ -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 != '';

2
resources/sql/autopatches/20141210.maniphestsubscribersmig.2.sql Normal file
View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task
DROP ccPHIDs;

3
src/__phutil_library_map__.php
View file

@ -1001,7 +1001,6 @@ phutil_register_library_map(array(
'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',
@ -4071,9 +4070,9 @@ phutil_register_library_map(array(
'ManiphestSearchIndexer' => 'PhabricatorSearchDocumentIndexer',
'ManiphestStatusConfigOptionType' => 'PhabricatorConfigJSONOptionType',
'ManiphestSubpriorityController' => 'ManiphestController',
'ManiphestSubscribeController' => 'ManiphestController',
'ManiphestTask' => array(
'ManiphestDAO',
'PhabricatorSubscribableInterface',
'PhabricatorMarkupInterface',
'PhabricatorPolicyInterface',
'PhabricatorTokenReceiverInterface',

2
src/applications/herald/adapter/HeraldManiphestTaskAdapter.php
View file

@ -139,7 +139,7 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
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(),

2
src/applications/maniphest/application/PhabricatorManiphestApplication.php
View file

@ -66,8 +66,6 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication {
),
'export/(?P<key>[^/]+)/' => 'ManiphestExportController',
'subpriority/' => 'ManiphestSubpriorityController',
'subscribe/(?P<action>add|rem)/(?P<id>[1-9]\d*)/'
=> 'ManiphestSubscribeController',
),
);
}

14
src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php
View file

@ -62,6 +62,8 @@ abstract class ManiphestConduitAPIMethod extends ConduitAPIMethod {
$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 @@ abstract class ManiphestConduitAPIMethod extends ConduitAPIMethod {
$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 @@ abstract class ManiphestConduitAPIMethod extends ConduitAPIMethod {
$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 @@ abstract class ManiphestConduitAPIMethod extends ConduitAPIMethod {
'phid' => $task->getPHID(),
'authorPHID' => $task->getAuthorPHID(),
'ownerPHID' => $task->getOwnerPHID(),
'ccPHIDs' => $task->getCCPHIDs(),
'ccPHIDs' => $task->getSubscriberPHIDs(),
'status' => $task->getStatus(),
'statusName' => ManiphestTaskStatus::getTaskStatusName(
$task->getStatus()),

2
src/applications/maniphest/conduit/ManiphestCreateTaskConduitAPIMethod.php
View file

@ -28,7 +28,7 @@ final class ManiphestCreateTaskConduitAPIMethod
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);
}

1
src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php
View file

@ -32,6 +32,7 @@ final class ManiphestInfoConduitAPIMethod extends ManiphestConduitAPIMethod {
$task = id(new ManiphestTaskQuery())
->setViewer($request->getUser())
->withIDs(array($task_id))
->needSubscriberPHIDs(true)
->executeOne();
if (!$task) {
throw new ConduitException('ERR_BAD_TASK');

1
src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php
View file

@ -64,6 +64,7 @@ class ManiphestQueryConduitAPIMethod extends ManiphestConduitAPIMethod {
$query = new ManiphestTaskQuery();
$query->setViewer($request->getUser());
$query->needSubscriberPHIDs(true);
$task_ids = $request->getValue('ids');
if ($task_ids) {

4
src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php
View file

@ -38,11 +38,13 @@ final class ManiphestUpdateConduitAPIMethod extends ManiphestConduitAPIMethod {
$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 @@ final class ManiphestUpdateConduitAPIMethod extends ManiphestConduitAPIMethod {
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);
}

51
src/applications/maniphest/controller/ManiphestBatchEditController.php
View file

@ -18,6 +18,7 @@ final class ManiphestBatchEditController extends ManiphestController {
PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT,
))
->needSubscriberPHIDs(true)
->execute();
$actions = $request->getStr('actions');
@ -171,8 +172,8 @@ final class ManiphestBatchEditController extends ManiphestController {
'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 @@ final class ManiphestBatchEditController extends ManiphestController {
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 @@ final class ManiphestBatchEditController extends ManiphestController {
continue 2;
}
break;
case ManiphestTransaction::TYPE_CCS:
case PhabricatorTransactions::TYPE_SUBSCRIBERS:
if (empty($value)) {
continue 2;
}
@ -274,12 +275,7 @@ final class ManiphestBatchEditController extends ManiphestController {
}
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);
@ -308,6 +304,39 @@ final class ManiphestBatchEditController extends ManiphestController {
}
$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;
}

51
src/applications/maniphest/controller/ManiphestSubscribeController.php
View file

@ -1,51 +0,0 @@
<?php
final class ManiphestSubscribeController extends ManiphestController {
private $id;
private $action;
public function willProcessRequest(array $data) {
$this->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());
}
}

60
src/applications/maniphest/controller/ManiphestTaskDetailController.php
View file

@ -23,6 +23,7 @@ final class ManiphestTaskDetailController extends ManiphestController {
$task = id(new ManiphestTaskQuery())
->setViewer($user)
->withIDs(array($this->id))
->needSubscriberPHIDs(true)
->executeOne();
if (!$task) {
return new Aphront404Response();
@ -65,12 +66,6 @@ final class ManiphestTaskDetailController extends ManiphestController {
$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 @@ final class ManiphestTaskDetailController extends ManiphestController {
$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 @@ final class ManiphestTaskDetailController extends ManiphestController {
->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 @@ final class ManiphestTaskDetailController extends ManiphestController {
'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 @@ final class ManiphestTaskDetailController extends ManiphestController {
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 @@ final class ManiphestTaskDetailController extends ManiphestController {
->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 @@ final class ManiphestTaskDetailController extends ManiphestController {
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());

19
src/applications/maniphest/controller/ManiphestTaskEditController.php
View file

@ -38,6 +38,7 @@ final class ManiphestTaskEditController extends ManiphestController {
PhabricatorPolicyCapability::CAN_EDIT,
))
->withIDs(array($this->id))
->needSubscriberPHIDs(true)
->executeOne();
if (!$task) {
return new Aphront404Response();
@ -215,7 +216,7 @@ final class ManiphestTaskEditController extends ManiphestController {
$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 @@ final class ManiphestTaskEditController extends ManiphestController {
$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 @@ final class ManiphestTaskEditController extends ManiphestController {
}
} 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 @@ final class ManiphestTaskEditController extends ManiphestController {
$phids = array_merge(
array($task->getOwnerPHID()),
$task->getCCPHIDs(),
$task->getSubscriberPHIDs(),
$task->getProjectPHIDs());
if ($parent_task) {
@ -512,8 +515,8 @@ final class ManiphestTaskEditController extends ManiphestController {
$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();
}

13
src/applications/maniphest/controller/ManiphestTransactionPreviewController.php
View file

@ -58,22 +58,19 @@ final class ManiphestTransactionPreviewController extends ManiphestController {
}
$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) {

15
src/applications/maniphest/controller/ManiphestTransactionSaveController.php
View file

@ -9,6 +9,7 @@ final class ManiphestTransactionSaveController extends ManiphestController {
$task = id(new ManiphestTaskQuery())
->setViewer($user)
->withIDs(array($request->getStr('taskID')))
->needSubscriberPHIDs(true)
->executeOne();
if (!$task) {
return new Aphront404Response();
@ -33,7 +34,7 @@ final class ManiphestTransactionSaveController extends ManiphestController {
$cc_transaction = new ManiphestTransaction();
$cc_transaction
->setTransactionType(ManiphestTransaction::TYPE_CCS);
->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS);
$transaction = new ManiphestTransaction();
$transaction
@ -64,7 +65,7 @@ final class ManiphestTransactionSaveController extends ManiphestController {
'+' => 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 @@ final class ManiphestTransactionSaveController extends ManiphestController {
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;
}

31
src/applications/maniphest/editor/ManiphestTransactionEditor.php
View file

@ -23,7 +23,6 @@ final class ManiphestTransactionEditor
$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 @@ final class ManiphestTransactionEditor
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 @@ final class ManiphestTransactionEditor
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 @@ final class ManiphestTransactionEditor
$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 @@ final class ManiphestTransactionEditor
}
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 @@ final class ManiphestTransactionEditor
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 @@ final class ManiphestTransactionEditor
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())

4
src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php
View file

@ -28,8 +28,8 @@ final class PhabricatorManiphestTaskTestDataGenerator
$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;

13
src/applications/maniphest/mail/ManiphestReplyHandler.php
View file

@ -106,14 +106,14 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler {
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 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler {
}
$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 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler {
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;
}

1
src/applications/maniphest/mail/ManiphestTaskMailReceiver.php
View file

@ -17,6 +17,7 @@ final class ManiphestTaskMailReceiver extends PhabricatorObjectMailReceiver {
$results = id(new ManiphestTaskQuery())
->setViewer($viewer)
->withIDs(array($id))
->needSubscriberPHIDs(true)
->execute();
return head($results);

47
src/applications/maniphest/query/ManiphestTaskQuery.php
View file

@ -50,6 +50,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
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 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
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 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
$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 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
}
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 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
$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 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
$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 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
}
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) {

15
src/applications/maniphest/search/ManiphestSearchIndexer.php
View file

@ -54,21 +54,6 @@ final class ManiphestSearchIndexer extends PhabricatorSearchDocumentIndexer {
$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;
}

40
src/applications/maniphest/storage/ManiphestTask.php
View file

@ -2,6 +2,7 @@
final class ManiphestTask extends ManiphestDAO
implements
PhabricatorSubscribableInterface,
PhabricatorMarkupInterface,
PhabricatorPolicyInterface,
PhabricatorTokenReceiverInterface,
@ -17,7 +18,6 @@ final class ManiphestTask extends ManiphestDAO
protected $authorPHID;
protected $ownerPHID;
protected $ccPHIDs = array();
protected $status;
protected $priority;
@ -33,10 +33,10 @@ final class ManiphestTask extends ManiphestDAO
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 @@ final class ManiphestTask extends ManiphestDAO
->setAuthorPHID($actor->getPHID())
->setViewPolicy($view_policy)
->setEditPolicy($edit_policy)
->attachProjectPHIDs(array());
->attachProjectPHIDs(array())
->attachSubscriberPHIDs(array());
}
public function getConfiguration() {
@ -141,8 +142,8 @@ final class ManiphestTask extends ManiphestDAO
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 @@ final class ManiphestTask extends ManiphestDAO
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 @@ final class ManiphestTask extends ManiphestDAO
$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 @@ final class ManiphestTask extends ManiphestDAO
}
/* -( PhabricatorSubscribableInterface )----------------------------------- */
public function isAutomaticallySubscribed($phid) {
return ($phid == $this->getOwnerPHID());
}
public function shouldShowSubscribersProperty() {
return true;
}
public function shouldAllowSubscription($phid) {
return true;
}
/* -( Markup Interface )--------------------------------------------------- */

3
src/applications/maniphest/storage/ManiphestTaskSubscriber.php
View file

@ -1,5 +1,8 @@
<?php
/**
* Deprecated; delete me.
*/
final class ManiphestTaskSubscriber extends ManiphestDAO {
protected $taskPHID;

7
src/applications/maniphest/storage/ManiphestTransaction.php
View file

@ -7,7 +7,6 @@ final class ManiphestTransaction
const TYPE_STATUS = 'status';
const TYPE_DESCRIPTION = 'description';
const TYPE_OWNER = 'reassign';
const TYPE_CCS = 'ccs';
const TYPE_PROJECTS = 'projects';
const TYPE_PRIORITY = 'priority';
const TYPE_EDGE = 'edge';
@ -21,6 +20,12 @@ final class ManiphestTransaction
// NOTE: this type is deprecated. Keep it around for legacy installs
// so any transactions render correctly.
const TYPE_ATTACH = 'attach';
/**
* TYPE_CCS is legacy and depracted in favor of
* PhabricatorTransactions::TYPE_SUBSCRIBERS; keep it around for legacy
* transaction-rendering.
*/
const TYPE_CCS = 'ccs';
const MAILTAG_STATUS = 'maniphest-status';

13
src/applications/search/controller/PhabricatorSearchAttachController.php
View file

@ -143,6 +143,7 @@ final class PhabricatorSearchAttachController
$targets = id(new ManiphestTaskQuery())
->setViewer($user)
->withPHIDs(array_keys($phids))
->needSubscriberPHIDs(true)
->execute();
if (empty($targets)) {
@ -156,9 +157,13 @@ final class PhabricatorSearchAttachController
->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 @@ final class PhabricatorSearchAttachController
$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)

2
src/applications/subscriptions/controller/PhabricatorSubscriptionsListController.php
View file

@ -23,8 +23,6 @@ final class PhabricatorSubscriptionsListController
if ($object instanceof PhabricatorSubscribableInterface) {
$subscriber_phids = PhabricatorSubscribersQuery::loadSubscribersForPHID(
$phid);
} else if ($object instanceof ManiphestTask) {
$subscriber_phids = $object->getCCPHIDs();
}
$handle_phids = $subscriber_phids;