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

[1.x] Adds Feature::activeForAny() #116

Draft
wants to merge 3 commits into
base: 1.x
Choose a base branch
from

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Aug 4, 2024

I want to be able to check if any of a user's related models have a particular feature enabled.

$schools = $user->loadMissing('schools');
$featureIsActive = Feature::for($schools)->someAreActive('enrolled-in-beta');

But this actually is checking that all of the schools have at least one of the features (in this case it's just one feature, 'enrolled-in-beta', which is gets wrapped as an array) being passed in. So if all of the user's schools don't have the feature enabled, $featureIsActive is false.

With the change in this PR, we can accomplish that with:

$featureIsActive = Feature::for($schools)->activeForAny('enrolled-in-beta');

I would have just changed someAreActive() but it seems like that would be a breaking change (proven by the fact I would have to modify tests) 😢 hence the new method.

@timacdonald timacdonald marked this pull request as draft August 5, 2024 02:57
@timacdonald
Copy link
Member

timacdonald commented Aug 5, 2024

@cosmastech, thinking out loud here, I feel like the API we really want here is something like this:

Feature::forSome($schools)->active('enrolled-in-beta');

This better indicates that the "some" applies to the schools and not the features.

$school1 = School::find(1);
$school2 = School::find(2);

Feature::forSome([$school1, $school2])->active('enrolled-in-beta'); // false

Feature::for($school2)->activate('enrolled-in-beta');

Feature::forSome([$school1, $school2])->active('enrolled-in-beta'); // true

We then have better composition, for example we could mix forSome with someAreActive:

$model1 = Model::find(1);
$model2 = Model::find(2);

Feature::forSome([$model1, $model2])->someAreActive(['feature-1', 'feature-2']); // false

Feature::for($model1)->activate('feature-2');

Feature::forSome([$model1, $model2])->someAreActive(['feature-1', 'feature-2']); // true

Thoughts?

@timacdonald timacdonald self-assigned this Aug 5, 2024
@cosmastech
Copy link
Contributor Author

cosmastech commented Aug 5, 2024

That's a great call @timacdonald. My first thought was "should we make scopes Enumerable?"

Feature::for($models)->some()->active('feature1');
// or even
Feature::for($models)->some->active('feature1');

But looking at Enumerable interface, I imagine most of those wouldn't apply. So maybe just methods for some() and every() would be sufficient.

edit: plus adding that Feature::forSome($models) helper method 👍

@cosmastech
Copy link
Contributor Author

cosmastech commented Aug 7, 2024

I just realized that this doesn't make any sense:

Feature::forSome(['taylor', 'tim'])->activate('foo'); // or deactivate

What do we do in that case? Throw an exception? Just activate them for both scopes? Maybe throw each scope into a lottery?

My thinking is that the composition makes it more confusing here. @timacdonald I have started on an implementation that would allow for querying by some(), but I'm starting to feel like adding explicit methods will cause less headaches.

@cosmastech cosmastech changed the title Adds Feature::activeForAny() [1.x] Adds Feature::activeForAny() Aug 7, 2024
@timacdonald
Copy link
Member

@cosmastech, I don't think all methods would be available. Only a subset of methods would be available after calling forSome.

I'm on something else at the moment, but will circle back to this. I don't want you to put in a heap of work on something we aren't sure on just yet.

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