sciencehistory / scihist_digicoll

Science History Institute Digital Collections
Other
13 stars 0 forks source link

Remove Ransack from collection model; use new component instead for table header links. #2744

Closed eddierubeiz closed 1 month ago

eddierubeiz commented 2 months ago

Ref #2450

Background Ransack did two things for us:

This PR separates these two tasks. The first is done in the controller; the second, inside a new component class.

More specifically:

1) Creating the list of collections to show: Searching, filtering, sorting and pagination happen entirely in the #index method, using sane and legible code.

2) Table header links:

Tradeoffs Since we intend to use this component in at least three (and likely more) admin controllers, this PR aims to simplify the model, controller and view, at the expense of slightly complicating the component.

Note: The #index method was not using permitted params, so we created an index_params method to return sanitized params that #index can use. These are then handed directly to the component code, along with keys that tell the component which keys important for what it's trying to do.

Note: Ransack search is currently broken in dev on the master branch. This is one of the many reasons we want to get rid of it, but it's easy to forget about. (Try going to http://localhost:3000/admin/works in dev on master and searching for Goat. You get an ArgumentError in Admin::WorksController#index with error message Invalid search term q unless you comment out c.ignore_unknown_conditions = Rails.env.production? in ransack.rb.

What this means If you are testing this PR in dev, and note that the search is broken on the admin works page, that's not because this PR broke it. It's because it is broken in master and this PR does not modify the admin works page.

jrochkind commented 2 months ago

I think this is a great approach, good work!

Some comments!