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

feat(schedule flag changes): tweak frontend #19488

Merged
merged 105 commits into from
Jan 4, 2024

Conversation

jurajmajerik
Copy link
Contributor

Changes

  • adjust for the new payload schema (field -> operation)
  • hide "Match by" dropdown
  • style table cells content
  • move the Schedule tab after Projects

UI follow-ups

  • fix DatePicker appearance
image

How did you test this code?

👀

jurajmajerik and others added 30 commits December 5, 2023 08:58
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@neilkakkar
Copy link
Collaborator

neilkakkar commented Dec 22, 2023

Ah, I see, this happens in attempt_set_tags , our fault indeed, that's the only thing acting after the update, so it's not too bad.

I don't think we need to change anything here, just fixing that should be good 👍 .


Found another more serious bug, not sure exactly what's happening, but when I have a multivariant flag and I add a scheduled change, it converts to a boolean flag. ... same for early access features

Another problem is that on group flags, the new condition added matches on users, which also isn't ok.

image

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@neilkakkar
Copy link
Collaborator

neilkakkar commented Dec 22, 2023

I think this is all a side effect of the way we're dealing with the flaglogic keyed schedule, which I mentioned in a previous PR. Resolving that somehow should resolve these too, but also ok to just fix it here, I will look into a broader refactor of that thing next week 😅

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

3 snapshot changes in total. 0 added, 3 modified, 0 deleted:

  • chromium: 0 added, 3 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

5 snapshot changes in total. 0 added, 5 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@neilkakkar
Copy link
Collaborator

neilkakkar commented Dec 22, 2023

oh wait, this problem has nothing to do with that refactor, it's this line, it's overriding everything in filters 😰

https://github.com/PostHog/posthog/blob/feat/schedule-feature-flag/posthog/models/feature_flag/feature_flag.py#L319

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@jurajmajerik
Copy link
Contributor Author

jurajmajerik commented Jan 3, 2024

Changes:

Follow-up:

  • add disabledReason for the DatePicker - not trivial, will tackle separately

} = useActions(featureFlagScheduleLogic)
const { aggregationLabel } = useValues(groupsModel)

const featureFlagId = useValues(featureFlagLogic).featureFlag.id
Copy link
Collaborator

Choose a reason for hiding this comment

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

keeping consistent, easier to read 😅

const { featureFlag } = useValues(featureFlagLogic)
const featureFlagId = featureFlagId
const aggregationGroupTypeIndex = ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is featureFlag is already destructured from featureFlagScheduleLogic, hence accessing the id here directly with the dot notation 😬

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahh, I see, minor nit, you can rename it when destructuring to prevent conflicts

const { featureFlag: originalFlag } = useValues(...)
const id = originalFlag.id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I didn't know this was possible, thanks!

@@ -873,6 +878,11 @@ export const featureFlagLogic = kea<featureFlagLogicType>([
if (scheduledChange && scheduledChange) {
lemonToast.success('Change scheduled successfully')
actions.loadScheduledChanges()
actions.setFeatureFlag({
...values.featureFlag,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this happens only on the keyed logic with id 'schedule' right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct!

Copy link
Collaborator

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

Let's get this in and test in prod :D

@jurajmajerik jurajmajerik merged commit 486b4a5 into master Jan 4, 2024
78 checks passed
@jurajmajerik jurajmajerik deleted the feat/schedule-feature-flag branch January 4, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants