Skip to content

Fix isFeatureEnabled returns false when multiple strategies are configured #33

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

Merged
merged 5 commits into from
Feb 7, 2022

Conversation

DanTheDJ
Copy link
Contributor

Hi there, firstly thanks for a great package. I am currently implementing Unleash to manage feature access in my application.

I have come across an issue when a feature has been configured with multiple different activation strategies. In my app, there are multiple scenarios under which I'd like a user to gain access to a feature e.g:

  • The user is on a specific plan
  • The user has a specific id
  • App environment matches specific value etc

What I am experiencing is that if any of these strategy evaluations fail, then the function returns the feature is not available.

From the unleash documentation (https://docs.getunleash.io/user_guide/important-concepts)

Activation strategies​
Feature toggles can have multiple activation strategies. An activation strategy will only run when a feature toggle is enabled and provides a way to control WHO will get access to the feature.
Activation strategies compound, and every single strategy will be evaluated. If any one of them returns true, the user will receive access to the feature.

I have found by modifying the isFeatureEnabled function, I can achieve the expected behaviour according to the unleash docs. I have also added a new test which now passes.

@mxkxf mxkxf self-assigned this Oct 27, 2021
@mxkxf
Copy link
Collaborator

mxkxf commented Oct 29, 2021

Thanks for the PR @DanTheDJ. I just had a couple of comments but need to test this out.

Change order of strategies returned by mock unleash server response
@DanTheDJ DanTheDJ requested a review from mxkxf November 22, 2021 18:07
@mxkxf
Copy link
Collaborator

mxkxf commented Nov 22, 2021

@DanTheDJ I'll get round to a review when I can, pretty busy at the moment with work (and other things, believe it or not) and I'd still like to test this end-to-end.

@mxkxf
Copy link
Collaborator

mxkxf commented Jan 4, 2022

@DanTheDJ Just getting round to test this e2e but struggling to think about how to set this up on the Unleash side. Working backwards, how are you detecting whether your user should see the toggle in your app?

@DanTheDJ
Copy link
Contributor Author

DanTheDJ commented Jan 9, 2022

@DanTheDJ Just getting round to test this e2e but struggling to think about how to set this up on the Unleash side. Working backwards, how are you detecting whether your user should see the toggle in your app?

Hi @mikefrancis The way we have this configured is that we have a feature toggle which can be made available to users by either:

  • The tenant/user's plan key (Custom strategy with value passed by the Laravel application during evaluation)
    OR
  • The user's ID, allowing manual override for individual users.

Here is the setup on our unleash server:
image

This means that any of the individual strategies evaluating to true causes the user to see the toggle within the application.

The way this is configured on the Laravel side (running with the PR branch) is as follows:

config/unleash.php
image

The TenantWithPlanStrategy implements the MikeFrancis\LaravelUnleash\Strategies\Contracts\Strategy interface and returns true if the tenantPlan constraint within Unleash matches.

When the Unleash feature is evaluated, it will check both tenantPlan and userWithId with either one evaluating to true meaning the user sees the toggle as enabled.

Let me know if this clears it up, if not I can provide some further examples

@mxkxf
Copy link
Collaborator

mxkxf commented Jan 10, 2022

@DanTheDJ Thanks for this.

I'm looking for how you are checking whether to show the feature in your app, are you using Blade, the Feature facade or something else? An example would be great.

@DanTheDJ
Copy link
Contributor Author

@mikefrancis, currently when checking in view files, I have a custom blade directive which then passes through to a helper class I have for tenancy.

In PlanFeatureServiceProvider.php:

Blade::directive('hasFeature', function ($featureCode) {
            return "<?php if (\Domain\Tenancy\Helpers\PlanFeatureHelpers::hasFeatureCode($featureCode)): ?>";
        });

Blade::directive('endHasFeature', function () {
    return '<?php endif; ?>';
});

The helper class just uses the Unleash Facade as follows:

<?php

namespace Domain\Tenancy\Helpers;

use MikeFrancis\LaravelUnleash\Unleash;

class PlanFeatureHelpers {

    static function hasFeatureCode($featureCode)
    {

        $unleash = app(Unleash::class);

        $enabled = $unleash->isFeatureEnabled($featureCode);

        return $enabled;
    
    }

}

I also use this helper around my application to check the eligibility of features via the static method of the Helper, so I guess you could say I only ever use the Unleash facade for my checks.

Copy link
Collaborator

@mxkxf mxkxf left a comment

Choose a reason for hiding this comment

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

Thanks for this one @DanTheDJ

@mxkxf mxkxf merged commit ed1a1b8 into laravel-unleash:master Feb 7, 2022
@DanTheDJ DanTheDJ deleted the fix-multiple-strategies-eval branch February 10, 2022 23:16
@DanTheDJ DanTheDJ restored the fix-multiple-strategies-eval branch February 10, 2022 23:16
@DanTheDJ DanTheDJ deleted the fix-multiple-strategies-eval branch February 10, 2022 23:18
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