fix: new flags default to 0% rollout instead of 100% automatically

[liyiy]

Problem

New flags are automatically rolled out to everyone if a user doesn't manually update the rollout conditions. This can cause problems for users who have not intentionally wanted to roll out their feature flag just yet but may want to still create a feature flag.

We can either add a warning confirmation to every new flag created when the user does not edit their rollout conditions before clicking "create"

OR

We can default new flags to 0% rollout if a user does not intentionally set their rollout conditions

Changes

On new flags, if a user does not

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

[posthog-bot]

Hey @liyiy! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

[neilkakkar]

I'd rather have the confirmation on rolling out to everyone, than defaulting to rolling out to no one even when the flag is enabled. This seems more confusing to me 😅

[liyiy]

I'd rather have the confirmation on rolling out to everyone, than defaulting to rolling out to no one even when the flag is enabled. This seems more confusing to me 😅

I think in general though the average feature flag user would think the behavior of a feature flag being created for the first time without a user intentionally enabling and rolling it out to people though should be defaulted to 0% rollout. I think we're just used to the behavior of flags being automatically rolled out to everyone because that's how we've been using posthog feature flags but I don't think that's a very secure way of preventing accidental rollouts.

The case for "oops I rolled this out to people when I didn't want to" is also worse than "oops I forgot to roll this out to people when I wanted to".

IMO a warning confirmation can be kind of annoying and doesn't really solve the issue because we're not fixing the problem which is the flow of the creation itself but rather putting another confirmation warning for the user to read and digest and click etc

We also don't do this for experiments where experiments are rolled out and start automatically once they're created, so why do we do it for feature flags? 🤔

Anyway, if you disagree strongly based on how users think/act I'm fine with inserting a warning confirmation, I just don't think it really solves the problem 😅 and the functionality that we're used to that makes sense for us may not make sense for users

[neilkakkar]

Fair, I'm open to trying this out, you make good points! We can discuss again if feedback is negative.

The code here is incorrect though: You're changing what null represents (it still represents 100% rollout - I'd be very strongly against changing what the default here means), what we instead want to change is that whenever a new group is added, the rollout_percentage is set to 0.

[liyiy]

Fair, I'm open to trying this out, you make good points! We can discuss again if feedback is negative.

The code here is incorrect though: You're changing what null represents (it still represents 100% rollout - I'd be very strongly against changing what the default here means), what we instead want to change is that whenever a new group is added, the rollout_percentage is set to 0.

yep it's just in draft mode rn, was deciding between fixing it at the backend level which would involve making a bunch of library updates too 😬 but I think I'll just stick with updating it on the FE where if the rollout % is null when you send the create new flag request, it sends in 0% rollout instead (yet to be added)

[neilkakkar]

yes, only in frontend please 😂 .

Please don't obfuscate it like that - just changing nulls here to 0 should do the trick:

https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/feature-flags/featureFlagLogic.ts#L57

https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/feature-flags/featureFlagLogic.ts#L225

https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/feature-flags/featureFlagLogic.ts#L363

probs worth abstracting in one place instead of all 3.

[neilkakkar]

Another component of this worth thinking through:

Say I have a country=USA rollout 100% flag.

Now if I delete the property filter (country=USA), does it make sense to reset the rollout to 0% as well? As otherwise, pretty counter-intuitively, deleting a condition leads to a flag being rolled out to a lot more people. 😅

[liyiy]

Another component of this worth thinking through:

Say I have a country=USA rollout 100% flag.

Now if I delete the property filter (country=USA), does it make sense to reset the rollout to 0% as well? As otherwise, pretty counter-intuitively, deleting a condition leads to a flag being rolled out to a lot more people. 😅

If that's the only rollout condition then I think it would make sense to reset to 0% if you're deleting/removing all conditions and have to add more conditions to intentionally roll out to more users

update: Actually I think it makes sense to leave the property filter as it is so when the user is deleting the country 100% rollout property filter, they are intentionally making updates to the rollout conditions and so leaving it to whatever rollout percentage as is would be fine? We'd only want to default it to a 0% rollout when the user is not intentionally making updates or if everything resets like when we change the "match by" groups type

I think that's why it would also make sense when a user clicks "add condition sets" to have those condition sets at 100% 🤔

What do you think? also @EDsCODE

[neilkakkar]

I think that's why it would also make sense when a user clicks "add condition sets" to have those condition sets at 100% 🤔

ah, no, this defeats half the purpose I think :/ . In most cases, the person who will make the mistake is someone who wouldn't edit existing conditions but add a new condition for person X, change their mind, delete the filter, and end up rolling out to 100%.

So, I'd be for putting the defaults to 0% everywhere.

[neilkakkar]

No strong feelings on the delete condition, reset to 0 case

[liyiy]

I think that's why it would also make sense when a user clicks "add condition sets" to have those condition sets at 100% 🤔

ah, no, this defeats half the purpose I think :/ . In most cases, the person who will make the mistake is someone who wouldn't edit existing conditions but add a new condition for person X, change their mind, delete the filter, and end up rolling out to 100%.

So, I'd be for putting the defaults to 0% everywhere.

fair! makes sense, will update

[EDsCODE]

Defaulting to 0% makes sense.

We'd only want to default it to a 0% rollout when the user is not intentionally making updates or if everything resets like when we change the "match by" groups type

I agree with this! We shouldn't be too trigger happy with automatic changes as it could actually be more jarring.

Idea I had while reading:
Should we add a highlight to the 100% condition if it's 100% to everyone? So for example, if you have three conditions like below, we actually "gray out" or subdue the conditions that are being overriden by the 100% condition. Alternatively, instead of graying out, have some sort of yellow warning state that draws attention to the fact that no condition will matter because there's a 100% box. This way the overridden conditions still present as editable

[liyiy]

Defaulting to 0% makes sense.

We'd only want to default it to a 0% rollout when the user is not intentionally making updates or if everything resets like when we change the "match by" groups type

I agree with this! We shouldn't be too trigger happy with automatic changes as it could actually be more jarring.

Idea I had while reading: Should we add a highlight to the 100% condition if it's 100% to everyone? So for example, if you have three conditions like below, we actually "gray out" or subdue the conditions that are being overriden by the 100% condition. Alternatively, instead of graying out, have some sort of yellow warning state that draws attention to the fact that no condition will matter because there's a 100% box. This way the overridden conditions still present as editable

yea it probably makes sense to add a highlight to 100% rollouts like we do on the feature flags table, will add in a follow up PR 😃