Page MenuHomePhabricator

Move workboard "filter" workflow to a separate controller
ClosedPublic

Authored by epriestley on Tue, Jul 2, 12:30 PM.

Details

Summary

Depends on D20629. Ref T4900. Currently, the "Advanced Filter..." workflow on workboards (where you build a custom query) is inline in the main board controller.

This is because the filter flow depends on some of the board view state: we want to start with the current filter applied to the board, and preserve other state after you change the filter.

Now that ViewState can handle state management, we can separate this stuff out pretty easily.

Test Plan
  • Changed filters on a board.
  • Applied a custom filter to a board.
  • Changed the ordering of a board, then applied a custom filter. Verified "Cancel" and "Apply Filter" both preserve the order state.
  • Changed the ordering of a board, then applied a custom filter, intentionally making a mistake in configuring the filter by entering an invalid date. Saw a dialog with an error. After correcting the error, saw state preserved properly.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Tue, Jul 2, 12:30 PM
epriestley requested review of this revision.Tue, Jul 2, 12:32 PM
epriestley added inline comments.Tue, Jul 2, 12:35 PM
src/applications/project/controller/PhabricatorProjectBoardViewController.php
30–32

Particularly, I think getting rid of this absolute nonsense is a big step forward in clarity. This is saying:

  • if this is a POST (with CSRF); and
  • doesn't look like it's because the user is trying to do a bulk move; and
  • doesn't look like it's because the user is trying to build a query from a column; and
  • doesn't look like it's because the user is trying to initialize a board; then

...apply a filter.

This is weird, non-obvious, non-local, and the approach ("do every possible action in this controller by guessing request parameters") doesn't scale very well, even ignoring how big the file has become in terms of raw line size.

src/applications/project/state/PhabricatorWorkboardViewState.php
91–93

(This isn't ideal from a correctness standpoint, but doesn't appear to actually cause any problematic behavior.)

amckinley accepted this revision.Tue, Jul 2, 8:35 PM
This revision is now accepted and ready to land.Tue, Jul 2, 8:35 PM