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

Ability to add options to scopes #47

Merged
merged 10 commits into from
Oct 26, 2018
Merged

Ability to add options to scopes #47

merged 10 commits into from
Oct 26, 2018

Conversation

slavadev
Copy link
Contributor

@slavadev slavadev commented Oct 11, 2018

Hi! Thank you for a great gem!

What is the purpose of this pull request?

Add the ability to add options to Scopes so it can be declared like this:

scope_for :relation do |relation, with_deleted: false, whatever: 42|
  ...
end

And instead of several scopes can be created only one

It can be used this way:

authorized blabla, type: :relation, scope_options: { with_deleted: true }

authorized(blabla, type: :relation, with_deleted: true) would look better, but I don't think we need to mix policy lookup options with scope options

What changes did you make? (overview)

  • Added scope_options to Behaviour#authorized
  • Added scope_options to Scoping#apply_scope and made it pass to the policy only when options are present to not break existing scopes which don't expect options
  • Updated existing tests to be green

Is there anything you'd like reviewers to focus on?

If you like the idea and the syntax, I'll add specs, documentation, and changelog for it

PR checklist:

  • Tests included
  • Documentation updated
  • Changelog entry added

@palkan palkan self-requested a review October 11, 2018 13:06
Copy link
Owner

@palkan palkan left a comment

Choose a reason for hiding this comment

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

👍

I like the idea and the proposed implementation.

Left a few comments.

Feel free to continue with docs and changelog!

Btw, could you share your use-case (and, probably, include it into the docs)?

policy_class == policy.class &&
type == actual_type &&
name == actual_name
name == actual_name &&
scope_options == actual_scope_options
Copy link
Owner

Choose a reason for hiding this comment

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

Suggestion: what if we use === here?
That would allow us to use RSpec composed matchers (like a_hash_including(...)).

lib/action_policy/testing.rb Show resolved Hide resolved
@slavadev
Copy link
Contributor Author

Updated PR, @palkan

CHANGELOG.md Outdated
# user_policy.rb
describe UserPolicy < Application do
relation_scope do |relation, with_deleted: false|
if with_deleted
Copy link
Owner

Choose a reason for hiding this comment

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

Let's update an example a little bit to make it less abstract:

relation_scope do |relation, with_deleted: false|
  some_logic(relation).yield_self do |rel|
    with_deleted ? rel.with_deleted : rel
  end
end

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, it might help a bit with understanding

@palkan palkan merged commit 4124a53 into palkan:master Oct 26, 2018
@palkan
Copy link
Owner

palkan commented Oct 26, 2018

Thanks!

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.

2 participants