-
Notifications
You must be signed in to change notification settings - Fork 105
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(admissions): make checkboxes into bhYesNoRadios #7824
feat(admissions): make checkboxes into bhYesNoRadios #7824
Conversation
Makes the checkboxes for patient admissions into bhYesNoRadios to remove ambiguity. Closes Third-Culture-Software#7800.
3629ce4
to
7af6d6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look fine.
There are end-to-end test failures related to admissions to see the failures
(details at the end of this review):
npm install
npx update-browserslist-db@latest
npx playwright install chromium
npm run test:e2e-4
A couple more things, which could be addressed in future issues/PRs:
- The information from registration about referrals, pregnancy, initial diagnosis are not visible anywhere that I noticed (including patient information from the action drop-down in the Admissions registry. In fact, the patient if is in the hospital, it would be good to add a section to the patient report about the current situation, including info from the admissions form.
- The system allowed me to mark a male patient pregnant
The end-to-end errors:
[48/79] [chromium] › test/end-to-end/patient/record.spec.js:68:3 › Patient Record › displays the correct number of patie 1) [chromium] › test/end-to-end/patient/record.spec.js:73:3 › Patient Record › admits a patient ──
Error: Expected a success notification, but could not find one.
expect(received).toBe(expected) // Object.is equality
Expected: true
Received: false
at test/end-to-end/shared/components/notify.js:28
26 | }
27 | }
> 28 | expect(success, 'Expected a success notification, but could not find one.').toBe(true);
| ^
29 | return dismiss();
30 | },
31 |
at Object.hasSuccess (/home/jmcameron/Bhima/test/end-to-end/shared/components/notify.js:28:81)
at /home/jmcameron/Bhima/test/end-to-end/patient/record.spec.js:81:5
────────────────────────────────────────────────────────────────────────────────────────────────
[50/79] [chromium] › test/end-to-end/patient/record.spec.js:92:3 › Patient Record › dicharges a patient with a new diagn 2) [chromium] › test/end-to-end/patient/record.spec.js:92:3 › Patient Record › dicharges a patient with a new diagnosis
Error: expect(received).toBe(expected) // Object.is equality
Expected: 2
Received: 1
103 | // this is part of the same visit so expect no difference in number of visits
104 | const visits = await TU.locator('tr[data-visit-line]').all();
> 105 | expect(visits.length).toBe(2);
| ^
106 | });
107 |
108 | // Upload patient documents
at /home/jmcameron/Bhima/test/end-to-end/patient/record.spec.js:105:27
────────────────────────────────────────────────────────────────────────────────────────────────
d933043
to
1a03954
Compare
Those end to end tests saved a bunch of silly mistakes. Thanks for running them and pointing them out. Everything should be fixed now. |
Following up on your other comments:
I've added most of this to #7805. We really need a proposal for a better report showing "everything that happened this visit", but I'm not prepared to propose that yet. It will take some thought.
Created #7847. |
Makes the checkboxes for patient admissions into bhYesNoRadios to remove ambiguity. Here is what it looks like:
Closes #7800.