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

fix(flagsmith): boolean defaults and flag state evaluation fixes #1026

Conversation

uriell
Copy link

@uriell uriell commented Jul 25, 2024

This PR

Hey team, I wrote a more detailed issue (#1025) but here is a summary:

The Flagsmith provider doesn't take into account falsey default values in boolean flags, and it is currently using a fixed truthy value in the code, and the evaluation for these boolean values was based off their existance/enablement, and not their actual value, preventing us from using overrides that change the value, not the enabled status.

This PR includes:

  • The fix in resolveBooleanEvaluation to allow boolean flags to use a specified default value;
  • The fix in evaluate to ensure boolean flags don't have a special scenario and also consume it's value instead of checking for the existance of the flag;
  • Test scenarios that now validate the state value of boolean flags instead of their enablement;
  • Additional test scenarios to ensure default values specified outside of constructor default values are also applied and used;

Related Issues

Fixes #1025

Notes

  • I took the liberty to make an adjustment to the readme since the suggested contribution command in the flagsmith provider's README wasn't it's actual project name;
  • I noticed the provider was written by one of Flagsmith's Founders (@kyle-ssg) and there was a mention in feat: Add Flagsmith Provider #836 regarding the behavior for boolean flags using the enablement status so there may be additional context they may have regarding Flagsmith's expected behavior, but upon using their platform it seemed counterintuitive in this scenario.

Follow-up Tasks

  • I haven't identified any follow up tasks, but feel free to flag any you may see.

How to test

@uriell uriell requested a review from a team as a code owner July 25, 2024 00:18
uriell added 6 commits July 24, 2024 21:24
Signed-off-by: Uriell <me+github@uriell.dev>
…lement status

Signed-off-by: Uriell <me+github@uriell.dev>
Signed-off-by: Uriell <me+github@uriell.dev>
…tatus

Signed-off-by: Uriell <me+github@uriell.dev>
…ructor is also taken into account

Signed-off-by: Uriell <me+github@uriell.dev>
@uriell
Copy link
Author

uriell commented Jul 25, 2024

I will temporarily close this Pull Request as I think a more thought out solution needs to be discussed in #1025 first.

@uriell uriell closed this Jul 25, 2024
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.

Flagsmith doesn't use boolean defaults and uses enablement status instead of flag value
1 participant