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

feat:error handling to patient form #1736

Merged

fox1t merged 35 commits into HospitalRun:master from M-BenAli:Mohamed

Jan 31, 2020

Contributor

M-BenAli commented Jan 18, 2020 •

edited by matteovivona

Loading

Changes proposed in this pull request:

error handling to patient form so users can't submit forms without a patient name
added .idea/ default folder to .gitignore for intellij IDE users(like myself)

jsf-clabot commented Jan 18, 2020 •

edited

Loading

All committers have signed the CLA.

vercel bot commented Jan 18, 2020 •

edited

Loading

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/h01xotey0
✅ Preview: https://hospitalrun-frontend-git-fork-m-benali-mohamed.hospitalrun.now.sh

vercel bot deployed to Preview

January 18, 2020 23:03

View deployment

jackcmeyer requested review from jackcmeyer, fox1t and matteovivona

January 18, 2020 23:07

jackcmeyer added the 🚀enhancement label

jackcmeyer added this to In progress in Version 2.0 via automation

jackcmeyer added this to the v2.0.0 milestone

M-BenAli changed the title ~~Mohamed~~ error handling to patient form

jackcmeyer suggested changes

View reviewed changes

src/patients/new/NewPatientForm.tsx Outdated

@@ @@ -274,7 +278,9 @@ const NewPatientForm = (props: Props) => { @@
  />
  </div>
  </div>
+ {errorMessage && (
+ <div className="alert alert-danger" role="alert">{t(errorMessage)}</div>

Member

jackcmeyer Jan 18, 2020

We can use the Alert component from @hospitalrun/component.

src/patients/new/NewPatientForm.tsx Outdated

@@ @@ -37,6 +38,9 @@ const NewPatientForm = (props: Props) => { @@
  })
  const onSaveButtonClick = async () => {
+ if (!patient.givenName && !patient.familyName) {

Member

jackcmeyer Jan 18, 2020

In order to support all different types of name formats, I think that we should require that ONE of these fields is present (doesn't matter which one). @fox1t @StefanoMiC @tehkapa @lauggh what do you all think here?

Member

fox1t Jan 19, 2020

I agree with you @jackcmeyer. For example we already found cases where familiy names are not used (Burkina Faso). I think that the best choice is to make this configurable: the admin can decide mandatory fields for "all" forms. What do you think?

src/patients/new/NewPatientForm.tsx Outdated

@@ @@ -19,6 +19,7 @@ const NewPatientForm = (props: Props) => { @@
  const [isEditable] = useState(true)
  const { onCancel, onSave } = props
  const [approximateAge, setApproximateAge] = useState(0)
+ const [errorMessage, setError] = useState('')

Member

jackcmeyer Jan 18, 2020

Rename setError to setErrorMesssage to keep the state variable and state update function consistent.

src/patients/new/NewPatientForm.tsx Outdated

@@ @@ -37,6 +38,9 @@ const NewPatientForm = (props: Props) => { @@
  })
  const onSaveButtonClick = async () => {
+ if (!patient.givenName && !patient.familyName) {
+ return setError("No patient name entered!")

Member

jackcmeyer Jan 18, 2020

This string needs to support internationalization.

To do that you can add a key and value to locales/en-US/translation.json. I think a key in the patient object called patientNameRequired or something like that would work.

src/patients/new/NewPatientForm.tsx Outdated

@@ @@ -274,7 +278,9 @@ const NewPatientForm = (props: Props) => { @@
  />
  </div>
  </div>
+ {errorMessage && (
+ <div className="alert alert-danger" role="alert">{t(errorMessage)}</div>

Member

jackcmeyer Jan 18, 2020

the value passed to the t function needs to exist in the translations file.

src/patients/new/NewPatientForm.tsx Outdated

@@ @@ -274,7 +278,9 @@ const NewPatientForm = (props: Props) => { @@
  />
  </div>
  </div>
+ {errorMessage && (
+ <div className="alert alert-danger" role="alert">{t(errorMessage)}</div>

Member

jackcmeyer Jan 18, 2020

I think that error messages should be above the form fields. @lauggh @StefanoMiC thoughts here?

Member

fox1t Jan 19, 2020

+1

Member

jackcmeyer commented Jan 18, 2020

Thanks for the contribution @M-BenAli!

A few things on this PR:

Left some feedback on the code
Some linting errors causing the build to fail
Need some tests surrounding this new functionality
Please make sure to sign the CLA with the account that you committed with.

jackcmeyer added the patients label

Member

jackcmeyer commented Jan 18, 2020

It looks like the account you committed with is not a GitHub user. Either you'll need to create an account for those commits, or you can amend your commits to match the account you opened the pull request with: https://stackoverflow.com/questions/3042437/how-to-change-the-commit-author-for-one-specific-commit/28845565

Member

jackcmeyer commented Jan 18, 2020

Also, congratulations on your first ever pull request @M-BenAli 🎉🎉🎉!!

M-BenAli force-pushed the Mohamed branch from a96a64a to 97f06a7 Compare

January 19, 2020 00:08

vercel bot deployed to Preview

January 19, 2020 00:08

View deployment

M-BenAli force-pushed the Mohamed branch from 97f06a7 to f0df64a Compare

January 19, 2020 00:22

vercel bot deployed to Preview

January 19, 2020 00:22

View deployment

Member

fox1t commented Jan 19, 2020

🙏 Thanks for your contribution! We will guide you to make this PR merged! ☺️

matteovivona added the in progress label

Contributor Author

M-BenAli commented Jan 19, 2020

Hi all! Thank you for the feedback and the welcome messages, I'll be looking at the requested changes and adjust the code.

matteovivona removed their request for review

January 19, 2020 16:34

vercel bot deployed to Preview

January 19, 2020 22:51

View deployment

M-BenAli changed the title ~~error handling to patient form~~ feat:error handling to patient form

M-BenAli added 3 commits

January 20, 2020 00:11


feat(patientform): added patient form error handling

c17aa0a


fix: add .idea/ default intellij folder to .gitignore

67bcda3


fix: adjusted code as requests in pr and fixed linting errors

59447dc

M-BenAli force-pushed the Mohamed branch from b9c42a4 to 59447dc Compare

January 19, 2020 23:12

vercel bot deployed to Preview

January 19, 2020 23:12

View deployment

M-BenAli added 2 commits

January 28, 2020 21:07


fix(newpatientform.test.tsx): fix to spacing according to lint rules

55043fa


Merge branch 'Mohamed' of https://github.com/M-BenAli/hospitalrun-fro…

a2993b8

…ntend into Mohamed

M-BenAli dismissed jackcmeyer’s stale review via

a2993b8

January 28, 2020 20:07

vercel bot deployed to Preview

January 28, 2020 20:07

View deployment

Contributor Author

M-BenAli commented Jan 28, 2020

The only question I have is: why all surrounding spaces are removed in {} and < />?

This problem should be solved now in the latest commit, npm run lint did the trick for me(using npm instead of yarn).


Merge branch 'master' into Mohamed

544816c

vercel bot deployed to Preview

January 28, 2020 20:25

View deployment


Merge branch 'master' into Mohamed

fc27120

vercel bot deployed to Preview

January 28, 2020 20:40

View deployment


Merge branch 'master' into Mohamed

4da7746

vercel bot deployed to Preview

January 28, 2020 20:52

View deployment


Merge branch 'master' into Mohamed

794dab9

vercel bot deployed to Preview

January 28, 2020 21:43

View deployment


Merge branch 'master' into Mohamed

6de004f

vercel bot deployed to Preview

January 29, 2020 07:54

View deployment


Merge branch 'master' into Mohamed

b092be9

vercel bot deployed to Preview

January 29, 2020 13:57

View deployment

matteovivona requested review from jackcmeyer, fox1t and matteovivona

January 29, 2020 14:51

fox1t approved these changes

View reviewed changes

Member

fox1t commented Jan 29, 2020

The only question I have is: why all surrounding spaces are removed in {} and < />?

This problem should be solved now in the latest commit, npm run lint did the trick for me(using npm instead of yarn).

We need to investigate into this. Thanks for the feedback!

fox1t mentioned this pull request

Lint command is not fixing properly the code #1775

Closed

Member

fox1t commented Jan 29, 2020

I opened a new regression issue: #1775

matteovivona requested review from fox1t and removed request for matteovivona

January 30, 2020 20:36

matteovivona self-assigned this

Member

fox1t commented Jan 31, 2020

LGTM

fox1t merged commit 6589ddb into HospitalRun:master

Version 2.0 automation moved this from In progress to Done

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.