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

Prevent normalization of polymorphic association #592

Conversation

eloyesp
Copy link
Contributor

@eloyesp eloyesp commented May 30, 2019

Polymorphic associations have no klass names, so those should be not
normalized. Fixes #588.

  • Add tests
  • Changelog entry

I might need help with the tests.

@eloyesp eloyesp marked this pull request as ready for review May 30, 2019 16:45
@eloyesp eloyesp force-pushed the skip_through_polymorphic_normalization branch from 245e7c8 to 01f719f Compare May 30, 2019 18:16
@eloyesp
Copy link
Contributor Author

eloyesp commented Jun 6, 2019

@coorasse any thought about this pull-request?

@coorasse
Copy link
Member

coorasse commented Aug 6, 2019

Hi! Thanks for your PR.
I like your solution, since this case is currently not well managed IMHO.
In an optimal case, it should be normalised to

expect(rule.conditions).to eq(attachments: { record_id: 11, record_type: 'Article' })

I'd merge it by adding a TODO that explains the best scenario, not currently possible.
What are your thought about it?

@coorasse coorasse added the bug label Aug 6, 2019
@coorasse coorasse self-assigned this Aug 6, 2019
@eloyesp
Copy link
Contributor Author

eloyesp commented Aug 7, 2019

Let me check if I can get it working that way.

@eloyesp eloyesp force-pushed the skip_through_polymorphic_normalization branch from 4b12089 to 159e752 Compare August 7, 2019 16:03
@eloyesp
Copy link
Contributor Author

eloyesp commented Aug 7, 2019

@coorasse The problem with that normalization is that it depends on querying for id, it will not work on other fields.

    # query for id
    rule = CanCan::Rule.new(true, :read, Blob, articles: { id: 11 })
    CanCan::ModelAdapters::ConditionsNormalizer.normalize(Blob, [rule])
    expect(rule.conditions).to eq(attachments: { record_id: 11, record_type: 'Article' })

    # but does not work for other queries
    rule = CanCan::Rule.new(true, :read, Blob, articles: { status: 'draft' })
    # expect(rule.conditions).to eq(attachments: { ???? })

I've fixed some missing options on the sample associations and updated for rails 6.0 rc2.

Regards.

@coorasse
Copy link
Member

It could be
expect(rule.conditions).to eq(attachments: { record: {id: 11, type: 'Article', status: 'draft' })?
We need to rebase, also. thanks

Polymorphic associations have no klass names, so those should be not
normalized. Fixes CanCanCommunity#588.

Add tests using an action_storage like schema and add changelog entry.
@eloyesp eloyesp force-pushed the skip_through_polymorphic_normalization branch from 159e752 to 096fc59 Compare August 22, 2019 18:23
@eloyesp
Copy link
Contributor Author

eloyesp commented Aug 22, 2019

@coorasse I've done the rebase.

It could be
expect(rule.conditions).to eq(attachments: { record: {id: 11, type: 'Article', status: 'draft' })?

I'm not sure if that would work in Rails, but the main issue is that I'm not sure how to modify the actual code to do it.

@coorasse coorasse merged commit 2236e25 into CanCanCommunity:develop Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polymorphic conditions in abilities no longer work as of 3.0.0
2 participants