O3-1409: Support for users changing passwords #43

jnsereko · 2024-02-01T14:52:30Z

Requirements

This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.
My work conforms to the OpenMRS 3.0 Styleguide.
I checked for feature overlap with existing widgets.

Summary

O3 screen to allow users to change their password as a follow-up for openmrs/openmrs-esm-core#902
cc @ibacher @michaelbontyes @vasharma05 @denniskigen

Screenshots

change-password.mov

None.

Issue

https://openmrs.atlassian.net/browse/O3-1409

None.

Other

None.

ibacher

Thanks @jnsereko! Getting much closer. A bunch of nit-picking remarks.

ibacher · 2024-02-02T18:58:46Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+    if (passwordInput.confirmPassword !== '') {
+      handleValidation(passwordInput.confirmPassword, 'confirmPassword');
+    }
+  }, [passwordInput]);


handleValidation is also a dependency here.

ibacher · 2024-02-02T18:59:05Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+  const handlePasswordChange = (event) => {
+    const passwordInputValue = event.target.value.trim();
+    const passwordInputFieldName = event.target.name;
+    const NewPasswordInput = { ...passwordInput, [passwordInputFieldName]: passwordInputValue };
+    setPasswordInput(NewPasswordInput);
+  };
+
+  const handleValidation = (passwordInputValue, passwordInputFieldName) => {


I would probably wrap both of these in useCallback()

ibacher · 2024-02-02T19:01:27Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+      try {
+        setIsSavingPassword(true);
+        await performPasswordChange(passwordInput.oldPassword, passwordInput.confirmPassword).then(() => {
+          close();
+          navigate({ to: `\${openmrsSpaBase}/logout` });
+          showToast({
+            title: t('userPassword', 'User password'),
+            description: t('userPasswordUpdated', 'User password updated successfully'),
+            kind: 'success',
+          });
+        });
+      } catch (error) {
+        setIsSavingPassword(false);
+        setErrorMessage(`${t('invalidPasswordCredentials', 'Invalid password provided')}: ${error.message}`);
+      }


This construction is weird...

performPasswordChange(passwordInput.oldPassword, passwordInput.confirmPassword).then(() => { close(); navigate({ to: `\${openmrsSpaBase}/logout` }); showToast({ title: t('userPassword', 'User password'), description: t('userPasswordUpdated', 'User password updated successfully'), kind: 'success', }); }).catch((err) => { setIsSavingPassword(false); setErrorMessage(`${t('invalidPasswordCredentials', 'Invalid password provided')}: ${error.message}`); });

Reads better to me. (Basically either use await and no then() or else just use it as a promise).

The success message should be a snackbar, not a toast. Also, we should probably show a notification when it submits but fails at the backend.

ibacher · 2024-02-02T19:03:19Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+      evt.preventDefault();
+      evt.stopPropagation();


ibacher · 2024-02-02T19:03:31Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+        setErrorMessage(`${t('invalidPasswordCredentials', 'Invalid password provided')}: ${error.message}`);
+      }
+
+      return false;


Also why are we returning false?

ibacher · 2024-02-02T19:08:12Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+          onClick={handleSubmit}
+          disabled={isSavingPassword || isNewPasswordInvalid || isConfirmPasswordInvalid || isOldPasswordInvalid}
+        >
+          {t('apply', 'Apply')}


Suggested change

{t('apply', 'Apply')}

{t('updatePassword', 'Update Password')}

ibacher · 2024-02-02T19:10:04Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.scss

