feat(feature flag scheduled changes): connect UI to the API #19322
Conversation
…/schedule-feature-flag
…/schedule-feature-flag
📸 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. |
| @@ -443,6 +445,7 @@ export const featureFlagLogic = kea<featureFlagLogicType>([ | |||
| surveys: [...(state.surveys || []), newSurvey], | |||
| } | |||
| }, | |||
| setId: (state, { id }) => ({ ...state, id }), | |||
There was a problem hiding this comment.
This is not ok, we shouldn't be changing a prop level data, that changes the logic key like so. Rather, the id should be passed in at logic initialisation time as a prop, from which everything else flows downwards, like loading the flag data. (like what we do in bindlogic)
In here, we have to be wary of having a new flag id connected to the old flag id data.
In what case does the id need to be 'schedule' for the featureflaglogic? iiuc, scheduling can only happen on an existing flag, so the regular bindlogic should work perfectly for scheduling as well, without the new 'schedule' id? Am I missing something?
There was a problem hiding this comment.
Ah, I'm guessing you need it right now to show a new release condition, even for an existing flag. Reasonable to have a different keyed logic there so the existing release conditions don't show up 👍 - wondering if there is a better way. But let's keep it isolated there and use the right logic everywhere else.
There was a problem hiding this comment.
Indeed, I'm using a separate copy of the logic to handle the state of the filters to be added.
Important to distinguish between the logic ID and the feature flag ID. Here, I'm setting the feature flag ID retrieved from the main flag logic. It's the only part of the state that needs to be shared. Better to call this action/reducer setFeatureFlagId - done
Also wondering if there's a better way 🤔
There was a problem hiding this comment.
ahh, you're right I was confusing the two. The problem though is that props.id == values.featureFlag.id whenever the props.id != new | schedule | unknown and this paves the way for breaking this assumption.
Okay, instead of adding this state, that would almost certainly mean the feature flag on the logic is in an inconsistent state (id and other data mismatch), how about we add the feature flag id as a parameter to loadScheduledChanges & createSchedule?
Or, maybe, all schedule related stuff becomes a separate new logic, which can then connect to a feature flag logic if need be, or have a setFeatureFlagId. Or, use the 'right' logic for the schedule tab: i.e. use the id=schedule keyed logic only when extracting what the scheduled change is from the release condition, but not as a default.
i.e use both logics here: https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/feature-flags/FeatureFlagSchedule.tsx#L16 - for the appropriate correct things. If it needs to access flag id related stuff, use the logic with the right keyed id. For new schedule related stuff, id=schedule.
There was a problem hiding this comment.
(ok to revisit, I'm not sure what's the best way here yet, will have a look once back 👀 )
There was a problem hiding this comment.
I guess I'm not fully appreciating where breaking the assumption might become an issue, but I see that this is becoming a bit of a spaghetti.
I did consider passing the feature flag id as a parameter. The issue is that the success listener also needs this value (to reload the list) - so it would require returning the flag id value from the loader, when the loader should just return the loaded value itself.
I do like reusing the same logic for both, while keeping the state separate by having the logic copied for the schedule use case. Any scheduled flag change, additive or not, changes the same model and thus should be possible to express with the same logic.
Also would love to revisit when you're back :) are you okay merging this to keep moving? Not planning to introduce any more changes/mess to the logic before we revisit this.
There was a problem hiding this comment.
[reminder for self: I suspect the ideal refactor here would be to FeatureFlagReleaseConditions to have an onChange handler that returns filters to you to do whatever with. The form handler just updates, the schedule handler sets is to scheduledFieldUpdate - so the only place the 'id=schedule' logic is required is the release condition, which keeps things decently isolated, and the regular flag logic is used everywhere.]
|
Size Change: +698 B (0%) Total Size: 1.99 MB ℹ️ View Unchanged
|
…og/posthog into feat/schedule-feature-flag
latest_migrations.manifest
Outdated
| @@ -5,6 +5,7 @@ contenttypes: 0002_remove_content_type_name | |||
| ee: 0015_add_verified_properties | |||
| otp_static: 0002_throttling | |||
| otp_totp: 0002_auto_20190420_0723 | |||
| posthog: 0374_scheduled_change | |||
There was a problem hiding this comment.
I don't think this belongs here?
There was a problem hiding this comment.
Hmm strange, this line was added in my previous PR, but then was removed by ... your PR 😅
So let's add it back :)
There was a problem hiding this comment.
wait this would get rid of the most recent migration no?
There was a problem hiding this comment.
There should only be 1 posthog migration here and it should be the most recent one which is 0375
There was a problem hiding this comment.
You're not even migrating anything in this PR, I'm confused? 😆
There was a problem hiding this comment.
There should only be one entry for posthog: that corresponds with the latest migration by number in posthog/migrations!
There was a problem hiding this comment.
oh I see, thanks for clarifying!
Changes
Call the
/scheduled_changesAPI from the frontend (create, delete)How did you test this code?
👀