-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: making role in organization as a required field #26205
Conversation
Size Change: 0 B Total Size: 1.15 MB ℹ️ View Unchanged
|
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.
Not a React expert, but it looks good to me :)
frontend/src/scenes/authentication/signup/signupForm/signupLogic.ts
Outdated
Show resolved
Hide resolved
frontend/src/scenes/authentication/signup/signupForm/signupLogic.ts
Outdated
Show resolved
Hide resolved
name: !name ? 'Please enter your name' : undefined, | ||
role_at_organization: !role_at_organization ? 'Please select your role in the organization' : undefined, |
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.
Yeah, you're learning kea!
@surbhi-posthog after you fix something feel free to re-request a review. For the failed E2E test, click into it and see if it's related to your change, if not you can try to re-run as it may be a flakey test. |
bff1626
to
d43f727
Compare
@zlwaterfield, i've added the changes to the cypress tests and they now pass. Could you take another look at this? In the meantime, I'm fixing the |
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.
Nice work! LGTM.
I ran |
Problem
since we want to understand the type of users using our product, having the role information is crucial and we want to try to make it as a required field.
Changes
Comparison: Before vs. After
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
self-hosted
How did you test this code?
Tested locally after commenting out the signup reroute in
sceneLogic.ts
. I verified that a new user was created and could access their posthog account.NOTE:
ran
DEBUG=1 CLOUD_DEPLOYMENT=1 bin/start
to clear the authentication credential issue when there are more than one users created in the same flow