+}
+
+.errorMessage {
+  position: absolute;


Absolute positioning for an error message feels wrong.

ibacher · 2024-02-02T19:10:22Z

packages/esm-system-admin-app/src/index.ts

+  options,
+);
+
+export const ChangePasswordModal = getAsyncLifecycle(


Suggested change

export const ChangePasswordModal = getAsyncLifecycle(

export const changePasswordModal = getAsyncLifecycle(

ibacher · 2024-02-02T19:10:32Z

packages/esm-system-admin-app/src/routes.json

+    },
+    {
+      "name": "change-password-modal",
+      "component": "ChangePasswordModal",


Suggested change

"component": "ChangePasswordModal",

"component": "changePasswordModal",

ibacher · 2024-02-02T19:11:27Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+          <Tile>
+            <form onSubmit={handleSubmit} ref={formRef}>
+              <div className={styles['input-group']}>
+                <PasswordInput


I think it's likely we need a Carbon Layer component in here.

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

denniskigen · 2024-02-06T10:20:35Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+    <>
+      <ModalHeader closeModal={close} title={t('changePassword', 'Change Password')} />
+      <ModalBody>
+        <Tile>


No need for Tile here. It adds unnecessary margin to the modal body.

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

denniskigen · 2024-02-06T10:24:32Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.scss

+  :global(.cds--text-input) {
+    background-color: white;
+  }


Use Layer instead of this override.

denniskigen · 2024-02-06T10:24:59Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.test.tsx

+import userEvent from '@testing-library/user-event';
+
+describe(`Change Password Modal`, () => {
+  it('should change user locale', async () => {


Shouldn't this be something about changing the password, not the locale?

denniskigen · 2024-02-06T10:28:03Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+                  onChange={handlePasswordChange}
+                  ref={newPasswordInputRef}
+                  required
+                  showPasswordLabel="Show new password"


Consider providing detailed helper text below the fields listing any requirements related to the data format, such as types of characters allowed per the password input usage guidelines.

denniskigen · 2024-02-06T10:40:03Z

packages/esm-system-admin-app/src/change-password-modal/change-password-modal.component.tsx

+  useEffect(() => {
+    if (passwordInput.oldPassword !== '') {
+      handleValidation(passwordInput.oldPassword, 'oldPassword');
+    }
+    if (passwordInput.newPassword !== '') {
+      handleValidation(passwordInput.newPassword, 'newPassword');
+    }
+    if (passwordInput.confirmPassword !== '') {
+      handleValidation(passwordInput.confirmPassword, 'confirmPassword');
+    }
+  }, [handleValidation, passwordInput]);


For the best client-side validation experience, we should probably use RHF and Zod here similar to how we're validating the Vitals and Biometrics form (and various other vanilla React forms in O3).

…-password-modal.component.tsx Co-authored-by: Dennis Kigen <[email protected]>

jnsereko changed the title ~~O3 1409~~ O3-1409: Support for users changing passwords Feb 1, 2024

jnsereko mentioned this pull request Feb 1, 2024

O3-1409: Support for users changing passwords openmrs/openmrs-esm-core#902

Closed

4 tasks

ibacher requested changes Feb 2, 2024

View reviewed changes

rerun build

cb4692f

jnsereko force-pushed the O3-1409 branch from 3b829aa to cb4692f Compare February 6, 2024 09:14

jnsereko added 2 commits February 6, 2024 12:17

Update ar.json

70a8f2d

Update en.json

cae39ea

denniskigen reviewed Feb 6, 2024

View reviewed changes

jnsereko and others added 6 commits February 6, 2024 13:51

Update packages/esm-system-admin-app/src/change-password-modal/change…

6159748

…-password-modal.component.tsx Co-authored-by: Dennis Kigen <[email protected]>

Update packages/esm-system-admin-app/src/change-password-modal/change…

a2feec6

…-password-modal.component.tsx Co-authored-by: Dennis Kigen <[email protected]>

Update packages/esm-system-admin-app/src/change-password-modal/change…

31a16d5

…-password-modal.component.tsx Co-authored-by: Dennis Kigen <[email protected]>

Update packages/esm-system-admin-app/src/change-password-modal/change…

12e1898

…-password-modal.component.tsx Co-authored-by: Dennis Kigen <[email protected]>

Update packages/esm-system-admin-app/src/change-password-modal/change…

3e17fb6

…-password-modal.component.tsx Co-authored-by: Dennis Kigen <[email protected]>

Update packages/esm-system-admin-app/src/change-password-modal/change…

c7431e5

…-password-modal.component.tsx Co-authored-by: Dennis Kigen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O3-1409: Support for users changing passwords #43

O3-1409: Support for users changing passwords #43

jnsereko commented Feb 1, 2024 •

edited by denniskigen

Loading

ibacher left a comment

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

ibacher Feb 2, 2024

denniskigen Feb 6, 2024 •

edited

Loading

denniskigen Feb 6, 2024

denniskigen Feb 6, 2024

denniskigen Feb 6, 2024

denniskigen Feb 6, 2024

	{t('apply', 'Apply')}
	{t('updatePassword', 'Update Password')}

	export const ChangePasswordModal = getAsyncLifecycle(
	export const changePasswordModal = getAsyncLifecycle(

	"component": "ChangePasswordModal",
	"component": "changePasswordModal",

O3-1409: Support for users changing passwords #43

Are you sure you want to change the base?

O3-1409: Support for users changing passwords #43

Conversation

jnsereko commented Feb 1, 2024 • edited by denniskigen Loading

Requirements

Summary

Screenshots

Issue

Other

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsereko commented Feb 1, 2024 •

edited by denniskigen

Loading

denniskigen Feb 6, 2024 •

edited

Loading