Review fixes

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}"

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.

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"

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,31 +189,53 @@ 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: " \
               "policy - the fallback must be a value the filter accepts"
       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

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