-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(flags): update payloads object when removing variants #25580
Conversation
@@ -638,7 +638,7 @@ function UsageTab({ featureFlag }: { id: string; featureFlag: FeatureFlagType }) | |||
) { | |||
enrichUsageDashboard() | |||
} | |||
}, [dashboard]) | |||
}, [dashboard, hasEnrichedAnalytics, enrichUsageDashboard]) |
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.
satisfying ESLint
<LemonButton onClick={distributeVariantsEqually} size="xsmall"> | ||
(Redistribute) | ||
</LemonButton> |
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 didn't like how big this button was alongside the Rollout
header, they were too close in size and that made it feel like the button was a non-obvious CTA for an action. This change differentiates the two elements more distinctly, which makes the fact that Redistribute
is a button more obvious
Before
(not selected)
(selected)
That said, open to feedback on not doing this. cc @jurajmajerik
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.
Hm, when not selected/hovered it looks weird to have the different font size. I'd prob use type="secondary"
, remove the parentheses, no padding, and align it over on the right.
I still don't love this mostly because we're fairly constrained on width here, I think an icon with tooltip might be better, but I don't see one that makes a lot of sense. @corywatilo might be inspired, maybe something like a 100 with an arrow or something.
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.
Great feedback, and agreed that we don't seem to have something that feels quite right. I'm going to pull this section out of this PR because it generated more discussion that I expected and I don't want to block on the user-reported bugfix. Let me clean up the PR to just contain the bugfix and then I can follow up with this button change.
const newPayloads: Record<number, JsonType> = {} | ||
flag.filters.multivariate?.variants.forEach((variant, index) => { | ||
newPayloads[index] = flag.filters.payloads?.[variant.key] | ||
if (flag.filters.payloads?.[variant.key] !== undefined) { | ||
newPayloads[index] = flag.filters.payloads[variant.key] | ||
} |
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.
appeasing the compiler
const newPayloads: Record<string, JsonType> = {} | ||
flag.filters.multivariate.variants.forEach(({ key }, index) => { | ||
if (flag.filters?.payloads?.[index] !== undefined) { | ||
newPayloads[key] = flag.filters.payloads[index] |
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.
appeasing the compiler
|
||
const currentPayloads = { ...state.filters.payloads } | ||
const newPayloads: Record<number, any> = {} | ||
|
||
// TRICKY: In addition to modifying the variant array, we also need to shift the payload indices | ||
// because the variant array is being modified and we need to make sure that the payloads object | ||
// stays in sync with the variant array. | ||
Object.keys(currentPayloads).forEach((key) => { | ||
const payloadIndex = parseInt(key) | ||
if (payloadIndex > index) { | ||
newPayloads[payloadIndex - 1] = currentPayloads[payloadIndex] | ||
} else if (payloadIndex < index) { | ||
newPayloads[payloadIndex] = currentPayloads[payloadIndex] | ||
} | ||
}) |
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.
this is the meat of the change that fixes the bug.
@@ -636,7 +655,7 @@ export const featureFlagLogic = kea<featureFlagLogicType>([ | |||
createScheduledChange: async () => { | |||
const { scheduledChangeOperation, scheduleDateMarker, currentTeamId, schedulePayload } = values | |||
|
|||
const fields = { | |||
const fields: Record<ScheduledChangeOperationType, keyof ScheduleFlagPayload> = { |
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.
appeasing the compiler
Size Change: 0 B Total Size: 1.14 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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.
🙌
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.
Ship it!
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Problem
We had a bug where deleting variants didn't upload the payloads associated with the variants, which led to some confusing and buggy behavior in the UI because it created mismatched variant : payload pairings. Additional context
Changes
The fix was pretty straightforward, in the
removeVariant
kea form, we just needed to upload the payload indices alongside the variants array.See this demo of the new behavior: https://www.loom.com/share/9ffab7a856e44fda9c0927c2f9c28c0a
Does this work well for both Cloud and self-hosted?
Yup
How did you test this code?
Manual UI testing.