Page MenuHomePhabricator

Workboards - make priority changes less aggressive and generally better
ClosedPublic

Authored by btrahan on Mar 26 2014, 10:51 PM.

Details

Summary

Fixes T4641.

Test Plan

Dragged a "normal" task between "high" and "low" tasks and it stayed as "normal". Generally seems correct when playing around.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

btrahan retitled this revision from to Workboards - make priority changes less aggressive and generally better.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.

Possibly a couple of weird edge cases in line, but I didn't actually play with it yet and might be making things up.

src/applications/project/controller/PhabricatorProjectMoveController.php
95

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

103

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.

120โ€“124

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.

This revision is now accepted and ready to land.Mar 26 2014, 10:59 PM
btrahan edited edge metadata.

tighten subpriority case

btrahan requested a review of this revision.Mar 27 2014, 1:09 AM
btrahan edited edge metadata.

Can you take another gander? I fond the subpriority stuff to be tricky.

src/applications/maniphest/editor/ManiphestTransactionEditor.php
445โ€“463

I feel low confidence here despite this seeming to work now in testing.

epriestley edited edge metadata.

That seems like it's OK to me, and I don't see any obvious ways to simplify it. It would also be kind of a pain in the butt to unit test, although we could do it with a fixture. Let's just go with this, and if we hit issues we can put some tests on it to cover whatever weird cases we end up hitting.

This revision is now accepted and ready to land.Mar 27 2014, 1:17 AM

(The subpriority stuff is kind of hacky anyway, and if this does hit issues it might be best to try to refactor it and come up with something a little better. Particularly, the float loses precision after something like 17 drag operations, so if you have "ABC" and repeatedly drag the top one to the middle over and over again, everything eventually stops making sense.)

btrahan updated this revision to Diff 20459.

Closed by commit rPde2da8355b8f (authored by @btrahan).