Skip to content
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
merged 35 commits into from
Jan 31, 2020
Merged

Conversation

M-BenAli
Copy link
Contributor

@M-BenAli M-BenAli commented Jan 18, 2020

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
Copy link

jsf-clabot commented Jan 18, 2020

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Jan 18, 2020

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

@jackcmeyer jackcmeyer added the 🚀enhancement an issue/pull request that adds a feature to the application label Jan 18, 2020
@jackcmeyer jackcmeyer added this to the v2.0.0 milestone Jan 18, 2020
@M-BenAli M-BenAli changed the title Mohamed error handling to patient form Jan 18, 2020
@@ -274,7 +278,9 @@ const NewPatientForm = (props: Props) => {
/>
</div>
</div>

{errorMessage && (
<div className="alert alert-danger" role="alert">{t(errorMessage)}</div>
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -37,6 +38,9 @@ const NewPatientForm = (props: Props) => {
})

const onSaveButtonClick = async () => {
if (!patient.givenName && !patient.familyName) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

@@ -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('')
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -37,6 +38,9 @@ const NewPatientForm = (props: Props) => {
})

const onSaveButtonClick = async () => {
if (!patient.givenName && !patient.familyName) {
return setError("No patient name entered!")
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -274,7 +278,9 @@ const NewPatientForm = (props: Props) => {
/>
</div>
</div>

{errorMessage && (
<div className="alert alert-danger" role="alert">{t(errorMessage)}</div>
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 the t function needs to exist in the translations file.

@@ -274,7 +278,9 @@ const NewPatientForm = (props: Props) => {
/>
</div>
</div>

{errorMessage && (
<div className="alert alert-danger" role="alert">{t(errorMessage)}</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 error messages should be above the form fields. @lauggh @StefanoMiC thoughts here?

Copy link
Member

Choose a reason for hiding this comment

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

+1

@jackcmeyer
Copy link
Member

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 jackcmeyer added the patients issue/pull request that interacts with patients module label Jan 18, 2020
@jackcmeyer
Copy link
Member

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

@jackcmeyer
Copy link
Member

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

@fox1t
Copy link
Member

fox1t commented Jan 19, 2020

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

@matteovivona matteovivona added the in progress indicates that issue/pull request is currently being worked on label Jan 19, 2020
@M-BenAli
Copy link
Contributor Author

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

@matteovivona matteovivona removed their request for review January 19, 2020 16:34
@M-BenAli M-BenAli changed the title error handling to patient form feat:error handling to patient form Jan 19, 2020
@M-BenAli
Copy link
Contributor Author

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).

@fox1t
Copy link
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
Copy link
Member

fox1t commented Jan 29, 2020

I opened a new regression issue: #1775

@matteovivona matteovivona requested review from fox1t and removed request for matteovivona January 30, 2020 20:36
@matteovivona matteovivona self-assigned this Jan 30, 2020
@fox1t
Copy link
Member

fox1t commented Jan 31, 2020

LGTM

@fox1t fox1t merged commit 6589ddb into HospitalRun:master Jan 31, 2020
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.

5 participants