-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: exp for nontechnical users in prod. analytics #26993
base: master
Are you sure you want to change the base?
Conversation
Size Change: +74 B (+0.01%) Total Size: 1.11 MB ℹ️ View Unchanged
|
7e87466
to
274ada4
Compare
274ada4
to
d639bc6
Compare
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'm just reviewing a couple of random PRs to get myself familiarized with the rest of the codebase, so take my review with a grain of salt, I don't know anything about current goals in the team :)
Wont approve nor request changes because Im just passing by
@@ -39,10 +41,25 @@ Products that will often be installed in multiple places, eg. web and mobile | |||
*/ | |||
export const multiInstallProducts = [ProductKey.PRODUCT_ANALYTICS, ProductKey.FEATURE_FLAGS] | |||
|
|||
const getOrderedSDKs = (sdks: SDK[]): SDK[] => { |
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 only "ordered" in terms of product analytics, so I'd be more semantic in this function name
const getOrderedSDKs = (sdks: SDK[]): SDK[] => { | |
const getOrderedProductAnalyticsSdks = (sdks: SDK[]): SDK[] => { |
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 work here :). Added a few requests.
frontend/src/lib/constants.tsx
Outdated
@@ -156,6 +156,7 @@ export const FEATURE_FLAGS = { | |||
FF_DASHBOARD_TEMPLATES: 'ff-dashboard-templates', // owner: @EDsCODE | |||
ARTIFICIAL_HOG: 'artificial-hog', // owner: @Twixes | |||
CS_DASHBOARDS: 'cs-dashboards', // owner: @pauldambra | |||
PRODUCT_ANALYTICS_ONBOARDING: 'product-analytics-onboarding', // owner: @surbhi |
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.
Can we make this experiment key more specific to these changes incase we want to add more in onboarding.
'/onboarding/:productKey': (_product_key, { sdk }) => { | ||
if ( | ||
values.productKey === ProductKey.PRODUCT_ANALYTICS && | ||
values.featureFlags[FEATURE_FLAGS.PRODUCT_ANALYTICS_ONBOARDING] |
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.
For an experiment we can do values.featureFlags[FEATURE_FLAGS.PRODUCT_ANALYTICS_ONBOARDING] === "test"
where "test" is the experiment variant.
// Set default source filter for Product Analytics | ||
if ( | ||
productKey === ProductKey.PRODUCT_ANALYTICS && | ||
values.featureFlags[FEATURE_FLAGS.PRODUCT_ANALYTICS_ONBOARDING] |
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.
Sam as comment below.
.filter((sdk) => { | ||
if (!values.sourceFilter || !sdk) { | ||
return true | ||
} | ||
return sdk.tags.includes(values.sourceFilter) | ||
}) | ||
.filter((sdk) => Object.keys(values.availableSDKInstructionsMap).includes(sdk.key)) | ||
|
||
if ( | ||
values.featureFlags[FEATURE_FLAGS.PRODUCT_ANALYTICS_ONBOARDING] && |
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.
Same as comment below.
I've made suggested changes. However, I'm still not able to see the feature flag values show up locally. This is currently undefined: |
@@ -198,7 +222,6 @@ export const sdksLogic = kea<sdksLogicType>([ | |||
}, | |||
[onboardingLogic.actionTypes.setProductKey]: () => { | |||
// TODO: This doesn't seem to run when the setProductKey action is called in onboardingLogic... | |||
actions.resetSDKs() |
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.
Any reason this was removed?
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.
Commenting on this file so we can thread the discussion...
I'm generally hesitant about this change because it effectively hides the rest of the SDKs we offer from the page.
The null hypothesis is that people won't notice the "recommended" filter, won't click the "X" to clear it, and won't see all the other SDKs we offer.
Why don't we just put the recommended ones at the top in a separate section, with some way to show that they are recommended?
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.
Another thing - I think with this we are maybe prioritizing the wrong set of users?
High ICP users - engineers - will want to see all the SDK options we have so they can find the one they need. Maybe the experience there needs to change for those users if their conversion through this step isn't great, but I don't see why we'd change the onboarding for our most popular product to cater to low ICP users.
If anything, we should only apply this change to people who we know are low-ICP (using role data is easy easy easy and required on signup for this exact reason). We can even skip this SDKs screen entirely for these people and do a custom invite thing where it says "you're all set up, now invite your technical team to implement" and in the email we send them we give them context about what needs to happen.
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 feel like my comments should be addressed before this is merged
After consolidating the feedback above, I've made a few significant changes to the experiment:
I’m hoping to get some signal from this test, and if anything learn how to collect signal correctly for this flow. This got me thinking about other ideas, some of which I know we’re already discussing:
|
@@ -250,6 +250,14 @@ export const userLogic = kea<userLogicType>([ | |||
return user?.theme_mode || 'light' | |||
}, | |||
], | |||
|
|||
isUserTechnical: [ |
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 think we should flip this so it defaults to technical for all users.
It's important to note we won't always have this data because it's only via email/password not social SSO (Google, Github, etc.)
("other", "Other"), | ||
) | ||
|
||
role_at_organization = models.CharField(max_length=64, choices=ROLE_CHOICES, null=True, blank=True) |
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.
Can you break this PR out into multiple PRs? Database migrations are always important to be methodical about:
- Add new column w/ migration
- Add changes to signup API to support the new attribute
- Update the frontend to send the new attribute (and make any other changes)
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.
Also we can move this row up to be with the other user details.
@@ -172,6 +172,19 @@ class User(AbstractUser, UUIDClassicModel): | |||
# Remove unused attributes from `AbstractUser` | |||
username = None | |||
|
|||
ROLE_CHOICES = ( |
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.
Move this up to the top with the others.
Problem
We noticed that many users entering our funnel are categorized as "Low ICP." To help these users convert through the onboarding flow in product analytics that has the most number of users, this PR introduces a few product recommendations:
For non-technical users in the experiment ONLY ~
Show HTML first – This is likely to be more useful for non-technical users.
Show Invite banner on top of the page, so its obvious they can use this feature if needed.
I also considered creating two different experiments for this but I'd rather keep it under one since these changes are relatively small
For more details on the hypothesis please refer to this notebook: https://us.posthog.com/project/2/notebooks/p9AvDIAK
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Cloud: yes
How did you test this code?
Test Plan
Screen.Recording.2024-12-20.at.9.43.06.AM.mov
User Signup
role_at_organization
data is captured in the model for future use.Invite New User Signup
role_at_organization
data is recorded.Product Analytics Installation Test
Impact on Other Products
[NOTE] I added role_at_organization changes for when the user signs up as a demo. I haven't tested this case. I need to verify what the UX looks for this case. TODO @surbhi-posthog