s/find_by_slug/friendly_find!/ and use everywhere

[glebm]

This is to avoid confusion with the Rails dynamic finder methods

[timdiggins]

Makes sense, but some small comments (which are not 100% part of this PR)

[timdiggins]

docs seem odd here -- "topic" ?

[glebm]

Oops, copy-paste accident. Fixed.

[timdiggins]

... "messageboard" ... ?

Also why are we using MessageboardNotFound rather than the more normal (but less specific) ActiveRecord::NotFound -- can see it's a style across different models, but wondering what it gives us

[glebm]

We're a custom error so that they can be handled it in a custom way by Thredded or main app. Currently the default Thredded behaviour is to render the "not found" template with exception.message for these custom errors (

thredded/app/controllers/thredded/application_controller.rb

Lines 17 to 24 in ffebb25

	rescue_from Thredded::Errors::MessageboardNotFound,
	Thredded::Errors::PrivateTopicNotFound,
	Thredded::Errors::TopicNotFound,
	Thredded::Errors::UserNotFound do |exception|
	@error = exception
	@message = exception.message
	render template: 'thredded/error_pages/not_found', status: :not_found
	end

). Maybe there is a better way to do this?

[jayroh]

Also why are we using MessageboardNotFound rather than the more normal (but less specific) ActiveRecord::NotFound -- can see it's a style across different models, but wondering what it gives us

@timdiggins - The generic exception is fine but there was a time when I wrote this that I wanted a specific error message to show up with regards to a messageboard not existing.

[timdiggins]

Bit of a nit pick, but I feel a bit uncomfortable with this style (hard to read) (previous version was odd too IMO).

Also -- not sure why, but the behaviour seems to change in this PR. Previously it was not raising MessageboardNotFound when messageboard_id empty, now it is...

Instead maybe extract the check to a required_messageboard_id or messageboard_id! (?!) method which does params[:messageboard_id].presence || fail Thredded::Errors::MessageboardNotFound Will make this method super simple

[glebm]

So, currently, messageboard, topic, and private_topic all fail if there is no messageboard. The previous behaviour was not correct and not consistent with topic and private_topic.

But actually, I've just tried calling friendly.find(nil) and it raises ActiveRecord::NotFound, so the check is not necessary! Removed.

[timdiggins]

@glebm regarding the errors: Feel like it would be less surprising if the errors extended (were specializations) of ActiveRecord::NotFound. However that's not part of this PR which is all good. Could look at that later if we care enough.