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
edx · Jun 27, 2024 · 2cfa4c0 · 2cfa4c0
1 parent 01f0ed2
commit 2cfa4c0
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 51 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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}
           />
         </Form.Group>
@@ -271,10 +283,13 @@ const ProgressiveProfilingForm = () => {
           <StatefulButton
             id="skip-optional-fields"
             name="skip-optional-fields"
+            className="authn-progressive-profiling-skip-button"
             type="submit"
             variant="outline-dark"
+            state={skipButtonState}
             labels={{
               default: formatMessage(messages.progressiveProfilingSkipForNowButtonText),
+              pending: '',
             }}
             onClick={handleSkip}
             onMouseDown={(e) => e.preventDefault()}

diff --git 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
@@ -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(<IntlProgressiveProfilingForm />));
-
-    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
@@ -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
@@ -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;