(feat) Edit Patient #1806

MatthewDorner · 2020-02-06T03:27:59Z

Implement Edit Patient functionality
Refactor New Patient and View Patient so that New, Edit and View Patient functions all use the same GeneralInformation form.
Changed behavior of isEditable prop in input components to make it false by default, as this seems to make more sense for boolean
Moved createPatient into patient slice instead of patients

Right now the tests should all pass but I still need to add the tests for Edit Patient and improve some other tests. I think I also need to add error handling back into the new form since I lost it when I deleted NewPatientForm.

1765

vercel · 2020-02-06T03:28:07Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/hospitalrun/hospitalrun-frontend/an1yuurn8
✅ Preview: https://hospitalrun-frontend-git-fork-matthewdorner-edit-patient.hospitalrun.now.sh

src/clients/db/Repository.ts

src/patients/related-persons/RelatedPersonTab.tsx

jackcmeyer · 2020-02-06T03:41:22Z

src/patients/GeneralInformation.tsx

- )}
- </form>
+ </Panel>
+ {isEditable ? (


Looks like some of the stylings may have accidentally changed here.

Github thinks this file is NewPatientForm.tsx (which didn't have panels) renamed to GeneralInformation.tsx, when in fact I deleted NewPatientForm.tsx, so that's why it thinks I put panels in.

jackcmeyer · 2020-02-06T03:48:03Z

src/patients/edit/EditPatient.tsx

+ const dispatch = useDispatch()
+
+ const [patient, setPatient] = useState({} as Patient)
+ const { patient: reduxPatient } = useSelector((state: RootState) => state.patient)


I think that we could add isLoading to this restructured object so that we can use it for displaying the loading spinner.

jackcmeyer · 2020-02-06T03:48:37Z

src/patients/edit/EditPatient.tsx

+ }
+
+ // see comment in ViewPatient
+ if (!reduxPatient) {


We could change this check to something like isLoading || !reduxPatient.id so that the loading spinner properly appears

isLoading makes sense to me, but what does the check for !reduxPatient.id add? or should it be !id, like if there's no parameter? but that seems like it would be handled separately with an error or something

so far I've just used isLoading which seems to work. Now, you actually see the loading icon when you change patients, etc.

jackcmeyer · 2020-02-06T03:49:24Z

src/patients/view/ViewPatient.tsx

@@ -34,6 +37,8 @@ const ViewPatient = () => {
 }
 }, [dispatch, id])

+ // this check doesn't work as an empty object isn't falsy. would it be more correct to display
+ // the spinner when the Redux patient state isLoading is true?
 if (!patient) {


I think that we can change this check to something like isLoading || !patient.id when we display the loading spinner.

in this one, test fails unless you use isLoading || !patient since it uses patient.id to render the tabs

jackcmeyer · 2020-02-06T03:53:14Z

src/__tests__/patients/patient-slice.test.ts

@@ -130,6 +237,7 @@ describe('patients slice', () => {
 })
 })

+ // should check for the Toast


Does a test still need to be written to check for the Toast?

yeah. I added a toast for updating a Patient and made it redirect history to /patients/${newPatient.id}, since saving on Edit Patient ought to redirect to that. as a side effect of that, adding a new Related Person now moves you off the Related Persons tab and back onto the General Information tab, but I'm not sure if should do anything else about that now.

added this test

jackcmeyer · 2020-02-06T03:53:35Z

src/clients/db/Repository.ts

