pennant icon indicating copy to clipboard operation
pennant copied to clipboard

[1.x] Adds `Feature::activeForAny()`

Open cosmastech opened this issue 1 year ago • 4 comments

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.

cosmastech avatar Aug 04 '24 13:08 cosmastech

@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 avatar Aug 05 '24 03:08 timacdonald

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 :+1:

cosmastech avatar Aug 05 '24 12:08 cosmastech

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 avatar Aug 07 '24 00:08 cosmastech

@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.

timacdonald avatar Aug 07 '24 03:08 timacdonald

@cosmastech, I'm going to close this one. This isn't a "no", more a "maybe later".

I don't think this is the API we want and we also want to see that more people want the feature before investing time in getting things right for it.

Thanks for the PR!

timacdonald avatar Nov 06 '24 21:11 timacdonald