Fix Active Filter Labeling and Record Not Found #5223

wspurgin · 2017-10-24T20:14:05Z

👋

ActiveFilter was added to make the current filter sidebar a bit more humanized. It adds some cool things like smart, humanized filter names and, where possible, link to related resources with human readable names! Very cool. However, there's a few issues I saw when messing around a bit.

If a filter with a related class (as ActiveFilter defines it) is given conditions that have no matching records in the related class, ActiveFilter produces an ActiveRecord::RecordNotFound when trying to generate a set of values to display in the "Current Filters" sidebar.
If a filter is added to the resource that has a label specified, ActiveFilter ignores it in favor of pulling from i18n. This doesn't seem too terrible but can pose problems to the developer (see below)

The first issue I believe to be a regression. Even though, truthfully, there is no related record matching the filter criteria (and thus no resource records either), AA would display a blank slate in all other cases where a filter(s) produces no matching records.

The second issue, is more subtle. Most cases of custom labels are covered by the current implementation of ActiveFilter. After all, if you want the label of Post#title to be something other than "Title" just put it inside of en.yml, and ActiveFilter will do as you command. Here's the rub, imagine a more complex translation situation:

# en.yml
en:
   activerecord:
      attributes:
         user:
            external_id: "%{client_name} External ID" 
#...

ActiveAdmin.register User do
#...
filter :external_id, label: config.resource_class.human_attribute_name(:external_id, client_name: ENV["CLIENT"])
end

In this example, we have an interpolation within our translation because the translation changes based on who's bought our super shiny app. In this situation, ActiveFilter would produce a I18n::MissingInterpolationArgument when trying to render the Current Filter sidebar with this attribute.

While this example is a bit contrived, IMHO it's better to go with whatever the developer specifies as the attribute name. If the label on the filter is set, then that's takes precedence. If not, then the translations are certainly the next best thing.

Related issues:

ActiveFilters ignores the primary_key setting of an association #5164

zorab47 · 2017-10-25T14:33:40Z

lib/active_admin/filters/active_filter.rb

@@ -19,7 +19,7 @@ def initialize(resource, condition)
      def values
        condition_values = condition.values.map(&:value)
        if related_class
-          related_class.find(condition_values)
+          related_class.where(id: condition_values)


Using id here isn't ideal because the primary key column may be some other field on the record.

Other locations dynamically lookup the primary key:

activeadmin/lib/active_admin/resource.rb

Line 197 in 5b711b6

[:find_by, { resource_class.primary_key => id }]

Yup. Great catch. 🙈

wspurgin · 2017-11-03T12:19:20Z

@varyonic @zorab47 The coverage failure for the last commit looks like a false positive to me. But if the ✅ is desired, I can rejig that line so it is "covered" by codecov pov. Just let me know.

varyonic · 2017-11-03T17:55:30Z

Right, CodeCov doesn't get it when you split a method chain across more than two lines.

wspurgin · 2017-11-03T18:46:44Z

I amended the commit to put that method chain on one line then. 👍

wspurgin · 2017-11-06T21:03:19Z

@zorab47 I think you'll need to re-approve these changes (since I added the association primary key support). Also let me know if a rebase is necessary.

zorab47

Good catch on primary key

zorab47 · 2017-11-07T14:46:14Z

lib/active_admin/filters/active_filter.rb

-            reject { |r| r.options[:polymorphic] }. #skip polymorphic
-            detect { |r| r.foreign_key.to_s == name.to_s }
-          assoc.class_name.constantize if assoc
+          predicate_association.class_name.constantize if predicate_association


Should this become an elsif for easier reading?

It was like that previously, so I didn't know if it was convention to avoid if - elsif statements with no else.

zorab47 · 2017-11-07T14:46:21Z

lib/active_admin/filters/active_filter.rb

+        if predicate_association
+          predicate_association.active_record_primary_key
+        else
+          related_class.primary_key if related_class


Should this become an elsif for easier reading?

Like line 93, since a there were other places where it seemed that dangling if - elsif weren't preferred, I stuck with the if - else. But IMHO I agree that it should just be an if - elsif block

Fivell · 2017-11-07T07:54:48Z

lib/active_admin/filters/active_filter.rb

+        end
+      end
+
+      def predicate_association


@wspurgin maybe you can add cashing this method using instance_variable_defined?

Would you prefer instance_variable_defined? or just:

def predicate_association @predicate_association ||= find_predicate_association end def find_predicate_association #... end

wspurgin · 2017-11-07T15:50:39Z

@zorab47 @Fivell I updated the PR with clearer if-elsif blocks, and caching of the predicate association. Appreciate the feedback 👍

varyonic · 2017-11-08T03:23:00Z

lib/active_admin/filters/active_filter.rb

+
+        filter[:label]
+      end
+


def filter_label filter && filter[:label] end

varyonic · 2017-11-08T03:23:45Z

lib/active_admin/filters/active_filter.rb

+        if predicate_association
+          predicate_association.active_record_primary_key
+        elsif related_class
+          related_class.primary_key


Personally I like terse boolean operators over if/else:

def related_primary_key predicate_association && predicate_association.active_record_primary_key || related_class && related_class.primary_key end

Hurts readability though. ~~You'll still need a block to return the proper primary key (as that only returns a true or false~~. Scratch that. It returns the proper value, but I still think it hurts readability.

It's a personal preference, maybe from writing too much C or perl. :-)

Definitely understand. I think I leave it as is for now though. It'll help me understand what's going on more quickly if I look at in a month than chained binary operators. 🙈

varyonic

Looks reasonable, though I've not worked on this before. Yes, a rebase and squash corrections would be good.

Fivell · 2017-11-08T07:25:39Z

lib/active_admin/filters/active_filter.rb

        end
      end
