Closed Plsr closed 5 years ago
@donschado wie würdest du das denn machen?
Totally second that. :)
I consider these before_action
s a smell, when all they do is setup some instance variables / data for the view. It kinda hides these wired dependency.
The thing is, this practice is shown in so many examples and even the rails scaffold does it that way... 🙄
When you want to reuse the same finders in a controller I prefer a memoizing method and explicitly set the instance variable in the corresponding action. Or if they are equal across controllers, you could even pull them up into the application controller, or a module/concern.
But it depends. :) these few finders here are so simple (no scopes etc) you could argue there might be no need to "DRY" them up yet.
Example of what I'm talking about:
before_action :ensure_post_or_redirect, only: [:show]
def show
@post = post
end
def some_other_action_to_show_another_benefit
# stuff...
@categories = post.categories # maybe even chain build/create through the association
end
private
def post
@_post ||= Post.friendly.find(params[:id])
end
def ensure_post_or_redirect
return if request.path == post_path(post)
redirect_to post, status: :moved_permanently
end
Regarding the and return
, I'm not completely sure if this is needed 🤔 http://guides.rubyonrails.org/action_controller_overview.html#filters
If a "before" filter renders or redirects, the action will not run
But, depending on your taste/opinion, you could also do it completely without filters: https://blog.arkency.com/2014/07/4-ways-to-early-return-from-a-rails-controller/
stale? :)
(☞゚∀゚)☞
Might have some time today and will wrap this up, on the weekend at the latest!
Did implement the feedback, here's what changed in a nutshell:
users
and categories
UniqueResourceUrl
)I'll implement @DonSchado's comments tonight, so please do not merge yet :)
:shipit:
We currently have the history feature of
friendly_id
enabled for posts and categories, which is working well. However, on change of the URL, we are still rendering the old resource under the old url, which may lead to a lot of duplicate content. This PR introduces the redirect to the new URL of said resources with a status of301 Moved Permanently
.