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(flags): update payloads object when removing variants #25580

Merged
merged 12 commits into from
Oct 17, 2024
6 changes: 4 additions & 2 deletions frontend/src/scenes/feature-flags/FeatureFlag.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ function UsageTab({ featureFlag }: { id: string; featureFlag: FeatureFlagType })
) {
enrichUsageDashboard()
}
}, [dashboard])
}, [dashboard, hasEnrichedAnalytics, enrichUsageDashboard])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

satisfying ESLint


const propertyFilter: AnyPropertyFilter[] = [
{
Expand Down Expand Up @@ -952,7 +952,9 @@ function FeatureFlagRollout({ readOnly }: { readOnly?: boolean }): JSX.Element {
</div>
<div className="col-span-4 flex items-center gap-1">
<span>Rollout</span>
<LemonButton onClick={distributeVariantsEqually}>(Redistribute)</LemonButton>
<LemonButton onClick={distributeVariantsEqually} size="xsmall">
(Redistribute)
</LemonButton>
Copy link
Contributor Author

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)
image
(selected)
image

After
(not selected)
image

(selected)
image

That said, open to feedback on not doing this. cc @jurajmajerik

Copy link
Member

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.

Arc 2024-10-14 19 51 58

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.

Copy link
Contributor Author

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.

</div>
</div>
{variants.map((variant, index) => (
Expand Down
35 changes: 27 additions & 8 deletions frontend/src/scenes/feature-flags/featureFlagLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
FilterType,
InsightModel,
InsightType,
JsonType,
MultivariateFlagOptions,
MultivariateFlagVariant,
NewEarlyAccessFeatureType,
Expand Down Expand Up @@ -132,9 +133,11 @@ export const variantKeyToIndexFeatureFlagPayloads = (flag: FeatureFlagType): Fea
return flag
}

const newPayloads = {}
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]
}
Comment on lines +136 to +140
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appeasing the compiler

})
return {
...flag,
Expand All @@ -147,11 +150,10 @@ export const variantKeyToIndexFeatureFlagPayloads = (flag: FeatureFlagType): Fea

const indexToVariantKeyFeatureFlagPayloads = (flag: Partial<FeatureFlagType>): Partial<FeatureFlagType> => {
if (flag.filters?.multivariate) {
const newPayloads = {}
flag.filters?.multivariate?.variants.forEach(({ key }, index) => {
const payload = flag.filters?.payloads?.[index]
if (payload) {
newPayloads[key] = payload
const newPayloads: Record<string, JsonType> = {}
flag.filters.multivariate.variants.forEach(({ key }, index) => {
if (flag.filters?.payloads?.[index] !== undefined) {
newPayloads[key] = flag.filters.payloads[index]
Comment on lines +153 to +156
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appeasing the compiler

}
})
return {
Expand Down Expand Up @@ -316,6 +318,22 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
}
const variants = [...(state.filters.multivariate?.variants || [])]
variants.splice(index, 1)

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]
}
})
Comment on lines +321 to +335
Copy link
Contributor Author

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.


return {
...state,
filters: {
Expand All @@ -324,6 +342,7 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
...state.filters.multivariate,
variants,
},
payloads: newPayloads,
},
}
},
Expand Down Expand Up @@ -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> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appeasing the compiler

[ScheduledChangeOperationType.UpdateStatus]: 'active',
[ScheduledChangeOperationType.AddReleaseCondition]: 'filters',
}
Expand Down
Loading