Skip to content

Hl7 38 file handling #17

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Hl7 38 file handling #17

wants to merge 2 commits into from

Conversation

JamesbZhu
Copy link
Contributor

Related JIRA tickets:

Please enter the whole URL so we can just click/copy it
-https://jira.amida.com/projects/HL7/issues/HL7-38

What exactly does this PR do?

i.e. Added logic to do the thing because the thing didn't exist before. Please provide as much detail as possible.

  • adding the ability to verify that a uploaded text file is an hl7 file by asserting that it starts with a message header (MSH|)

What else was added outside of the scope of the ask?

i.e. Simple bugfix, corrected typos, cleaned up code, etc

Do ANY of these changes introduce a breaking change? (in-scope or otherwise)

i.e. Users need to update their .env files with the following... etc

Manual test cases?

Steps to manually test introduced changes can go here.
Remember the prerequisites!
Remember edge-cases you've caught in your code, etc..
i.e. ignoring the button and clicking on the "NEXT" button should trigger a warning event because the thing that clicking the new button does didn't happen

  • run yarn start
  • upload a text file that does not start with "MSH|" on postman
  • assert that the returned error reads "Please upload a valid HL7 file"

Any additional context/background?

N/A if this is straight-forward

  • N/A

Screenshots (if appropriate):

@JamesbZhu JamesbZhu requested a review from Perry5 September 5, 2019 21:25
Copy link
Contributor

@Perry5 Perry5 left a comment

Choose a reason for hiding this comment

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

Good work!

Left a small comment. Maybe we can also add a unit test for this case?

I will also leave this PR opened until we make the change to prevent saving the file to the disk. @amathew8 is creating the ticket for this.

@@ -70,6 +70,9 @@ function parseFile(req, res, next) {
}).catch(err =>
next(new APIError(err, httpStatus.BAD_REQUEST))
).then((data) => {
if (!data.startsWith('MSH|')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach works since (I believe) all HL7 messages have to start with the message header (MSH)

@@ -70,6 +70,9 @@ function parseFile(req, res, next) {
}).catch(err =>
next(new APIError(err, httpStatus.BAD_REQUEST))
).then((data) => {
if (!data.startsWith('MSH|')) {
throw new Error('Please upload a valid HL7 file');
Copy link
Contributor

Choose a reason for hiding this comment

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

Throwing an error here like this will cause the sever to return a 500. We can instead do this:
next(new APIError('Please upload a valid HL7 file', httpStatus.BAD_REQUEST)); - this will return a 400 Bad Request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants