Skip to content
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

Fhir server integration and patient searching/opening #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wmaethner
Copy link
Collaborator

@wmaethner wmaethner commented Aug 19, 2018

  • Added configurable FHIR server support
  • Can search for patients on that FHIR server by ID or name
  • The search displays a list of results which can be selected and opened
  • The open patient's info is then displayed
  • Updated the UI as well

Initial FHIR server integration. Uses a default hardcoded FHIR server and can search a patient and study on that server.
Added configurable FHIR server support and patient searching given an ID or name.
Copy link
Collaborator

@lbergnehr lbergnehr left a comment

Choose a reason for hiding this comment

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

I can't get it to build and neither can Travis. Check if you can resolve that and I'll have another go at it. Perhaps it has something to do with that added reference I mentioned in another comment.

<PackageReference Include="Microsoft.AspNetCore.Mvc.Core" Version="2.1.1" />
</ItemGroup>

<ItemGroup>
<Reference Include="Microsoft.AspNetCore.Mvc.ViewFeatures">
<HintPath>..\..\..\..\..\..\Program Files\dotnet\sdk\NuGetFallbackFolder\microsoft.aspnetcore.mvc.viewfeatures\2.1.1\lib\netstandard2.0\Microsoft.AspNetCore.Mvc.ViewFeatures.dll</HintPath>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this will build as it's an absolute reference on your computer. Could you add that reference as a NuGet package instead? Like what you're doing on line 8 above.

@wmaethner
Copy link
Collaborator Author

Fixed that issue @lbergnehr, it passed the Travis build so should be good now.

Copy link
Collaborator

@lbergnehr lbergnehr left a comment

Choose a reason for hiding this comment

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

Sorry for the late review! I don't think we should have things stuck in review if they can be of use in the field. Let's fix things as they come up instead; but here are a few things that I came across:

  • Using bootstrap from a cdn will effectively require you to have internet access when developing on this.
  • Some camelCase where it should be PascalCase.
  • Formatting (which we haven't set) with braces on the next line, whereas the history of the code so far has been mainly braces on the same line as the method (or whatever construct).
  • The static model (internalModel) will be a world of hurt for me, or us, when merging the signalr branch since that kind of challenges that model completely.

Great work with fetching the data from a FHIR server! Will sure come in handy when using this cross vendors with different data sets.

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