-
Notifications
You must be signed in to change notification settings - Fork 546
feat: signup corporate-only experiment with exposure and new_signup events #7979
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
base: main
Are you sure you want to change the base?
Changes from all commits
32af124
ed57a05
d0147c8
9af5723
f855915
56c672e
71683f0
0f99759
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { useEffect, useState } from 'react' | ||
| import flagsmith from '@flagsmith/flagsmith' | ||
| import { storageGet, storageSet } from './safeLocalStorage' | ||
|
|
||
| export default function useSignupExperiment(useEnvironmentFlag: boolean) { | ||
| const [variant, setVariant] = useState<string>() | ||
|
|
||
| useEffect(() => { | ||
| if (useEnvironmentFlag) { | ||
| return | ||
| } | ||
| const identifyAndExpose = async () => { | ||
| const id = storageGet('signup_anonymous_id') || crypto.randomUUID() | ||
| storageSet('signup_anonymous_id', id) | ||
| // @ts-expect-error transient is missing from the SDK's identify type | ||
| await flagsmith.identify(id, {}, true) | ||
| const flag = flagsmith.getExperimentFlag('signup_corporate_only') | ||
| setVariant(flag?.enabled ? flag.variant : undefined) | ||
| } | ||
| identifyAndExpose() | ||
| }, [useEnvironmentFlag]) | ||
|
Comment on lines
+8
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: git ls-files 'frontend/common/*' | rg 'useSignupExperiment|HomePage|signup|Experiment'Repository: Flagsmith/flagsmith Length of output: 406 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'frontend/common/*' | rg 'useSignupExperiment|HomePage|signup|Experiment'Repository: Flagsmith/flagsmith Length of output: 413 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'Search usages:\n'
rg -n "useSignupExperiment" frontend/common || true
printf '\nHook file:\n'
sed -n '1,200p' frontend/common/useSignupExperiment.ts
printf '\nHomePage candidates:\n'
rg -n "blockGenericEmailDomain|signup_corporate_only|variant" frontend/common || trueRepository: Flagsmith/flagsmith Length of output: 1970 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path('frontend/common/useSignupExperiment.ts'),
]
for p in paths:
print(f'--- {p} ---')
if p.exists():
for i, line in enumerate(p.read_text().splitlines(), 1):
print(f'{i:4}: {line}')
else:
print('missing')
PYRepository: Flagsmith/flagsmith Length of output: 1185 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'useSignupExperiment usages:\n'
rg -n "useSignupExperiment" frontend/common || true
printf '\nuseSignupExperiment.ts:\n'
cat -n frontend/common/useSignupExperiment.ts
printf '\nPotential consumers / related terms:\n'
rg -n "blockGenericEmailDomain|signup_corporate_only|variant" frontend/common || trueRepository: Flagsmith/flagsmith Length of output: 2180 🏁 Script executed: #!/bin/bash
set -euo pipefail
git ls-files 'frontend/**/*.ts' 'frontend/**/*.tsx' | rg 'useSignupExperiment|HomePage|Signup|signup' || trueRepository: Flagsmith/flagsmith Length of output: 239 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'HomePage outline:\n'
ast-grep outline frontend/web/components/pages/HomePage.tsx --view expanded || true
printf '\nHomePage relevant section:\n'
sed -n '1,260p' frontend/web/components/pages/HomePage.tsx
printf '\nSearch for signup email gating:\n'
rg -n "blockGenericEmailDomain|useSignupExperiment|signup_corporate_only|generic email|corporate" frontend/web/components/pages/HomePage.tsx frontend/common || trueRepository: Flagsmith/flagsmith Length of output: 10025 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'HomePage around email gating and submit:\n'
sed -n '560,760p' frontend/web/components/pages/HomePage.tsx
printf '\nSubmit handlers:\n'
rg -n "handle.*Submit|onSubmit|blockGenericEmailDomain|isFreeEmailDomain|signupVariant" frontend/web/components/pages/HomePage.tsxRepository: Flagsmith/flagsmith Length of output: 8780 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'Submit handler around first form:\n'
sed -n '340,430p' frontend/web/components/pages/HomePage.tsx
printf '\nSubmit handler around second form:\n'
sed -n '490,550p' frontend/web/components/pages/HomePage.tsxRepository: Flagsmith/flagsmith Length of output: 7473 Wait for the experiment flag before treating signup as ready.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ignoring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(๑•́ - •̀๑) |
||
|
|
||
| return variant | ||
| } | ||
|
Comment on lines
+5
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff Local This hook manages As per coding guidelines, "Use Redux Toolkit and RTK Query for state management, with the store located in Source: Coding guidelines |
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ import AccountStore from 'common/stores/account-store' | |
| import { LoginRequest, RegisterRequest } from 'common/types/requests' | ||
| import { useGetBuildVersionQuery } from 'common/services/useBuildVersion' | ||
| import { useUTMs } from 'common/useUTMs' | ||
| import useSignupExperiment from 'common/useSignupExperiment' | ||
|
|
||
| const HomePage: React.FC = () => { | ||
| const history = useHistory() | ||
|
|
@@ -180,6 +181,12 @@ const HomePage: React.FC = () => { | |
| currentLocation.indexOf('signup') !== -1) | ||
| const disableSignup = preventSignup && isSignup | ||
| const preventEmailPassword = Project.preventEmailPassword | ||
|
|
||
| const signupVariant = useSignupExperiment( | ||
| !isSignup || isInvite || !!preventEmailPassword || !!AccountStore.getUser(), | ||
| ) | ||
| const blockGenericEmailDomain = | ||
| signupVariant === 'signup_corporate_only' && isFreeEmailDomain(email) | ||
|
Comment on lines
+184
to
+189
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Duplicated magic string The variant name is hardcoded here and again in ♻️ Suggested shared constant// common/useSignupExperiment.ts
export const SIGNUP_CORPORATE_ONLY_VARIANT = 'signup_corporate_only'- const blockGenericEmailDomain =
- signupVariant === 'signup_corporate_only' && isFreeEmailDomain(email)
+ const blockGenericEmailDomain =
+ signupVariant === SIGNUP_CORPORATE_ONLY_VARIANT && isFreeEmailDomain(email)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignoring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(◡﹏◡✿) |
||
| const disableForgotPassword = Project.preventForgotPassword | ||
| const oauths: React.ReactNode[] = [] | ||
| const disableOauthRegister = Utils.getFlagsmithHasFeature( | ||
|
|
@@ -627,12 +634,16 @@ const HomePage: React.FC = () => { | |
| name='email' | ||
| id='email' | ||
| /> | ||
| {isFreeEmailDomain(email) && ( | ||
| <InfoMessage> | ||
| Signing up with a work email makes it easier | ||
| for co-workers to join your Flagsmith | ||
| organisation. | ||
| </InfoMessage> | ||
| {blockGenericEmailDomain ? ( | ||
| <ErrorMessage error='Please use your work email address to create your account.' /> | ||
| ) : ( | ||
| isFreeEmailDomain(email) && ( | ||
| <InfoMessage> | ||
| Signing up with a work email makes it | ||
| easier for co-workers to join your | ||
| Flagsmith organisation. | ||
| </InfoMessage> | ||
| ) | ||
| )} | ||
| <InputGroup | ||
| title='Password' | ||
|
|
@@ -663,7 +674,10 @@ const HomePage: React.FC = () => { | |
| disabled={ | ||
| isLoading || | ||
| isSaving || | ||
| !allRequirementsMet | ||
| !allRequirementsMet || | ||
| !firstName.trim() || | ||
| !lastName.trim() || | ||
| blockGenericEmailDomain | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| className='px-4 mt-3 full-width' | ||
| type='submit' | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Relative import violates path-import guideline.
./safeLocalStorageis a relative import; per repository guidelines this file should import via thecommon/path instead.♻️ Proposed fix
As per coding guidelines, "Use only
common/,components/, andproject/import paths; do not use relative imports" forfrontend/**/*.{js,jsx,ts,tsx}.📝 Committable suggestion
Source: Coding guidelines
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 enforced by the repo's linter:
@dword-design/import-alias/prefer-aliasrewrites imports to thecommon/alias except for same-directory imports, which it keeps relative —eslint --fixconvertscommon/safeLocalStorageback to./safeLocalStoragehere. Keeping the linter's form.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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 5028
Confirmed — the
.eslintrc.jsconfig enables@dword-design/import-alias/prefer-aliaswithcommonaliased to./common/, and this plugin intentionally keeps same-directory imports relative rather than rewriting them to the alias. SinceuseSignupExperiment.tsandsafeLocalStorageboth live infrontend/common/, the relative import here is the linter's expected/enforced form, not a violation.@Zaimwa9thanks for the clarification — that makes sense, and my original suggestion would have been immediately reverted byeslint --fix. Withdrawing the comment.(=^・ω・^=) ah, makes sense now