Remove ransack

[eddierubeiz]

Ref https://github.com/sciencehistory/scihist_digicoll/issues/2450

We were only using Ransack in two controllers. This takes care of the second one: the admin works controller.

Controller changes

• a one-line sort_link_maker method creates the link factory;
• a new index_params method validates and sanitizes params, returning a permitted strongparams object;
  • Genre, format, and department params are actually checked against their controlled vocabs;
  • The search phrase is sanitized. I don't believe it was before.
• Search and filtering happen in the #index method now;
  • The "new" code, which replaces what Ransack used to do for us, is ordinary ActiveRecord scope manipulation;
  • Several parameters are given more legible names (e.g. q_parent_id_null is now named include_child_works).

Other changes

• Removes the Ransack gem;
• Removes references to ransack in the Work model;
• View code is not substantially more complicated, aside from the abovementioned changes to param names.

Note This PR depends on #2744, which took care of the collections controller.

[jrochkind]

Very nice! More code than I expected to replace ransack searching -- but I see we had a lot of it before ANYWAY, becuase we were doing non-standard ransack things like jsonb searches anyway, we've just moved it around.

The search phrase is sanitized. I don't believe it was before.

Hm.... in many cases ActiveRecord will santize for us. .where(something: value) should be sanitized for us, as will .where("something operator ?", value). You only need to sanitize yourself if you are constructing SQL yourself, say with ruby string interpolation (instead of passing a value to an AR method), like something > #{my_value} -- that would need explicit sanitization.

I'm worried if you sanitize where not required, it could result in a "double escaping" issue for some input. I'm not actually sure where you added the sanitization, can we look at it and/or find a way to test it to ensure it's needed?

returning a permitted strongparams object

I don't think actually required -- strongparams is for when you're like taking ALL of the input params and giving them to something that's going to use everything that's there. When you are explicitly choosing keys like params[:some_key] it's not required -- but also shouldn't hurt?

[eddierubeiz]

@jrochkind, I'd be happy to look at SQL escaping together, yes! I do feel like we're doing things right in this case. This is easy to test: on current master, if you search for a percent sign or an underscore using this form, you get the same results as a blank search. Conversely, on this branch, searching for a percent sign returns only the four titles containing a percent sign.

[eddierubeiz]

Re: strongparams : actually, your use case does cover the use of strong params here. The component actually does use all the params, because the links we use need to preserve the state of the entire form. So this is a reasonable use for using strongparams.

[jrochkind]

Nice, thanks for checking escaping so carefully, I appreciate it!

Wow, I'm realizing you maybe ALWAYS need to sanitize/escape input for ActiveRecord "like" escapes, and I haven't realized that and haven't been doing it? I think this may be a really little known AR gotcha! That AR doesn't give you great tools for conveniently handling. Good job noticing and patching it!

Approved, great work.