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

add support for polymorphic associations in active record #814

Conversation

WriterZephos
Copy link
Contributor

@WriterZephos WriterZephos commented Jan 24, 2023

This PR attempts to add support for polymorphic associations in Active Record. It addresses this issue: #793

The solution works for the most part but there are some difficulties with making it work correctly for using the association name instead of the association id.

I am not very familiar with the CanCan code base so I was hoping to get some help finalizing this and ensuring backwards compatibility.

If anyone could clone this and run the tests, you will see some failures highlighting where things aren't quite working as needed. I have attempted to make some changes to address them without breaking compatibility, but I can't afford to go too deep into this at the time so I am putting it up in hopes someone can take it across the finish line (or tell me how to go about it).

UPDATE: I had a moment of inspiration and found a way to get tests passing. I am still not convinced this is the most elegant or best solution, but tests pass and it allows me to define abilities on polymorphic associations.

Feedback welcome!

Thanks!

@AkosKovacs0
Copy link

Hey @WriterZephos, thanks for your PR, I have tested it on our project and it does solve the issue. 🙏

It would be awesome if someone would be able to take a look at this and merge it. 🤩

hat would help us a lot!

@rhatherall
Copy link

Hi @WriterZephos, this works well for my projects and allows me to finally update to 3.4. One side-effect I have found though is noise in my specs coming from line 76 of lib/cancan/conditions_matcher.rb. I know you didn't put it there but combined with your changes, I now see it triggered frequently. It might be worth getting rid of that puts statement with these changes to clear up the debug noise it introduces.

Otherwise, thank you! I'd love to see this merged/released as soon as possible. 🙏

@WriterZephos
Copy link
Contributor Author

@rhatherall I can take a look at that today or later this week and see if I can make that change. Thanks for trying it out!

@WriterZephos
Copy link
Contributor Author

@rhatherall okay that line is removed.

@rhatherall
Copy link

Awesome @WriterZephos. I have tried it out and all noise gone 🎉

Now all we need is a maintainer approve this 🙏

@WriterZephos
Copy link
Contributor Author

WriterZephos commented Feb 23, 2023

@coorasse are you a maintainer? Could you help us get this merged?

Copy link
Member

@coorasse coorasse left a comment

Choose a reason for hiding this comment

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

Thank you!! 👏

@coorasse coorasse merged commit fa19c5c into CanCanCommunity:develop Mar 5, 2023
@rhatherall
Copy link

Thank you, @coorasse!

@coorasse
Copy link
Member

coorasse commented Mar 5, 2023

Available now on 3.5.0

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.

4 participants