Changeset View
Standalone View
src/applications/maniphest/controller/ManiphestTransactionSaveController.php
Show All 15 Lines | if (!$task) { | ||||
return new Aphront404Response(); | return new Aphront404Response(); | ||||
} | } | ||||
$task_uri = '/'.$task->getMonogram(); | $task_uri = '/'.$task->getMonogram(); | ||||
$transactions = array(); | $transactions = array(); | ||||
$action = $request->getStr('action'); | $action = $request->getStr('action'); | ||||
// Compute new CCs added by @mentions. Several things can cause CCs to | $added_ccs = array(); | ||||
epriestley: Replace this code with `$added_ccs = array();`, then... | |||||
// be added as side effects: mentions, explicit CCs, users who aren't | |||||
// CC'd interacting with the task, and ownership changes. We build up a | |||||
// list of all the CCs and then construct a transaction for them at the | |||||
// end if necessary. | |||||
$added_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions( | |||||
$user, | |||||
array( | |||||
$request->getStr('comments'), | |||||
)); | |||||
$cc_transaction = new ManiphestTransaction(); | $cc_transaction = new ManiphestTransaction(); | ||||
$cc_transaction | $cc_transaction | ||||
->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); | ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); | ||||
$transaction = new ManiphestTransaction(); | $transaction = new ManiphestTransaction(); | ||||
$transaction | $transaction | ||||
->setTransactionType($action); | ->setTransactionType($action); | ||||
Show All 19 Lines | switch ($action) { | ||||
->setNewValue( | ->setNewValue( | ||||
array( | array( | ||||
'+' => array_fuse($projects), | '+' => array_fuse($projects), | ||||
)); | )); | ||||
break; | break; | ||||
case PhabricatorTransactions::TYPE_SUBSCRIBERS: | case PhabricatorTransactions::TYPE_SUBSCRIBERS: | ||||
// Accumulate the new explicit CCs into the array that we'll add in | // Accumulate the new explicit CCs into the array that we'll add in | ||||
// the CC transaction later. | // the CC transaction later. | ||||
$added_ccs = array_merge($added_ccs, $request->getArr('ccs')); | $added_ccs = $request->getArr('ccs'); | ||||
Not Done Inline ActionsChange this to $added_ccs = $request->getArr('ccs');. epriestley: Change this to `$added_ccs = $request->getArr('ccs');`. | |||||
// Throw away the primary transaction. | // Throw away the primary transaction. | ||||
$transaction = null; | $transaction = null; | ||||
break; | break; | ||||
case ManiphestTransaction::TYPE_PRIORITY: | case ManiphestTransaction::TYPE_PRIORITY: | ||||
$transaction->setNewValue($request->getInt('priority')); | $transaction->setNewValue($request->getInt('priority')); | ||||
break; | break; | ||||
case PhabricatorTransactions::TYPE_COMMENT: | case PhabricatorTransactions::TYPE_COMMENT: | ||||
▲ Show 20 Lines • Show All 66 Lines • ▼ Show 20 Lines | public function processRequest() { | ||||
$added_ccs = array_filter(array_diff( | $added_ccs = array_filter(array_diff( | ||||
$added_ccs, | $added_ccs, | ||||
$task->getSubscriberPHIDs())); | $task->getSubscriberPHIDs())); | ||||
if ($added_ccs) { | if ($added_ccs) { | ||||
// We've added CCs, so include a CC transaction. | // We've added CCs, so include a CC transaction. | ||||
$all_ccs = array_merge($task->getSubscriberPHIDs(), $added_ccs); | $all_ccs = array_merge($task->getSubscriberPHIDs(), $added_ccs); | ||||
$cc_transaction->setNewValue(array('=' => $all_ccs)); | $cc_transaction->setNewValue(array('=' => $all_ccs)); | ||||
$transactions[] = $cc_transaction; | $transactions[] = $cc_transaction; | ||||
} | } | ||||
Not Done Inline ActionsI think this should be unnecessary. Even though this code isn't fully modernized, it ultimately goes through the PhabricatorApplicationTransactionEditor pathway, so it should hit the other change. Or was that not the case? Oh, is this handling "Add CCs: ..." explicitly? Let's just do mentions first in this diff, I think the various ways to "Add CCs:" will be a bit more complicated. See note later on. epriestley: I //think// this should be unnecessary. Even though this code isn't fully modernized, it… | |||||
Not Done Inline ActionsIt just seems to be used when commenting on a task. If i remove i can comment an mention users having no permission and they will get added. fabe: It just seems to be used when commenting on a task. If i remove i can comment an mention users… | |||||
Not Done Inline ActionsLet's not do this yet -- this prevents explicit CC's, right? We can move that to another diff later on. epriestley: Let's not do this yet -- this prevents explicit CC's, right? We can move that to another diff… | |||||
Not Done Inline ActionsNo this is actually needed for checking the policy when commenting on a task. fabe: No this is actually needed for checking the policy when commenting on a task.
Every other place… | |||||
$comments = $request->getStr('comments'); | $comments = $request->getStr('comments'); | ||||
if (strlen($comments) || !$transactions) { | if (strlen($comments) || !$transactions) { | ||||
$transactions[] = id(new ManiphestTransaction()) | $transactions[] = id(new ManiphestTransaction()) | ||||
->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) | ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) | ||||
->attachComment( | ->attachComment( | ||||
id(new ManiphestTransactionComment()) | id(new ManiphestTransactionComment()) | ||||
->setContent($comments)); | ->setContent($comments)); | ||||
Show All 15 Lines | public function processRequest() { | ||||
$editor = id(new ManiphestTransactionEditor()) | $editor = id(new ManiphestTransactionEditor()) | ||||
->setActor($user) | ->setActor($user) | ||||
->setContentSourceFromRequest($request) | ->setContentSourceFromRequest($request) | ||||
->setContinueOnMissingFields(true) | ->setContinueOnMissingFields(true) | ||||
->setContinueOnNoEffect($request->isContinueRequest()); | ->setContinueOnNoEffect($request->isContinueRequest()); | ||||
try { | try { | ||||
$editor->applyTransactions($task, $transactions); | $editor->applyTransactions($task, $transactions); | ||||
Not Done Inline ActionsI still don't think the block above is required -- we'll go into the transaction editor here, and it seems to works fine locally for me. When this block is removed I can still "Add CCs", but that's expected and desirable (for now). @mentioning users correctly greys out the ones who can't see the object. Am I just doing something wrong? epriestley: I still don't think the block above is required -- we'll go into the transaction editor here… | |||||
} catch (PhabricatorApplicationTransactionNoEffectException $ex) { | } catch (PhabricatorApplicationTransactionNoEffectException $ex) { | ||||
return id(new PhabricatorApplicationTransactionNoEffectResponse()) | return id(new PhabricatorApplicationTransactionNoEffectResponse()) | ||||
->setCancelURI($task_uri) | ->setCancelURI($task_uri) | ||||
->setException($ex); | ->setException($ex); | ||||
} | } | ||||
$draft = id(new PhabricatorDraft())->loadOneWhere( | $draft = id(new PhabricatorDraft())->loadOneWhere( | ||||
'authorPHID = %s AND draftKey = %s', | 'authorPHID = %s AND draftKey = %s', | ||||
Show All 21 Lines |
Replace this code with $added_ccs = array();, then...