Page MenuHomePhabricator
Differential D8622 Diff 20459 src/applications/project/controller/PhabricatorProjectMoveController.php

Changeset View

Standalone View

View Options

src/applications/project/controller/PhabricatorProjectMoveController.php

Show All 10 Linesfinal class PhabricatorProjectMoveController
public function processRequest() { public function processRequest() {
$request = $this->getRequest(); $request = $this->getRequest();
$viewer = $request->getUser(); $viewer = $request->getUser();
$column_phid = $request->getStr('columnPHID'); $column_phid = $request->getStr('columnPHID');
$object_phid = $request->getStr('objectPHID'); $object_phid = $request->getStr('objectPHID');
$after_phid = $request->getStr('afterPHID'); $after_phid = $request->getStr('afterPHID');
$before_phid = $request->getStr('beforePHID');
$project = id(new PhabricatorProjectQuery()) $project = id(new PhabricatorProjectQuery())
->setViewer($viewer) ->setViewer($viewer)
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
▲ Show 20 Lines • Show All 46 Lines • ▼ Show 20 Lines $xactions[] = id(new ManiphestTransaction())
->setTransactionType(ManiphestTransaction::TYPE_PROJECT_COLUMN) ->setTransactionType(ManiphestTransaction::TYPE_PROJECT_COLUMN)
->setNewValue(array( ->setNewValue(array(
'columnPHIDs' => array($column->getPHID()), 'columnPHIDs' => array($column->getPHID()),
'projectPHID' => $column->getProjectPHID())) 'projectPHID' => $column->getProjectPHID()))
->setOldValue(array( ->setOldValue(array(
'columnPHIDs' => $edge_phids, 'columnPHIDs' => $edge_phids,
'projectPHID' => $column->getProjectPHID())); 'projectPHID' => $column->getProjectPHID()));
$task_phids = array();
if ($after_phid) { if ($after_phid) {
$after_task = id(new ManiphestTaskQuery()) $task_phids[] = $after_phid;
}
if ($before_phid) {
$task_phids[] = $before_phid;
}
if ($task_phids) {
$tasks = id(new ManiphestTaskQuery())
->setViewer($viewer) ->setViewer($viewer)
->withPHIDs(array($after_phid)) ->withPHIDs($task_phids)
->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT)) ->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT))
->executeOne(); ->execute();
if (!$after_task) { if (count($tasks) != count($task_phids)) {
epriestleyUnsubmitted
Not Done
Inline Actions

This should maybe be if (count($tasks) != count($task_phids))? That is, if we get one of the tasks but not the other, 404?

epriestley: This should maybe be `if (count($tasks) != count($task_phids))`? That is, if we get one of the…
return new Aphront404Response(); return new Aphront404Response();
} }
$after_pri = $after_task->getPriority(); $tasks = mpull($tasks, null, 'getPHID');
$after_sub = $after_task->getSubpriority();
$a_task = idx($tasks, $after_phid);
$b_task = idx($tasks, $before_phid);
if ($a_task &&
epriestleyUnsubmitted
Not Done
Inline Actions

I think these two conditions might need to be a little more complex -- something like:

if ((task.priority < object.priority) ||
    (task.priority == object.priority && task.subpriority < object.subpriority)) {

e.g., if we drag a "medium" task below another "medium" task, we might still need to update the subpriority even if we don't update the priority.

epriestley: I think these two conditions might need to be a little more complex -- something like: if…
(($a_task->getPriority() < $object->getPriority()) ||
($a_task->getPriority() == $object->getPriority() &&
$a_task->getSubpriority() >= $object->getSubpriority()))) {
$after_pri = $a_task->getPriority();
$after_sub = $a_task->getSubpriority();
$xactions[] = id(new ManiphestTransaction()) $xactions[] = id(new ManiphestTransaction())
->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
->setNewValue(array( ->setNewValue(array(
'newPriority' => $after_pri, 'newPriority' => $after_pri,
'newSubpriorityBase' => $after_sub)); 'newSubpriorityBase' => $after_sub,
'direction' => '>'));
} else if ($b_task &&
(($b_task->getPriority() > $object->getPriority()) ||
($b_task->getPriority() == $object->getPriority() &&
$b_task->getSubpriority() <= $object->getSubpriority()))) {
$before_pri = $b_task->getPriority();
$before_sub = $b_task->getSubpriority();
epriestleyUnsubmitted
Not Done
Inline Actions

I would expect one of these two transactions to end up with the dropped tasks on the wrong side of the adjacent task? e.g., you drag "X" between "A" and "B", and end up with "XAB" or "ABX", with the right priority but the wrong subpriority? This transaction doesn't know whether the subpriority should be smaller or larger than the base.

epriestley: I would expect one of these two transactions to end up with the dropped tasks on the wrong side…
$xactions[] = id(new ManiphestTransaction())
->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
->setNewValue(array(
'newPriority' => $before_pri,
'newSubpriorityBase' => $before_sub,
'direction' => '<'));
}
} }
$editor = id(new ManiphestTransactionEditor()) $editor = id(new ManiphestTransactionEditor())
->setActor($viewer) ->setActor($viewer)
->setContinueOnMissingFields(true) ->setContinueOnMissingFields(true)
->setContinueOnNoEffect(true) ->setContinueOnNoEffect(true)
->setContentSourceFromRequest($request); ->setContentSourceFromRequest($request);
$editor->applyTransactions($object, $xactions); $editor->applyTransactions($object, $xactions);
Show All 20 Lines
Phabricator · Built by Phacility