-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Search scopes #276
Search scopes #276
Conversation
spec/features/search_spec.rb
Outdated
|
||
scenario "admin searches using a model scope", :js do | ||
query = "subscribed:" | ||
subscribed_customer = create(:customer, name: "Dan Croak", email_subscriber: true) |
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.
Line is too long. [86/80]
spec/features/search_spec.rb
Outdated
name: "Dan Croak", | ||
email_subscriber: true) | ||
other_customer = create(:customer, | ||
name: "Foo Bar", |
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.
Align the parameters of a method call if they span more than one line.
Does not send the possible scope to the class if it looks suspicious, that's to say: - contains any of the BLACKLISTED_WORDS - ends with an exclamation mark I've also made public the :resolver, :term and :scope to be able to test this behavior (:resolver and :term not needed but can't see the problem of them being public too).
spec/lib/administrate/search_spec.rb
Outdated
end | ||
end | ||
|
||
search = Administrate::Search.new(resource_resolver, 'bang!:') |
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.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
Search#run implementation is still pending
lib/administrate/search.rb
Outdated
if term && (term[-1, 1] == ":") | ||
possible_scope = term[0..-2] | ||
possible_scope if resource_class.respond_to?(possible_scope) && | ||
!banged?(possible_scope) && |
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.
Use 4 (not 18) spaces for indenting a condition in an if
statement spanning multiple lines.
Conflicts: spec/lib/administrate/search_spec.rb
|
||
COLLECTION_SCOPES = { | ||
status: [:active, :inactive, "idle", "with_argument:*"], | ||
other: [:last_week, :old, "with_argument(3)",], |
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.
Avoid comma after the last item of an array, unless each item is on its own line.
+1 |
As suggested by Tyson: thoughtbot#276 (comment) (thanks Tyson!)
Conflicts: spec/example_app/app/models/customer.rb spec/spec_helper.rb
@@ -14,3 +15,38 @@ | |||
end | |||
|
|||
WebMock.disable_net_connect!(allow_localhost: true) | |||
|
|||
class MockDashboard < Administrate::BaseDashboard | |||
ATTRIBUTE_TYPES = { |
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.
Freeze mutable objects assigned to constants.
will it ever be merged anytime soon ? |
A few months later... |
We are using this branch merged with others that let use Administrate w/ Globalize and Pundit with this lines in our Gemfile:
We are using Administrate to manage resources in our new website and are very happy and grateful to Thoughtbot and Grayson for this amazing gem (thanks again!!) Hopefully they merge it one day. It would be a great honor for me, i admire Thoughtbot from its very beginning ;) |
@Graysonwright @tysongach what's up with this PR? It's clearly something people are requesting as in: #157, #183, #277, #291, #563, #445 I'd be glad to help solve any issues or interferences you see upcoming with this code. 🌮 |
@nando I believe you unfortunately are the only one who can rebase this :) |
I'd be willing to help getting the PR in shape and already have a branch where this has been merged into a fresh master over at https://github.com/substancelab/administrate/tree/collection_scopes. It's a huge PR, though, and there are still some issues I need to weed out (layout and a few failing specs) - I guess that is to be expected with a year old feature branch that has lived through v0.2 That said, I have no idea what the best approach is. Just submit a new PR with what I have? Somehow rebase and clean up the branch (that seems near-impossible)? Cherry-pick the relevant commits on top of master? Also, I'd very much prefer to not step on @nando's toes here by stealing credit or ruffling feathers, please advice. |
Hi @koppen! When I pick up old PRs and the original author hasn't replied for a while, I usually take their branch, rebase it against current As it is quite big, if you think it could be broken down I'd do that and cherry pick a few commits over a collection of different PRs. As long as you work with @nando's original commits (even if you break them apart a bit), the credit will stay. |
I'm closing this as we've got #947 now. |
DISCLAIMER: the original message of this PR written two months ago has been updated to represent better its current state (however the original one has been kept at the end).
Problem:
If the resource managed by the dashboard has scopes defined there's no way to limit the scope of the search with them.
Solution:
Declare a new constant in the dashboard (COLLECTION_SCOPES) to define the valid scopes that could be used in the query while searching in the index page. The following is the current definition of the COLLECTION_SCOPES constant in the template used to generate new dashboards:
ORIGINAL MESSAGE
I hope you like the idea (I've modified the original PR to avoid sending the scope to the model if it looks suspicious. We do not have that security problem in our project since our AdministrateController has 'before_filter :require_admin' at the beginning of its definition.