From 2cfa4c0999b23dc6c2e30ac4569c359c678dbe34 Mon Sep 17 00:00:00 2001 From: Mubbshar Anwar <78487564+mubbsharanwar@users.noreply.github.com> Date: Thu, 27 Jun 2024 16:38:02 +0500 Subject: [PATCH] fix: progressive profiling bug fixes (#90) - fix padding top bottom issue - subjects loading issue fix - add loading state for skip button - update message copy of progressive profiling skip flow --- .gitignore | 1 + src/authn-component/tests/index.test.jsx | 18 +++---- .../data/reducers.js | 5 ++ .../progressive-profiling-popup/index.jsx | 27 +++++++--- .../progressive-profiling-popup/index.scss | 6 ++- .../index.test.jsx | 51 +++++++------------ .../progressive-profiling-popup/messages.js | 6 +++ src/forms/registration-popup/index.jsx | 10 ++++ 8 files changed, 73 insertions(+), 51 deletions(-) diff --git a/.gitignore b/.gitignore index 44783962..bab04097 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ node_modules temp src/i18n/transifex_input.json .vscode/ +.env.private *~ module.config.js .idea/ diff --git a/src/authn-component/tests/index.test.jsx b/src/authn-component/tests/index.test.jsx index 5cc63840..8dd2fe40 100644 --- a/src/authn-component/tests/index.test.jsx +++ b/src/authn-component/tests/index.test.jsx @@ -19,7 +19,6 @@ import { REGISTRATION_FORM, } from '../../data/constants'; import { AuthnContext } from '../../data/storeHooks'; -import useSubjectList from '../../forms/progressive-profiling-popup/data/hooks/useSubjectList'; import { getThirdPartyAuthContext, setCurrentOpenedForm } from '../data/reducers'; import { AuthnComponent, SignInComponent, SignUpComponent } from '../index'; @@ -42,7 +41,6 @@ jest.mock('@edx/frontend-platform/i18n', () => ({ getLocale: jest.fn(), getMessages: jest.fn(), })); -jest.mock('../../forms/progressive-profiling-popup/data/hooks/useSubjectList', () => jest.fn()); describe('AuthnComponent Test', () => { let store = {}; @@ -217,15 +215,7 @@ describe('AuthnComponent Test', () => { it('renders PROGRESSIVE_PROFILING_FORM form if currentForm=PROGRESSIVE_PROFILING_FORM', () => { getLocale.mockImplementation(() => ('en-us')); getAuthenticatedUser.mockReturnValue({ userId: 3, username: 'abc123', name: 'Test User' }); - useSubjectList.mockReturnValue({ - subjectsList: { - options: [ - { label: 'Computer' }, - { label: 'Science' }, - ], - }, - subjectsLoading: false, - }); + store = mockStore({ ...initialState, commonData: { @@ -234,6 +224,12 @@ describe('AuthnComponent Test', () => { }, progressiveProfiling: { submitState: DEFAULT_STATE, + subjectsList: { + options: [ + { label: 'Computer' }, + { label: 'Science' }, + ], + }, }, }); const { container, getByTestId } = render(reduxWrapper( diff --git a/src/forms/progressive-profiling-popup/data/reducers.js b/src/forms/progressive-profiling-popup/data/reducers.js index 3a38ff39..b4560a6d 100644 --- a/src/forms/progressive-profiling-popup/data/reducers.js +++ b/src/forms/progressive-profiling-popup/data/reducers.js @@ -19,6 +19,7 @@ export const PROGRESSIVE_PROFILING_SLICE_NAME = 'progressiveProfiling'; export const progressiveProfilingInitialState = { submitState: DEFAULT_STATE, redirectUrl: '', + subjectsList: {}, }; export const progressiveProfilingSlice = createSlice({ @@ -37,6 +38,9 @@ export const progressiveProfilingSlice = createSlice({ setProgressiveProfilingRedirectUrl: (state, { payload: redirectUrl }) => { state.redirectUrl = redirectUrl; }, + setSubjectsList: (state, { payload }) => { + state.subjectsList = payload; + }, }, }); @@ -45,6 +49,7 @@ export const { saveUserProfileSuccess, saveUserProfileFailure, setProgressiveProfilingRedirectUrl, + setSubjectsList, } = progressiveProfilingSlice.actions; export default progressiveProfilingSlice.reducer; diff --git a/src/forms/progressive-profiling-popup/index.jsx b/src/forms/progressive-profiling-popup/index.jsx index affab575..e3408629 100644 --- a/src/forms/progressive-profiling-popup/index.jsx +++ b/src/forms/progressive-profiling-popup/index.jsx @@ -14,11 +14,16 @@ import { import { Language } from '@openedx/paragon/icons'; import { extendedProfileFields, optionalFieldsData } from './data/constants'; -import useSubjectsList from './data/hooks/useSubjectList'; import { saveUserProfile } from './data/reducers'; import messages from './messages'; import { setCurrentOpenedForm } from '../../authn-component/data/reducers'; -import { COMPLETE_STATE, LOGIN_FORM } from '../../data/constants'; +import { + COMPLETE_STATE, + DEFAULT_STATE, + FAILURE_STATE, + LOGIN_FORM, + PENDING_STATE, +} from '../../data/constants'; import { useDispatch, useSelector } from '../../data/storeHooks'; import { getCountryCookieValue } from '../../data/utils'; import { @@ -42,10 +47,10 @@ const ProgressiveProfilingForm = () => { const dispatch = useDispatch(); const countryCookieValue = getCountryCookieValue(); - const { subjectsList, subjectsLoading } = useSubjectsList(); const countryList = useMemo(() => getCountryList(getLocale()), []); const submitState = useSelector(state => state.progressiveProfiling.submitState); + const subjectsList = useSelector(state => state.progressiveProfiling.subjectsList); const redirectUrl = useSelector(state => state.progressiveProfiling.redirectUrl); const authContextCountryCode = useSelector(state => state.commonData.thirdPartyAuthContext.countryCode); const finishAuthUrl = useSelector(state => state.commonData.thirdPartyAuthContext.finishAuthUrl); @@ -54,6 +59,7 @@ const ProgressiveProfilingForm = () => { const [formData, setFormData] = useState({}); const [formErrors, setFormErrors] = useState({}); const [autoFilledCountry, setAutoFilledCountry] = useState({ value: '', displayText: '' }); + const [skipButtonState, setSkipButtonState] = useState(DEFAULT_STATE); useEffect(() => { let countryCode = null; @@ -160,12 +166,18 @@ const ProgressiveProfilingForm = () => { const handleSkip = (e) => { e.preventDefault(); + setSkipButtonState(PENDING_STATE); const hasCountry = !!countryCookieValue || !!authContextCountryCode; + if (hasFormErrors() && !hasCountry) { setFormErrors({ ...formErrors, country: formatMessage(messages.progressiveProfilingCountryFieldErrorMessage) }); + setSkipButtonState(FAILURE_STATE); } else if (!hasFormErrors() && !hasCountry) { - // TODO update error message copy here if user has no country value set in the backend and wants to skip this form - setFormErrors({ ...formErrors, country: formatMessage(messages.progressiveProfilingCountryFieldErrorMessage) }); + setFormErrors({ + ...formErrors, + country: formatMessage(messages.progressiveProfilingCountryFieldBlockingErrorMessage), + }); + setSkipButtonState(FAILURE_STATE); } else if (hasCountry) { // link tracker trackProgressiveProfilingSkipLinkClick(redirectUrl)(e); @@ -219,7 +231,7 @@ const ProgressiveProfilingForm = () => { name="subject" placeholder={formatMessage(messages.progressiveProfilingSubjectFieldPlaceholder)} label={formatMessage(messages.progressiveProfilingSubjectFieldLabel)} - options={subjectsLoading ? [] : subjectsList.options} + options={subjectsList?.options} onChangeHandler={handleSelect} /> @@ -271,10 +283,13 @@ const ProgressiveProfilingForm = () => { e.preventDefault()} diff --git a/src/forms/progressive-profiling-popup/index.scss b/src/forms/progressive-profiling-popup/index.scss index 21912ad9..48c97888 100644 --- a/src/forms/progressive-profiling-popup/index.scss +++ b/src/forms/progressive-profiling-popup/index.scss @@ -3,7 +3,7 @@ } .authn__popup-progressive-profiling-container { - padding: 7.5rem 4rem !important; + padding: 4rem 7.5rem !important; @media (max-width: 767px) { padding: 2.5rem 1.5rem !important; } @@ -15,3 +15,7 @@ .authn-progressive-profiling-submit-button { min-width: 5.938rem !important; } + +.authn-progressive-profiling-skip-button { + min-width: 8.68rem !important; +} diff --git a/src/forms/progressive-profiling-popup/index.test.jsx b/src/forms/progressive-profiling-popup/index.test.jsx index 4b57a0e8..acc9c331 100644 --- a/src/forms/progressive-profiling-popup/index.test.jsx +++ b/src/forms/progressive-profiling-popup/index.test.jsx @@ -10,7 +10,6 @@ import { act } from 'react-dom/test-utils'; import { MemoryRouter } from 'react-router-dom'; import configureStore from 'redux-mock-store'; -import useSubjectList from './data/hooks/useSubjectList'; import { saveUserProfile } from './data/reducers'; import { DEFAULT_STATE } from '../../data/constants'; import { AuthnContext } from '../../data/storeHooks'; @@ -36,8 +35,6 @@ jest.mock('@edx/frontend-platform/i18n', () => ({ getMessages: jest.fn(), })); -jest.mock('./data/hooks/useSubjectList', () => jest.fn()); - describe('ProgressiveProfilingForm Test', () => { let store = {}; @@ -56,6 +53,12 @@ describe('ProgressiveProfilingForm Test', () => { const initialState = { progressiveProfiling: { submitState: DEFAULT_STATE, + subjectsList: { + options: [ + { label: 'Computer' }, + { label: 'Science' }, + ], + }, }, commonData: { thirdPartyAuthContext: { @@ -75,15 +78,6 @@ describe('ProgressiveProfilingForm Test', () => { beforeEach(() => { store = mockStore(initialState); getLocale.mockImplementationOnce(() => ('en-us')); - useSubjectList.mockReturnValue({ - subjectsList: { - options: [ - { label: 'Computer' }, - { label: 'Science' }, - ], - }, - subjectsLoading: false, - }); }); afterEach(() => { @@ -269,6 +263,12 @@ describe('ProgressiveProfilingForm Test', () => { }, progressiveProfiling: { redirectUrl: 'http://example.com', + subjectsList: { + options: [ + { label: 'Computer' }, + { label: 'Science' }, + ], + }, }, }); @@ -299,27 +299,12 @@ describe('ProgressiveProfilingForm Test', () => { ...initialState, progressiveProfiling: { redirectUrl: 'http://example.com', - }, - }); - - delete window.location; - window.location = { - assign: jest.fn().mockImplementation((value) => { window.location.href = value; }), - href: getConfig().LMS_BASE_URL, - }; - const { container } = render(reduxWrapper()); - - const skipButton = container.querySelector('#skip-optional-fields'); - fireEvent.click(skipButton); - - expect(window.location.href).not.toEqual('http://example.com'); - }); - - it('should not redirect on skip button click if country not selected', () => { - store = mockStore({ - ...initialState, - progressiveProfiling: { - redirectUrl: 'http://example.com', + subjectsList: { + options: [ + { label: 'Computer' }, + { label: 'Science' }, + ], + }, }, }); diff --git a/src/forms/progressive-profiling-popup/messages.js b/src/forms/progressive-profiling-popup/messages.js index dd55392f..86248371 100644 --- a/src/forms/progressive-profiling-popup/messages.js +++ b/src/forms/progressive-profiling-popup/messages.js @@ -36,6 +36,12 @@ const messages = defineMessages({ defaultMessage: 'Select a valid option', description: 'Error text appers on the country field when country is not selected and the user submit the form', }, + // TODO update error message copy here when design team will provide it + progressiveProfilingCountryFieldBlockingErrorMessage: { + id: 'progressive.profiling.country.field.error.message', + defaultMessage: 'To proceed, please save your country of residence', + description: 'Error msg for country field when the user country is not detected on registration step and user want to skip progressive profiling form', + }, progressiveProfilingDataCollectionTitle: { id: 'progressive.profiling.data.collection.title', defaultMessage: 'Personalize your experience', diff --git a/src/forms/registration-popup/index.jsx b/src/forms/registration-popup/index.jsx index a5dc5e7e..aadc34ff 100644 --- a/src/forms/registration-popup/index.jsx +++ b/src/forms/registration-popup/index.jsx @@ -47,6 +47,8 @@ import { NameField, PasswordField, } from '../fields'; +import useSubjectsList from '../progressive-profiling-popup/data/hooks/useSubjectList'; +import { setSubjectsList } from '../progressive-profiling-popup/data/reducers'; /** * RegisterForm component for handling user registration. @@ -69,6 +71,7 @@ const RegistrationForm = () => { const registerErrorAlertRef = useRef(null); const socialAuthnButtonRef = useRef(null); const queryParams = useMemo(() => getAllPossibleQueryParams(), []); + const { subjectsList, subjectsLoading } = useSubjectsList(); const registrationResult = useSelector(state => state.register.registrationResult); const userPipelineDataLoaded = useSelector(state => state.register.userPipelineDataLoaded); @@ -134,6 +137,13 @@ const RegistrationForm = () => { } }, [thirdPartyAuthApiStatus, providers]); + useEffect(() => { + if (!subjectsLoading) { + dispatch(setSubjectsList(subjectsList)); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [dispatch, subjectsLoading]); + const handleOnChange = (event) => { const { name } = event.target; const value = event.target.type === 'checkbox' ? event.target.checked : event.target.value;