diff --git a/Gemfile b/Gemfile index fb68db9..4d98681 100644 --- a/Gemfile +++ b/Gemfile @@ -4,7 +4,7 @@ source "https://rubygems.org" gemspec -default_rails_version = "7.2.0" +default_rails_version = "8.0.0" default_activeadmin_version = "3.5.0" gem "rails", "~> #{ENV['RAILS'] || default_rails_version}" diff --git a/README.md b/README.md index 0d4cb67..8203b65 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,25 @@ gem "active_admin_filter_extensions", github: "activeadmin-plugins/active_admin_ Not on RubyGems yet; pin the tag. +## The extensions: container + +Everything this gem adds lives under one filter option, so its names can never collide with +Formtastic's. A gem key left flat raises rather than quietly meaning something else: + +```ruby +filter :title_cont, as: :string, extensions: { default: "acme" } # right +filter :title_cont, as: :string, default: "acme" # raises +``` + +With one carve-out: `required:` and `min:` are Formtastic input options in their own right, so +flat they belong to the form and pass through untouched. This gem answers to them only inside +`extensions:`. + +```ruby +filter :position, as: :number, min: 0 # Formtastic's - renders min="0" +filter :created_at, as: :date_range, extensions: { min: -> { 2.months.ago } } # the gem's floor +``` + ## Usage A default is a **value**. Which Ransack key it lands under is the input's business, and the @@ -71,6 +90,8 @@ filter :title, extensions: { default: "acme" } **A Range** fills a two-ended input, one bound per end; leave an end off and that end is left to the admin. Handing a single value to a two-ended input raises, rather than picking an end for you. +So does an exclusive range (`a...b`): the input submits `gteq` and `lteq`, so the excluded end has +no spelling there and would be searched after all. Anything relative to now belongs in a Proc, as above. `filter` runs when the resource file is loaded, so `default: 1.week.ago..` would pin the window to the moment the process booted and let @@ -221,7 +242,9 @@ require_any_filter notice: "You must specify at least one filter" `require_any_filter` makes an index answer no question until it is asked one: a bare visit - a blank form submission included - renders an empty collection with the notice, never the -whole table. Resource-level on purpose: "any filter at all" belongs to no single filter. +whole table. Resource-level on purpose: "any filter at all" belongs to no single filter. A +scope tab counts as a question: Active Admin puts a count on it, and answering it with an empty +page would contradict the tab. ## on_invalid: what a violated policy does @@ -235,7 +258,8 @@ filter :name_eq, extensions: { validate: ->(v) { ... }, on_invalid: :empty } * `:reset` - drop the offending key(s), `flash[:error]`, the declared `default:` refills. * `:reject` - `flash[:error]` and bounce back the way the request came (bare index when - there is no referer). + there is no referer). Off the HTML path - a CSV or JSON index - it degrades to `:reset`: + there is nowhere to bounce to and no flash to read. * `:empty` - render an empty collection with the message, keeping the admin's input in the form: "nothing found because the value is malformed" stops masquerading as an honestly empty result. @@ -262,7 +286,8 @@ refills. An open upper bound is measured against now; a FUTURE lower bound with bound selects a not-yet-written window and passes. A default that would itself violate the policy raises on first use - resetting into a refused window is a loop, not a fallback. Messages are i18n keys under `active_admin_filter_extensions.*` (`range_invalid`, -`range_too_wide` with `%{limit}`, `before_floor` with `%{floor}`), shipped with English +`range_too_wide` with `%{limit}` (the cap in the units it was declared in, "90 days" or +"6 hours"), `before_floor` with `%{floor}`), shipped with English defaults. ## Value policies: sanitize and validate @@ -305,7 +330,7 @@ visit, and a bookmarked URL pins the defaults of the day it was made rather than the code. Non-HTML requests (CSV, JSON) are never redirected; the defaults filter them all the same. -Three prints in small type: +Four prints in small type: * **Code that rewrites `q` in a `before_action`** (sanitizers that delete invalid ranges, say) runs after the redirect has already compared the request against the defaults - the @@ -315,6 +340,12 @@ Three prints in small type: * **The redirect runs after authentication but before Active Admin's index authorization**, so an authenticated admin without read rights on a resource still receives the 302 whose Location spells out that resource's defaults. Keep per-role secrets out of defaults. +* **Ransack's structural keys are stripped on resources that declare something of this + gem's.** `q[m]`, `q[g]` and `q[c]` turn a required, capped window into `window OR anything` + and nest conditions no value policy ever sees, so where a policy guards the filters they + are a bypass rather than an answer. A resource that declares no extension keeps them - the + hand-built `q[g]` OR link is the standard Active Admin workaround. Sorting (`q[s]`) is + never a condition and is never touched. * **Anything that reads `params[:q]` for the effective filters** (drill-down links, exports) should read `filtering_params` instead - on the rendered page they agree, but only `filtering_params` is also right on the non-redirected formats. diff --git a/config/locales/en.yml b/config/locales/en.yml index 956663b..403dc48 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -4,7 +4,7 @@ en: active_admin_filter_extensions: invalid: "%{filter} filter has an invalid value." range_invalid: "%{filter} filter has an invalid date range value." - range_too_wide: "%{filter} filter allows a maximum date range of %{limit} days." + range_too_wide: "%{filter} filter allows a maximum date range of %{limit}." before_floor: "%{filter} filter cannot start before %{floor}." given_together: "%{filters} filters must be given together." require_any_filter: "You must specify at least one filter" diff --git a/lib/active_admin_filter_extensions/filter_defaults.rb b/lib/active_admin_filter_extensions/filter_defaults.rb index 4a3831e..3af37a6 100644 --- a/lib/active_admin_filter_extensions/filter_defaults.rb +++ b/lib/active_admin_filter_extensions/filter_defaults.rb @@ -10,9 +10,14 @@ module ActiveAdminFilterExtensions module FilterDefaults # Ransack reads more than conditions out of q: `m` switches the combinator to OR - turning # a required, capped window into `window OR anything` - and `g` nests whole condition - # groups no value policy ever saw. No Active Admin filter form submits them; here they are - # a bypass, not an answer, and they are stripped before anything reads the request. - RANSACK_STRUCTURE = %w[m g c s conditions sorts groupings combinator].freeze + # groups no value policy ever saw. No Active Admin filter form submits them; where this gem + # guards the filters they are a bypass, not an answer, and they are stripped before anything + # reads the request. On a resource that declares nothing of this gem's there is no policy to + # bypass, and `q[g]` is the app's own standard OR workaround - see #strip_ransack_structure?. + RANSACK_STRUCTURE = %w[m g c conditions groupings combinator].freeze + # Sorting is not a condition: it reaches Ransack untouched, and it is not an answer to the + # question the defaults exist to answer. + RANSACK_SORT = %w[s sorts].freeze protected @@ -55,11 +60,22 @@ def complete_with_groups(defaults) with_groups.each do |keys| next unless keys.any? { |key| defaults.key?(key) } + # A member that opted out this request leaves the group uncompletable, and half a group + # is the search the all-or-none contract refuses - so the group imposes nothing. + next defaults = defaults.except(*keys) if keys.any? { |key| nil_default_filter_keys.include?(key) } + defaults = filter_default_values.slice(*keys).merge(defaults) end defaults end + # The keys of filters that declare a default: which resolved to nil for this request. A nil + # Proc result imposes nothing - that is how a per-admin default opts out - so it is not the + # missing declaration the with: completeness check refuses. + def nil_default_filter_keys + @nil_default_filter_keys ||= [] + end + # The defaults of keys a policy just reset - refilled whatever filter_defaults_apply? # says, because the reset itself is what emptied them. def reset_filter_default_values @@ -78,8 +94,11 @@ def filter_defaults_engaged? requested = requested_filtering_params || {} if filter_defaults_apply? # At least one default must actually reach the search: a widened apply? beside a - # request that answered every defaulted key itself engaged nothing. - return filter_default_values.keys.any? { |key| requested[key].nil? } + # request that answered every defaulted key itself engaged nothing. Through + # #complete_with_groups, which is what decides that on the merge path - a with: group + # one member opted out of imposes nothing, and announcing it would describe a default + # slice over the unfiltered collection. + return complete_with_groups(filter_default_values).keys.any? { |key| requested[key].nil? } end required_filter_defaults_absent_from(requested).any? @@ -100,7 +119,7 @@ def filter_defaults_engaged? # Judged on the NORMALIZED request: a q holding nothing but blanks or structural keys is # a bare request on every format - the CSV gets no canonicalizing hop to say otherwise. def filter_defaults_apply? - requested_filtering_params.blank? && params[:commit].blank? + (requested_filtering_params || {}).except(*RANSACK_SORT).blank? && params[:commit].blank? end # @return [Hash] the search values declared with `filter ..., default:` @@ -110,7 +129,9 @@ def filter_default_values before = result.keys add_filter_default_value(result, attribute, options) - (@filter_default_keys ||= {})[attribute] = result.keys - before + added = result.keys - before + (@filter_default_keys ||= {})[attribute] = added + nil_default_filter_keys.concat(policy_search_keys(attribute, options)) if added.empty? # Value policies first: the window must be judged on what the sanitizer stored. validate_default_against_value_policies(attribute, options, result, result.keys - before) validate_default_against_policies(attribute, options, result) @@ -122,13 +143,18 @@ def filter_default_values # enforcement even runs. def validate_defaults_against_with_groups(defaults) with_groups.each do |keys| - answered = keys.select { |key| defaults.key?(key) } - next if answered.empty? || answered.size == keys.size + # By member, so a half-open Range default - which fills only the end it has - answers + # its two-ended member rather than reading as half a group. + members = with_group_members(keys) + answered = members.select do |member| + member.any? { |key| defaults.key?(key) || nil_default_filter_keys.include?(key) } + end + next if answered.empty? || answered.size == members.size raise ArgumentError, "filters #{keys.join(', ')} are declared with: one another, but only " \ - "#{answered.join(', ')} declare defaults - the fallback must answer the whole " \ - "group or none of it" + "#{answered.flatten.join(', ')} declare defaults - the fallback must answer the " \ + "whole group or none of it" end end @@ -163,24 +189,17 @@ def validate_default_against_value_policies(attribute, options, result, keys) value = result[key] next if value.nil? - # Scalar values go through as the String the URL will say - a -20 under :digits must - # search as "20" on every format. Multi-valued defaults are not scalar submissions. - if sanitizer && !value.is_a?(Enumerable) + # Judged the way the URL will say it: a scalar as the String it becomes, an Array + # element by element - the shape a multi-valued input submits. + if sanitizer resolved = Policies::Sanitizers.resolve(sanitizer) - sanitized = ::MethodOrProcHelper.render_in_context(self, resolved, value.to_s.dup).to_s - # The same fixpoint check submitted values face: without it the CSV searches the - # once-sanitized value while the HTML converges on another, and nobody raises. - if ::MethodOrProcHelper.render_in_context(self, resolved, sanitized.dup).to_s != sanitized - raise ArgumentError, - "filter :#{attribute}'s sanitize: is not idempotent " \ - "(#{value.inspect} -> #{sanitized.inspect} -> keeps changing) - " \ - "a default must sanitize to a fixpoint" - end - - result[key] = sanitized + result[key] = sanitize_default_value(attribute, resolved, value) end next unless validator - next unless ::MethodOrProcHelper.render_in_context(self, Policies::Validators.resolve(validator), result[key].to_s) + + resolved_validator = Policies::Validators.resolve(validator) + next unless default_value_strings(result[key]) + .any? { |string| ::MethodOrProcHelper.render_in_context(self, resolved_validator, string) } raise ArgumentError, "filter :#{attribute} declares a default that violates its own validate: " \ @@ -188,6 +207,35 @@ def validate_default_against_value_policies(attribute, options, result, keys) end end + # The values a default actually submits: an Array element by element, anything else as the + # one String the URL will say. Blank ones are dropped the way #requested_filtering_params + # drops them on the way back in - [nil, "published"] rides the URL as + # ?q[k][]=&q[k][]=published and searches the second alone, and a blank scalar cannot ride + # it at all - so judging either refuses a value nothing ever searches. + # + # Array.wrap, not Array(): it asks for to_ary, so a Hash default stays one value to judge + # instead of being splatted into its pairs. + def default_value_strings(value) + Array.wrap(value).map(&:to_s).reject(&:blank?) + end + + def sanitize_default_value(attribute, sanitizer, value) + return value.map { |element| sanitize_default_value(attribute, sanitizer, element) } if value.is_a?(Array) + return value if value.is_a?(Enumerable) + + sanitized = ::MethodOrProcHelper.render_in_context(self, sanitizer, value.to_s.dup).to_s + # The same fixpoint check submitted values face: without it the CSV searches the + # once-sanitized value while the HTML converges on another, and nobody raises. + if ::MethodOrProcHelper.render_in_context(self, sanitizer, sanitized.dup).to_s != sanitized + raise ArgumentError, + "filter :#{attribute}'s sanitize: is not idempotent " \ + "(#{value.inspect} -> #{sanitized.inspect} -> keeps changing) - " \ + "a default must sanitize to a fixpoint" + end + + sanitized + end + private def requested_filtering_params @@ -204,11 +252,24 @@ def requested_filtering_params requested = requested.transform_values do |value| value.is_a?(Array) ? value.reject { |element| element.blank? && element != false } : value end + strip_structure = strip_ransack_structure? requested.reject do |key, value| - RANSACK_STRUCTURE.include?(key.to_s) || (value.blank? && value != false) + (strip_structure && RANSACK_STRUCTURE.include?(key.to_s)) || (value.blank? && value != false) end end + # Whether this resource declares anything of this gem's. Only then is a structural key a + # bypass of something; on every other resource it is the app's own search and stripping it + # silently widens the export to the whole table. + def strip_ransack_structure? + return @strip_ransack_structure if defined?(@strip_ransack_structure) + + @strip_ransack_structure = + active_admin_config.require_any_filter_notice.present? || + active_admin_config.default_filters_notice.present? || + visible_filters.any? { |_attribute, options| options.key?(:extensions) } + end + # A default is a value, and where that value goes is the input's business - see SearchKeys. # # filter :status, as: :select, default: "active" @@ -244,6 +305,13 @@ def assign_derived_filter_default(result, attribute, options, default) raise_filter_default_error(attribute, "a Range needs an input with two ends, and this one submits #{names.join(' and ')}") end + # The input submits gteq and lteq, so an excluded end has no spelling here: stored as + # lteq it would search the very bound the Range left out, quietly and in every copied + # URL. An endless exclusive Range has no end to exclude and is fine. + if default.exclude_end? && !default.end.nil? + raise_filter_default_error(attribute, "an exclusive Range has no gteq/lteq spelling, so its end would be searched after all - use an inclusive range, or name the predicates with a Hash") + end + result[names.first] = default.begin unless default.begin.nil? result[names.last] = default.end unless default.end.nil? else diff --git a/lib/active_admin_filter_extensions/filter_options.rb b/lib/active_admin_filter_extensions/filter_options.rb index 5202977..11b1a01 100644 --- a/lib/active_admin_filter_extensions/filter_options.rb +++ b/lib/active_admin_filter_extensions/filter_options.rb @@ -11,6 +11,22 @@ module FilterOptions # form is given anything. NOT_FOR_INPUT = %i[extensions if unless].freeze EXTENSION_KEYS = %i[default required with sanitize validate on_invalid max_range min message].freeze + # required: and min: are documented Formtastic input options in their own right. The gem + # answers to them only INSIDE extensions:, so flat they are the host's: no stray-key raise, + # no policy, straight through to the input. + HOST_INPUT_KEYS = %i[required min].freeze + STRAY_KEYS = (EXTENSION_KEYS - HOST_INPUT_KEYS).freeze + # Where #merge_filter_extensions parks the host's own required:/min:, out of reach of the + # gem's flat reads until #input_options hands them back to the input. + HOST_INPUT_OPTIONS = :extensions_host_input_options + + # What the form may see: the gem's container and conditions out, the keys the container + # itself declared out, and the host's own options back in. + def self.input_options(options) + options.except(*NOT_FOR_INPUT, HOST_INPUT_OPTIONS, + *(options[:extensions] || {}).keys.map(&:to_sym)) + .merge(options[HOST_INPUT_OPTIONS] || {}) + end protected @@ -31,18 +47,24 @@ def visible_filters # (required:, most notably). Internally the sub-hash merges flat; a gem key left outside # extensions: raises rather than silently meaning something else to Formtastic. def merge_filter_extensions(attribute, options) - stray = options.keys.map(&:to_sym) & EXTENSION_KEYS + stray = options.keys.map(&:to_sym) & STRAY_KEYS if stray.any? raise ArgumentError, "filter :#{attribute}: #{stray.map(&:inspect).join(', ')} " "belong inside extensions: { ... }" end - extensions = options[:extensions] || {} + # Symbolized, because the unknown-key check below symbolizes too: a string-keyed container + # otherwise passed it and then merged under a key the gem never reads. + extensions = (options[:extensions] || {}).transform_keys(&:to_sym) unknown = extensions.keys.map(&:to_sym) - EXTENSION_KEYS if unknown.any? raise ArgumentError, "filter :#{attribute}: unknown extensions: #{unknown.map(&:inspect).join(', ')} - " "the gem ships #{EXTENSION_KEYS.map(&:inspect).join(', ')}" end - options.merge(extensions) + # The host's own required:/min: are parked rather than merged: left in, the gem would read + # a flat min: as a window policy on an input that submits one key. + host = options.slice(*HOST_INPUT_KEYS) + merged = options.except(*HOST_INPUT_KEYS).merge(extensions) + host.any? ? merged.merge(HOST_INPUT_OPTIONS => host) : merged end end end diff --git a/lib/active_admin_filter_extensions/filters_form.rb b/lib/active_admin_filter_extensions/filters_form.rb index 71a26ce..a061750 100644 --- a/lib/active_admin_filter_extensions/filters_form.rb +++ b/lib/active_admin_filter_extensions/filters_form.rb @@ -25,7 +25,7 @@ def filters_sidebar_section # method, so that this gem carries no copy of a method body that belongs to Active Admin. module ViewHelper def active_admin_filters_form_for(search, filters, options = {}) - super(search, filters.transform_values { |opts| opts.except(*FilterOptions::NOT_FOR_INPUT, *FilterOptions::EXTENSION_KEYS) }, options) + super(search, filters.transform_values { |opts| FilterOptions.input_options(opts) }, options) end end diff --git a/lib/active_admin_filter_extensions/policies.rb b/lib/active_admin_filter_extensions/policies.rb index 7c8287c..109909e 100644 --- a/lib/active_admin_filter_extensions/policies.rb +++ b/lib/active_admin_filter_extensions/policies.rb @@ -137,8 +137,12 @@ def enforce_require_any_filter return if notice.nil? || any_declared_filter_answered? @filter_policy_render_empty = true + # Active Admin reads a q-less index as "this table has no rows" and renders the blank + # slate, Create link and all; a present-but-empty q renders an empty result instead. + params[:q] ||= {} flash.now[:warning] = - notice.is_a?(String) ? notice : I18n.t(notice, scope: :active_admin_filter_extensions) + notice.is_a?(String) ? notice : I18n.t(notice, scope: :active_admin_filter_extensions, + default: :require_any_filter) end # Ransack silently ignores a condition nobody declared - junk, a stale bookmark after @@ -146,6 +150,13 @@ def enforce_require_any_filter # when a declared filter submits it as-is (scope-named filters carry no predicate), or # when it carries a real predicate on a declared filter's attribute. def any_declared_filter_answered? + # A scope tab is a question, and the one Active Admin puts a count on: answering it + # with an empty page and "specify at least one filter" contradicts the tab. One Active + # Admin actually resolves, though - #current_scope is nil for an id no resource + # declares, and an unresolved scope narrows nothing, so counting its mere presence + # hands back the whole table behind a scope-shaped URL. + return true if params[:scope].present? && current_scope.present? + exact = visible_filters.flat_map { |attribute, options| policy_search_keys(attribute, options) } bases = exact.map { |key| strip_ransack_predicate(key) } @@ -163,15 +174,40 @@ def enforce_with_groups(q) # raw present? check while Ransack drops it - half the group executing silently. answered = requested_filtering_params || {} with_groups.each do |keys| - present = keys.select { |key| answered.key?(key) } - next if present.empty? || present.size == keys.size + members = with_group_members(keys) + present = members.select { |member| member.any? { |key| answered.key?(key) } } + next if present.empty? || present.size == members.size filter_policy_errors << keys filter_policy_reset_keys.concat(keys) names = keys.map { |k| strip_ransack_predicate(k).humanize }.uniq.join(" and ") flash.now[:warning] = I18n.t(:given_together, scope: :active_admin_filter_extensions, filters: names) - present.each { |key| q.delete(key) } + present.flatten.each { |key| q.delete(key) } + end + end + + # A group's members, not its keys: a two-ended filter submits two of them and is answered + # by either bound - the touched-is-answered rule required: already follows. Counted as + # separate keys, one bound beside the partner read as a half-answered group. + # + # Only keys ONE input submits together are combined. Grouping by the stripped attribute + # instead would also merge two single-ended filters declared separately on one column + # (position_gteq with: position_lteq), and half that answer would pass as a whole group. + def with_group_members(keys) + shared = multi_key_filter_keys.filter_map do |input_keys| + own = input_keys & keys + own if own.size > 1 + end + + (keys - shared.flatten).map { |key| [key] } + shared + end + + # The key sets of filters whose input submits more than one - the two-ended ones. + def multi_key_filter_keys + @multi_key_filter_keys ||= visible_filters.filter_map do |attribute, options| + keys = policy_search_keys(attribute, options) + keys if keys.size > 1 end end @@ -217,9 +253,17 @@ def strip_ransack_predicate(name) def policy_search_keys(attribute, options) filter_search_keys(attribute, options) rescue ArgumentError + fallback_search_key_attributes << attribute [attribute.to_s] end + # Which filters got their key from the fallback rather than from their input. A window + # policy reads this: one key is a configuration error when the input said so, and merely + # unknowable when nothing could ask the input. + def fallback_search_key_attributes + @fallback_search_key_attributes ||= [] + end + def enforce_scalar_policies(q, attribute, options, keys) return unless options[:sanitize] || options[:validate] @@ -231,34 +275,33 @@ def enforce_scalar_policies(q, attribute, options, keys) original = value - # Ransack executes a Hash- or Array-wrapped value byte-identically to the scalar; - # a policed key that is not a plain string is a bypass, not a shape to ignore. - unless value.is_a?(String) + # A `:check_boxes` or multi-select filter submits an Array of Strings - the shape + # that input legitimately has - so the policies judge it element by element. Any + # other shape (a Hash, a nested Array) executes byte-identically to the scalar on + # Ransack's side and is a bypass, not a shape to ignore. + unless policed_value_shape?(value) apply_invalid_filter_policy(attribute, options, scalar_reset_scope(options, keys, key), :invalid) next end + value = value.reject(&:blank?) if value.is_a?(Array) + if (sanitizer = options[:sanitize]) sanitizer = Sanitizers.resolve(sanitizer) - # On dup'd copies: a sanitizer that mutates in place and returns the same - # object would otherwise be compared against itself and wave its loop through. - sanitized = ::MethodOrProcHelper.render_in_context(self, sanitizer, value.dup).to_s - # The sanitized value lands in the URL and comes back for - # another pass - a sanitizer that keeps rewriting its own output redirect-loops - # the index into unreachability. Catch it on the first request instead. - if sanitized != value && - ::MethodOrProcHelper.render_in_context(self, sanitizer, sanitized.dup).to_s != sanitized - raise ArgumentError, - "filter :#{attribute}'s sanitize: is not idempotent " "(#{value.inspect} -> #{sanitized.inspect} -> keeps changing) - " "that is an infinite redirect" - end - + sanitized = sanitize_policed_value(attribute, sanitizer, value) value = sanitized q[key] = value end next unless (validator = options[:validate]) - message = ::MethodOrProcHelper.render_in_context(self, Validators.resolve(validator), value) + validator = Validators.resolve(validator) + # Blank input array elements are discarded before searching - ?q[k][]=&q[k][]=draft + # is the shape a multi-select submits - so remove them before sanitizing. Sanitized + # elements are all judged, including ones the sanitizer emptied. + elements = value.is_a?(Array) ? value : [value] + message = elements.lazy + .filter_map { |element| ::MethodOrProcHelper.render_in_context(self, validator, element) }.first # nil AND false mean "valid" - `condition && "message"` is how Ruby writes these. next unless message @@ -269,6 +312,32 @@ def enforce_scalar_policies(q, attribute, options, keys) end end + # A String, or an Array of Strings - what a multi-valued input submits. + def policed_value_shape?(value) + value.is_a?(String) || (value.is_a?(Array) && value.all?(String)) + end + + # Element-wise for an Array, so a multi-valued filter is sanitized the way it is + # submitted. On dup'd copies: a sanitizer that mutates in place and returns the same + # object would otherwise be compared against itself and wave its loop through. + def sanitize_policed_value(attribute, sanitizer, value) + return value.map { |element| sanitize_policed_value(attribute, sanitizer, element) } if value.is_a?(Array) + + sanitized = ::MethodOrProcHelper.render_in_context(self, sanitizer, value.dup).to_s + # The sanitized value lands in the URL and comes back for another pass - a sanitizer + # that keeps rewriting its own output redirect-loops the index into unreachability. + # Catch it on the first request instead. + if sanitized != value && + ::MethodOrProcHelper.render_in_context(self, sanitizer, sanitized.dup).to_s != sanitized + raise ArgumentError, + "filter :#{attribute}'s sanitize: is not idempotent " \ + "(#{value.inspect} -> #{sanitized.inspect} -> keeps changing) - " \ + "that is an infinite redirect" + end + + sanitized + end + # On a window-policied filter a value violation resets ALL its keys, not just the # offender: half the admin's window completed from the default pair is a window nobody # asked for and nothing validated. @@ -285,6 +354,10 @@ def enforce_window_policies(q, attribute, options, keys) return unless options[:max_range] || options[:min] unless keys.size == 2 + # Nothing could ask the input what it submits, so one key is not a verdict about the + # declaration - and raising here is the 500 the fallback exists to prevent. + return warn_underivable_window(attribute) if fallback_search_key_attributes.include?(attribute) + raise ArgumentError, "filter :#{attribute} declares max_range:/min: but its input submits " "#{keys.join(' and ')} - a window policy needs a two-ended input" end @@ -300,6 +373,25 @@ def enforce_window_policies(q, attribute, options, keys) apply_invalid_filter_policy(attribute, options, keys, violation.first, violation.last) end + # Once per resource: an index is hot, and the declaration is the same on every request. + def warn_underivable_window(attribute) + warned = self.class.instance_variable_get(:@extensions_underivable_windows) || + self.class.instance_variable_set(:@extensions_underivable_windows, []) + return if warned.include?(attribute) + + warned << attribute + policy_logger&.warn( + "filter :#{attribute} declares max_range:/min: but its search key could not be " \ + "derived, so the window is not enforced - name the predicates with a Hash" + ) + nil + end + + # Kept for the unit suite, which deliberately runs without Rails loaded. + def policy_logger + ::Rails.logger if defined?(::Rails) + end + def window_violation(options, from_raw, to_raw) from = parse_window_bound(from_raw) to = parse_window_bound(to_raw) @@ -307,40 +399,51 @@ def window_violation(options, from_raw, to_raw) return [:range_invalid, {}] if (from_raw.present? && from.nil?) || (to_raw.present? && to.nil?) return [:range_invalid, {}] if from && to && from > to - if (floor = options[:min]) - # in_time_zone, not to_time: the bounds are parsed in Time.zone, and Date#to_time - # speaks the SERVER zone - apart, a floor-equal bound is off by the offset. - floor = ::MethodOrProcHelper.render_in_context(self, floor).in_time_zone - if [from, to].compact.any? { |bound| bound < floor } - return [:before_floor, { floor: floor.to_date }] - end + floor = window_floor(options) + + if floor && [from, to].compact.any? { |bound| bound < floor } + return [:before_floor, { floor: floor.to_date }] end if (max = options[:max_range]) # An upper bound alone is an infinitely wide window - the outage query the cap # exists to refuse. - return [:range_too_wide, { limit: (max / 1.day).to_i }] if from.nil? && to + return [:range_too_wide, { limit: window_limit_text(max) }] if from.nil? && to if from # An open upper bound is measured against now; a FUTURE lower bound with no upper # bound selects a not-yet-written window and passes - measuring it against now # would read it as inverted. return nil if to.nil? && from >= Time.current - return [:range_too_wide, { limit: (max / 1.day).to_i }] if (to || Time.current) - from > max + return [:range_too_wide, { limit: window_limit_text(max) }] if (to || Time.current) - from > max end end # A min: standing alone still refuses a window with no lower bound - everything # before the floor is the query the floor exists to refuse. After max_range:, whose # too-wide verdict is the more actionable one when both are declared. - if from.nil? && to && (floor = options[:min]) - floor = ::MethodOrProcHelper.render_in_context(self, floor).in_time_zone - return [:before_floor, { floor: floor.to_date }] - end + return [:before_floor, { floor: floor.to_date }] if from.nil? && to && floor nil end + # The cap in the units it was declared in: divided into whole days, every cap shorter than + # one reported itself as "0 days". + def window_limit_text(max) + max.is_a?(::ActiveSupport::Duration) ? max.inspect : "#{max.to_i / 1.day} days" + end + + # `min:` resolved for this request, once. nil imposes nothing - that is how a per-admin + # floor opts out, the way a nil default: does. + def window_floor(options) + floor = options[:min] + return nil unless floor + + # in_time_zone, not to_time: the bounds are parsed in Time.zone, and Date#to_time speaks + # the SERVER zone - apart, a floor-equal bound is off by the offset. + ::MethodOrProcHelper.render_in_context(self, floor)&.in_time_zone + end + def parse_window_bound(raw) return nil if raw.blank? # An Array/Hash-wrapped bound would date-parse via #to_s while Ransack silently drops @@ -362,10 +465,13 @@ def apply_invalid_filter_policy(attribute, options, keys, message, interpolation case options.fetch(:on_invalid, :reset) when :reset - flash.now[:error] = text - filter_policy_reset_keys.concat(keys) - keys.each { |key| params[:q].delete(key) } + reset_violating_filter_keys(text, keys) when :reject + # Only where a redirect means something. A CSV or JSON index is answered in one + # request everywhere else in the gem, a flash it cannot render would be lost, and + # url_for drops the format - so off the HTML path the export is bounded the :reset way. + return reset_violating_filter_keys(text, keys) unless request.format.html? + flash[:error] = text # Browsers keep the original Referer across a 302, so when back IS this index the # bounce would re-reject itself to the browser's redirect cap. Home instead. @@ -387,6 +493,13 @@ def apply_invalid_filter_policy(attribute, options, keys, message, interpolation end end + # Drop the offending key(s) and let the declared default refill through the merge. + def reset_violating_filter_keys(text, keys) + flash.now[:error] = text + filter_policy_reset_keys.concat(keys) + keys.each { |key| params[:q].delete(key) } + end + # Read by apply_filtering: an :empty policy keeps the request intact - the form must # still show what the admin typed - and blanks the collection instead. def filter_policy_render_empty? @@ -395,15 +508,31 @@ def filter_policy_render_empty? def filter_policy_message(attribute, options, message, interpolations = {}) return message if message.is_a?(String) - # `message:` on the filter speaks for every symbolic violation of this filter - the - # app's own words without duplicating a whole validator to say them. - return options[:message] if options[:message] values = { filter: options[:label] || attribute.to_s.humanize }.merge(interpolations) + # `message:` on the filter speaks for every symbolic violation of this filter - the + # app's own words without duplicating a whole validator to say them. Interpolated, so + # those words can name %{limit} and %{floor} as the shipped texts do. + if (override = options[:message]) + return begin + I18n.interpolate(override, values) + rescue I18n::ArgumentError + override + end + end + # The English texts ship as plain i18n (config/locales/en.yml); an application - # overrides them under the same keys. Unknown symbols read as :invalid. - I18n.t(message, scope: :active_admin_filter_extensions, - default: :invalid, **values) + # overrides them under the same keys. + policy_translation(message, values) + end + + # A validate: may answer any Symbol in the gem's scope, including the parameterized ones - + # which have nothing to interpolate from. A violation message is not worth a 500, so an + # unknown key and a missing interpolation both read as :invalid. + def policy_translation(message, values) + I18n.t(message, scope: :active_admin_filter_extensions, raise: true, **values) + rescue I18n::ArgumentError + I18n.t(:invalid, scope: :active_admin_filter_extensions, **values) end end end diff --git a/lib/active_admin_filter_extensions/redirect.rb b/lib/active_admin_filter_extensions/redirect.rb index eb10107..df1b570 100644 --- a/lib/active_admin_filter_extensions/redirect.rb +++ b/lib/active_admin_filter_extensions/redirect.rb @@ -10,6 +10,20 @@ module ActiveAdminFilterExtensions # redirect on every bare visit, and a bookmarked URL pins the defaults of the day it was # made rather than following the code. module Redirect + # The filters as a URL can carry them, so that the two sides of the comparison below are + # comparable at all. A value the URL drops or hands back in another shape must be absent on + # both sides or the hop alternates with the bare address forever: an empty Array or Hash is + # dropped by to_query entirely, ?q[k]= comes back with the blank stripped, and an Array of + # blanks comes back as the empty Array. `false` round-trips, and is a real answer. + def self.canonical_filters(filters) + filters.each_with_object({}) do |(key, value), result| + value = value.reject { |element| element != false && element.blank? } if value.is_a?(Enumerable) && !value.is_a?(Hash) + next if value != false && value.blank? + + result[key] = value + end + end + def self.included(base) # ActiveAdmin.before_load fires on every dev-mode reload against the same non-reloaded # controller class, and Ruby runs this hook on every include call - without the guard the @@ -36,10 +50,8 @@ def self.included(base) # so the equality below compares Hashes on both sides. requested = nil unless requested.is_a?(Hash) - # A blank value is one the URL cannot round-trip: an empty Array or Hash is dropped - # by to_query entirely, and ?q[k]= comes back in with the blank stripped - redirecting - # on either alternates with the bare URL forever. Blanks stay out of the redirect. - effective = effective.reject { |_, value| value != false && value.blank? } + effective = Redirect.canonical_filters(effective) + requested = Redirect.canonical_filters(requested) if requested # One redirect, not a loop, whatever #filter_defaults_apply? has been widened to: once # the query string says exactly what the search runs, the two sides are equal and the diff --git a/lib/active_admin_filter_extensions/required_filters.rb b/lib/active_admin_filter_extensions/required_filters.rb index 031c625..05a9837 100644 --- a/lib/active_admin_filter_extensions/required_filters.rb +++ b/lib/active_admin_filter_extensions/required_filters.rb @@ -48,12 +48,36 @@ def required_conditions(attribute, options) # whose unless: keys. def required_filter_defaults_absent_from(requested) required_filter_default_values.each_with_object({}) do |(_attribute, entry), result| - next if entry[:keys].any? { |key| requested[key].present? } - next if entry[:if].any? && entry[:if].none? { |key| requested[key].present? } - next if entry[:unless].any? { |key| requested[key].present? } + next if required_group_answered?(requested, entry[:keys]) + next if entry[:if].any? && entry[:if].none? { |key| answered_value?(requested[key]) } + next if entry[:unless].any? { |key| answered_value?(requested[key]) } result.merge!(entry[:defaults]) end end + + # Any predicate of the filter's own attribute is an answer about it - the admin picking + # "equals" on a :string filter submits title_eq where the derived key is title_cont, and + # completing that from the default searches two conditions on one column. Read exactly the + # way #any_declared_filter_answered? reads a question: an exact declared key, or a real + # predicate on the declared attribute - never a predicate-less stem Ransack ignores, which + # would switch a mandatory window off with a key that searches nothing. + def required_group_answered?(requested, keys) + bases = keys.map { |key| strip_ransack_predicate(key) } + + requested.any? do |key, value| + next false unless answered_value?(value) + + key = key.to_s + next true if keys.include?(key) + + key != strip_ransack_predicate(key) && bases.include?(strip_ransack_predicate(key)) + end + end + + # `false` is a value, not a blank - the rule the normalized request already follows. + def answered_value?(value) + value.present? || value == false + end end end diff --git a/lib/active_admin_filter_extensions/search_keys.rb b/lib/active_admin_filter_extensions/search_keys.rb index 0e7b855..1ee1210 100644 --- a/lib/active_admin_filter_extensions/search_keys.rb +++ b/lib/active_admin_filter_extensions/search_keys.rb @@ -29,7 +29,11 @@ def filter_search_keys(attribute, options) end names.map { |name| name.to_s[/\Aq\[([^\]]+)\]/, 1] || name.to_s } - rescue StandardError => e + # Narrow on purpose: #policy_search_keys answers an ArgumentError from here by searching + # the declared filter name instead, so a catch-all would turn any internal failure into a + # policy quietly guarding the wrong key. A resource with no `ransack`, and an input class + # that does not exist, are both NameErrors - and they are what "cannot derive" means. + rescue NameError => e raise_filter_default_error(attribute, "could not work out which search key it submits (#{e.class}: #{e.message}) - name the predicate with a Hash instead") end @@ -46,7 +50,7 @@ def build_filter_input(attribute, options) # a way nothing reports: `Proc#[]` is `call`, so Formtastic asking `input_html[:multiple]` # invokes it, gets a truthy Hash back, and a `:select` derives `_in` rather than `_eq`. def filter_input_options(options, builder) - options = options.except(*FilterOptions::NOT_FOR_INPUT, *FilterOptions::EXTENSION_KEYS) + options = FilterOptions.input_options(options) return options unless options[:input_html].is_a?(Proc) options.merge(input_html: builder.template.instance_exec(&options[:input_html])) diff --git a/spec/data_access_spec.rb b/spec/data_access_spec.rb index 49b9c0f..ac9e70c 100644 --- a/spec/data_access_spec.rb +++ b/spec/data_access_spec.rb @@ -174,6 +174,44 @@ def controller.filter_defaults_apply? = true end end + describe "a required filter answered with false" do + # Rack never delivers a boolean - `?q[starred_eq]=false` arrives as the String - but an + # override of #filtering_params can, and `false` is an answer everywhere else in the gem. + let(:filters) { { starred_eq: { extensions: { default: true, required: true } } } } + let(:params) { { q: { "starred_eq" => false } } } + + it "is answered, so no default engages" do + expect(controller.send(:filter_defaults_engaged?)).to be(false) + end + end + + describe "key derivation" do + # Deriving builds a real input, so the unit suite reaches it through a stub. A catch-all + # rescue turned every internal failure into "could not work out which search key it + # submits", which policy_search_keys then answered with a key of its own invention. + def deriving(error) + Class.new do + include ActiveAdminFilterExtensions::SearchKeys + define_method(:build_filter_input) { |_attribute, _options| raise error } + end.new + end + + it "reports an input it cannot build" do + # No `ransack` on the resource - what a proxy page backed by an HTTP API answers. + controller = deriving(NoMethodError.new("undefined method 'ransack'")) + + expect { controller.send(:filter_search_keys, :created_at, {}) } + .to raise_error(ArgumentError, /could not work out which search key it submits/) + end + + it "lets a failure that is not a derivation failure through" do + controller = deriving(RuntimeError.new("boom")) + + expect { controller.send(:filter_search_keys, :created_at, {}) } + .to raise_error(RuntimeError, "boom") + end + end + describe "a policy on a filter whose search key cannot be derived" do # The derivation needs a resource Ransack can search; a proxy page backed by an HTTP API # has none, and a policy must not turn every index of such a page into a 500. The declared @@ -193,5 +231,22 @@ def filter_search_keys(_attribute, _options) controller.send(:enforce_filter_value_policies) expect(controller.params[:q]["time_start_gteq"]).to eq("2026-01-01") end + + context "when the filter declares a window policy" do + # The fallback gives one key, and a window policy demands two - so the very page the + # fallback keeps alive raised "needs a two-ended input" on every request instead. + let(:filters) { { time_start_gteq: { extensions: { max_range: 86_400 } } } } + let(:params) { { q: { "time_start_gteq" => "2026-01-01" } } } + + it "logs and skips the window instead of raising" do + warnings = [] + logger = Object.new + logger.define_singleton_method(:warn) { |message| warnings << message } + controller.define_singleton_method(:policy_logger) { logger } + + expect { controller.send(:enforce_filter_value_policies) }.not_to raise_error + expect(warnings.first).to match(/could not be derived/) + end + end end end diff --git a/spec/integration/extensions_container_spec.rb b/spec/integration/extensions_container_spec.rb index e266df4..8fb8ef4 100644 --- a/spec/integration/extensions_container_spec.rb +++ b/spec/integration/extensions_container_spec.rb @@ -6,11 +6,31 @@ # are loud: a gem key left flat would otherwise mean something else to Formtastic (required:), # and a typo inside would silently guard nothing. RSpec.describe "the extensions: container", type: :feature do + it "reads a string-keyed container" do + # The unknown-key check symbolizes before comparing, so "default" => ... passed it and then + # merged under a String key the gem never reads: a declared default that quietly did nothing. + Post.create!(title: "keep me", status: "published", position: 20) + Post.create!(title: "drop me", status: "draft", position: 1) + + visit "/admin/string_keyed_extensions_posts" + + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + it "raises for a gem option left outside extensions:" do expect { visit "/admin/banana_flat_option_posts" } .to raise_error(ArgumentError, /:default.*belong inside extensions:/m) end + it "leaves a flat required: and min: to Formtastic" do + # `filter :position, as: :numeric, min: 0` is legal Active Admin. Annexed, the gem read + # min: as a window policy on a single-ended input and 500'd every index of the resource. + visit "/admin/host_option_posts" + + expect(page).to have_css("input[name='q[position]'][min='0']") + end + it "raises for an unknown key inside extensions:" do expect { visit "/admin/banana_policy_key_posts" } .to raise_error(ArgumentError, /unknown extensions: :defult/m) diff --git a/spec/integration/filter_defaults_spec.rb b/spec/integration/filter_defaults_spec.rb index 8167160..74d55e3 100644 --- a/spec/integration/filter_defaults_spec.rb +++ b/spec/integration/filter_defaults_spec.rb @@ -52,6 +52,14 @@ expect { visit "/admin/ambiguous_posts" } .to raise_error(ArgumentError, /filter :published_date declares a `default:`.*single value cannot say which to fill/m) end + + it "says so for an exclusive Range instead of widening it" do + # The inputs submit gteq and lteq, so an excluded end has no spelling: stored as lteq it + # searched the day the Range left out, and the redirect froze that bound into every + # copied URL. Every other Range ambiguity here raises; this one changed data quietly. + expect { visit "/admin/banana_exclusive_range_posts" } + .to raise_error(ArgumentError, /exclusive Range.*use an inclusive range/m) + end end # Filtering the collection is only half of it: the admin has to be able to see what the page diff --git a/spec/integration/on_invalid_spec.rb b/spec/integration/on_invalid_spec.rb index bb98cae..7f9460b 100644 --- a/spec/integration/on_invalid_spec.rb +++ b/spec/integration/on_invalid_spec.rb @@ -50,6 +50,16 @@ def query_of(url) page.driver.header("Referer", nil) end + it "degrades to :reset off the HTML path" do + # A CSV index is answered in one request everywhere else in the gem, a flash it cannot + # render would be lost, and url_for drops the format - so the export is bounded the + # :reset way instead of being bounced into an HTML page. + visit "/admin/reject_posts.csv?q%5Bposition_eq%5D=abc" + + expect(page.response_headers["Content-Type"]).to include("text/csv") + expect(page.body).to include("keep me") + end + it "bounces once when value and window policies are violated together" do visit "/admin/reject_window_posts?q%5Bpublished_date_gteq%5D=2020-01-01X&q%5Bpublished_date_lteq%5D=2026-01-01X" @@ -84,6 +94,16 @@ def query_of(url) expect(page).to have_no_content("drop me") end + it "validates array elements emptied by the sanitizer and preserves the original input" do + visit "/admin/empty_digits_posts?q%5Bposition_eq%5D%5B%5D=abc" + + expect(page).to have_content("filter has an invalid value") + expect(query_of(page.current_url)["q"]).to eq("position_eq" => ["abc"]) + expect(page).to have_field("q[position_eq]", with: "0") + expect(page).to have_no_content("keep me") + expect(page).to have_no_content("drop me") + end + it "leaves a valid search alone" do visit "/admin/empty_posts?q%5Btitle_cont%5D=drop" diff --git a/spec/integration/policies_spec.rb b/spec/integration/policies_spec.rb index 7c8ca73..4dcd085 100644 --- a/spec/integration/policies_spec.rb +++ b/spec/integration/policies_spec.rb @@ -93,6 +93,65 @@ def query_of(url) end end + describe "a multi-valued filter" do + it "lets its Array submission through" do + visit "/admin/policed_boxes_posts?q%5Bstatus_in%5D%5B%5D=draft" + + expect(page).to have_no_content("No x statuses.") + expect(page).to have_content("drop me") + expect(page).to have_no_content("keep me") + end + + it "judges each element and resets the key on a bad one" do + visit "/admin/policed_boxes_posts?q%5Bstatus_in%5D%5B%5D=draft&q%5Bstatus_in%5D%5B%5D=xdraft" + + expect(page).to have_content("No x statuses.") + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "ignores a blank element beside a real selection" do + # ?q[status_in][]=&q[status_in][]=draft is the shape a multi-select submits, and + # normalization drops the blank before searching - judged, it flashed an error about a + # value nothing searches and reset a legitimate selection to the default. + visit "/admin/policed_boxes_posts?q%5Bstatus_in%5D%5B%5D=&q%5Bstatus_in%5D%5B%5D=draft" + + expect(page).to have_no_content("Letters only.") + expect(page).to have_content("drop me") + expect(page).to have_no_content("keep me") + end + + it "removes blank elements before sanitizing a multi-valued filter" do + visit "/admin/integer_sanitized_boxes_posts?q%5Bposition_in%5D%5B%5D=&q%5Bposition_in%5D%5B%5D=20" + + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "still refuses a Hash-wrapped value on the same filter" do + visit "/admin/policed_boxes_posts?q%5Bstatus_in%5D%5Bvalue%5D=draft" + + expect(page).to have_content("has an invalid value") + end + + it "ignores a blank member of an Array default" do + # nil became "", the validator refused it, and materialization raised on every request - + # for exactly the admins whose per-admin member resolved to nothing. + visit "/admin/nil_member_policed_posts" + + expect(query_of(page.current_url)["q"]).to eq("status_in" => ["published"]) + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "materializes an Array default beside its validator" do + visit "/admin/policed_boxes_posts" + + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + end + describe "message: overrides the violation text" do it "speaks the filter's own words for a built-in validator" do visit "/admin/ip_guarded_posts?q%5Btitle_cont%5D=not-an-ip" @@ -108,6 +167,34 @@ def query_of(url) expect(page).to have_no_content("translation missing") end + it "interpolates the override" do + # Used verbatim, the app's own words could never name %{limit} or %{floor}. + visit "/admin/interpolated_message_posts?q%5Bpublished_date_gteq%5D=2020-01-01" \ + "&q%5Bpublished_date_lteq%5D=2026-01-01" + + expect(page).to have_content("Pick at most 90 days.") + end + + it "does not refuse a blank default" do + # Blank is no value: the URL cannot carry it and Ransack would strip it, so judging it + # raised over a default that never reaches a search. + visit "/admin/blank_default_policed_posts" + + expect(URI.parse(page.current_url).query).to be_nil + expect(page).to have_content("keep me") + expect(page).to have_content("drop me") + end + + it "falls back to the generic text for a parameterized symbol it cannot interpolate" do + # The docs invite a validate: that returns a gem scope symbol; the parameterized ones + # have nothing to interpolate from, and raising there 500s the request the policy is + # meant to answer. + visit "/admin/parameterized_symbol_posts?q%5Btitle_cont%5D=keep123" + + expect(page).to have_content("Title cont filter has an invalid value.") + expect(page).to have_no_content("translation missing") + end + it "stays quiet for a valid value" do visit "/admin/ip_guarded_posts?q%5Btitle_cont%5D=10.0.0.0%2F8" diff --git a/spec/integration/redirect_spec.rb b/spec/integration/redirect_spec.rb index 41e4897..81ee25e 100644 --- a/spec/integration/redirect_spec.rb +++ b/spec/integration/redirect_spec.rb @@ -112,6 +112,16 @@ def query_of(url) expect(page).to have_content("drop me") end + it "does not chase an Array default whose members are all blank" do + # [nil] survives a plain blank-reject, serializes to ?q[k][]= and parses back as [""] - + # which normalization drops, so the defaults re-apply and the two sides never agree. + visit "/admin/nil_member_default_posts" + + expect(URI.parse(page.current_url).query).to be_nil + expect(page).to have_content("keep me") + expect(page).to have_content("drop me") + end + it "does not redirect a resource that declares no defaults" do visit "/admin/no_defaults_posts" diff --git a/spec/integration/require_any_filter_spec.rb b/spec/integration/require_any_filter_spec.rb index bace84a..1b194ac 100644 --- a/spec/integration/require_any_filter_spec.rb +++ b/spec/integration/require_any_filter_spec.rb @@ -21,6 +21,16 @@ expect(page).to have_no_content("drop me") end + it "renders an empty result, not Active Admin's blank slate" do + # The guard blanks the collection; with no `q` in the request Active Admin reads that as + # "this table has no rows at all" and offers a Create link beside the warning. + visit "/admin/picky_posts" + + expect(page).to have_content("No Picky Posts found") + expect(page).to have_no_content("There are no Picky Posts yet") + expect(page).to have_no_link("Create one") + end + it "answers once asked" do visit "/admin/picky_posts?q%5Btitle_cont%5D=keep" @@ -69,6 +79,34 @@ expect(page).to have_no_content("keep me") end + it "counts a scope tab as asked" do + # Active Admin puts a count on the tab, so rendering it empty beside "specify at least one + # filter" contradicted the tab the admin had just clicked. + visit "/admin/picky_posts?scope=published" + + expect(page).to have_no_content("You must specify at least one filter") + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "treats a scope no resource declares as unasked" do + # Active Admin resolves an unknown scope id to nothing and applies no scope, so counting + # its mere presence hands back the whole table behind a scope-shaped URL. + visit "/admin/picky_posts?scope=does_not_exist" + + expect(page).to have_content("You must specify at least one filter") + expect(page).to have_no_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "treats a scope no resource declares as unasked on CSV too" do + # The format that gets no canonicalizing hop - and no warning to read either. + visit "/admin/picky_posts.csv?scope=does_not_exist" + + expect(page.body).not_to include("keep me") + expect(page.body).not_to include("drop me") + end + it "counts a sibling predicate of a declared filter as asked" do visit "/admin/picky_posts?q%5Btitle_eq%5D=keep%20me" @@ -76,6 +114,13 @@ expect(page).to have_content("keep me") end + it "falls back to the shipped text for a notice key no locale defines" do + visit "/admin/banana_notice_key_posts" + + expect(page).to have_content("You must specify at least one filter") + expect(page).to have_no_content("translation missing") + end + it "notices when a policy just reset the only question" do # The validator empties q; without re-checking afterwards the whole table renders with no # warning - the unfiltered dump the guard was declared to forbid. diff --git a/spec/integration/required_spec.rb b/spec/integration/required_spec.rb index 6a31a19..bb8e5ca 100644 --- a/spec/integration/required_spec.rb +++ b/spec/integration/required_spec.rb @@ -30,6 +30,25 @@ def query_of(url) expect(page).to have_no_content("drop me") end + it "counts a sibling predicate of its own filter as an answer" do + # Judged on the derived key alone, the default merged in beside the admin's own condition: + # two conditions on one column, both in the canonical URL, and the notice greeting it. + visit "/admin/required_sibling_posts?q%5Btitle_eq%5D=drop%20me" + + expect(query_of(page.current_url)["q"]).to eq("title_eq" => "drop me") + expect(page).to have_content("drop me") + expect(page).to have_no_content("Showing kept titles by default") + end + + it "still applies beside a predicate-less stem Ransack ignores" do + # q[title] is no condition at all - counting it would switch a mandatory window off with a + # key that searches nothing. + visit "/admin/required_sibling_posts?q%5Btitle%5D=drop" + + expect(query_of(page.current_url)["q"]).to include("title_cont" => "keep") + expect(page).to have_content("keep me") + end + it "applies even to a blank filters-form submission" do # A required window has no 'show me everything' escape - that is what required means. visit "/admin/required_posts?commit=Filter" diff --git a/spec/integration/structural_keys_spec.rb b/spec/integration/structural_keys_spec.rb index 59305b2..e1894b4 100644 --- a/spec/integration/structural_keys_spec.rb +++ b/spec/integration/structural_keys_spec.rb @@ -47,6 +47,26 @@ def query_of(url) expect(page.body).not_to include("drop me") end + it "leaves a plain resource's own grouping alone" do + # q[g] is the standard Active Admin workaround for an OR search. On a resource that + # declares no extension there is no policy to bypass, and stripping it answered a narrowed + # export with the whole table. + visit "/admin/plain_posts.csv?q%5Bg%5D%5B0%5D%5Btitle_cont%5D=keep" + + expect(page.body).to include("keep me") + expect(page.body).not_to include("drop me") + end + + it "keeps a Ransack sort, which is no condition" do + # Stripped, q[s] was dropped from the search and erased from the canonical URL - the 302 + # handed back a link that had lost the sort it was made for. + visit "/admin/posts?q%5Bs%5D=title+desc" + + expect(query_of(page.current_url)["q"]) + .to eq("s" => "title desc", "status_eq" => "published", + "published_date_gteq" => "2026-01-01") + end + it "strips q[g] groupings no value policy ever saw" do visit "/admin/guarded_posts?q%5Bg%5D%5B0%5D%5Btitle_cont%5D=keep123" diff --git a/spec/integration/window_spec.rb b/spec/integration/window_spec.rb index 3f51268..198e268 100644 --- a/spec/integration/window_spec.rb +++ b/spec/integration/window_spec.rb @@ -63,6 +63,25 @@ def visit_window(gteq: nil, lteq: nil) .to eq("published_date_gteq" => "2026-05-01", "published_date_lteq" => "2026-06-30") end + it "names a sub-day cap in the units it was declared in" do + # Divided into whole days, a cap shorter than one read "a maximum date range of 0 days". + visit "/admin/short_window_posts?q%5Bpublished_date_gteq%5D=2026-01-01" \ + "&q%5Bpublished_date_lteq%5D=2026-06-30" + + expect(page).to have_content("maximum date range of 6 hours") + expect(page).to have_no_content("of 0 days") + end + + it "imposes nothing when a min: Proc resolves to nil" do + # An upper bound alone: both floor lookups are on this path, and nil.in_time_zone 500'd + # the request the floor was only meant to narrow. + visit "/admin/nil_floor_posts?q%5Bpublished_date_lteq%5D=2026-06-30" + + expect(page).to have_no_content("cannot start before") + expect(page).to have_content("keep me") + expect(page).to have_content("drop me") + end + it "resets the whole pair when a value policy rejects one bound" do # Half the admin's window completed from the default pair is a window nobody asked for # and nothing validated - the CSV would execute it without the HTML's healing hop. diff --git a/spec/integration/with_spec.rb b/spec/integration/with_spec.rb index 7d76ddd..1c05618 100644 --- a/spec/integration/with_spec.rb +++ b/spec/integration/with_spec.rb @@ -60,6 +60,51 @@ def query_of(url) expect(page.body).not_to include("drop me") end + it "counts a two-ended member answered by either bound" do + # Each bound counted as a group key of its own, so one date bound beside the partner read + # as two of three: both reset, and with no defaults the whole table rendered - the scan the + # guard exists to prevent. + visit "/admin/paired_date_posts?q%5Bpublished_date_gteq%5D=2026-01-01&q%5Bposition_eq%5D=20" + + expect(page).to have_no_content("must be given together") + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "counts a two-ended member whose default fills only one end" do + # Same arithmetic on the fallback side: the half-open default answered one of three keys + # and read as a half-declared group. + visit "/admin/open_paired_posts" + + expect(query_of(page.current_url)["q"]) + .to eq("published_date_gteq" => "2026-05-01", "position_eq" => "20") + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "keeps separately declared partners on one attribute distinct" do + # Two single-ended filters are two members, not one two-ended input: grouping them by the + # stripped attribute made half an answer pass as a whole group. + visit "/admin/split_pair_posts?q%5Bposition_gteq%5D=10" + + expect(page).to have_content("must be given together") + expect(page).to have_content("keep me") + expect(page).to have_content("drop me") + end + + it "lets both halves of a split pair through" do + visit "/admin/split_pair_posts?q%5Bposition_gteq%5D=10&q%5Bposition_lteq%5D=30" + + expect(page).to have_no_content("must be given together") + expect(page).to have_content("keep me") + expect(page).to have_no_content("drop me") + end + + it "raises when only one half of a split pair declares a default" do + expect { visit "/admin/banana_split_pair_posts" } + .to raise_error(ArgumentError, /answer the whole\s+group or none/m) + end + it "lets a fully-answered group through" do visit "/admin/paired_posts?q%5Bstatus_eq%5D=draft&q%5Bstarred_eq%5D=false" @@ -94,6 +139,27 @@ def query_of(url) .to eq("title_cont" => "keep", "status_eq" => "published", "starred_eq" => "false") end + it "imposes nothing when a member's default resolves to nil" do + # A nil Proc result is how a per-admin default opts out. Read as a missing declaration it + # raised the half-group config error on every bare visit, per admin. + visit "/admin/nil_paired_posts" + + expect(page).to have_no_content("must be given together") + expect(URI.parse(page.current_url).query).to be_nil + expect(page).to have_content("keep me") + expect(page).to have_content("drop me") + end + + it "says nothing about a default slice the suppressed group is not showing" do + # The group imposed nothing, so the collection is unfiltered - and the notice announced a + # default slice over the whole table, which is the notice lying about the page. + visit "/admin/nil_paired_posts" + + expect(page).to have_no_content("Showing published starred posts by default") + expect(page).to have_content("keep me") + expect(page).to have_content("drop me") + end + it "raises when defaults answer only half the group" do # The fallback is held to the same all-or-none contract the enforcement imposes. expect { visit "/admin/banana_half_paired_posts" } diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 96a4dc4..07dc014 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -32,6 +32,7 @@ class FakeController # The resource settings the policy layer reads unconditionally in a real install. Config = Struct.new(:filters) do def require_any_filter_notice = nil + def default_filters_notice = nil end def self.before_action(*); end # the guard registers one; FakeController has no chain diff --git a/spec/support/rails_template.rb b/spec/support/rails_template.rb index d981541..bf64ad8 100644 --- a/spec/support/rails_template.rb +++ b/spec/support/rails_template.rb @@ -95,6 +95,36 @@ def current_admin_user filter :status, as: :check_boxes, collection: %w[draft published], extensions: { default: ["published"] } end + ActiveAdmin.register Post, as: "PolicedBoxesPost" do + # A multi-valued filter under a value policy: _in[] submits an Array of Strings, which + # is the shape this input legitimately has - not a bypass. + filter :status, as: :check_boxes, collection: %w[draft published], + extensions: { default: ["published"], + validate: lambda { |value| + next "Letters only." unless value.match?(/\\A[a-z]+\\z/) + + "No x statuses." if value.start_with?("x") + } } + end + + ActiveAdmin.register Post, as: "IntegerSanitizedBoxesPost" do + filter :position, as: :check_boxes, collection: [1, 20], + extensions: { sanitize: ->(value) { Integer(value, 10).to_s }, validate: :integer } + end + + ActiveAdmin.register Post, as: "NilMemberPolicedPost" do + # An Array default with a blank member beside a real one - `[current_admin_user.team, + # "published"]` for an admin with no team. The blank is not a value the request would + # search, so it is not one the validator should judge. + filter :status, as: :check_boxes, collection: %w[draft published], + extensions: { default: [nil, "published"], + validate: lambda { |value| + next "Letters only." unless value.match?(/\\A[a-z]+\\z/) + + "No x statuses." if value.start_with?("x") + } } + end + ActiveAdmin.register Post, as: "BooleanPost" do filter :starred, extensions: { default: true } end @@ -119,6 +149,13 @@ def current_admin_user filter :published_date, as: :date_range, extensions: { default: Date.new(2026, 1, 1) } end + ActiveAdmin.register Post, as: "BananaExclusiveRangePost" do + # The excluded end has no gteq/lteq spelling - the suite proves that raises instead of + # searching the day the Range left out. + filter :published_date, as: :date_range, + extensions: { default: -> { Date.new(2026, 1, 1)...Date.new(2026, 2, 1) } } + end + ActiveAdmin.register Post, as: "ProcInputHtmlPost" do filter :status, as: :select, collection: %w[draft published], input_html: proc { { class: "select2" } }, extensions: { default: "published" } @@ -142,6 +179,21 @@ def current_admin_user filter :position, as: :numeric, extensions: { default: { in: -> { [] } } } end + ActiveAdmin.register Post, as: "NilMemberDefaultPost" do + # `default: -> { [current_admin_user.team_id] }` for an admin with no team: an Array whose + # only member is blank survives a plain blank-reject but cannot round-trip through a URL. + filter :status, as: :check_boxes, collection: %w[draft published], + extensions: { default: -> { [nil] } } + end + + ActiveAdmin.register Post, as: "BlankDefaultPolicedPost" do + # A blank default beside a validate:. Blank is no value - the URL cannot even carry it - + # so there is nothing for the validator to refuse. + filter :title_cont, as: :string, + extensions: { default: "", + validate: ->(value) { "Letters only." unless value.match?(/\\A[a-z]+\\z/) } } + end + ActiveAdmin.register Post, as: "BananaBlankDefaultPost" do # A blank default is a value ?q[k]= cannot round-trip (the blank is stripped on the way # back in) - it must stay out of the redirect, not loop it. @@ -213,6 +265,14 @@ def filter_defaults_apply? extensions: { default: "published", required: { if: :title_cont } } end + ActiveAdmin.register Post, as: "RequiredSiblingPost" do + default_filters_notice "Showing kept titles by default" + + # A :string filter carries the predicate dropdown: picking "equals" submits title_eq + # where the derived key is title_cont. Both are answers about the same column. + filter :title, extensions: { default: "keep", required: true } + end + ActiveAdmin.register Post, as: "RequiredBananaPost" do # required with nothing to fall back on - the suite proves that raises. filter :title_cont, as: :string, extensions: { required: true } @@ -245,6 +305,22 @@ def filter_defaults_apply? default: -> { Date.new(2026, 5, 1)..Date.new(2026, 6, 30) } } end + ActiveAdmin.register Post, as: "ShortWindowPost" do + # A cap shorter than a day - reported in whole days it read "0 days". + filter :published_date, as: :date_range, extensions: { max_range: 6.hours } + end + + ActiveAdmin.register Post, as: "StringKeyedExtensionsPost" do + # A string-keyed container: accepted by the unknown-key check and then silently inert. + filter :title_cont, as: :string, extensions: { "default" => "keep" } + end + + ActiveAdmin.register Post, as: "NilFloorPost" do + # A per-admin retention floor that opts out for this admin - nil imposes nothing, the way + # a nil default: does. + filter :published_date, as: :date_range, extensions: { min: -> { nil } } + end + ActiveAdmin.register Post, as: "ValidatedWindowPost" do # Value AND window policies with a default: a scalar reset must not leave half the # admin's window to be completed from the default pair unchecked. @@ -268,6 +344,12 @@ def filter_defaults_apply? filter :title_cont, as: :string, default: "keep" end + ActiveAdmin.register Post, as: "HostOptionPost" do + # required: and min: are Formtastic input options in their own right. The gem answers to + # them only inside extensions:, so flat they belong to the form - not to a window policy. + filter :position, as: :number, required: true, min: 0 + end + ActiveAdmin.register Post, as: "BananaPolicyKeyPost" do # A typo inside extensions: raises instead of silently guarding nothing. filter :title_cont, as: :string, extensions: { defult: "keep" } @@ -369,6 +451,16 @@ def filter_defaults_apply? filter :starred end + ActiveAdmin.register Post, as: "NilPairedPost" do + default_filters_notice "Showing published starred posts by default" + + # A member of a with: group whose default opts out for this admin. Declared, so the group + # is complete; nil, so it imposes nothing - and neither does the partner. + filter :status, as: :select, collection: %w[draft published], + extensions: { default: "published", with: :starred_eq } + filter :starred, extensions: { default: -> { nil } } + end + ActiveAdmin.register Post, as: "BananaHalfPairedPost" do # Half the with: group declares a default - the fallback would execute the very # half-group the enforcement refuses. The suite proves that raises. @@ -377,6 +469,20 @@ def filter_defaults_apply? filter :starred end + ActiveAdmin.register Post, as: "SplitPairPost" do + # Two single-ended filters on ONE attribute, linked by with:. Grouped by stripped + # attribute they look like one two-ended input, so half an answer passed as a whole group. + filter :position_gteq, as: :string, extensions: { with: :position_lteq } + filter :position_lteq, as: :string + end + + ActiveAdmin.register Post, as: "BananaSplitPairPost" do + # Only one half of a split pair declares a default - the fallback would run the very + # half-group the enforcement refuses. The suite proves that raises. + filter :position_gteq, as: :string, extensions: { default: "10", with: :position_lteq } + filter :position_lteq, as: :string + end + ActiveAdmin.register Post, as: "PairedDatePost" do # Declared on ONE member - the partner's side must trip the same group - and on a # two-ended filter, whose predicates the warning must still humanize away. @@ -384,10 +490,21 @@ def filter_defaults_apply? filter :position end + ActiveAdmin.register Post, as: "OpenPairedPost" do + # A two-ended member of a with: group whose default fills only the end it has - the group + # is answered by that bound, not left half-declared. + filter :published_date, as: :date_range, + extensions: { default: -> { Date.new(2026, 5, 1).. }, with: :position_eq } + filter :position, extensions: { default: { eq: 20 } } + end + ActiveAdmin.register Post, as: "PickyPost" do # The active-calls shape: this index answers no question at all until it is asked one. require_any_filter notice: "You must specify at least one filter" + # A scope tab is a question too - and the one Active Admin puts a count on. + scope(:published) { |posts| posts.where(status: "published") } + filter :title_cont, as: :string, extensions: { validate: ->(value) { value.match?(/\\d/) && "Title must not contain digits." } } filter :status, as: :select, collection: %w[draft published] @@ -454,6 +571,27 @@ def filter_defaults_apply? extensions: { validate: ->(value) { value.match?(/\\d/) && :bogus_key } } end + ActiveAdmin.register Post, as: "ParameterizedSymbolPost" do + # A validate: answering one of the gem's own parameterized symbols - which the docs + # invite - with nothing to interpolate %{limit} from. + filter :title_cont, as: :string, + extensions: { validate: ->(value) { value.match?(/\\d/) && :range_too_wide } } + end + + ActiveAdmin.register Post, as: "InterpolatedMessagePost" do + # message: in the app's own words, naming the gem's own interpolation. + filter :published_date, as: :date_range, + extensions: { max_range: 90.days, + message: "Pick at most %{limit}." } + end + + ActiveAdmin.register Post, as: "BananaNoticeKeyPost" do + # require_any_filter named with a Symbol no locale defines. + require_any_filter notice: :no_such_notice_key + + filter :title_cont, as: :string + end + ActiveAdmin.register Post, as: "NoDefaultsPost" do filter :title end diff --git a/tasks/test.rake b/tasks/test.rake index 7686c20..76bd3de 100644 --- a/tasks/test.rake +++ b/tasks/test.rake @@ -17,6 +17,11 @@ task :setup do ].join(" ") app_dir = "spec/rails/rails-#{Rails::VERSION::STRING}" + # From scratch, always: `rails new` git-inits the app, so a nested repo survives + # `git clean -fdx` and the generator would then write into the surviving tree - leaving a + # stale resource behind a freshly stamped digest, which is a green the stamp exists to deny. + require "fileutils" + FileUtils.rm_rf(app_dir) sh "bundle exec rails new #{app_dir} #{args}" # Stamped so rails_helper can refuse an app generated from an older template instead of