Fixes T4408. I had to add a "status" to colum. I think we'll need this once we get fancier anyway but for now we have "active" and deleted.
Details
- Reviewers
epriestley - Maniphest Tasks
- Restricted Maniphest Task
- Commits
- Restricted Diffusion Commit
rP809e5a038985: Workboards - let users delete columns
deleted a column. noted reloaded workboard with all those tasks back in the default colun. loaded a task and saw the initial transaction had a "Disabled" icon next to the deleted workboard. also saw the new transaction back to the default column worked.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
whoops, I need to add some withStatus code to the ColumnQuery class, make sure deleted columns aren't leaking anywhere, etc
I think I have at least a little bit of meat here.
Particularly, I think we should just respect deleted columns when sequencing columns, and are going to run into trouble if we don't. Here are two cases where I think we get into trouble, specifically:
Case 1: Delete Twice
- Create column (say, sequence = 5).
- Delete it.
- Create column (sequence = 5 again).
- Try to delete it.
- Unique key violation?
Case 2: Undelete
- (You can't do this today, but probably should be able to some day.)
- Create column (say, sequence = 5).
- Delete it.
- Create column (sequence = 5 again).
- Undelete the first column.
- Unique key violation?
If we just respect deleted columns when sequencing, the only trouble we run into is if a board has like 10,000 deleted columns, I think, since reordering stuff will take a little more effort then. I can't come up with any reason why having "holes" in the sequence for deleted columns is bad, either. Am I missing cases where we run into trouble?
resources/sql/autopatches/20140314.projectcolumn.1.statuscol.sql | ||
---|---|---|
3 | You should be able to emit COLLATE ... on an INT column. | |
resources/sql/autopatches/20140314.projectcolumn.2.statuskey.sql | ||
2–6 | Hmm, this is weird to me. I'd expect that an archived column keeps its sequence, and that it's never possible to have two columns with sequence, say, 15. Let me look at the rest of the diff... | |
src/applications/project/controller/PhabricatorProjectBoardController.php | ||
33 | Let's make this withStatuses(). I've generally been having better luck building UIs and Conduit around that. Particularly, users and UIs often want to get all "open" or "closed" things, but this can correspond to several statuses. So we either need to define magic statuses (which we used to do, but which I think is super gross/messy) or have a multi-value API on the Query (which I've been doing more recently, and which I think works better). This has also made it easy to move advanced search to just use a list of checkboxes, which is easy for users to understand and works like everyone expects. | |
src/applications/project/controller/PhabricatorProjectBoardDeleteController.php | ||
4–5 | I think this is OK for now, but in the mid term we should move away from it and have a contextual delete action. Specifically:
| |
49–51 | We should just call isDefaultColumn() on each column instead of doing logic here, we might have non-first default columns in the future. Or, add withIsDefault(false) to the Query or something. | |
60–92 | I'm hesitant about putting this in the web UI, although we'll probably need "move all..." eventually. Particularly, if a column has like 10,000 tasks, this will take a long-ass time and probably timeout and fail. This will also send a ton of email. Finally, it's not a great default action -- it's pretty reasonable, but often it's probably not really what the user would prefer. A much easier alternative which has fewer problems is to prevent users from deleting columns which contain tasks -- just fail with "This column still has some tasks in it. Before you can delete a column, you must move all the tasks out of it.". This is a little more cumbersome, but way way simpler, and maybe a better approach? | |
src/applications/project/controller/PhabricatorProjectBoardEditController.php | ||
74 ↗ | (On Diff #20273) | Could we just leave this as-is, and still consider deleted columns, so there's always exactly one sequence over all the columns? |
updates as requested EXCEPT the "delete column" action is still there, etc. I think my next diff will be to give columns their own detail page w/ transactions and clean this bit up.
also kill the "WARNING" on the form since it no longer applies (you must now remove all tasks before you can delete a column)
One inline on the .sql.
resources/sql/autopatches/20140314.projectcolumn.2.statuskey.sql | ||
---|---|---|
2–6 | This should probably just add a non-unique key now, not drop + replace? |