Move workboard "filter" workflow to a separate controller
ClosedPublic
Actions

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

Details

Reviewers

amckinley

Maniphest Tasks

T4900: Workboards updating in real time

Commits

rP577020aea951: Move workboard "filter" workflow to a separate controller

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Jul 2 2019, 12:30 PM

Harbormaster completed remote builds in B23086: Diff 49217.Jul 2 2019, 12:31 PM

epriestley requested review of this revision.Jul 2 2019, 12:32 PM

epriestley added inline comments.Jul 2 2019, 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.)