fix deprecation warnings

[ebenenglish]

This PR addresses issues related to deprecation warnings that come up when using recent versions of Blacklight.

• #70 Move add_spatial_search_to_solr method to a Blacklight::SearchBuilder subclass module
• #71 Replace deprecated facet_by_field_name method with aggregations

The .gemspec has been updated to require Blacklight >= 5.12.0, and some information has been added to the README to indicate which versions of the gem work with which versions of Blacklight. (This assumes we will bump the version to 0.4.0 once this PR has been merged.)

[ebenenglish]

Ugh, all the test failures in the build are because of this same error:

ActionView::Template::Error:
undefined method `type' for .focus:Sass::Selector::Class
(in /home/travis/build/projectblacklight/blacklight-maps/spec/internal/app/assets/stylesheets/blacklight.css.scss)

This appears to be an issue with the version of sass used by bootstrap-sass, looks like DPLA ran into this recently as well.

Could pin bootstrap-sass version to 3.3.4.1, although it seems like recent blacklight builds are working OK with boostrap-sass 3.3.5.1 (builds appear to have same version of sass (3.4.15) and sass-rails (5.0.3)).

[ndushay]

fwiw, I just had success fixing the same error in the SearchWorks blacklight app (a branch of it, actually) with the DPLA fix.

[ebenenglish]

Thanks, @ndushay. I'm going to play around with changing the bootstrap-sass version and see what happens. (Can squash/modify commits later for a cleaner history.) Not sure I'm in love with the idea of pinning bootstrap-sass to a specific version.

[ebenenglish]

@mejackreed Any thoughts? Should we pin bootstrap-sass to 3.3.4.1?

[mejackreed]

I think it depends on whether or not we want to require Rails 4.2. I'm all for that (requiring > 4.2).

[mejackreed]

I'm wondering if we can just remove this:
https://github.com/projectblacklight/blacklight-maps/blob/master/Gemfile#L11-L15

Seems to work for me when running tests locally

[ebenenglish]

@mejackreed Are you generating a new app and running the tests? I think that's the only way to really figure out if it works. The blacklight-maps Gemfile only gets used by the generated test app, right? Not by an implementer's application that has included blacklight-maps in their local Gemfile. I could be wrong about this.

I'm all for following Blacklight patterns as closely as possible, so maybe I'll take a stab at mirroring what's happening here:

https://github.com/projectblacklight/blacklight/blob/master/Gemfile

Once we get everything working with the CI builds, I'll squash all these messy commits.

[mejackreed]

Yes I am creating a new test app:

rm -rf spec/internal
bundle exec rake

I think just removing those lines may solve the problem. We ran into this on GeoBlacklight and fixed it here: geoblacklight/geoblacklight@b8b9cee

[ebenenglish]

OK, the GeoBlacklight fix (geoblacklight/geoblacklight@b8b9cee) seems to have worked. I'm going to squash the last few commits related to the bootstrap-sass stuff and we'll take it from there.

[mejackreed]

👏 nice!

[ebenenglish]

So are we cool to merge this or what?

[mejackreed]

Does it make sense to make this a class rather than module? And subclass it from Blacklight::Solr::SearchBuilder?

That may would allow us to remove this logic: https://github.com/projectblacklight/blacklight-maps/pull/74/files#diff-544618e3e5d4681c08c1bf5fc29e5584R10

[ebenenglish]

I'm all for aligning the patterns as closely as possible between GeoBL and blacklight-maps. So the logic then gets included like so: https://github.com/geoblacklight/geoblacklight/blob/master/lib/geoblacklight/search_builder.rb#L3?

[mejackreed]

Yep, and you specify the search building in CatalogController, like this: https://github.com/geoblacklight/geoblacklight/blob/master/lib/generators/geoblacklight/templates/catalog_controller.rb#L24

[ebenenglish]

Hmm. That seems troubling. It's possible that the implementing application may already be using it's own SearchBuilder with custom logic (which is definitely the case with Digital Commonwealth), and we can't then force implementers to use BlacklightMaps::MapsSearchBuilder as config.search_builder_class.

So if we're not injecting the logic via BlacklightMaps::ControllerOverride (https://github.com/projectblacklight/blacklight-maps/pull/74/files#diff-544618e3e5d4681c08c1bf5fc29e5584R10), then you'd need to be able to include BlacklightMaps::MapsSearchBuilder into a pre-existing SearchBuilder class in the implementing application. Which points more to a module, rather than a class, for BlacklightMaps::MapSearchBuilder?

The local implementor's SearchBuilder would look something like:

class SearchBuilder < Blacklight::SearchBuilder
  include Blacklight::Solr::SearchBuilderBehavior
  include BlacklightMaps::MapsSearchBuilder

  def some_local_implementation_logic(solr_parameters = {})
    ...
  end

end

[mejackreed]

Gotcha... I see what you are saying now. I guess then stick with the module pattern. Good to then?

[ebenenglish]

I'm good on my end. Will bump the version to 0.4.0 once the merge is done.

[mejackreed]

A couple of comments but overall 👍