-
Notifications
You must be signed in to change notification settings - Fork 556
fix(signup): add a route to login when the email already has an account #8177
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
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 |
|---|---|---|
|
|
@@ -24,6 +24,10 @@ export interface InputProps | |
| autoValidate?: boolean | ||
| centered?: boolean | ||
| inputClassName?: string | ||
| // Already known to be wrong, e.g. the API rejected it. Shows immediately, | ||
| // unlike isValid, which waits until the field has been touched so a pristine | ||
| // form is not red before anyone has typed. | ||
| isInvalid?: boolean | ||
| isValid?: boolean | ||
| ref?: Ref<InputMethods> | ||
| search?: boolean | ||
|
|
@@ -53,6 +57,7 @@ const Input: React.FC<InputProps> = ({ | |
| className = '', | ||
| disabled, | ||
| inputClassName, | ||
| isInvalid = false, | ||
| isValid = true, | ||
| onBlur: onBlurProp, | ||
| onChange, | ||
|
|
@@ -101,8 +106,8 @@ const Input: React.FC<InputProps> = ({ | |
| onKeyDownProp?.(e) | ||
| } | ||
|
|
||
| const invalid = shouldValidate && !isValid | ||
| const success = isValid && showSuccess | ||
| const invalid = isInvalid || (shouldValidate && !isValid) | ||
|
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. New prop rather than reusing |
||
| const success = isValid && !invalid && showSuccess | ||
| const sizeClassName = size ? sizeClassNames[size] : '' | ||
| const containerClassName = cn( | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| // Draws the eye to the way out once the form knows the address already has an | ||
| // account. Finite on purpose: something that blinks until you act on it cannot | ||
| // be dismissed, which is what WCAG 2.2.2 asks for. | ||
| @keyframes login-prompt-highlight { | ||
| 0%, | ||
| 100% { | ||
| background-color: transparent; | ||
| } | ||
| 50% { | ||
| background-color: var(--color-surface-action-subtle); | ||
| } | ||
| } | ||
|
|
||
| .login-prompt { | ||
| padding: 8px 12px; | ||
| border-radius: var(--radius-md); | ||
|
|
||
| &--highlight { | ||
| animation: login-prompt-highlight 0.7s ease-in-out 2; | ||
|
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. Two pulses, not a loop. A continuous blink cannot be dismissed, which is what WCAG 2.2.2 covers. Under |
||
| } | ||
| } | ||
|
|
||
| // The signal still needs to land, so it stays as a static tint rather than | ||
| // disappearing along with the motion. | ||
| @media (prefers-reduced-motion: reduce) { | ||
| .login-prompt--highlight { | ||
| animation: none; | ||
| background-color: var(--color-surface-action-subtle); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,12 +9,10 @@ import Constants from 'common/constants' | |
| import ErrorMessage from 'components/ErrorMessage' | ||
| import Button from 'components/base/forms/Button' | ||
| import PasswordRequirements from 'components/PasswordRequirements' | ||
| import { informationCircleOutline } from 'ionicons/icons' | ||
| import { IonIcon } from '@ionic/react' | ||
| import { Icon } from 'components/icons' | ||
| import classNames from 'classnames' | ||
| import InfoMessage from 'components/InfoMessage' | ||
| import OnboardingPage from './OnboardingPage' | ||
| import OnboardingPage from 'components/pages/OnboardingPage' | ||
| import isFreeEmailDomain from 'common/utils/isFreeEmailDomain' | ||
| import InputGroup from 'components/base/forms/InputGroup' | ||
| import { Link } from 'react-router-dom' | ||
|
|
@@ -32,6 +30,38 @@ import { LoginRequest, RegisterRequest } from 'common/types/requests' | |
| import { useGetBuildVersionQuery } from 'common/services/useBuildVersion' | ||
| import { useUTMs } from 'common/useUTMs' | ||
| import useSignupExperiment from 'common/useSignupExperiment' | ||
| import './HomePage.scss' | ||
|
|
||
| type EmailFieldError = string | string[] | ||
| type EmailError = { email?: EmailFieldError } | undefined | ||
|
|
||
| // The error object never clears itself, and it describes the address that was | ||
| // submitted, so it stops applying the moment the field holds something else. | ||
| const currentEmailError = ( | ||
| error: EmailError, | ||
| email: string, | ||
| submittedEmail: string, | ||
| ) => (email === submittedEmail ? error?.email : undefined) | ||
|
Contributor
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. This should be lowercased to mirror the backend matching with
Contributor
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. also I think the calls should be gated with a |
||
|
|
||
| // Matched on the message because the API sends no error code for this. Raised | ||
| // by CustomUserCreateSerializer.validate. | ||
| const isEmailTaken = ( | ||
|
Contributor
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. Idem with |
||
| error: EmailError, | ||
| email: string, | ||
| submittedEmail: string, | ||
| ) => { | ||
| const current = currentEmailError(error, email, submittedEmail) | ||
| const messages = Array.isArray(current) ? current : [current] | ||
| return messages.some((message) => | ||
| message?.toLowerCase().includes('already exists'), | ||
| ) | ||
| } | ||
|
|
||
| // The banner is for errors with no field to attach to. Anything belonging to a | ||
| // field is shown under that field instead, so one failure gives one message. | ||
| const SIGNUP_FIELDS = ['email', 'first_name', 'last_name', 'password'] | ||
| const hasFieldError = (error?: Record<string, unknown>) => | ||
| SIGNUP_FIELDS.some((field) => !!error?.[field]) | ||
|
|
||
| const HomePage: React.FC = () => { | ||
| const history = useHistory() | ||
|
|
@@ -42,6 +72,9 @@ const HomePage: React.FC = () => { | |
| const [lastName, setLastName] = useState('') | ||
| const [marketingConsentGiven] = useState(true) | ||
| const [password, setPassword] = useState('') | ||
| // The error object never clears on its own, so remember which address it was | ||
| // about. | ||
| const [submittedEmail, setSubmittedEmail] = useState('') | ||
|
|
||
| const [samlError, setLocalError] = useState(false) | ||
| const [samlLoading, setSamlLoading] = useState(false) | ||
|
|
@@ -386,13 +419,11 @@ const HomePage: React.FC = () => { | |
| }} | ||
| > | ||
| {isInvite && ( | ||
| <div className='notification flex-row'> | ||
| <span className='notification__icon mb-2'> | ||
| <IonIcon | ||
| icon={informationCircleOutline} | ||
| /> | ||
| <div className='notification d-flex align-items-center justify-content-center gap-2 mb-3'> | ||
| <span className='notification__icon d-flex'> | ||
| <Icon name='info-outlined' width={20} /> | ||
| </span> | ||
| <p className='notification__text pl-3'> | ||
| <p className='notification__text mb-0'> | ||
| Log in to accept your invite | ||
| </p> | ||
| </div> | ||
|
|
@@ -521,6 +552,7 @@ const HomePage: React.FC = () => { | |
| const isInvite = | ||
| document.location.href.indexOf('invite') !== | ||
| -1 | ||
| setSubmittedEmail(email) | ||
| register( | ||
| { | ||
| email, | ||
|
|
@@ -535,7 +567,7 @@ const HomePage: React.FC = () => { | |
| ) | ||
| }} | ||
| > | ||
| {error && ( | ||
| {error && !hasFieldError(error) && ( | ||
| <Row> | ||
| <div | ||
| id='error-alert' | ||
|
|
@@ -553,29 +585,13 @@ const HomePage: React.FC = () => { | |
| </Row> | ||
| )} | ||
| {isInvite && ( | ||
| <div> | ||
| <div className='notification flex-row'> | ||
| <span className='notification__icon mb-2'> | ||
| <IonIcon | ||
| icon={informationCircleOutline} | ||
| /> | ||
| </span> | ||
| <p className='notification__text pl-3'> | ||
| Create an account to accept your invite | ||
| </p> | ||
| </div> | ||
| <Row className='justify-content-center'> | ||
| Have an account?{' '} | ||
| <Button | ||
| theme='text' | ||
| className='ml-1 fw-bold' | ||
| onClick={() => { | ||
| window.location.href = `/login${redirect}` | ||
| }} | ||
| > | ||
| Log in | ||
| </Button> | ||
| </Row> | ||
| <div className='notification d-flex align-items-center justify-content-center gap-2 mb-3'> | ||
| <span className='notification__icon d-flex'> | ||
| <Icon name='info-outlined' width={20} /> | ||
| </span> | ||
| <p className='notification__text mb-0'> | ||
| Create an account to accept your invite | ||
| </p> | ||
| </div> | ||
| )} | ||
| <fieldset id='details'> | ||
|
|
@@ -616,10 +632,21 @@ const HomePage: React.FC = () => { | |
| <InputGroup | ||
| title='Email Address' | ||
| data-test='email' | ||
| isInvalid={ | ||
| !!currentEmailError( | ||
| error, | ||
| email, | ||
| submittedEmail, | ||
| ) | ||
| } | ||
| inputProps={{ | ||
| autoComplete: 'on', | ||
| className: 'full-width', | ||
| error: error && error.email, | ||
| error: currentEmailError( | ||
| error, | ||
| email, | ||
| submittedEmail, | ||
| ), | ||
| name: 'email', | ||
| }} | ||
| onChange={( | ||
|
|
@@ -671,7 +698,8 @@ const HomePage: React.FC = () => { | |
| !allRequirementsMet || | ||
| !firstName.trim() || | ||
| !lastName.trim() || | ||
| blockGenericEmailDomain | ||
| blockGenericEmailDomain || | ||
| isEmailTaken(error, email, submittedEmail) | ||
| } | ||
| className='px-4 mt-3 full-width' | ||
| type='submit' | ||
|
|
@@ -683,6 +711,29 @@ const HomePage: React.FC = () => { | |
| </form> | ||
| )} | ||
| </Card> | ||
| <Row | ||
| className={classNames( | ||
| 'login-prompt justify-content-center', | ||
| { | ||
| 'login-prompt--highlight': isEmailTaken( | ||
| error, | ||
| email, | ||
| submittedEmail, | ||
| ), | ||
| }, | ||
| )} | ||
| > | ||
| {isEmailTaken(error, email, submittedEmail) | ||
| ? 'You already have an account.' | ||
| : 'Have an account?'}{' '} | ||
| <Button | ||
| theme='text' | ||
| className='ml-1 fw-bold' | ||
| href={`/login${redirect}`} | ||
| > | ||
| Log in | ||
| </Button> | ||
| </Row> | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| </> | ||
| )} | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { default } from './HomePage' |
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 | 🔵 Trivial
Run the added invite E2E coverage before merge.
The enabled-state assertions are appropriate, but the PR context states that this coverage was not run. Execute
frontend/e2e/tests/invite-test.pw.tsin the supported Playwright environment. Confirm the invite login and duplicate-email paths pass against the current signup response.Also applies to: 81-83