-
-
Notifications
You must be signed in to change notification settings - Fork 658
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: add strategy bug when strategySplittedButton flag is on #4071
Conversation
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Thenks for a quick fix. Added minor improvements suggestions in two places.
@@ -101,11 +101,24 @@ export const FeatureStrategyCreate = () => { | |||
|
|||
useEffect(() => { | |||
if (shouldUseDefaultStrategy) { | |||
setStrategy((defaultStrategy as any) || DEFAULT_STRATEGY); | |||
const strategyTemplate = defaultStrategy || DEFAULT_STRATEGY; | |||
if ( |
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.
Can be simplified to
if (strategyTemplate.parameters?.groupId === '' && featureId) {
strategyTemplate.parameters.groupId = featureId;
}
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.
Thanks
) { | ||
strategyTemplate.parameters.groupId = featureId; | ||
} | ||
setStrategy(strategyTemplate as any); |
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.
as any can be avoided by changing
export interface IFeatureStrategy {
...other fields
title?: string | null;
disabled?: boolean | null;
}
Our FE code didn't match openapi schema so let's adjust them instead of ignorign types
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 added nullable constraint ont itle and disabled as indicated by the openapi schema
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 doing the type change opens a huge can of worms. I think more important to fix the functionality. Decided to skip this for now
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Fixes 2 bugs after the recent strategy improvements v2 changes:
When creating a strategy the
groupId
param of the Gradual Rollout strategy now populates the groupId (when using default strategy, the groupId will only be overwritten when it is an empty string ) with the feature name (as it was before)When editing/setting a default strategy for an environment the
groupId
param should be an empty string, but editable.About the changes
Closes #
Important files
Discussion points