+
+      def predicate_association
+        @predicate_association ||= find_predicate_association


@wspurgin since find_predicate_association can return nil,

method can look like

return @predicate_association if defined? @predicate_association @predicate_association ||= find_predicate_association

Gotcha. That way we don't iterate every time if their is no predicate association. I'll make that update.

= is sufficient if you know it's not defined. Alternately:

@predicate_association = find_predicate_association unless defined?(@predicate_association) @predicate_association

wspurgin · 2017-11-08T23:09:42Z

@varyonic Rebased.

wspurgin · 2017-11-09T16:36:07Z

@varyonic That failure in the CI looks completely unrelated to this PR. Looks broken elsewhere too. I'm guessing that's what #5237 is for?

Use label on the filter as the prefix (if given) Also move other prefixes' logic into separate methods for cleanliness. Support rails 4.2 in specs Use related class' primary key

Use association primary key when possible Closes activeadmin#5164 Use elsif to enhance readability Cache predicate_association Only search for predicate once

wspurgin · 2017-11-09T19:45:02Z

@varyonic rebased and squashed. Thanks for the fixing the builds! 💯

…1-1-stable

zorab47 reviewed Oct 25, 2017

View reviewed changes

zorab47 previously approved these changes Nov 2, 2017

View reviewed changes

wspurgin mentioned this pull request Nov 2, 2017

ActiveFilters ignores the primary_key setting of an association #5164

Closed

wspurgin dismissed zorab47’s stale review via c17be84 November 2, 2017 22:53

wspurgin force-pushed the fix-active-filter-labels-and-404ing branch from c17be84 to 76c5e68 Compare November 3, 2017 18:45

zorab47 previously approved these changes Nov 7, 2017

View reviewed changes

Fivell reviewed Nov 7, 2017

View reviewed changes

wspurgin dismissed zorab47’s stale review via a28e112 November 7, 2017 15:49

varyonic reviewed Nov 8, 2017

View reviewed changes

lib/active_admin/filters/active_filter.rb

filter[:label]

end

Copy link

Contributor

varyonic Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def filter_label filter && filter[:label] end

varyonic reviewed Nov 8, 2017

View reviewed changes

Fivell reviewed Nov 8, 2017

View reviewed changes

wspurgin force-pushed the fix-active-filter-labels-and-404ing branch from 32308be to ab9ca47 Compare November 8, 2017 23:09

Will Spurgin added 3 commits November 9, 2017 13:34

Avoid causing 404 errors with find

f18784d

Use given label on filter if available

89d992a

Use label on the filter as the prefix (if given) Also move other prefixes' logic into separate methods for cleanliness. Support rails 4.2 in specs Use related class' primary key

Use association primary key

3b154b1

Use association primary key when possible Closes activeadmin#5164 Use elsif to enhance readability Cache predicate_association Only search for predicate once

wspurgin force-pushed the fix-active-filter-labels-and-404ing branch from ab9ca47 to 3b154b1 Compare November 9, 2017 19:43

zorab47 approved these changes Nov 9, 2017

View reviewed changes

Fivell approved these changes Nov 10, 2017

View reviewed changes

varyonic merged commit dc7aeb0 into activeadmin:master Nov 10, 2017

zorab47 added a commit to zorab47/active_admin that referenced this pull request Dec 4, 2017

Backport activeadmin#5223, activeadmin#5240, and activeadmin#5238 to …

88168f0

…1-1-stable

zorab47 mentioned this pull request Dec 4, 2017

Backports for 1-1-stable #5269

Merged

wasifhossain mentioned this pull request Aug 8, 2018

Fix resource filtering by association when the resource has custom primary key #5446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Active Filter Labeling and Record Not Found #5223

Fix Active Filter Labeling and Record Not Found #5223

wspurgin commented Oct 24, 2017 •

edited

Loading

zorab47 Oct 25, 2017 •

edited

Loading

wspurgin Oct 25, 2017

wspurgin commented Nov 3, 2017

varyonic commented Nov 3, 2017

wspurgin commented Nov 3, 2017

wspurgin commented Nov 6, 2017

zorab47 left a comment

zorab47 Nov 7, 2017

wspurgin Nov 7, 2017

zorab47 Nov 7, 2017

wspurgin Nov 7, 2017

Fivell Nov 7, 2017

wspurgin Nov 7, 2017

wspurgin commented Nov 7, 2017

varyonic Nov 8, 2017

varyonic Nov 8, 2017

wspurgin Nov 8, 2017 •

edited

Loading

varyonic Nov 8, 2017

wspurgin Nov 8, 2017

varyonic left a comment •

edited

Loading

Fivell Nov 8, 2017

wspurgin Nov 8, 2017

varyonic Nov 8, 2017

wspurgin commented Nov 8, 2017

wspurgin commented Nov 9, 2017 •

edited

Loading

wspurgin commented Nov 9, 2017

Fix Active Filter Labeling and Record Not Found #5223

Fix Active Filter Labeling and Record Not Found #5223

Conversation

wspurgin commented Oct 24, 2017 • edited Loading

zorab47 Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wspurgin commented Nov 3, 2017

varyonic commented Nov 3, 2017

wspurgin commented Nov 3, 2017

wspurgin commented Nov 6, 2017

zorab47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wspurgin commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wspurgin Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varyonic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wspurgin commented Nov 8, 2017

wspurgin commented Nov 9, 2017 • edited Loading

wspurgin commented Nov 9, 2017

wspurgin commented Oct 24, 2017 •

edited

Loading

zorab47 Oct 25, 2017 •

edited

Loading

wspurgin Nov 8, 2017 •

edited

Loading

varyonic left a comment •

edited

Loading

wspurgin commented Nov 9, 2017 •

edited

Loading