Allow custom scope for dashboard resource #910
Conversation
spec/lib/administrate/search_spec.rb
Outdated
| end | ||
|
|
There was a problem hiding this comment.
Extra empty line detected at block body end.
spec/lib/administrate/search_spec.rb
Outdated
| end | ||
| class UserDashboard < Administrate::BaseDashboard | ||
| def resource_scope | ||
| User.my_scope |
| end | ||
| end | ||
| resolver = Administrate::ResourceResolver.new("admin/posts") | ||
| expect(resolver.resource_scope).to eq('a resource scope') |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| class Post; end | ||
| class PostDashboard | ||
| def resource_scope | ||
| 'a resource scope' |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| end | ||
| end | ||
|
|
||
| it 'uses defined scope when present' do |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| end | ||
| class PostDashboard; end | ||
| resolver = Administrate::ResourceResolver.new("admin/posts") | ||
| expect(resolver.resource_scope).to eq('default scope') |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| begin | ||
| class Post | ||
| def self.default_scoped | ||
| 'default scope' |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| @@ -60,6 +60,38 @@ module Blog; class Post; end; end | |||
| end | |||
| end | |||
|
|
|||
| describe '#resource_scope' do | |||
| it 'defaults to model default scope when no override defined' do | |||
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| @@ -60,6 +60,38 @@ module Blog; class Post; end; end | |||
| end | |||
| end | |||
|
|
|||
| describe '#resource_scope' do | |||
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
|
Thanks for this! I didn't have any specific comments, so I've merged it. |
|
@nickcharlton Ah, bad timing ! A colleague of mine has been working on what I consider to be a better implementation. He has moved the scoping into the controller, so you can override a method in each controller to provide the relevant scope. This allows using things like properties of I was actually going to close this PR as I think my colleague's implementation is superior. He should have a PR soon. What do you think is the best way to proceed ? |
|
@nickcharlton Just discussed with my colleague - he's going to submit a new PR that reverts my changes and adds his changes instead. I'm optimistic you'll agree that it's a better implementation as it allows the controller to decide the scope, optionally basing it on the request, which I think is much cleaner. |
This reverts commit c660ecd. We feel scoping should occur in the controller which is more consistent with rails and find_resource customisation
|
Oh dammit! I look forward to the new PR! |
|
@nickcharlton Here ya go: #914 We're very keen to see this merged so if you have any feedback just let us know and we will act on it ASAP. Thanks for getting back to us on this ! |
* Revert "Allow custom scope for dashboard resource (#910)" This reverts commit c660ecd. We feel scoping should occur in the controller which is more consistent with rails and find_resource customisation * Scoping for the index action should happen in the controller. It's standard behaviour for scoping to happen in a controller. Also makes it more consistent with find_resource.
Based on @nickcharlton 's original branch here:
https://github.com/thoughtbot/administrate/tree/nc-allow-overriding-resource-class