You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
What the full review of #3 found, one commit per fix. No version bump: 1.0.0 is being re-cut, the tag moves to the merge.
Value policies
Multi-valued filters are policed element by element. Before, a check_boxes filter with sanitize:/validate: reset every legitimate submission as a bypass, and an Array default: beside validate: raised on every request.
A validate: returning one of the shipped i18n symbols, or a message: carrying %{limit}, no longer raises on the very request the policy exists to handle gracefully.
on_invalid: :reject degrades to :reset on CSV/JSON instead of answering an export with a 302 to the HTML index.
Windows
A min: that resolves to nil imposes no floor instead of crashing, the way a nil default: already imposes nothing.
max_range:/min: on a filter whose search key could not be derived (proxy pages with no Ransack) warns once instead of raising on every filtered request.
A sub-day cap reports its real size instead of "0 days"; an exclusive Range default raises instead of quietly searching one day more than it says.
Required filters and groups
required: counts any predicate of its own filter as answered, so picking "equals" from the dropdown no longer merges the default in beside the admin's own condition.
A with: group member whose Proc default resolves to nil is a runtime no-op, not a configuration error; a group containing a two-ended filter counts a single bound as an answer.
require_any_filter takes a scope as the question it was waiting for, and renders empty results instead of Active Admin's blank slate ("There are no Posts yet" beside a warning that filters are required).
Redirect and parameters
An Array default containing nil no longer loops the canonicalizing redirect forever.
Structural Ransack keys are stripped only where the resource declares gem options, and s/sorts are left alone: hand-built OR links and Ransack sorting on untouched resources keep working.
Options and harness
extensions: no longer annexes Formtastic's own top-level required:/min:, which used to 500 every index of an upgraded app; a string-keyed extensions hash raises instead of being silently inert.
The dev bundle resolves the Rails the project supports, and rake setup regenerates the sample app from scratch - rails new git-inits it, so a stale app survived git clean -fdx and then passed the template-digest guard.
A :check_boxes or multi-select filter submits an Array of Strings - the
shape that input legitimately has - and the scalar policies refused it
as a bypass, so a sanitize:/validate: on such a filter rejected every
real submission, and an Array default: was judged as its Array#to_s and
raised "violates its own validate:" on the first index request. Arrays
are now sanitized and validated element-wise, in the request and at
materialization; any other shape stays a refused bypass.
CI already runs 8.0 and 8.1 and the README states that floor; the dev
pin still resolved 7.2, so a local `rake spec` tested a Rails this gem
no longer supports.
The guard blanked the collection but left the request without a `q`, and
Active Admin reads that as "this table has no rows yet" - so the warning
arrived beside "There are no Posts yet" and a Create link. A
present-but-empty `q` still reads as a bare request everywhere in the
gem, so no redirect and no defaults decision changes.
Both are documented Formtastic input options, and the stray-key guard
raised ArgumentError for either of them at the top level - so an app
declaring the legal `filter :position, as: :number, min: 0` booted green
and then 500'd on every index of that resource. They now pass through to
the input untouched, and the gem answers to them only inside
extensions:; the form strip drops exactly the keys the container
declared instead of every name the gem knows.
`rails new` git-inits the generated app, so the directory survives
`git clean -fdx spec/rails` and the generator writes into the surviving
tree - leaving resources an older template declared in place while the
task re-stamps the digest, which is exactly the stale-app green the
stamp exists to deny.
Key derivation falls back to the declared filter name on a proxy page
with no resource Ransack can search - and a window policy then demanded
two keys and got one, raising "a window policy needs a two-ended input"
on every request: the exact 500 the fallback was added to prevent. The
fallback now records itself, and a fallen-back single key logs once per
resource and leaves the window unenforced. A genuinely derived single
key still raises - that is a real configuration error.
It issued redirect_back/url_for for a CSV or JSON index too, where the
redirect module deliberately does nothing: the flash would never be
rendered and url_for drops the format, so the export was answered with
an HTML page. Those formats now take the :reset path and stay bounded
in one request.
An Array default with a blank member - `-> { [current_admin_user.team_id] }`
for an admin who has none - survived the blank-reject, serialized to
?q[k][]= and parsed back as [""], which normalization drops: the
defaults re-applied and the two sides never agreed, so the index
answered ERR_TOO_MANY_REDIRECTS for exactly those admins. Both sides of
the comparison are now normalized element-wise, and `false` still
survives as the answer it is.
The group-completeness check read the materialized defaults, so it could
not tell a member that declares no default: from one whose Proc answered
nil for this admin - the gem's own convention for imposing nothing - and
the bare index raised the half-group configuration error per admin. Only
a missing declaration raises now; a runtime nil leaves the group
uncompletable, so it imposes nothing rather than searching half of
itself.
It reached nil.in_time_zone instead, so a per-admin retention floor that
opts out - the convention a nil default: already follows - answered
NoMethodError on every request carrying a bound. The floor is now
resolved once per call and a nil one simply does not narrow the window.
Three ways the message path failed instead of speaking: a validate:
returning one of the shipped parameterized symbols - which the docs
invite - raised I18n::MissingInterpolationArgument because there was
nothing to interpolate %{limit} from; a `message:` override was used
verbatim, so the app's own words could never name %{limit} or %{floor};
and a require_any_filter notice naming a key no locale defines flashed
the raw "Translation missing" line. Overrides interpolate now, and a
missing key or interpolation falls back to the shipped text.
It looked only for its derived key, so an admin picking "equals" on a
:string filter sent title_eq, the group saw no title_cont, and the
default merged in beside the admin's own condition - two conditions on
one column, both frozen into the canonical URL, greeted by the defaults
notice. Any real predicate on the filter's attribute now counts as an
answer, read the way the resource-level guard reads one: a
predicate-less stem is still no answer, and still no way to switch a
mandatory window off.
The strip was global, so a plain resource's hand-built q[g] OR link -
the standard Active Admin workaround - searched nothing and exported the
whole table, and a Ransack sort was dropped from the search and erased
from the canonical URL by the 302. Stripping now applies to resources
that declare something of this gem's, where a structural key really is a
bypass, and q[s]/q[sorts] are left alone everywhere: sorting is no
condition, so it reaches Ransack untouched and is not an answer to the
question the defaults exist to answer.
A two-ended member submits two keys, so one date bound beside the
partner counted as two of three: the group reset itself with "must be
given together" and, with no defaults to refill, rendered the whole
table - the scan the guard exists to prevent. The fallback side had the
same arithmetic, where a half-open Range default read as a
half-declared group and raised. Either bound now answers its member,
the touched-is-answered rule required: already follows.
The input submits gteq and lteq, so `a...b` stored b as the upper bound
and searched the day the Range excluded - then the redirect froze that
wrong bound into every copied URL. Every other Range ambiguity here
raises; this one changed what was searched quietly. An endless exclusive
Range has no end to exclude and still passes.
Clicking a scope rendered an empty page with "You must specify at least
one filter" beside the tab Active Admin had just put a count on - so
scopes were unusable on exactly the resources this guard targets.
A max_range: shorter than a day divided to "a maximum date range of 0
days"; the cap is now named in the units it was declared in, and the
shipped text no longer hardcodes "days".
A string-keyed extensions: container passed the unknown-key check, which
symbolizes, and then merged under a key the gem never reads - a declared
default that quietly did nothing. The container is symbolized first.
`rescue StandardError` around key derivation reported every internal
failure as "could not work out which search key it submits", which
policy_search_keys answered by guarding a key of its own invention. It
now rescues NameError, which is what a resource with no `ransack` and a
missing input class both raise.
`required:` read a boolean false as unanswered. Rack never delivers one -
?q[k]=false arrives as the String - so this is reachable only through an
override of #filtering_params, but `false` is an answer everywhere else
in the gem.
[nil, "published"] turned nil into "", the validator refused it, and
materialization raised on every request - so an Array default with a
per-admin member resolving to nothing broke the index for exactly those
admins. Blank members are dropped before the per-element check, the way
request normalization drops them on the way back in; a wholly blank
Array is already absent.
?q[status_in][]=&q[status_in][]=draft is the shape a multi-select
submits, and normalization discards the blank before searching - but
the validator saw it, flashed an error about a value nothing searches
and reset a legitimate selection to the default. Blank elements are
skipped now. A blank SCALAR is still judged: the sanitizer may have
just emptied it, which is what :empty exists to report.
Any non-blank scope satisfied the guard, so ?scope=does_not_exist - which
Active Admin resolves to nothing and applies as nothing - returned the
whole table from an index declared to answer nothing unasked, on CSV as
well as HTML. The check now asks #current_scope, which is nil for an id
no resource declares.
Array.wrap collapses the explicit Array branch, and it is the right
wrapper rather than Array(): it asks for to_ary, so a Hash default -
which add_filter_default_value can store under a predicate key - stays
one value to judge instead of being splatted into its pairs.
It also unifies the blank handling the branch left out: a blank scalar
default now yields nothing to validate instead of being judged as "".
That is a behavior change, and the convention it joins - a nil default
imposes nothing, a blank value is no answer anywhere else in the gem.
A blank default cannot ride the URL and Ransack strips it, so refusing
one raised over a value that never reaches a search.
#filter_defaults_engaged? inspected the materialized defaults, not the
set #complete_with_groups hands Ransack, so a with: group suppressed
because one member opted out still counted as engaged: a resource with
default_filters_notice announced a default slice while rendering the
unfiltered collection. It now asks the same completion the merge path
asks.
Grouping members by the stripped attribute also merged two single-ended
filters declared separately on one column - position_gteq with:
position_lteq - so submitting one bound passed as a whole group and a
default on one half passed the all-or-none check. Members are now built
from the key sets the inputs themselves submit, so a genuinely
two-ended input is still answered by either bound while separately
declared partners stay distinct.
An empty added does not mean this default resolved to nil. A select on :status and a separate :status_eq filter can both produce status_eq: the second non-nil default overwrites the existing key and is incorrectly recorded as an opt-out. If that key belongs to a with: group, complete_with_groups then discards the group's narrowing defaults. Materialize each filter into a separate hash and test whether it produced values, rather than whether it introduced new keys.
Track explicit predicate keys for nil Hash defaults
Nil Hash defaults are tracked under the input's derived keys rather than their explicitly named predicates. A string filter on :title with default: { eq: -> { nil } } records title_cont; a defaulted status filter paired with it using with: :title_eq therefore still raises the incomplete-default-group error. Record the explicit predicate keys when materializing nil values so this per-admin opt-out suppresses the group without evaluating the Proc twice.
Array defaults now send blank members through the sanitizer before validation can discard them. For example, [nil, "20"] with sanitize: ->(s) { Integer(s, 10).to_s } raises during default materialization because nil becomes an empty string. Remove blank array members before sanitizing, preserving false as a real value and leaving scalar behavior unchanged.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8a02966. Originally blank array entries are removed before sanitization; all remaining sanitized entries, including empty strings, are validated, with the original value retained for :empty restoration. Added regressions for both cases.
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
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.
What the full review of #3 found, one commit per fix. No version bump: 1.0.0 is being re-cut, the tag moves to the merge.
Value policies
check_boxesfilter withsanitize:/validate:reset every legitimate submission as a bypass, and an Arraydefault:besidevalidate:raised on every request.validate:returning one of the shipped i18n symbols, or amessage:carrying%{limit}, no longer raises on the very request the policy exists to handle gracefully.on_invalid: :rejectdegrades to:reseton CSV/JSON instead of answering an export with a 302 to the HTML index.Windows
min:that resolves to nil imposes no floor instead of crashing, the way a nildefault:already imposes nothing.max_range:/min:on a filter whose search key could not be derived (proxy pages with no Ransack) warns once instead of raising on every filtered request.Required filters and groups
required:counts any predicate of its own filter as answered, so picking "equals" from the dropdown no longer merges the default in beside the admin's own condition.with:group member whose Proc default resolves to nil is a runtime no-op, not a configuration error; a group containing a two-ended filter counts a single bound as an answer.require_any_filtertakes a scope as the question it was waiting for, and renders empty results instead of Active Admin's blank slate ("There are no Posts yet" beside a warning that filters are required).Redirect and parameters
s/sortsare left alone: hand-built OR links and Ransack sorting on untouched resources keep working.Options and harness
extensions:no longer annexes Formtastic's own top-levelrequired:/min:, which used to 500 every index of an upgraded app; a string-keyed extensions hash raises instead of being silently inert.rake setupregenerates the sample app from scratch -rails newgit-inits it, so a stale app survivedgit clean -fdxand then passed the template-digest guard.