Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

feat(new): warn about potential duplicate patient (#2187) #2240

Merged
merged 18 commits into from
Jul 31, 2020

Conversation

janmarkusmilan
Copy link
Contributor

Fixes #2187

Changes proposed in this pull request:

  • A modal appears when a patient with matching name, sex and date of birth is created
  • The modal outputs the message "Potential duplicate of: [Patient Name](link to patient record). Are you sure you want to create this patient?"
  • Clicking on the patient name will redirect to the patient view
  • Clicking on the cancel button will close the modal
  • Clicking on the continue button will dispatch create patient, redirect to the new patient view and output a success message
  • Added a new test file for the modal

No new dependency added

@jsf-clabot
Copy link

jsf-clabot commented Jul 17, 2020

CLA assistant check
All committers have signed the CLA.

@gitpod-io
Copy link

gitpod-io bot commented Jul 17, 2020

@vercel
Copy link

vercel bot commented Jul 18, 2020

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

🔍 Inspect: https://vercel.com/hospitalrun/hospitalrun-frontend/dqbxrat2q
✅ Preview: https://hospitalrun-fro-git-fork-janmarkusmilan-duplicate-patien-bed13f.hospitalrun.vercel.app

@jackcmeyer jackcmeyer requested review from jackcmeyer, a user and fox1t and removed request for jackcmeyer July 18, 2020 04:15
@jackcmeyer jackcmeyer added patients issue/pull request that interacts with patients module 🚀enhancement an issue/pull request that adds a feature to the application labels Jul 18, 2020
@jackcmeyer jackcmeyer added this to the v2.0 milestone Jul 18, 2020
Comment on lines 54 to 57
loggedPatient.givenName === patient.givenName &&
loggedPatient.familyName === patient.familyName &&
loggedPatient.sex === patient.sex &&
loggedPatient.dateOfBirth === patient.dateOfBirth
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 that we should extra this to a helper util file called is-possible-duplicate-patient.ts. The file can then export one function isPossibleDuplicatePatient(newPatient, existingPatient).

With this, we can then unit test that capability independently, plus reuse if necessary.

Comment on lines 23 to 29
<p>
Possible duplicate of:{' '}
{duplicatePatient !== undefined && (
<Link to={`/patients/${duplicatePatient.id}`}>{duplicatePatient.fullName}</Link>
)}
</p>
<p>Are you sure you want to create this patient?</p>
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 maybe this could go in a warning alert to bring more attention to the warning we are providing.

Comment on lines 23 to 29
<p>
Possible duplicate of:{' '}
{duplicatePatient !== undefined && (
<Link to={`/patients/${duplicatePatient.id}`}>{duplicatePatient.fullName}</Link>
)}
</p>
<p>Are you sure you want to create this patient?</p>
Copy link
Member

Choose a reason for hiding this comment

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

All text should be provided through the internationalization process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for clarification, do you mean using the translator? For example:

<Alert color="danger" message={t('addTheTextToBeInternationalizedHere')} />

<div className="row">
<div className="col-md-12">
<p>
Possible duplicate of:{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be internationalized.


const [patient, setPatient] = useState({} as Patient)
const [duplicatePatient, setDuplicatePatient] = useState({} as Patient)
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 it makes more sense to represent this state as

useState<Patient | undefiend>(undefined). I think more clearly represents the presence/absence of a duplicate patient.

@@ -21,8 +22,11 @@ const NewPatient = () => {
const history = useHistory()
const dispatch = useDispatch()
const { createError } = useSelector((state: RootState) => state.patient)
const { patients } = Object(useSelector((state: RootState) => state.patients))
Copy link
Member

Choose a reason for hiding this comment

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

This line can be simplified to:

const { patients } = useSelector((state: RootState) => state.patients)

Copy link
Contributor Author

@janmarkusmilan janmarkusmilan Jul 23, 2020

Choose a reason for hiding this comment

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

I originally had that line, but the tests would always fail. The message is:

TypeError: Cannot destructure property 'patients' of '(0 , _reactRedux.useSelector)(...)' as it is undefined.

The only solution I found was adding the Object(...) at the beginning. I can't seem to get it working without it.


const [patient, setPatient] = useState({} as Patient)
const [duplicatePatient, setDuplicatePatient] = useState({} as Patient)
Copy link
Member

Choose a reason for hiding this comment

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

After thinking about this functionality some more and seeing it action. I think it make sense to support possibly finding multiple duplicates?

If there were already two other patients named "Test User" born on 7/16/2000 and are male. I think all of the should appear in the warning.

Comment on lines 47 to 67
const onSave = () => {
dispatch(createPatient(patient, onSuccessfulSave))
let isDuplicatePatient = false
const patientsObj = {}
Object.assign(patientsObj, patients)
Object.keys(patientsObj).forEach((patientInfo: any) => {
const loggedPatient = patients[patientInfo]
if (
loggedPatient.givenName === patient.givenName &&
loggedPatient.familyName === patient.familyName &&
loggedPatient.sex === patient.sex &&
loggedPatient.dateOfBirth === patient.dateOfBirth
) {
setShowDuplicateNewPatientModal(true)
setDuplicatePatient(loggedPatient as Patient)
isDuplicatePatient = true
}
})
if (!isDuplicatePatient) {
dispatch(createPatient(patient, onSuccessfulSave))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

After adding support for finding all possible duplicate patients, rather than just the first, this function would be a good candidate for Arrays.filter https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter.

const onSave = () => {
	 const duplicatePatients = patients.filter((existingPatient) => isPossibleDuplicatePatient(patient, existingPatient));
	
	if(duplicatePatients.length > 0) {
       setShowDuplicateNewPatientModal(true)
       setDuplicatePatient(existingPatient)
	} else {
      dispatch(createPatient(patient, onSuccessfulSave))
	}
}

@@ -10,6 +10,7 @@ import Patient from '../../shared/model/Patient'
import { RootState } from '../../shared/store'
import GeneralInformation from '../GeneralInformation'
import { createPatient } from '../patient-slice'
import DuplicateNewPatientModal from './DuplicateNewPatientModal'
Copy link
Member

Choose a reason for hiding this comment

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

There should be some tests to test this new workflow.

Basically, have an already existing patient in the redux store, try to create a new patient with the same information, and test to make sure that the new modal shows.

Comment on lines 77 to 96
const onContinueButtonClickSpy = jest.fn()
const store = mockStore({
patient: {
patient: {
id: '1234',
},
},
} as any)

const wrapper = mount(
<Provider store={store}>
<DuplicateNewPatientModal
show
toggle={jest.fn()}
onCloseButtonClick={jest.fn()}
onContinueButtonClick={onContinueButtonClickSpy}
/>
</Provider>,
)
wrapper.update()
Copy link
Member

Choose a reason for hiding this comment

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

This code block is duplicated several times in this file. I think we can extract it to a setup function that looks like:

const setup = () => {
 const onContinueButtonClickSpy = jest.fn()
      const store = mockStore({
        patient: {
          patient: {
            id: '1234',
          },
        },
      } as any)

      const wrapper = mount(
        <Provider store={store}>
          <DuplicateNewPatientModal
            show
            toggle={jest.fn()}
            onCloseButtonClick={jest.fn()}
            onContinueButtonClick={onContinueButtonClickSpy}
          />
        </Provider>,
      )
	wrapper.update()

   return { wrapper: wrapper as ReactWrapper }
}

@gitpod-io
Copy link

gitpod-io bot commented Jul 20, 2020

@janmarkusmilan
Copy link
Contributor Author

janmarkusmilan commented Jul 20, 2020

Sorry, I accidentally requested a merge to master (did not make the changes yet). I am making the changes now.

@janmarkusmilan
Copy link
Contributor Author

janmarkusmilan commented Jul 24, 2020

Recently Pushed Requested Changes

1. Helper util file (is-possible-duplicate-patient.ts)

is-possible-duplicate-patient.ts

Created helper util file to allow for reusability if necessary.


2. Support for finding multiple duplicate patients

NewPatient.tsx

NewPatient.tsx

DuplicateNewPatientModal.tsx

DuplicateNewPatientModal.tsx

Used the Arrays.filter method and the newly created helper util file is-possible-duplicate-patient.ts to loop through the existing patients and determine if there are zero, one or multiple patients with the same information as the one being created. Also styled the look of the modal based on if there is one or multiple patients.


3. New alert and internationalized text

DuplicateNewPatientModal.tsx

Updated_Alert

Added an alert to bring more attention to the warning being provided. Also, all the text is internationalized (still missing english translation key in console).

Modal (Multiple Patients)

Multiple_Patients

Modal (Single Patient)

Single_Patient


4. Setup function in test file

DuplicateNewPatientModal.test.tsx

DuplicateNewPatientModal.text.tsx

Using the setup function

Code_Reusability

Since this code block was duplicated multiple times, it was extracted to a setup function for easy reusability.


5. Test case for new modal

NewPatient.test.tsx

NewPatient.test.tsx

NewPatient.tsx

Test_Patient

A test needed to be created in order to test the new modal feature. Given some existing patient in the redux store, if a newly created patient had the same information as the existing one, then the modal should render (or in this case return true).


Other Things

  1. Changed const [duplicatePatient, setDuplicatePatient] = useState({} as Patient) to const [duplicatePatient, setDuplicatePatient] = useState<Patient | undefined>(undefined)
  2. Had to keep const { patients } = Object(useSelector((state: RootState) => state.patients)) as it is, since removing the Object(...) results in errors when running the test cases for NewPatient.tsx

Copy link
Member

@jackcmeyer jackcmeyer left a comment

Choose a reason for hiding this comment

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

Few comments on the code! I really like the new functionality for the multiple duplicate patients!

const { duplicatePatient, show, toggle, onCloseButtonClick, onContinueButtonClick } = props

const alertMessage =
'Patient with matching information found in database. Are you sure you want to create this patient?'
Copy link
Member

Choose a reason for hiding this comment

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

this alert message needs to have the string internationalized.

<Alert color="danger" title={t('Warning!')} message={t(alertMessage)} />
<div className="row">
<div className="col-md-12">
{`${t('Possible duplicate patient')}: `}
Copy link
Member

Choose a reason for hiding this comment

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

The value passed to be a key that is defined in the locales folder.


const body = (
<>
<Alert color="danger" title={t('Warning!')} message={t(alertMessage)} />
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on having this as a warning color rather than danger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense. It's more of a warning message than an error message, and it still stands out.

@@ -0,0 +1,13 @@
import Patient from '../../shared/model/Patient'

export function isPossibleDuplicatePatient(newPatient: Patient, existingPatient: Patient) {
Copy link
Member

Choose a reason for hiding this comment

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

as trivial as it is, we should have a test around this functionality.

@@ -0,0 +1,13 @@
import Patient from '../../shared/model/Patient'

export function isPossibleDuplicatePatient(newPatient: Patient, existingPatient: Patient) {
Copy link
Member

Choose a reason for hiding this comment

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

I would expect a function named isPossibleDuplicatePatient to return a boolean rather than a object.

I think this function can be simplified as:

export function isPossibleDuplicatePatient(newPatient: Patient, existingPatient: Patient) {
	return newPatient.givenName === existingPatient.givenName &&
    newPatient.familyName === existingPatient.familyName &&
    newPatient.sex === existingPatient.sex &&
    newPatient.dateOfBirth === existingPatient.dateOfBirth
}

Comment on lines 27 to 43
<div className="col-md-12">
{`${t('Possible duplicate patient')}: `}
{duplicatePatient !== undefined &&
Object.entries(duplicatePatient).length === 1 &&
Object.entries(duplicatePatient).map(([key, patient]) => (
<Link key={key.toString()} to={`/patients/${patient.id}`}>
{patient.fullName}
</Link>
))}
{duplicatePatient !== undefined &&
Object.entries(duplicatePatient).length > 1 &&
Object.entries(duplicatePatient).map(([key, patient]) => (
<li key={key.toString()}>
<Link to={`/patients/${patient.id}`}>{patient.fullName}</Link>
</li>
))}
</div>
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 that it is ok if it always displays a list no matter the number of duplicate patients returned. If we do that, then we can simplify this code.

@vercel vercel bot temporarily deployed to Preview July 30, 2020 13:14 Inactive
@matteovivona matteovivona added the in progress indicates that issue/pull request is currently being worked on label Jul 30, 2020
@janmarkusmilan
Copy link
Contributor Author

Newly Pushed Changes

1. Changed alert color and internationalized text

I changed the color of the alert to "warning" instead of danger, and internationalized the text by adding them in the locales folder.


2. Changed is-possible-duplicate-patient.ts to return a boolean instead of an object, and created test file

I simplified the util file to return a boolean instead of an object, and created a test file for the function.


3. Set modal to display a list regardless of the amount of duplicate patients

I simplified the code by always rendering a list of duplicate patients in the modal.


Modal

@jackcmeyer jackcmeyer merged commit f49831c into HospitalRun:master Jul 31, 2020
@janmarkusmilan janmarkusmilan deleted the duplicate-patient-modal branch August 5, 2020 01:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🚀enhancement an issue/pull request that adds a feature to the application in progress indicates that issue/pull request is currently being worked on patients issue/pull request that interacts with patients module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn about potential duplicate patient during creation process
4 participants