Modularize workboard column orders
ClosedPublic
Actions

Authored by epriestley on Mar 11 2019, 4:56 AM.

Details

Reviewers

amckinley

Maniphest Tasks

T10333: Add groups to workboard columns

Commits

rP9a8019d4a98b: Modularize workboard column orders

Summary

Depends on D20267. Depends on D20268. Ref T10333. Currently, we support "Natural" and "Priority" orders, but a lot of the particulars are pretty hard-coded, including some logic in ManiphestTask.

Although it's not clear that we'll ever put other types of objects on workboards, it seems generally bad that you need to modify ManiphestTask to get a new ordering.

Pull the ordering logic out into a ProjectColumnOrder hierarchy instead, and let each ordering define the things it needs to work (name, icon, what headers look like, how different objects are sorted, and how to apply an edit when you drop an object under a header).

Then move the existing "Natural" and "Priority" orders into this new hierarchy.

This has a minor bug where using the "Edit" workflow to change a card's priority on a priority-ordered board doesn't fully refresh card/header order since the response isn't ordering-aware. I'll fix that in an upcoming change.

Test Plan

Grouped workboards by "Natural" and "Priority", dragged stuff around within and between columns, grepped for all touched symbols.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Mar 11 2019, 4:56 AM

Harbormaster completed remote builds in B22247: Diff 48384.Mar 11 2019, 4:58 AM

epriestley requested review of this revision.Mar 11 2019, 4:58 AM

epriestley added a child revision: D20270: Implement "Group by Owner" on Workboards.Mar 11 2019, 5:01 AM

amckinley accepted this revision.Mar 12 2019, 7:47 PM

amckinley added inline comments.

src/applications/maniphest/storage/ManiphestTask.php
254	Just because this looked like a typo while scrolling... This is the same as `(int) (-1 * $this->getPriority())`, right?
274	No cast to `int` here?
src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php
31	As above, I slightly prefer `(-1 * $priority)`, but this is fine.

This revision is now accepted and ready to land.Mar 12 2019, 7:47 PM

epriestley added inline comments.Mar 12 2019, 7:55 PM

src/applications/maniphest/storage/ManiphestTask.php
254	Yeah. Some future diff swaps this to `-(int)X` which is arguably more intuitive than `(int)-X` even though I think they both do the same thing for all inputs since the unary `-` implies numeric cast. The `(int)` in `(int)-X` isn't technically redundant because `-"1.1"` has type `double`. This is all ultimately rooted in T12678. Basically: kind of a goofy mess, cleaned up slightly in future changes, means the same thing as `(int)((int)-1 * (int)$x)`, all the tricky cases end up working out for all actual inputs as long as `(int)` is in there somewhere.
274	(This was just being used to build a `"priority(123)"` string so the cast didn't actually matter, and I got rid of it in a future change by consolidating the string construction in PHP instead of spreading it out between PHP and JS.)