refactoring and improvements - #1
Merged
Merged
Conversation
A default used to be written as a Ransack key: `default: { gteq_datetime: ... }`,
`default: { id_in: [...] }`, `default: { cont: "acme" }`. That is the query
language leaking into the filter declaration - the author wanted to say "show
the active ones by default" and had to say how Ransack spells it.
The input already knows. It is built against an empty search and asked, so a
value is enough: `:select` answers `_eq`, `:check_boxes` answers `_in` under the
association's primary key, `:date_range` answers both bounds, and `:string`
answers whichever predicate heads its dropdown - which `filters:` on the filter,
or `string_input_filters` on the resource or namespace, may have reordered. A
hardcoded table would have been wrong in all three of those cases.
Asking also carries a host app's own patches for free. One of ours rewrites
DateRangeInput#gt_input_name to append `_datetime` for datetime columns, and the
derived key follows it without the gem knowing anything about it.
Two details the inputs do not volunteer. `current_filter` raises on a filter
whose name already carries its predicate, since it goes looking for
`title_eq_cont`, so `seems_searchable?` is checked first. And it prefers a
predicate the current request already carries, which is the wrong answer for a
default, so the input is built against an empty search rather than the live one.
A Hash still names predicates outright, unchanged, for when the input's own is
not the one you want. A value that cannot be placed - a single value for a
two-ended input - raises rather than picking an end.
#view_context instantiates a fresh view class on every call, and deriving a search key was calling it once per defaulted filter. The builder depends only on the resource class and the view, which are the same for every filter in a request, so there is nothing to keep apart. The input now also takes its template and object from the builder rather than being handed separately built ones, which could have disagreed with it. Per-boot caching would go further - the derived keys do not depend on the request at all - but it needs a holder that resets when Active Admin reloads, and derivation only runs on requests that carry no filters, for filters that declare a default. That is one or two per page here, so it is an optimisation to make on a measurement rather than on a hunch.
The examples handed out a literal Range for a moving window, which is wrong in the way that does not show up in a test: `filter` runs when the resource file is loaded, so `default: 1.week.ago..` pins the window to boot and lets it drift for the life of the process. Short-lived processes hide it; long-running workers do not. Caught while trying the branch on a real app, where the author had written the Proc form without being told to.
DataAccess had grown into three jobs under one name: replacing Active Admin's #apply_filtering, working out what the declared defaults come to, and building Formtastic inputs to ask them which Ransack key they submit under. Only the first is data access; the last is view introspection that never touches the collection. DataAccess is now the one method this gem replaces, eighteen lines of it, and it reads `filtering_params`. FilterDefaults is everything that method means. It is included rather than prepended. Nothing in it replaces anything Active Admin defines, and every seam in it - #filtering_params, #filter_defaults_apply?, #visible_filters - is one a resource is meant to override in its own `controller do` block. Including puts the class ahead of the module, which is the order that wants. The unit harness got simpler in the process, which is the useful signal: it had to prepend its stand-in for the key derivation, because the real one sat in a prepended module and a method on the class could never be reached. Now it is a plain method on the class.
Two things found by trying the branch on real apps. A `:string` filter submits the head of its predicate dropdown, and an app that re-registers Ransack's `contains` / `equals` / `starts_with` aliases reorders that list - so a bare string default searches for equality there, not a substring. Correct, since it is what the form submits, and surprising to anyone reading the example here. A resource Ransack cannot search has no input to ask. Five pages in one app are ActiveResource models fronting an HTTP API; the derivation raises with the reason and the remedy rather than guessing a key.
The suite proved the collection came back filtered, and proved the value reached the form for exactly one input type. That is the wrong half to leave untested: a default that filters invisibly is the failure worth catching, and it is also what Clear Filters returning to the defaults rests on. Every type now asserts the rendered value - the selected option of a select and of a boolean, the checked box of a check boxes filter, both ends of a date range, the numeric field under the predicate its Hash named, the string field, and the one read off the signed-in admin. They discriminate: on a resource that declares no default the same field renders with no value attribute at all, so the assertions fail there.
The filters form resolves `:input_html` against the view before it builds an
input; the derivation was handing the Proc straight over. That is not merely
incomplete, it is wrong in the way nothing reports: `Proc#[]` is `call`, so
Formtastic asking `input_html[:multiple]` invoked the Proc, got a truthy Hash
back, and a :select derived `_in` instead of `_eq` - a key that filters nothing,
with no error.
`input_html: proc { ... }` is ordinary in Active Admin and appears several times
in one of the apps this was tried on, so this would have shipped and then failed
quietly on whichever filter happened to combine the two.
The set was written out twice, in different orders - once where the filters form strips them, once where the derivation builds its input. They have to agree, and nothing made them: a fourth option added to the gem would have been remembered in one place and forgotten in the other, reaching Formtastic silently. Which is how the Proc in :input_html got through. FilterDefaults owns what `:default` means, so it owns the list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.