Hl7 22 am list all parsed messages #14
Conversation
Perry5
left a comment
Perry5
left a comment
There was a problem hiding this comment.
Looks good for the most part. One small comment.
Also, does your editor automatically fix linting issues? You should always pay attention to that because when you commit a PR it makes it look like you made changes that you actually didn't. In this case, it was good. But if a change not intentionally done by you is committed, it should be pointed out.
I don't think that is necessary |
orndorffgrant
left a comment
orndorffgrant
left a comment
There was a problem hiding this comment.
Looks good! Left a couple suggestions
server/hl7/hl7.route.js
Outdated
| /** GET /api/hl7/files - Retrieves all user files */ | ||
| .get(hl7Ctrl.getUserFiles); | ||
|
|
||
| router.route('/files/parsedMessages/:fileId') |
There was a problem hiding this comment.
lets change this route to /files/:fileId/messages/
We want the route structure to be a resource-centric tree - so the caller specifies that they care about files then they specify which file ( :fileId) then they specify that they want the messages in that file. This follows a nice hierarchy. Also I generally avoid upper case anything in HTTP URLs, and parsed seems unnecessary because I don't think we'll ever want to GET unparsed messages.
Duckduckgo "REST" and what it stands for to get some context on why I am suggesting this structure
There was a problem hiding this comment.
I might have led @amathew8 astray here. I think I suggested /pasrsedmessage/ but valid point to take it out.
There was a problem hiding this comment.
I mean I basically agree, I just suggest messages after the fileId instead of before to make it more hierarchical 🤷♂
|
@amathew8 |
|
+1 on test |
…it test to avoid race conditions
.vscode/settings.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
+1
we should try to look out for files being added before committing. I guess that's why some people discourage using git add . 🤷♂
| done(); | ||
| done(); | ||
| }) | ||
| .catch(done); |
There was a problem hiding this comment.
| .catch(done); | |
| .catch(done); |
|
@amathew8 test looks good! |
Perry5
left a comment
Perry5
left a comment
There was a problem hiding this comment.
+1 to Grants comments & merge conflict.
I ran into a weird case where after I made a request (Upload file or anything) and I try to GET parsed messages, the request hangs.
@amathew8, @orndorffgrant did either of you run into this problem at all?
.vscode/settings.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
+1
we should try to look out for files being added before committing. I guess that's why some people discourage using git add . 🤷♂
Related JIRA tickets:
-https://jira.amida.com/browse/HL7-22
What exactly does this PR do?
Do ANY of these changes introduce a breaking change? (in-scope or otherwise)
Manual test cases?
Any additional context/background?
Screenshots (if appropriate):