Page MenuHomePhabricator

Workboards - let users delete columns
ClosedPublic

Authored by btrahan on Mar 15 2014, 12:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 27, 9:43 PM
Unknown Object (File)
Sat, Apr 27, 9:43 PM
Unknown Object (File)
Sat, Apr 27, 9:42 PM
Unknown Object (File)
Sat, Apr 27, 9:42 PM
Unknown Object (File)
Sat, Apr 27, 9:01 PM
Unknown Object (File)
Wed, Apr 24, 10:40 PM
Unknown Object (File)
Thu, Apr 11, 3:52 PM
Unknown Object (File)
Thu, Apr 11, 1:27 AM
Subscribers

Details

Reviewers
epriestley
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP809e5a038985: Workboards - let users delete columns
Summary

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.

Test Plan

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

btrahan added a task: Restricted Maniphest Task.
btrahan retitled this revision from to Workboards - let users delete columns.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

whoops, I need to add some withStatus code to the ColumnQuery class, make sure deleted columns aren't leaking anywhere, etc

withStatus to query and appropriate deployment of such

epriestley edited edge metadata.

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:

  • The "gear" icon ('edit column') should be called "Column Settings" or something and take you to a standard "column details" page, instead of directly to an edit form.
  • The "column details" page would have an timeline/edit history once we use applicationtransactions (e.g. who deleted the column and when? who renamed it and when?), detailed information, and then actions ("Edit Column", "Delete Column").
  • The current "Edit Column" page would be behind the "Edit Column" link.
  • A new confirm dialog would be behind the "Delete Column" link.
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?

This revision now requires changes to proceed.Mar 18 2014, 2:53 PM
btrahan edited edge metadata.

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.

btrahan edited edge metadata.

also kill the "WARNING" on the form since it no longer applies (you must now remove all tasks before you can delete a column)

epriestley edited edge metadata.

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?

This revision is now accepted and ready to land.Mar 18 2014, 5:34 PM
btrahan edited edge metadata.

drop + replace unique key => add non unique key

btrahan updated this revision to Diff 20321.

Closed by commit rP809e5a038985 (authored by @btrahan).