Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

default invalid_solr_id_error just a default 404 #766

Merged
merged 1 commit into from
Feb 11, 2014

Conversation

jrochkind
Copy link
Member

much like ActiveRecord::RecordNotFound

Fixes #756.

My goals were:

  1. Leave #invalid_solr_id_error there so local apps can easily override it to do more complicated things
  2. the simple implementation should be as much as possible like what Rails by default, in production, does with uncaught ActiveRecord::RecordNotFound

This solution does that. Another solution that would also is to have our invalid_solr_id_error simply raise a ActionController::RoutingError. In production, if that is raised and not caught, Rails will display the standard default 404.html page, or an appropriate XML or JSON response in html or json format.

However, Rails will only do that in production, in dev and importantly test, it will not. Which means, if we did id that way, our tests couldn't actually test for status 404, they'd just test for raises ActionController::RoutingError. I didn't like that in the tests.

But if people would prefer the raise ActionController::RoutingError implementation, I am quite happy to change to that one.

@cbeer
Copy link
Member

cbeer commented Feb 11, 2014

+1

jcoyne added a commit that referenced this pull request Feb 11, 2014
…not_found

default invalid_solr_id_error just a default 404
@jcoyne jcoyne merged commit ac434f2 into master Feb 11, 2014
@jcoyne jcoyne deleted the simple_404_for_record_not_found branch February 11, 2014 00:54
@jrochkind
Copy link
Member Author

I forgot to get rid of the i18n locale string for 'blacklight.search.errors.invalid_solr_id', which is no longer used since we don't do the flash message anymore.

Should I get rid of it? Or leave it there in case someone else wants to use it or something?

@cbeer
Copy link
Member

cbeer commented Feb 11, 2014

I think we should keep it in case down-stream apps are using it, yes.

@cbeer cbeer added this to the 5.1.0 milestone Feb 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

double render error from invalid_solr_id_error
3 participants