Refactor how Rails handles filter categories passed to and from Vue #546

h-m-m · 2020-06-27T19:43:50Z

Why

The contribution filtering works nicely, but difficult to understand. We'd like to add new filters, and that's hard at the moment. I wrote the original code and am unhappy with how confusing it is. Its confusing complexity is what makes adding or editing filters more difficult for us, and also slows down bringing people into the project and up to speed on how all this stuff works.

What

The big change is making each type of filter its own class.
Each class

accepts an ActiveRecord relation
knows how to filter that relation by request parameters passed from the controller

Also used this opportunity to:

make the controller code easier to follow
simplify the logic around conditionally passing links to the frontend (do we show the link to contact someone? does the user have permission to see this?
work on our nomenclature for clarity & simplicity

And hopefully this will make adding new filters easier in the future

Testing

Updated some tests. Could probably use more work in this regard

Next Steps

Some next steps are already called out in the comments when this PR was in Draft status.
Also: add a filter for Date now that it should be easier to do so

Accessibility

We were very careful to leave the existing UI unchanged in any visible way, so this should be accessibility-neutral

Security

Hopefully this improves the security what with less dynamic/functional code and code that's easier for future devs to maintain

Pre-Merge Checklist

Security & accessibility have been considered
Tests have been added, or an explanation has been given why the features cannot be tested
Documentation and comments have been added to the codebase where required
Entry added to CHANGELOG.md if appropriate
Outstanding questions and concerns have been resolved
Any next steps have been turned into Issues or Discussions as appropriate

h-m-m

This is now functional

TODO:

add tests
check if everyone feels this is legible enough
add additional filters (maybe in a separate PR)
discuss sorting results on the Vue side

I added a bunch of comments. Please suggest cleanup/rewording/etc. of them, whatever would help you understand the code better

Sorry, @maebeale, for the flipping back and forth about what goes in instances and what goes in classes. I added on to your work some of the structure I thought @exbinary was interested in, but I might have misremembered that whole conversation

h-m-m · 2020-09-23T23:18:41Z

app/filters/contribution_type_filter.rb

+  def scopes
+    classes = parameters.blank? ? ALL_ALLOWED_TYPES : parameters.keys
+    classes.intersection(ALL_ALLOWED_TYPES).map do |type|
+      type.constantize.matchable


Is this right — that matchable is what we want here?

yes. we'll need to add that scope to CommunityResource to return objects that are still published

solebared

Looks great, @h-m-m ! A few comments inline but overall really like how this is shaping.

app/blueprints/filter_options_blueprint.rb

app/models/browse_filter.rb

solebared · 2020-10-01T03:06:05Z

app/filters/contact_method_filter.rb

+
+  def filter(scope)
+    return super unless parameters
+    scope.joins(:person).where(people: { preferred_contact_method: parameters.keys })


I'm noticing all the filters use parameters.keys. Could we get the keys in BrowseFilter or BaseFilter.new and make just the keys available to individual filters?

Relatedly, the shape of parameters is not clear from looking at the code. Could be solved by looking at specs, or maybe a comment describing how params will come in?

the shape of parameters is not clear from looking at the code

This is really the key point here. Thanks for surfacing this, because this point is helping me find the most confusing points.

I started writing some tests around this and got stuck because it was hard. I think I've introduced a small, modest mess in BrowseFilter#contributions and cleaning up the parameters structure here is dependent on cleaning that up, too.

There's a part of me that wants to get this PR out first and then focus on cleaning up that problem. I want to do that because this PR has taken a very long time and not directly for any code-specific reasons. This PR seems so big to me already, and I'd really like to focus on the refactor I'll need here as a separate thing from the other changes in this PR.

I'm open to suggestions. Let me know what you think

solebared · 2020-10-01T03:23:39Z

app/filters/category_filter.rb

+    {
+      name: "Categories",
+      # Currently only filtering by top-level categories
+      filters: FilterOptionsBlueprint.render_as_hash(Category.roots)
+    }


Serializing inside the filters doesn't feel quite right. Is it possible to have options return a relation as the value of filters and let the controller handle serialization?

Suggested change

{

name: "Categories",

# Currently only filtering by top-level categories

filters: FilterOptionsBlueprint.render_as_hash(Category.roots)

}

{

name: "Categories",

# Currently only filtering by top-level categories

filters: Category.roots

}

This might require introducing another blueprint at the level of a filter that then has an association :filters.

My only issue with your suggestion is that the Ask/Offer distinction is one I'm doing manually:

mutual-aid/app/filters/contribution_type_filter.rb

Lines 1 to 7 in 39a0d09

class ContributionTypeFilter

def self.filter_group

{ name: 'Contribution Types', filter_options: [

{ id: 'ContributionType[Ask]', name: 'Ask' },

{ id: 'ContributionType[Offer]', name: 'Offer' }

]}

end

Ah yes, i see the challenge. I had a go at extracting the serialization out in 8b8c8a9, curious your thoughts?
In any case, i don't think this question should hold up this PR.

app/controllers/contributions_controller.rb

app/models/browse_filter.rb

app/controllers/contributions_controller.rb

We may want to later check the Vue side so that they're both consistent

Conflicts: * CHANGELOG.md * app/controllers/contributions_controller.rb * app/models/browse_filter.rb * app/models/category.rb * app/models/contribution_type.rb * spec/blueprints/contribution_blueprint_spec.rb * spec/blueprints/filter_type_blueprint_spec.rb

solebared

@h-m-m , i merged main, resolved conflicts and pushed to this branch.
Should be good to merge once you give it a quick look.

app/filters/contribution_type_filter.rb

h-m-m · 2021-05-15T14:02:14Z

Notes on what we're thinking of following up on as a subsequent refactoring:

Removing paramters.keys references in the filters so filter classes can scan the full parameter list as they see fit (comment
Move serialization concerns out of the filter classes, perhaps by breaking up filter_grouping in to one method that returns the filter grouping's name and another method that returns the filter options for that filter grouping (comment)

h-m-m force-pushed the filter-by-date branch from c091cec to 416b7a1 Compare June 27, 2020 19:44

solebared changed the base branch from master to main September 2, 2020 23:44

h-m-m force-pushed the filter-by-date branch from 2d217cf to cf69d4f Compare September 23, 2020 22:58

h-m-m mentioned this pull request Sep 23, 2020

Bug: Filters not partially working #727

Open

h-m-m force-pushed the filter-by-date branch from cf69d4f to 9bb4c30 Compare September 23, 2020 23:18

h-m-m commented Sep 23, 2020

View reviewed changes

solebared approved these changes Oct 1, 2020

View reviewed changes

h-m-m mentioned this pull request Mar 12, 2021

[skip ci] add initial draft of contribution filtering doc #902

Merged

4 tasks

h-m-m force-pushed the filter-by-date branch from 9bb4c30 to 8ce7722 Compare March 19, 2021 00:27

h-m-m force-pushed the filter-by-date branch 3 times, most recently from 8799453 to 39a0d09 Compare April 9, 2021 15:01

h-m-m commented Apr 10, 2021

View reviewed changes

app/controllers/contributions_controller.rb Outdated Show resolved Hide resolved

h-m-m commented Apr 10, 2021

View reviewed changes

app/models/browse_filter.rb Outdated Show resolved Hide resolved

h-m-m commented Apr 10, 2021

View reviewed changes

app/controllers/contributions_controller.rb Outdated Show resolved Hide resolved

Howard M. Miller and others added 9 commits April 15, 2021 19:59

refactor filter categories passed from Rails to Vue

05a7450

WIP: refactoring contribution filters

d5afa82

I think this is everything for the refactor

2d6a09b

rename FilterOptionsBlueprint to FilterOptionBlueprint

a14d4a3

move an HTML comment into a Ruby comment

52126ec

refactor names in Rails around filter groups

86ab4a1

We may want to later check the Vue side so that they're both consistent

simplify rails-to-vue flow of link URLs for each contribution

a9044d8

remove extra binding.pry

2715452

rename filterGroups to filterGroupings

63d505d

h-m-m force-pushed the filter-by-date branch from 39a0d09 to 63d505d Compare April 16, 2021 00:22

Howard M. Miller added 2 commits April 15, 2021 20:27

remove unused browse filter code

965d4b2

shorten lenghty BrowseFilter constant

5062a99

h-m-m force-pushed the filter-by-date branch 2 times, most recently from 0b47685 to 5062a99 Compare April 16, 2021 00:32

h-m-m marked this pull request as ready for review April 16, 2021 00:55

note filtering refactor in the changelog

a77c2a1

h-m-m force-pushed the filter-by-date branch from 88aa166 to a77c2a1 Compare April 16, 2021 01:01

h-m-m mentioned this pull request Apr 17, 2021

More filtering cleanup #945

Draft

6 tasks

solebared reviewed May 15, 2021

View reviewed changes

app/filters/contribution_type_filter.rb Outdated Show resolved Hide resolved

explain why ContributionTypeFilter violates Liskov substitution

dcfa193

h-m-m merged commit 23600d7 into main May 15, 2021

h-m-m deleted the filter-by-date branch May 15, 2021 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor how Rails handles filter categories passed to and from Vue #546

Refactor how Rails handles filter categories passed to and from Vue #546

h-m-m commented Jun 27, 2020 •

edited

Loading

h-m-m left a comment •

edited

Loading

h-m-m Sep 23, 2020

maebeale Apr 10, 2021

solebared left a comment

solebared Oct 1, 2020

h-m-m Apr 10, 2021

solebared Oct 1, 2020

h-m-m Apr 10, 2021 •

edited

Loading

solebared Apr 12, 2021

solebared left a comment

h-m-m commented May 15, 2021

	class ContributionTypeFilter
	def self.filter_group
	{ name: 'Contribution Types', filter_options: [
	{ id: 'ContributionType[Ask]', name: 'Ask' },
	{ id: 'ContributionType[Offer]', name: 'Offer' }
	]}
	end

Refactor how Rails handles filter categories passed to and from Vue #546

Refactor how Rails handles filter categories passed to and from Vue #546

Conversation

h-m-m commented Jun 27, 2020 • edited Loading

Why

What

Testing

Next Steps

Accessibility

Security

Pre-Merge Checklist

h-m-m left a comment • edited Loading

Choose a reason for hiding this comment

h-m-m Sep 23, 2020

Choose a reason for hiding this comment

maebeale Apr 10, 2021

Choose a reason for hiding this comment

solebared left a comment

Choose a reason for hiding this comment

solebared Oct 1, 2020

Choose a reason for hiding this comment

h-m-m Apr 10, 2021

Choose a reason for hiding this comment

solebared Oct 1, 2020

Choose a reason for hiding this comment

h-m-m Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

solebared Apr 12, 2021

Choose a reason for hiding this comment

solebared left a comment

Choose a reason for hiding this comment

h-m-m commented May 15, 2021

h-m-m commented Jun 27, 2020 •

edited

Loading

h-m-m left a comment •

edited

Loading

h-m-m Apr 10, 2021 •

edited

Loading