-
-
Notifications
You must be signed in to change notification settings - Fork 744
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
feat: add option to return disabled strategies #5059
feat: add option to return disabled strategies #5059
Conversation
Signed-off-by: andreas-unleash <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: andreas-unleash <[email protected]>
Signed-off-by: andreas-unleash <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should improve the architecture slightly before merging
if ( | ||
this.isUnseenStrategyRow(feature, row) && | ||
(includeDisabledStrategies ? true : !row.strategy_disabled) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the correct place to put this. createBaseFeature should only care about what is common between the two API endpoints. Instead you should take this check and move into the respective methods: buildFeatureToggleListFromRows
and buildPlaygroundFeaturesFromRows
and have each method have different checks for which strategies they allow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that both of those functions are only used for the Admin API - for which we (everywhere) return the disabled strategies
Signed-off-by: andreas-unleash <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is only relevant to the playground API
if ( | ||
this.isUnseenStrategyRow(feature, r) && | ||
includeDisabledStrategies && | ||
r.strategy_disabled | ||
) { | ||
feature.strategies?.push(this.rowToStrategy(r)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the feature toggle list need disabled strategies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am misunderstanding something here. Following the calls
featureToggleRowConverter.buildFeatureToggleListFromRows
is called from
featureToggleStore.getFeatureToggleList
is called from
featureToggleService.getFeatureToggles
is called from
featureToggleLegacyController.getAllToggles
that is initialised from the AdminApi.
I added it because of following up to the Admin api admin/features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following to the frontend - the disabled strategies are not needed
Signed-off-by: andreas-unleash <[email protected]>
Not only relevant to playground. To be more specific - this is needed for the admin api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds the option to include disabled strategies (behind the playgroundImprovements flag
Closes # 1-1505