-
Notifications
You must be signed in to change notification settings - Fork 216
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
s/find_by_slug/friendly_find!/ and use everywhere #482
Conversation
This is to avoid confusion with the Rails dynamic finder methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but some small comments (which are not 100% part of this PR)
@@ -88,10 +88,17 @@ def pundit_user | |||
thredded_current_user | |||
end | |||
|
|||
# Returns the `@messageboard` instance variable. | |||
# If `@messageboard` is not set, it first sets it to the topic with the slug or ID given by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs seem odd here -- "topic" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, copy-paste accident. Fixed.
# `params[:messageboard_id]`. | ||
# | ||
# @return [Thredded::Messageboard] | ||
# @raise [Thredded::Errors::MessageboardNotFound] if the topic with the given slug does not exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... "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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
rescue ActiveRecord::RecordNotFound | ||
raise Thredded::Errors::MessageboardNotFound | ||
@messageboard ||= begin | ||
fail Thredded::Errors::MessageboardNotFound unless params[:messageboard_id].presence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
This is to avoid confusion with the Rails dynamic finder methods