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

Scoping index action #914

Merged
merged 2 commits into from
Jul 4, 2017
Merged

Conversation

damau
Copy link
Contributor

@damau damau commented Jun 20, 2017

Hi

@bobf discussed this and feel this is more of a railsy implementation for scoping the index action.

The purpose is to allow people to override the controller action

def scoped_resource
     # Do something special with what's displayed in the index action
end

In order to manage what's displayed on the index page.

Which will certainly help Bob and I use roles within administrate.

Ta
Ad

adam added 2 commits June 20, 2017 14:12
This reverts commit c660ecd.
We feel scoping should occur in the controller which is more
consistent with rails and find_resource customisation
It's standard behaviour for scoping to happen in a controller.
Also makes it more consistent with find_resource.
@damau
Copy link
Contributor Author

damau commented Jun 20, 2017

Rails 4 is failing on master as well.

@bobf
Copy link
Contributor

bobf commented Jun 20, 2017

@nickcharlton FYI I am using this in a real application and it works great. Having scoping applied at the controller level is a huge improvement. Previously I was storing current_user in Thread.current so that the dashboards could access it, which was horrible at best.

@damau
Copy link
Contributor Author

damau commented Jun 30, 2017

@nickcharlton any chance of a sneaky merge next week?

@nickcharlton
Copy link
Member

Hi @damau! This is looking good. I'm going to go ahead and merge!

@nickcharlton nickcharlton merged commit be9d3c5 into thoughtbot:master Jul 4, 2017
@bobf
Copy link
Contributor

bobf commented Jul 4, 2017

@nickcharlton Awesome ! You just made our lives MUCH easier ! Thanks a lot for this. Glad to be back on master :-)

helper_method :namespace
helper_method :resource_name

def dashboard_class
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be delegated in the lines above, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it! I'd merge such a PR if you wanted to open one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think so. If I'm honest I'm drawing a blank as to why I even changed the "dashboard" function at all.

@damau
Copy link
Contributor Author

damau commented Jul 4, 2017

@nickcharlton thanks so much for the merge and all the work you put into the engine!!!
@pusewicz good spot.

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.

4 participants