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

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

Open
wants to merge 71 commits into
base: main
Choose a base branch
from

Conversation

PiusKariuki
Copy link
Contributor

@PiusKariuki PiusKariuki commented Dec 5, 2024

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

Screenshot from 2024-12-05 13-49-19
Screenshot from 2024-12-05 13-49-26
Screenshot from 2024-12-05 13-49-38

Related Issue

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

Other

@PiusKariuki
Copy link
Contributor Author

@vasharma05 @denniskigen can you please review this for me

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

@brandones brandones left a comment

Choose a reason for hiding this comment

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

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.

@PiusKariuki
Copy link
Contributor Author

@brandones @denniskigen . I resolved the failing E2E and suggestions too. Please review and revert incase of any more feedback

Copy link
Collaborator

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

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

Thank you @PiusKariuki! A few minor nitpicks

e2e/pages/program-page.ts Outdated Show resolved Hide resolved
Comment on lines 27 to 29
beforeEach(() => {
jest.clearAllMocks();
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also please note this.

Comment on lines 11 to 14
jest.mock('@openmrs/esm-framework', () => ({
showSnackbar: jest.fn(),
getCoreTranslation: jest.fn((key, defaultText) => defaultText),
}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

And please update the mocks

@@ -29,16 +28,12 @@ import {
import { CardHeader, EmptyState, ErrorState, launchPatientWorkspace } from '@openmrs/esm-patient-common-lib';
import { findLastState, usePrograms } from './programs.resource';
import styles from './programs-detailed-summary.scss';
import { ProgramsActionsMenu } from './programs-actions-menu.component';
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 the correct name for this component and file should be ProgramsActionMenu and programs-action-menu.component respectively.

@@ -158,10 +153,12 @@ const ProgramsDetailedSummary: React.FC<ProgramsDetailedSummaryProps> = ({ patie
{rows.map((row, i) => (
<TableRow key={row.id}>
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
<TableRow key={row.id}>
<TableRow key={row.id} {...getRowProps({ row })}>

@@ -158,10 +153,12 @@ const ProgramsDetailedSummary: React.FC<ProgramsDetailedSummaryProps> = ({ patie
{rows.map((row, i) => (
<TableRow key={row.id}>
{row.cells.map((cell) => (
<TableCell key={cell.id}>{cell.value?.content ?? cell.value}</TableCell>
<TableCell className="cds--table-column-menu" key={cell.id}>
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
<TableCell className="cds--table-column-menu" key={cell.id}>
<TableCell key={cell.id}>

cds--table-column-menu isn't strictly necessary here.

))}
<TableCell className="cds--table-column-menu">
<ProgramEditButton programEnrollmentId={enrollments[i]?.uuid} t={t} />
<TableCell>
Copy link
Member

Choose a reason for hiding this comment

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

But it is here to ensure the OverflowMenu takes up the appropriate amount of space.

Suggested change
<TableCell>
<TableCell className="cds--table-column-menu">

Copy link
Member

Choose a reason for hiding this comment

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

This and all it's related files should be renamed to programs-action-menu for consistency.

[programEnrollmentId],
);

const launchDeleteProgramDialog = () => {
Copy link
Member

Choose a reason for hiding this comment

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

We can wrap this in a useCallback.

};

return (
<Layer>
Copy link
Member

@denniskigen denniskigen Jan 9, 2025

Choose a reason for hiding this comment

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

Suggested change
<Layer>
<Layer className={styles.layer}>

And then the associated styles would look something like this:

.layer {
  height: 100%;

  :global(.cds--overflow-menu) {
    min-height: unset;
  }
}

.menuItem {
  max-width: none;
}

This will make your UI visually consistent with other flows like the Conditions edit experience.

Comment on lines 31 to 36
<OverflowMenu
name={t('editOrDeleteProgram', 'Edit or delete program')}
aria-label={t('editOrDeleteProgram', 'Edit or delete program')}
size={isTablet ? 'lg' : 'sm'}
flipped
>
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
<OverflowMenu
name={t('editOrDeleteProgram', 'Edit or delete program')}
aria-label={t('editOrDeleteProgram', 'Edit or delete program')}
size={isTablet ? 'lg' : 'sm'}
flipped
>
<OverflowMenu
align="left"
aria-label={t('editOrDeleteProgram', 'Edit or delete program')}
flipped
size={isTablet ? 'lg' : 'sm'}
>

The OverflowMenu should be left-aligned so it's content does not flow over the width of the datatable. Additionally, we should remove the name prop as it's not a known prop of the OverflowMenu component.

Comment on lines 37 to 38
<OverflowMenuItem id="editProgram" onClick={launchEditProgramsForm} itemText={t('edit', 'Edit')} />
<OverflowMenuItem id="deleteProgam" onClick={launchDeleteProgramDialog} itemText={t('delete', 'Delete')} />
Copy link
Member

@denniskigen denniskigen Jan 9, 2025

Choose a reason for hiding this comment

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

Suggested change
<OverflowMenuItem id="editProgram" onClick={launchEditProgramsForm} itemText={t('edit', 'Edit')} />
<OverflowMenuItem id="deleteProgam" onClick={launchDeleteProgramDialog} itemText={t('delete', 'Delete')} />
<OverflowMenuItem
className={styles.menuItem}
id="editProgram"
onClick={launchEditProgramsForm}
itemText={t('edit', 'Edit')}
/>
<OverflowMenuItem
className={styles.menuItem}
hasDivider
id="deleteProgam"
isDelete
onClick={launchDeleteProgramDialog}
itemText={t('delete', 'Delete')}
/>

<TableCell className="cds--table-column-menu">
<ProgramEditButton programEnrollmentId={enrollments[i]?.uuid} t={t} />
<TableCell>
<ProgramsActionsMenu patientUuid={patientUuid} programEnrollmentId={enrollments[i]?.uuid} />
Copy link
Member

Choose a reason for hiding this comment

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

We should add this to the ProgramsOverview component as well for consistency.

showSnackbar({
isLowContrast: true,
kind: 'success',
title: t('programDeleted', 'Program enrollment deleted'),
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
title: t('programDeleted', 'Program enrollment deleted'),
title: t('programEnrollmentDeleted', 'Program enrollment deleted'),

<Button kind="secondary" onClick={closeDeleteModal}>
{getCoreTranslation('cancel', 'Cancel')}
</Button>
<Button kind="danger" onClick={handleDelete} disabled={isDeleting}>
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
<Button kind="danger" onClick={handleDelete} disabled={isDeleting}>
<Button className={styles.deleteButton} kind="danger" onClick={handleDelete} disabled={isDeleting}>

And the styles for that would be:

@use '@carbon/layout';
@use '@carbon/type';

.deleteButton {
  :global(.cds--inline-loading) {
    min-height: layout.$spacing-05;
  }

  :global(.cds--inline-loading__text) {
    @include type.type-style('body-01');
  }
}

Comment on lines +86 to +93
export function deleteProgramEnrollment(programEnrollmentUuid: string) {
return openmrsFetch(`${restBaseUrl}/programenrollment/${programEnrollmentUuid}`, {
method: 'DELETE',
headers: {
'Content-Type': 'application/json',
},
});
}
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
export function deleteProgramEnrollment(programEnrollmentUuid: string) {
return openmrsFetch(`${restBaseUrl}/programenrollment/${programEnrollmentUuid}`, {
method: 'DELETE',
headers: {
'Content-Type': 'application/json',
},
});
}
export function deleteProgramEnrollment(programEnrollmentUuid: string) {
const abortController = new AbortController();
return openmrsFetch(`${restBaseUrl}/programenrollment/${programEnrollmentUuid}`, {
method: 'DELETE',
headers: {
'Content-Type': 'application/json',
},
signal: abortController.signal
});
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants