(feat) O3-4040: Add delete ability to program enrollments #2133

PiusKariuki · 2024-12-05T10:52:31Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

This PR adds DELETE ability to program enrollments

Screenshots

Related Issue

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

Other

…dit" and "delete" options for individual programs

…delete workflows

…t button and the new delete button

…ved(clicking on the overflow menu) to get to the "edit" button

PiusKariuki · 2024-12-05T10:53:22Z

@vasharma05 @denniskigen can you please review this for me

brandones

Nice work, @PiusKariuki ! The main thing is to clean up the user-facing text. We don't want to scare users by making them think they might be deleting a whole program, when they are in fact just deleting the patient's enrollment.

packages/esm-patient-programs-app/src/routes.json

packages/esm-patient-programs-app/src/programs/program-actions-menu.scss

packages/esm-patient-programs-app/src/index.ts

packages/esm-patient-programs-app/src/programs/delete-program.modal.tsx

…kbar

PiusKariuki · 2024-12-16T09:54:50Z

@brandones I have made the necessary updates requested.

brandones

Thanks @PiusKariuki ! Excellent work.

In the future, if you could work on figuring out how to add commits to your work in a way that makes it easy for reviewers to see what changed since last review, that would be really helpful. Right now it looks like you made like 14 new commits, which would be inconvenient to view one-by-one. Maybe check out git rebase --interactive if you haven't.

Just a suggestion, nice work otherwise!

brandones · 2024-12-19T03:04:15Z

Oops, looks like the E2E test failure is related to this work. Please fix the E2E test.

NethmiRodrigo

Thank you @PiusKariuki! Left a few nitpicks about the tests

NethmiRodrigo · 2024-12-19T07:09:23Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+}));
+
+const mockMutateEnrollments = jest.fn();
+(useEnrollments as jest.Mock).mockImplementation(() => ({ mutateEnrollments: mockMutateEnrollments }));


Please see the correct way to mock functions where we prefer to use jest.mocked where possible - https://o3-docs.openmrs.org/docs/frontend-modules/unit-and-integration-testing#annotating-mocks-with-type-information

NethmiRodrigo · 2024-12-19T07:10:39Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+  beforeEach(() => {
+    jest.clearAllMocks();
+  });


We don't need to explicitly do this, since we have this specified by default in the jest.config

Suggested change

beforeEach(() => {

jest.clearAllMocks();

});

NethmiRodrigo · 2024-12-19T07:17:57Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+jest.mock('@openmrs/esm-framework', () => ({
+  showSnackbar: jest.fn(),
+  getCoreTranslation: jest.fn((key, defaultText) => defaultText),
+}));


Please see how to extend framework mocks - https://o3-docs.openmrs.org/docs/frontend-modules/unit-and-integration-testing#you-should-not-need-to-use-partial-mocks.

NethmiRodrigo · 2024-12-19T07:19:16Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+  });
+
+  it('handles delete action successfully', async () => {
+    (deleteProgramEnrollment as jest.Mock).mockResolvedValue({ ok: true });


NethmiRodrigo · 2024-12-19T07:19:38Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+
+  it('handles delete action error', async () => {
+    const errorMessage = 'failed to delete';
+    (deleteProgramEnrollment as jest.Mock).mockRejectedValue(new Error(errorMessage));


NethmiRodrigo · 2024-12-19T07:20:11Z

packages/esm-patient-programs-app/src/programs/program-actions-menu.component.test.tsx

+jest.mock('@openmrs/esm-framework', () => ({
+  showModal: jest.fn(),
+  useLayoutType: jest.fn(() => 'desktop'),
+}));
+
+jest.mock('@openmrs/esm-patient-common-lib', () => ({
+  launchPatientWorkspace: jest.fn(),
+}));


Same here for this test case about mocking patterns

This is fine for @openmrs/esm-patient-common-lib, @NethmiRodrigo.

denniskigen

Good start, @PiusKariuki. I've left some feedback.

denniskigen · 2024-12-19T09:24:31Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+jest.mock('react-i18next', () => ({
+  useTranslation: () => ({ t: (key, defaultText) => defaultText }),
+}));


We shouldn't need to mock react-i18next, it's already mocked here using this mock.

denniskigen · 2024-12-19T09:28:00Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+describe('DeleteProgramModal', () => {
+  const closeDeleteModalMock = jest.fn();
+  const programEnrollmentId = '123';
+  const patientUuid = 'abc';


We could add this import to the top:

import { mockPatient } from 'tools';

So that we can do:

Suggested change

const patientUuid = 'abc';

const patientUuid = mockPatient.id

denniskigen · 2024-12-19T09:33:10Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+


A good pattern for setting up the test is doing something like:

const mockDeleteProgramEnrollment = jest.mocked(deleteProgramEnrollment); const mockShowSnackbar = jest.mocked(showSnackbar); const mockMutateEnrollments = jest.fn(() => Promise.resolve(undefined)); const mockUseEnrollments = jest.mocked(useEnrollments); const mockCloseDeleteModal = jest.fn(); const testProps = { closeDeleteModal: mockCloseDeleteModal, patientUuid: mockPatient.id, programEnrollmentId: '9b1deb4d-5b9f-4fec-a6ea-6b0c0f3a3d0f', }; const renderDeleteProgramModal = () => { render( <DeleteProgramModal {...testProps} />, ); };

So that in the tests we can just call:

it('renders something', () => { renderDeleteProgramModal(); // assertions })

denniskigen · 2024-12-19T09:37:24Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

+    render(
+      <DeleteProgramModal
+        closeDeleteModal={closeDeleteModalMock}
+        programEnrollmentId={programEnrollmentId}
+        patientUuid={patientUuid}
+      />,
+    );
+    expect(screen.getByText('Delete Program Enrollment')).toBeInTheDocument();
+    expect(screen.getByText('Are you sure you want to delete this program enrollment?')).toBeInTheDocument();
+    expect(screen.getByText('Cancel')).toBeInTheDocument();
+    expect(screen.getByText('Confirm')).toBeInTheDocument();


Prefer using a11y roles wherever possible to make your tests more robust:

Suggested change

render(

<DeleteProgramModal

closeDeleteModal={closeDeleteModalMock}

programEnrollmentId={programEnrollmentId}

patientUuid={patientUuid}

/>,

);

expect(screen.getByText('Delete Program Enrollment')).toBeInTheDocument();

expect(screen.getByText('Are you sure you want to delete this program enrollment?')).toBeInTheDocument();

expect(screen.getByText('Cancel')).toBeInTheDocument();

expect(screen.getByText('Confirm')).toBeInTheDocument();

renderDeleteProgramModal();

expect(screen.getByRole('heading', { name: /delete program enrollment/i })).toBeInTheDocument();

expect(screen.getByText(/are you sure you want to delete this program enrollment?/i)).toBeInTheDocument();

expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument();

expect(screen.getByRole('button', { name: /confirm/i })).toBeInTheDocument();

denniskigen · 2024-12-19T09:42:27Z

packages/esm-patient-programs-app/src/programs/delete-program-modal.test.tsx

@@ -0,0 +1,100 @@
+import React from 'react';
+import { render, screen, fireEvent } from '@testing-library/react';


Prefer userEvent over fireEvent.

denniskigen · 2024-12-19T10:18:39Z

packages/esm-patient-programs-app/src/programs/delete-program.modal.tsx

+    <div>
+      <ModalHeader
+        closeModal={closeDeleteModal}
+        title={t('deletePatientProgramEnrollment', 'Delete Program Enrollment')}


Suggested change

title={t('deletePatientProgramEnrollment', 'Delete Program Enrollment')}

title={t('deletePatientProgramEnrollment', 'Delete program enrollment')}

Sentence case is preferable to title case for body text.

denniskigen · 2024-12-19T10:19:39Z

packages/esm-patient-programs-app/src/programs/program-actions-menu.component.test.tsx

+jest.mock('react-i18next', () => ({
+  useTranslation: () => ({ t: (key: string, defaultText: string) => defaultText }),
+}));


Same as above, we shouldn't mock this. A lot of the tips from the programs modal test can be applied here too.

denniskigen · 2024-12-19T10:22:43Z

packages/esm-patient-programs-app/src/programs/program-actions-menu.component.test.tsx

+jest.mock('@openmrs/esm-framework', () => ({
+  showModal: jest.fn(),
+  useLayoutType: jest.fn(() => 'desktop'),
+}));
+
+jest.mock('@openmrs/esm-patient-common-lib', () => ({
+  launchPatientWorkspace: jest.fn(),
+}));


This is fine for @openmrs/esm-patient-common-lib, @NethmiRodrigo.

denniskigen · 2024-12-19T10:24:09Z

packages/esm-patient-programs-app/src/programs/program-actions-menu.component.test.tsx

+jest.mock('@openmrs/esm-framework', () => ({
+  showModal: jest.fn(),
+  useLayoutType: jest.fn(() => 'desktop'),
+}));


Suggested change

jest.mock('@openmrs/esm-framework', () => ({

showModal: jest.fn(),

useLayoutType: jest.fn(() => 'desktop'),

}));

const mockShowModal = jest.mocked(showModal);

const mockUseLayoutType = jest.mocked(useLayoutType);

And then further below you could do this inside a beforeEach block:

beforeEach(() => { mockUseLayoutType.mockReturnValue('small-desktop'); // or 'large-desktop' or 'tablet' })

To override the useLayoutType implementation to suit your test.

denniskigen · 2024-12-19T10:24:51Z

packages/esm-patient-programs-app/src/programs/programs-detailed-summary.component.tsx

@@ -160,8 +155,8 @@ const ProgramsDetailedSummary: React.FC<ProgramsDetailedSummaryProps> = ({ patie
                      {row.cells.map((cell) => (
                        <TableCell key={cell.id}>{cell.value?.content ?? cell.value}</TableCell>
                      ))}
-                      <TableCell className="cds--table-column-menu">


I think we still need this className...

denniskigen · 2024-12-19T10:34:51Z

The programs e2e test is failing because this button doesn't exist after moving it to the ProgramsActionsMenu component. You'll likely need to mimic this setup.

PiusKariuki · 2024-12-20T08:05:33Z

Thank you @denniskigen and @NethmiRodrigo . I am working on fixing these tests right now

PiusKariuki added 16 commits December 5, 2024 11:09

(chore) Created a program actions menu component that displays the "e…

cbbff83

…dit" and "delete" options for individual programs

feat: created a program actions menu test component testing edit and …

b81e251

…delete workflows

feat: created a delete modal test component

80c62ed

(feat) registered the delete confirmation dialog

14e044a

(chore) generated translation keys

2ede494

(feat) created a HTTP request function to delete programs

a66f460

(feat): created a dialog that shows up when deleting a program

a6684fa

(feat) added a React component to render the edit-program button

576c2ad

(feat) swapped the edit button for an Action menu that houses the edi…

ac33454

…t button and the new delete button

(feat) added styles for the action table cell

8ddee66

(chore) edited the modal prop name to ensure the test runs correctly

4ee37bb

(chore) included a name for the overflow menu item for testing purposes

a683be1

(chore) typo fix

470f200

(chore) edited the test component to accommodate the extra step invol…

5ea6b91

…ved(clicking on the overflow menu) to get to the "edit" button

(chore) optimize imports

f4b550f

(chore) removed styling that caused overflow

3236b0d

brandones changed the title ~~(feat) Add delete ability to program enrollments#4040~~ (feat) O3-4040: Add delete ability to program enrollments Dec 15, 2024

brandones requested changes Dec 15, 2024

View reviewed changes

PiusKariuki added 11 commits December 16, 2024 11:05

chore: moved the delete enrollment component to the modals array

7ab780f

chore: removed useless styling

de252d4

chore: renamed Dialog to Modal

9d444b6

chore: more descriptive text for a successful deletion on the snackbar

49e52d3

chore: more descriptive text for an error during deletion on the snac…

847261c

…kbar

chore: more descriptive modal header for deleting a program

4352fe8

chore: more descriptive modal body for deleting a program

17a7079

chore: updated tests

7840ee8

chore: updated tests

9869444

chore: using getCoreTranslation

421d61f

chore: updated the modal name

f871f46

PiusKariuki added 2 commits December 16, 2024 12:48

chore: updated the translation strings

cc0cfb0

chore: updated the tests

6762df0

brandones approved these changes Dec 19, 2024

View reviewed changes

NethmiRodrigo requested changes Dec 19, 2024

View reviewed changes

denniskigen reviewed Dec 19, 2024

View reviewed changes

PiusKariuki added 2 commits December 19, 2024 15:54

chore: removed unnecessary mock for react-i18

db768d8

chore: used mockPatient instead of arbitrary values

7223a13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) O3-4040: Add delete ability to program enrollments #2133

(feat) O3-4040: Add delete ability to program enrollments #2133

PiusKariuki commented Dec 5, 2024 •

edited by brandones

Loading

PiusKariuki commented Dec 5, 2024

brandones left a comment

PiusKariuki commented Dec 16, 2024

brandones left a comment

brandones commented Dec 19, 2024

NethmiRodrigo left a comment

NethmiRodrigo Dec 19, 2024

NethmiRodrigo Dec 19, 2024

NethmiRodrigo Dec 19, 2024

NethmiRodrigo Dec 19, 2024

NethmiRodrigo Dec 19, 2024

NethmiRodrigo Dec 19, 2024

denniskigen Dec 19, 2024

denniskigen left a comment

denniskigen Dec 19, 2024

denniskigen Dec 19, 2024 •

edited

Loading

denniskigen Dec 19, 2024 •

edited

Loading

denniskigen Dec 19, 2024

denniskigen Dec 19, 2024

denniskigen Dec 19, 2024

denniskigen Dec 19, 2024

denniskigen Dec 19, 2024

denniskigen Dec 19, 2024

denniskigen Dec 19, 2024 •

edited

Loading

denniskigen Dec 19, 2024

denniskigen commented Dec 19, 2024

PiusKariuki commented Dec 20, 2024

	const patientUuid = 'abc';
	const patientUuid = mockPatient.id

		@@ -0,0 +1,100 @@
		import React from 'react';
		import { render, screen, fireEvent } from '@testing-library/react';

	title={t('deletePatientProgramEnrollment', 'Delete Program Enrollment')}
	title={t('deletePatientProgramEnrollment', 'Delete program enrollment')}

(feat) O3-4040: Add delete ability to program enrollments #2133

Are you sure you want to change the base?

(feat) O3-4040: Add delete ability to program enrollments #2133

Conversation

PiusKariuki commented Dec 5, 2024 • edited by brandones Loading

Requirements

Summary

Screenshots

Related Issue

Other

PiusKariuki commented Dec 5, 2024

brandones left a comment

Choose a reason for hiding this comment

PiusKariuki commented Dec 16, 2024

brandones left a comment

Choose a reason for hiding this comment

brandones commented Dec 19, 2024

NethmiRodrigo 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

denniskigen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

denniskigen Dec 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen commented Dec 19, 2024

PiusKariuki commented Dec 20, 2024

PiusKariuki commented Dec 5, 2024 •

edited by brandones

Loading

denniskigen Dec 19, 2024 •

edited

Loading

denniskigen Dec 19, 2024 •

edited

Loading

denniskigen Dec 19, 2024 •

edited

Loading