Skip to content
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

O3-1409: Support for users changing passwords #43

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import React, { useCallback } from 'react';
import { useTranslation } from 'react-i18next';
import { SwitcherItem } from '@carbon/react';
import { Settings } from '@carbon/react/icons';
import { showModal } from '@openmrs/esm-framework';
import styles from './change-password-button.scss';

export interface ChangePasswordLinkProps {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anywhere. Might as well remove it.


const ChangePasswordLink: React.FC<ChangePasswordLinkProps> = () => {
const { t } = useTranslation();

const launchChangePasswordModal = useCallback(() => {
showModal('change-password-modal');
}, []);

return (
<>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we don't need this outer React fragment.

<SwitcherItem aria-label="Switcher Container">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<SwitcherItem aria-label="Switcher Container">
<SwitcherItem aria-label={t('changePassword', 'Change password')}>

<div className={styles.changePasswordButton} role="button" onClick={launchChangePasswordModal} tabIndex={0}>
<Settings size={20} />
<p>{t('changePassword', 'Change Password')}</p>
</div>
</SwitcherItem>
</>
);
};

export default ChangePasswordLink;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.changePasswordButton {
width: 16rem;
display: inline-flex;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';
import ChangePasswordButton from './change-password-button.component';
import { render, screen } from '@testing-library/react';
import { showModal } from '@openmrs/esm-framework';
import userEvent from '@testing-library/user-event';

const showModalMock = showModal as jest.Mock;

describe('<ChangePasswordButton/>', () => {
beforeEach(() => {
render(<ChangePasswordButton />);
});

it('should display the `Change Password` button', async () => {
const user = userEvent.setup();
const changePasswordButton = await screen.findByRole('button', {
name: /Change Password/i,
});

await user.click(changePasswordButton);

expect(showModalMock).toHaveBeenCalledWith('change-password-modal');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
import React, { useCallback, useEffect, useRef, useState } from 'react';
import { useTranslation } from 'react-i18next';
import { Button, ModalBody, ModalFooter, ModalHeader, Tile, PasswordInput, Form, Layer } from '@carbon/react';
import { navigate, showSnackbar } from '@openmrs/esm-framework';
import styles from './change-password-modal.scss';
import { performPasswordChange } from './change-password-model.resource';

interface ChangePasswordModalProps {
close(): void;
}

export default function ChangePasswordModal({ close }: ChangePasswordModalProps) {
const { t } = useTranslation();
const [isSavingPassword, setIsSavingPassword] = useState(false);
const oldPasswordInputRef = useRef<HTMLInputElement>(null);
const newPasswordInputRef = useRef<HTMLInputElement>(null);
const confirmPasswordInputRef = useRef<HTMLInputElement>(null);
const formRef = useRef<HTMLFormElement>(null);
const [newPasswordError, setNewPasswordErr] = useState('');
const [oldPasswordError, setOldPasswordErr] = useState('');
const [confirmPasswordError, setConfirmPasswordError] = useState('');
const [isOldPasswordInvalid, setIsOldPasswordInvalid] = useState<boolean>(true);
const [isNewPasswordInvalid, setIsNewPasswordInvalid] = useState<boolean>(true);
const [isConfirmPasswordInvalid, setIsConfirmPasswordInvalid] = useState<boolean>(true);
const [passwordInput, setPasswordInput] = useState({
oldPassword: '',
newPassword: '',
confirmPassword: '',
});

const handleValidation = useCallback(
(passwordInputValue, passwordInputFieldName) => {
if (passwordInputFieldName === 'newPassword') {
const uppercaseRegExp = /(?=.*?[A-Z])/;
const lowercaseRegExp = /(?=.*?[a-z])/;
const digitsRegExp = /(?=.*?[0-9])/;
const minLengthRegExp = /.{8,}/;
const passwordLength = passwordInputValue.length;
const uppercasePassword = uppercaseRegExp.test(passwordInputValue);
const lowercasePassword = lowercaseRegExp.test(passwordInputValue);
const digitsPassword = digitsRegExp.test(passwordInputValue);
const minLengthPassword = minLengthRegExp.test(passwordInputValue);
let errMsg = '';
if (passwordLength === 0) {
errMsg = t('passwordIsEmpty', 'Password is empty');
jnsereko marked this conversation as resolved.
Show resolved Hide resolved
} else if (!uppercasePassword) {
errMsg = t('atLeastOneUppercase', 'At least one Uppercase');
jnsereko marked this conversation as resolved.
Show resolved Hide resolved
} else if (!lowercasePassword) {
errMsg = t('atLeastOneLowercase', 'At least one Lowercase');
jnsereko marked this conversation as resolved.
Show resolved Hide resolved
} else if (!digitsPassword) {
errMsg = t('atLeastOneDigit', 'At least one digit');
jnsereko marked this conversation as resolved.
Show resolved Hide resolved
} else if (!minLengthPassword) {
errMsg = t('minimum8Characters', 'Minimum 8 characters');
jnsereko marked this conversation as resolved.
Show resolved Hide resolved
} else if (passwordInput.oldPassword.length > 0 && passwordInput.newPassword === passwordInput.oldPassword) {
errMsg = t('newPasswordMustNotBeTheSameAsOld', 'New password must not be the same as password');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errMsg = t('newPasswordMustNotBeTheSameAsOld', 'New password must not be the same as password');
errMsg = t('newPasswordMustNotBeTheSameAsOld', 'Your new password cannot be the same as your current password. Please choose a different password.');

} else {
errMsg = '';
setIsNewPasswordInvalid(false);
}
setNewPasswordErr(errMsg);
} else if (
passwordInputFieldName === 'confirmPassword' ||
(passwordInputFieldName === 'newPassword' && passwordInput.confirmPassword.length > 0)
) {
if (passwordInput.confirmPassword !== passwordInput.newPassword) {
setConfirmPasswordError(t('confirmPasswordMustBeTheSameAsNew', 'Confirm password must be the same as new'));
} else {
setConfirmPasswordError('');
setIsConfirmPasswordInvalid(false);
}
} else {
if (passwordInput.newPassword.length > 0 && passwordInput.newPassword === passwordInput.oldPassword) {
setOldPasswordErr(t('oldPasswordMustNotBeTheSameAsNew', 'Old password must not be the same as new'));
} else {
setOldPasswordErr('');
setIsOldPasswordInvalid(false);
}
}
},
[passwordInput.confirmPassword, passwordInput.newPassword, passwordInput.oldPassword, t],
);

useEffect(() => {
if (passwordInput.oldPassword !== '') {
handleValidation(passwordInput.oldPassword, 'oldPassword');
}
if (passwordInput.newPassword !== '') {
handleValidation(passwordInput.newPassword, 'newPassword');
}
if (passwordInput.confirmPassword !== '') {
handleValidation(passwordInput.confirmPassword, 'confirmPassword');
}
}, [handleValidation, passwordInput]);
Comment on lines +83 to +93
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


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

const handleSubmit = useCallback(
async (evt: React.FormEvent<HTMLFormElement>) => {
evt.preventDefault();
setIsSavingPassword(true);
performPasswordChange(passwordInput.oldPassword, passwordInput.confirmPassword)
.then(() => {
close();
navigate({ to: `\${openmrsSpaBase}/logout` });
showSnackbar({
isLowContrast: true,
kind: 'success',
subtitle: t('userPasswordUpdated', 'User password updated successfully'),
title: t('userPassword', 'User password'),
jnsereko marked this conversation as resolved.
Show resolved Hide resolved
});
})
.catch((error) => {
setIsSavingPassword(false);
showSnackbar({
title: t('invalidPasswordCredentials', 'Invalid password provided'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the only reason why this might fail be because an invalid password was provided? It might be more useful to show a more generic error message along the lines of Password change failed with the error message providing more context about why the failure happened.

kind: 'error',
isLowContrast: false,
subtitle: error?.message,
});
});
},
[close, passwordInput.confirmPassword, passwordInput.oldPassword, t],
);

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

@denniskigen denniskigen Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<Form onSubmit={handleSubmit} ref={formRef}>
<div className={styles['input-group']}>
<Layer>
<PasswordInput
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

id="oldPassword"
invalid={oldPasswordError.length > 0}
invalidText={oldPasswordError}
labelText={t('oldPassword', 'Old Password')}
name="oldPassword"
value={passwordInput.oldPassword}
onChange={handlePasswordChange}
ref={oldPasswordInputRef}
required
showPasswordLabel="Show old password"
/>
</Layer>
<Layer>
<PasswordInput
id="newPassword"
invalid={newPasswordError.length > 0}
invalidText={newPasswordError}
labelText={t('newPassword', 'New Password')}
name="newPassword"
value={passwordInput.newPassword}
onChange={handlePasswordChange}
ref={newPasswordInputRef}
required
showPasswordLabel="Show new password"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/>
</Layer>
<Layer>
<PasswordInput
id="confirmPassword"
invalid={confirmPasswordError.length > 0}
invalidText={confirmPasswordError}
labelText={t('confirmPassword', 'Confirm Password')}
name="confirmPassword"
value={passwordInput.confirmPassword}
onChange={handlePasswordChange}
ref={confirmPasswordInputRef}
required
showPasswordLabel="Show confirm password"
/>
</Layer>
</div>
</Form>
</Tile>
</ModalBody>
<ModalFooter>
<Button kind="secondary" onClick={close}>
{t('cancel', 'Cancel')}
</Button>
<Button
type="submit"
onClick={handleSubmit}
disabled={isSavingPassword || isNewPasswordInvalid || isConfirmPasswordInvalid || isOldPasswordInvalid}
>
{t('updatePassword', 'Update Password')}
</Button>
</ModalFooter>
</>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
.input-group {
:global(.cds--label) {
margin-top: 1rem;
}

:global(.cds--text-input) {
background-color: white;
}
Comment on lines +6 to +8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Layer instead of this override.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import ChangePasswordModal from './change-password-modal.component';
import userEvent from '@testing-library/user-event';

describe(`Change Password Modal`, () => {
it('should change user locale', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const user = userEvent.setup();

render(<ChangePasswordModal close={jest.fn()} />);
expect(screen.getByRole('button', { name: /Apply/ })).toBeDisabled();

await user.type(screen.getByLabelText(/Old Password/i), 'my-password');
await user.type(screen.getByLabelText(/New Password/i), 'my-password');
await user.type(screen.getByLabelText(/Confirm Password/i), 'my-password');
await user.click(screen.getByRole('button', { name: /Apply/i }));
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { openmrsFetch } from '@openmrs/esm-framework';

export async function performPasswordChange(oldPassword: string, newPassword: string) {
return openmrsFetch(`/ws/rest/v1/password`, {
headers: {
'Content-Type': 'application/json',
},
method: 'POST',
body: {
oldPassword: oldPassword,
newPassword: newPassword,
},
}).then((res) => {
return res;
});
Comment on lines +13 to +15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}).then((res) => {
return res;
});
}).then((res) => res);

}
10 changes: 10 additions & 0 deletions packages/esm-system-admin-app/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,13 @@ export const legacySystemAdminPageCardLink = getAsyncLifecycle(
() => import('./dashboard/legacy-admin-page-link.component'),
options,
);

export const changePasswordButton = getAsyncLifecycle(
() => import('./change-password-button/change-password-button.component'),
options,
);

export const changePasswordModal = getAsyncLifecycle(
() => import('./change-password-modal/change-password-modal.component'),
options,
);
14 changes: 14 additions & 0 deletions packages/esm-system-admin-app/src/routes.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,20 @@
"slot": "system-admin-page-card-link-slot",
"component": "legacySystemAdminPageCardLink",
"order": 0
},
{
"name": "change-password",
"slot": "user-panel-slot",
"component": "changePasswordButton",
"online": true,
"offline": true,
"order": 3
},
{
"name": "change-password-modal",
"component": "changePasswordModal",
"online": true,
"offline": true
}
],
"pages": [
Expand Down
18 changes: 18 additions & 0 deletions packages/esm-system-admin-app/translations/ar.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
{
"atLeastOneDigit": "رقم واحد على الأقل",
"atLeastOneLowercase": "حرف صغير واحد على الأقل",
"atLeastOneUppercase": "حرف كبير واحد على الأقل",
"cancel": "يلغي",
"changePassword": "تغيير كلمة المرور",
"config": "الإعدادات",
"confirmPassword": "تأكيد كلمة المرور",
"confirmPasswordMustBeTheSameAsNew": "تأكيد كلمة المرور يجب أن تكون هي نفسها الجديدة",
"invalidPasswordCredentials": "",
"legacyAdmin": "إدارة النظام القديم",
"systemAdmin": "إدارة النظام"
"minimum8Characters": "الحد الأدنى 8 أحرف",
"newPassword": "كلمة المرور الجديدة",
"newPasswordMustNotBeTheSameAsOld": "كلمة المرور الجديدة يجب ألا تكون نفس كلمة المرور",
"oldPassword": "كلمة المرور القديمة",
"oldPasswordMustNotBeTheSameAsNew": "يجب ألا تكون كلمة المرور القديمة هي نفس كلمة المرور الجديدة",
"passwordIsEmpty": "كلمة المرور فارغة",
"systemAdmin": "إدارة النظام",
"updatePassword": "تطوير كلمة السر",
"userPassword": "كلمة مرور المستخدم",
"userPasswordUpdated": "تم تحديث كلمة مرور المستخدم بنجاح"
}
18 changes: 18 additions & 0 deletions packages/esm-system-admin-app/translations/en.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
{
"atLeastOneDigit": "At least one digit",
"atLeastOneLowercase": "At least one Lowercase",
"atLeastOneUppercase": "At least one Uppercase",
"cancel": "Cancel",
"changePassword": "Change Password",
"config": "Configurations",
"confirmPassword": "Confirm Password",
"confirmPasswordMustBeTheSameAsNew": "Confirm password must be the same as new",
"invalidPasswordCredentials": "",
"legacyAdmin": "Legacy Admin",
"systemAdmin": "System Administration"
"minimum8Characters": "Minimum 8 characters",
"newPassword": "New Password",
"newPasswordMustNotBeTheSameAsOld": "New password must not be the same as password",
"oldPassword": "Old Password",
"oldPasswordMustNotBeTheSameAsNew": "Old password must not be the same as new",
"passwordIsEmpty": "Password is empty",
"systemAdmin": "System Administration",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"systemAdmin": "System Administration",

This translation already exists. Same applies in ar.json.

"updatePassword": "Update Password",
"userPassword": "User password",
"userPasswordUpdated": "User password updated successfully"
}
Loading