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

Support scopes of STI classes as ability conditions #702

Merged
merged 1 commit into from
Jul 6, 2022
Merged

Support scopes of STI classes as ability conditions #702

merged 1 commit into from
Jul 6, 2022

Conversation

honigc
Copy link
Contributor

@honigc honigc commented Apr 6, 2021

This fixes a bug introduced in #649 that broke using scopes of STI models as conditions. Without the change the new test produces this error:

ArgumentError:
Unknown key: "type". Valid keys are: :includes, :eager_load, :preload, :select, :group, :order, :joins, :left_outer_joins, :references, :extending, :unscope, :limit, :offset, :lock, :readonly, :reordering, :reverse_order, :distinct, :create_with, :skip_query_cache, :where, :having, :from
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/sti_normalizer.rb:34:in `build_rule_for_subclass'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/sti_normalizer.rb:27:in `update_rule'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/sti_normalizer.rb:13:in `block (2 levels) in normalize'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/sti_normalizer.rb:12:in `select'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/sti_normalizer.rb:12:in `block in normalize'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/sti_normalizer.rb:11:in `delete_if'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/sti_normalizer.rb:11:in `normalize'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_adapters/active_record_adapter.rb:17:in `initialize'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/ability.rb:172:in `new'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/ability.rb:172:in `model_adapter'
/usr/packages/ruby-2.7.2/gems/cancancan-3.2.1/lib/cancan/model_additions.rb:24:in `accessible_by'

This happens because the STI normalizer calls #merge on the scope, which assumes the argument will be a relation or a hash of relation options. Using #where_values_hash in this case is the same thing already done in CanCan::Rule#inspect. This PR does not address the case of string conditions, which probably have a similar issue right now.

@JacobEvelyn
Copy link

Hi @coorasse—is there any interest in merging this PR? It would address a big pain point for us.

@coorasse coorasse self-requested a review March 23, 2022 08:17
@@ -30,8 +30,16 @@ def update_rule(subject, rule, rules_cache)

# create a new rule for the subclasses that links on the inheritance_column
def build_rule_for_subclass(rule, subject)
sti_conditions = { subject.inheritance_column => subject.name }
Copy link

Choose a reason for hiding this comment

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

Suggested change
sti_conditions = { subject.inheritance_column => subject.name }
sti_conditions = { subject.inheritance_column => subject.sti_name }

@coorasse coorasse assigned coorasse and honigc and unassigned coorasse Mar 23, 2022
@coorasse
Copy link
Member

@honigc thank you very much, can you please look at the suggestions from @Liberatys and also rebase? Thanks!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 Small edit: { We recently changed subject.name to subject.sti_name }. Thank you for the change.

@honigc
Copy link
Contributor Author

honigc commented Mar 31, 2022

@coorasse @Liberatys Made the requested change and rebased. Thank you!

@ghost
Copy link

ghost commented Mar 31, 2022

@coorasse LGTM

@coorasse coorasse merged commit f7a6055 into CanCanCommunity:develop Jul 6, 2022
@coorasse
Copy link
Member

coorasse commented Jul 6, 2022

Thanks!

@honigc honigc deleted the bugfix/sti-ability-with-scope branch November 9, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants