Create instance and apply params separately

brendon commented 6 years ago

First creating the instance, then applying the parameters allows us to work around a quirk in active_record where the relations aren't established on the object until after the parameters are applied. This means if any of our attribute setters reference the relation, they will fail if we don't seperate these steps.

Points to consider:

The attributes method may not exist on non-active_record exposures. Are we assuming too much?
In your feature spec file, the BirdsController isn't thrown away between each test, so redefining something like expose :bird can break other tests in a non-deterministic way (due to test ordering). It hasn't been exposed yet because you don't redefine any exposures in your tests, but just wanted to give a heads-up as I ran into this when trying to redefine :bird. A seperate controller class per test would probably work best to avoid this problem.
I had to adjust a lot of controller tests to cope with the new expectation that we'll always call new without arguments, and then subsequently call attributes=.

Fixes #191

mattpolito commented 6 years ago

@brendon Thank you very much for putting the time into this PR. After looking through it I'm realizing that DecentExposure has users that don't always use an ActiveRecord with this API. We cannot rely on #attributes to be there.

brendon commented 6 years ago

That's correct. AR also isn't a dependency. I guess there's two options, close the PR, or look at extending Behavior so that it checks if the object: method_defined? :attributes= and acts accordingly. Up to you :)

mattpolito commented 6 years ago

This seems to specifically be an ActiveRecord issue. Why don’t you try out the logic if the object is an AR. See how that feels. That will remove the changes from the tests and then we can have a specific test case for the new functionality.

On August 21, 2018 at 4:55:21 PM, Brendon Muir (notifications@github.com(mailto:notifications@github.com)) wrote:

That's correct. AR also isn't a dependency. I guess there's two options, close the PR, or look at extending Behavior so that it checks if the object: method_defined? :attributes= and acts accordingly. Up to you :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub(https://github.com/hashrocket/decent_exposure/pull/192#issuecomment-414818698), or mute the thread(https://github.com/notifications/unsubscribe-auth/AAAIYeOHmmjp5wWpN_L1xj18LduxFlCoks5uTHO5gaJpZM4WEyh_).

brendon commented 6 years ago

Hi @mattpolito, just been working on this. There is an issue that we need to actually test for both ActiveRecord::Base and ActiveRecord::Relation as we may have either as a scope. It's a bit long winded but I'll commit what I've got and perhaps you'll have some ideas on making it cleaner?

brendon commented 4 years ago

Just pinging this one. Starting a new project and was considering using decent_exposure on it :D

mattpolito commented 4 years ago

@brendon Thanks for pinging this. I never saw your updates come through. I'll look over the changes today and hopefully I can get this in for you.

Thank you.

mattpolito commented 3 years ago

Closing explanation in #191

hashrocket / decent_exposure

Create instance and apply params separately #192