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

FakeUnleash doesn't take into account global activation/deactivation for certain isEnabled method #239

Closed
GFriedrich opened this issue May 3, 2024 · 2 comments · Fixed by #243
Assignees

Comments

@GFriedrich
Copy link

Describe the bug

The FakeUnleash method using the fallbackAction doesn't take into account the global activation/deactivation flags.
Right now the code at

public boolean isEnabled(
String toggleName, BiPredicate<String, UnleashContext> fallbackAction) {
if (!features.containsKey(toggleName)) {
return fallbackAction.test(toggleName, UnleashContext.builder().build());
}
return isEnabled(toggleName);
}

just checks whether a certain flag is configured. But if you use the global flags, then no specific feature is set. Therefore this method always falls back to the default action instead, which is unexpected.
Therefore I would suggest to change the line of
if (!features.containsKey(toggleName)) {
to something like

if ((!enableAll && !disableAll || excludedFeatures.containsKey(toggleName)) && !features.containsKey(toggleName)) {

Steps to reproduce the bug

  1. Use FakeUnleash and enable all flags by calling enableAll
  2. Run a method like isEnabled("abc", (a, b) -> false)
  3. See that it still returns false even though all flags have been enabled.

Expected behavior

FakeUnleash should correctly take into account global flags.

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

9.2.0

Subscription type

None

Hosting type

None

SDK information (language and version)

No response

@sighphyre
Copy link
Member

Oh that's a nice catch, this probably isn't something we'll tackle immediately but we'd be more than happy to take a PR

@sighphyre sighphyre moved this from New to Todo in Issues and PRs May 6, 2024
@ivarconr
Copy link
Member

ivarconr commented May 7, 2024

This is an oversight on our end. Your suggestion makes sense at first sight, feel free to push a PR!

Thanks 👍🏼

@chriswk chriswk self-assigned this May 8, 2024
chriswk added a commit that referenced this issue May 8, 2024
As discussed in #239 - When all is enabled, we had a bit of a surprising
behaviour where we'd fallback to fallback action for
`isEnabled(featureName, fallback)` even if all was enabled and feature
did not exist.

This PR fixes that, and adds tests to confirm this behaviour is
intentional.

closes: #239
@chriswk chriswk linked a pull request May 8, 2024 that will close this issue
chriswk added a commit that referenced this issue May 8, 2024
As discussed in #239 - When all is enabled, we had a bit of a surprising
behaviour where we'd fallback to fallback action for
`isEnabled(featureName, fallback)` even if all was enabled and feature
did not exist.

This PR fixes that, and adds tests to confirm this behaviour is
intentional.

closes: #239
chriswk added a commit that referenced this issue May 14, 2024
As discussed in #239 - When all is enabled, we had a bit of a surprising
behaviour where we'd fallback to fallback action for
`isEnabled(featureName, fallback)` even if all was enabled and feature
did not exist.

This PR fixes that, and adds tests to confirm this behaviour is
intentional.

closes: #239
@github-project-automation github-project-automation bot moved this from Todo to Done in Issues and PRs May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants