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

(refactor) O3-3846: Replace Carbon datepickers with reusable OpenmrsDatePicker #2154

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jwnasambu
Copy link
Contributor

@jwnasambu jwnasambu commented Dec 13, 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 replaces usages of the Carbon DatePicker component with our custom reusable OpenmrsDatePicker component.

Screenshots

Related Issue

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

Other

@jwnasambu jwnasambu marked this pull request as draft December 13, 2024 14:22
@jwnasambu jwnasambu force-pushed the feat/O3-3846 branch 2 times, most recently from 83b8b3b to 0b16f3d Compare December 16, 2024 13:56
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.

Thanks @jwnasambu! Left a few nitpicks:

@NethmiRodrigo NethmiRodrigo changed the title feat/O3-3846: All Calendar Date Pickers should use the same OpenmrsDatePicker component across entire EMR (refactor) O3-3846: Replace the DatePickers to use the OpenmrsDatePicker Dec 16, 2024
@jwnasambu jwnasambu force-pushed the feat/O3-3846 branch 3 times, most recently from 25100b4 to a111ec8 Compare December 18, 2024 12:14
@jwnasambu
Copy link
Contributor Author

@ibacher One thing I am picking from the error I am getting on this PR is specificMockImpl.apply is not a function. I want to believe it occurs when a mocked function is not properly set up, or its implementation does not correctly replicate the original behavior of the function. In this case, OpenmrsDatePicker is being mocked using jest.fn(realOpenmrsDatePicker), which can be a problem if realOpenmrsDatePicker is either a class or a React component and not a function. It’s for that reason I am thinking Instead of using jest.fn(realOpenmrsDatePicker), we mock it directly as a functional component? kindly what is your take?

@denniskigen
Copy link
Member

denniskigen commented Dec 19, 2024

Try using this stub implementation of the OpenmrsDatePicker component in your tests:

import { OpenmrsDatePicker } from '@openmrs/esm-framework';

const mockOpenmrsDatePicker = jest.mocked(OpenmrsDatePicker);

mockOpenmrsDatePicker.mockImplementation(({ id, labelText, value, onChange }) => {
  return (
    <>
      <label htmlFor={id}>{labelText}</label>
      <input
        aria-label={labelText.toString()}
        id={id}
        onChange={(evt) => {
          onChange(dayjs(evt.target.value).toDate());
        }}
        type="text"
        // @ts-ignore
        value={value ? dayjs(value).format('DD/MM/YYYY') : ''}
      />
    </>
  );
});

@Muppasanipraneeth
Copy link

Muppasanipraneeth commented Dec 25, 2024

@jwnasambu Standardize the use of the OpenMRS date picker Across All Apps O3-4287 however you are working on it . I checked what are changes you made . you already working on the visits, medication,conditions,Immunization,program,and other I think you did not worked on the Appointments so please work on it also Thank you .

@denniskigen denniskigen changed the title (refactor) O3-3846: Replace the DatePickers to use the OpenmrsDatePicker (refactor) O3-3846: Replace Carbon datepickers with reusable OpenmrsDatePicker Jan 7, 2025
className={styles.datePicker}
dateFormat="d/m/Y"
datePickerType="single"
id="deceasedDate"
Copy link
Member

Choose a reason for hiding this comment

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

We need to maintain parity between the props passed to the Carbon datepicker and those passed to OpenmrsDatePicker. In this case, this implementation is missing several props including labelText, isInvalid, and invalidText. It should look like this instead:

<OpenmrsDatePicker
    className={styles.datePicker}
    id="deceasedDate"
    isInvalid={!!errors?.deathDate}
    invalidText={errors?.deathDate?.message}
    labelText={t('dateOfDeath', 'Date of death')}
    maxDate={new Date().toISOString()}
    onChange={(date) => onChange(date)}
    value={value}
  />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @denniskigen

@denniskigen
Copy link
Member

denniskigen commented Jan 7, 2025

So I looked at the failing programs form tests and I couldn't exactly figure out why they were failing to set the correct dates. In the interest of getting the tests passing, I think we could modify it as follows:

describe('ProgramsForm', () => {
  const inpatientWardUuid = 'b1a8b05e-3542-4037-bbd3-998ee9c40574';
  const oncologyScreeningProgramUuid = '11b129ca-a5e7-4025-84bf-b92a173e20de';

  it('renders a success toast notification upon successfully recording a program enrollment', async () => {
    await user.type(enrollmentDateInput, '2020-05-05'); // remove this line so we rely on the default enrollment date
  });

  it('updates a program enrollment', async () => {
    const completionDateInput = screen.getByRole('textbox', { name: /date completed/i }); // remove this line and replace it with the next line
    const enrollmentLocationInput = screen.getByRole('combobox', { name: /enrollment location/i });

    // so we can simulate changing the enrollment location instead of the date

    // remove the next two lines
    await user.type(completionDateInput, '05/05/2020');
    await user.tab();

    // and replace them with the next two lines
    await user.click(enrollmentLocationInput);
    await user.selectOptions(enrollmentLocationInput, [inpatientWardUuid]);

    expect(mockUpdateProgramEnrollment).toHaveBeenCalledTimes(1);

    // and then assert using this payload instead
    expect(mockUpdateProgramEnrollment).toHaveBeenCalledWith(
      mockEnrolledProgramsResponse[0].uuid,
      expect.objectContaining({
        dateCompleted: null,
        dateEnrolled: expect.stringMatching(/^2020-01-16/),
        location: mockLocationsResponse[1].uuid,
        patient: mockPatient.id,
        program: mockEnrolledProgramsResponse[0].program.uuid,
      }),
      new AbortController(),
    );
  });
};

@denniskigen
Copy link
Member

Another observation I made is that you'd need to add the OpenmrsDatePicker stub to the following tests to fix the specificMockImpl.apply is not a function error:

  • mark-patient-deceased-form.test.tsx
  • visit-form.test.tsx
  • conditions-form.test.tsx
  • immunizations-form.test.tsx
  • add-drug-order.test.tsx
  • visit-notes-form.test.tsx
  • order-details-table.test.tsx
  • programs-form.test.tsx

@jwnasambu
Copy link
Contributor Author

jwnasambu commented Jan 7, 2025 via email

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.

5 participants