@@ -39,11 +39,11 @@ export default class Repository<T extends AbstractDBModel> {
 }

 async findAll(): Promise<T[]> {
- const allPatients = await this.db.allDocs({
+ const allDocs = await this.db.allDocs({


thanks for renaming these :)

src/clients/db/Repository.ts

jackcmeyer · 2020-02-06T03:58:05Z

src/patients/view/ViewPatient.tsx

@@ -3,12 +3,13 @@ import { useDispatch, useSelector } from 'react-redux'
 import { useParams, withRouter, Route, useHistory, useLocation } from 'react-router-dom'
 import { Panel, Spinner, TabsHeader, Tab } from '@hospitalrun/components'
 import { useTranslation } from 'react-i18next'
+


extra whitespace here?

just thought it helped to separate the types of imports

I really like separating type of imports:external deps -> 1 empty line -> internal deps
:)

I think there's an eslint setting for this, that's why I instinctively did it.

jackcmeyer · 2020-02-06T04:02:07Z

src/__tests__/patients/GeneralInformation.test.tsx

- const wrapper = setup()
-
- const dateOfBirthInput = wrapper.findWhere((w) => w.prop('name') === 'address')
+ const dateOfBirthInput = wrapper.findWhere((w: any) => w.prop('name') === 'address')


I think this variable name should be addressTextField. This is probably my bad, copy paste :(

jackcmeyer · 2020-02-06T04:04:12Z

src/__tests__/patients/GeneralInformation.test.tsx

- const wrapper = setup()
-
- const dateOfBirthInput = wrapper.findWhere((w) => w.prop('name') === 'occupation')
+ const dateOfBirthInput = wrapper.findWhere((w: any) => w.prop('name') === 'occupation')


this should probably be occupationInput

jackcmeyer · 2020-02-06T04:04:24Z

src/__tests__/patients/GeneralInformation.test.tsx

- const wrapper = setup()
-
- const dateOfBirthInput = wrapper.findWhere((w) => w.prop('name') === 'preferredLanguage')
+ const dateOfBirthInput = wrapper.findWhere((w: any) => w.prop('name') === 'preferredLanguage')


preferredLanguageInput

jackcmeyer · 2020-02-06T04:04:33Z

src/__tests__/patients/GeneralInformation.test.tsx

- const wrapper = setup()
-
- const dateOfBirthInput = wrapper.findWhere((w) => w.prop('name') === 'phoneNumber')
+ const dateOfBirthInput = wrapper.findWhere((w: any) => w.prop('name') === 'phoneNumber')


phoneNumberInput

jackcmeyer · 2020-02-06T04:05:36Z

src/HospitalRun.tsx

+ <PrivateRoute
+ isAuthenticated={
+ permissions.includes(Permissions.WritePatients) &&
+ permissions.includes(Permissions.ReadPatients)
+ }
+ exact
+ path="/patients/edit/:id"
+ component={EditPatient}
+ />


I think that we need some tests in HospitalRun.test.tsx surrounding this new route, especially for permissions.

Added these

jackcmeyer

UI Styling comment:

Edit button should be in the top right corner, outlined success variant, and a pencil icon.

Add tests into GeneralInformation that were in the now-deleted NewPatientForm

…talrun-frontend into edit-patient

MatthewDorner · 2020-02-06T22:08:26Z

Made the requested UI changes.
Brought some of the tests from NewPatientForm.test.tsx back into GeneralInformation.test.tsx though probably still need more.
Fixed other tests.
Fixed the logic to display spinner in EditPatient and ViewPatient
Added error handling and test for error handling back in.

Still need to add tests for the new route in HospitalRun component, and the actual tests for EditPatient.

jackcmeyer · 2020-02-06T23:53:26Z

src/patients/GeneralInformation.tsx

- {isEditable && (
- <div className="row float-right">
- <div className="btn-group btn-group-lg">
- <Button className="mr-2" color="success" onClick={onSaveButtonClick}>
- {t('actions.save')}
- </Button>
- <Button color="danger" onClick={() => onCancel()}>
- {t('actions.cancel')}
- </Button>
- </div>


I think we want to keep these buttons

that's where the buttons were in NewPatientForm, the entire file is deleted but Github thinks I renamed it to GeneralInformation and deleted GeneralInformation. The buttons are still there, they're just slightly further down. GeneralInformation has a different layout than NewPatientForm because it has the panels.

ok, I restored the previous style of buttons

MatthewDorner · 2020-02-07T00:34:47Z

Added tests for EditPatient component and the patients/edit route, still having some test failures and not exactly sure why. Also couldn't figure out how to mock useParams (see the commented out test in EditPatient.test.tsx)

matteovivona · 2020-02-07T20:12:25Z

@MatthewDorner this afternoon we fixed an annoying bug with husky/lint. Could you Resolve conflicts and errors? thank you!

…talrun-frontend into edit-patient

MatthewDorner · 2020-02-07T21:19:50Z

src/patients/GeneralInformation.tsx

+ onChange={
+ (event: React.ChangeEvent<HTMLTextAreaElement>) =>
+ onFieldChange && onFieldChange('address', event.currentTarget.value)
+ // eslint-disable-next-line react/jsx-curly-newline
+ }


Can't get eslint to accept this any way I can think to format it, or via the eslint automatic fix. There must be two conflicting rules.

Instead of conditionally rendering buttons in GeneralInformation, render them in their parent components (New/View/Edit Patient).

…talrun-frontend into edit-patient

MatthewDorner · 2020-02-07T22:36:19Z

Fixed merge conflicts
Moved buttons out of GeneralInformation form and into the parent forms (NewPatient, EditPatient, ViewPatient)
Fixed some tests

MatthewDorner · 2020-02-08T02:47:34Z

Tests passing now. Looks like I should add some more tests, although I was thinking of going through the tests more thoroughly after this gets merged anyway.

fox1t · 2020-02-08T08:52:13Z

src/patients/view/ViewPatient.tsx

@@ -3,12 +3,13 @@ import { useDispatch, useSelector } from 'react-redux'
 import { useParams, withRouter, Route, useHistory, useLocation } from 'react-router-dom'
 import { Panel, Spinner, TabsHeader, Tab } from '@hospitalrun/components'
 import { useTranslation } from 'react-i18next'
+


I really like separating type of imports:external deps -> 1 empty line -> internal deps
:)

fox1t · 2020-02-08T08:53:06Z

@MatthewDorner Great PR!

fox1t · 2020-02-08T08:53:15Z

LGTM

MatthewDorner added 2 commits February 5, 2020 21:16

feat(edit patient): implement Edit Patient functionality

8e3355f

1765

test(edit patient): fix tests related to implementing Edit Patient

4e851ea

vercel bot deployed to Preview February 6, 2020 03:28 View deployment

MatthewDorner commented Feb 6, 2020

View reviewed changes

src/clients/db/Repository.ts Outdated Show resolved Hide resolved

MatthewDorner commented Feb 6, 2020

View reviewed changes

src/patients/related-persons/RelatedPersonTab.tsx Outdated Show resolved Hide resolved

jackcmeyer suggested changes Feb 6, 2020

View reviewed changes

jackcmeyer requested review from fox1t and matteovivona February 6, 2020 04:42

jackcmeyer added 🚀enhancement an issue/pull request that adds a feature to the application patients issue/pull request that interacts with patients module labels Feb 6, 2020

jackcmeyer added this to In progress in Version 2.0 via automation Feb 6, 2020

jackcmeyer added this to the v2.0.0 milestone Feb 6, 2020

jackcmeyer suggested changes Feb 6, 2020

View reviewed changes

Merge branch 'master' into edit-patient

9048737

vercel bot deployed to Preview February 6, 2020 11:29 View deployment

Merge branch 'master' into edit-patient

f3cf768

vercel bot deployed to Preview February 6, 2020 11:38 View deployment

Merge branch 'master' into edit-patient

19ac42d

vercel bot deployed to Preview February 6, 2020 20:47 View deployment

MatthewDorner added 2 commits February 6, 2020 15:56

test(edit patient): fix tests, improve tests for GeneralInformation

4a23204

Add tests into GeneralInformation that were in the now-deleted NewPatientForm

Merge branch 'edit-patient' of https://github.com/matthewdorner/hospi…

c32e682

…talrun-frontend into edit-patient

vercel bot deployed to Preview February 6, 2020 21:58 View deployment

jackcmeyer suggested changes Feb 6, 2020

View reviewed changes

test(edit patient): add tests for EditPatient component and route

12fde6e

vercel bot deployed to Preview February 7, 2020 00:29 View deployment

vercel bot temporarily deployed to Preview February 7, 2020 15:15 Inactive

matteovivona force-pushed the master branch from 4561092 to db8b247 Compare February 7, 2020 15:19

Merge branch 'master' into edit-patient

222681f

vercel bot deployed to Preview February 7, 2020 15:19 View deployment

Merge branch 'master' into edit-patient

4784fb2

vercel bot deployed to Preview February 7, 2020 17:08 View deployment

fox1t previously approved these changes Feb 7, 2020

View reviewed changes

matteovivona self-assigned this Feb 7, 2020

matteovivona removed their request for review February 7, 2020 20:12

MatthewDorner added 2 commits February 7, 2020 15:11

Merge remote-tracking branch 'upstream/master' into edit-patient

8c85408

Merge branch 'edit-patient' of https://github.com/matthewdorner/hospi…

36e83e4

…talrun-frontend into edit-patient

MatthewDorner dismissed fox1t’s stale review via 36e83e4 February 7, 2020 21:13

vercel bot deployed to Preview February 7, 2020 21:13 View deployment

MatthewDorner commented Feb 7, 2020

View reviewed changes

Merge branch 'master' into edit-patient

d116975

vercel bot deployed to Preview February 7, 2020 21:41 View deployment

MatthewDorner added 2 commits February 7, 2020 16:32

feat(edit patient): moved buttons out of GeneralInformation

403e49f

Instead of conditionally rendering buttons in GeneralInformation, render them in their parent components (New/View/Edit Patient).

Merge branch 'edit-patient' of https://github.com/matthewdorner/hospi…

3c8c6ac

…talrun-frontend into edit-patient

vercel bot deployed to Preview February 7, 2020 22:33 View deployment

test(edit patient): fix test errors

cee1f82

vercel bot deployed to Preview February 8, 2020 02:24 View deployment

jackcmeyer approved these changes Feb 8, 2020

View reviewed changes

fox1t approved these changes Feb 8, 2020

View reviewed changes

fox1t merged commit 3626609 into HospitalRun:master Feb 8, 2020

Version 2.0 automation moved this from In progress to Done Feb 8, 2020

(feat) Edit Patient #1806

(feat) Edit Patient #1806

Conversation

MatthewDorner commented Feb 6, 2020

vercel bot commented Feb 6, 2020 • 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

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

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

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

jackcmeyer left a comment

Choose a reason for hiding this comment

MatthewDorner commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewDorner commented Feb 7, 2020

matteovivona commented Feb 7, 2020

Choose a reason for hiding this comment

MatthewDorner commented Feb 7, 2020

MatthewDorner commented Feb 8, 2020

Choose a reason for hiding this comment

fox1t commented Feb 8, 2020

fox1t commented Feb 8, 2020

vercel bot commented Feb 6, 2020 •

edited

Loading