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 Configuration #248

Merged
merged 23 commits into from
Jan 13, 2025
Merged

FHIR Server Configuration #248

merged 23 commits into from
Jan 13, 2025

Conversation

@nickclyde nickclyde marked this pull request as ready for review January 8, 2025 17:17
Copy link
Collaborator

@katyasoup katyasoup left a comment

Choose a reason for hiding this comment

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

Looking good! Couple design things I noticed (ignore if these are out of scope!)

Screenshot 2025-01-08 at 14 18 48
  • the Delete button looks like it should be left-justified per the Figma mockup
Screenshot 2025-01-08 at 14 18 55
  • when I click Test Connection and get a success result, the buttons all grow in height; looks like Figma only shows the success response without the Delete button so I'd double check with @mikang on that
  • "Save Changes" always results in a "Failed to update the server configuration." error (but maybe that is out of scope?)

@mikang
Copy link
Collaborator

mikang commented Jan 8, 2025

Thanks @katyasoup !

  • The cancel button should be just a link, not a white button. Figma below:
image
  • Actually, that's a good catch. The Delete button should be there if the user is editing and then testing the connection. Can we keep the Delete button and change the text to "[checkmark] Success!" ? That shortens the text, so the buttons don't grow in height.

@m-goggins
Copy link
Collaborator

Would you be able to add the tickets that this PR covers? It looks like a lot of the ones written up in December but it'd be helpful to see what is left before the Connectathon.

@nickclyde
Copy link
Member Author

@katyasoup @mikang Just pushed the design fixes for the stuff you pointed out!

@m-goggins added the list of tickets this addresses so far. I plan on implementing delete functionality next and moving the hardcoded servers into the table after that as discussed at standup!

Copy link
Collaborator

@robertandremitchell robertandremitchell left a comment

Choose a reason for hiding this comment

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

looks good in npm run dev! I was able to successfully add and remove a fhir server without issue.

running into this error on npm run build


./src/app/fhir-servers.ts:14:3
Type error: Type '{ id: string; name: string; hostname: string; headers: {}; }' is not assignable to type 'DevFhirServerConfig'.
  Type '{ id: string; name: string; hostname: string; headers: {}; }' is missing the following properties from type 'FhirServerConfig': last_connection_attempt, last_connection_successful

  12 |   process.env.E2E_LOCAL_FHIR_SERVER ?? "http://hapi-fhir-server:8080/fhir";
  13 | export const fhirServers: Record<string, DevFhirServerConfig> = {
> 14 |   "HELIOS Meld: Direct": {
     |   ^
  15 |     id: "HELIOS Meld: Direct",
  16 |     name: "HELIOS Meld: Direct",
  17 |     hostname: "https://gw.interop.community/HeliosConnectathonSa/open",

Copy link
Collaborator

@fzhao99 fzhao99 left a comment

Choose a reason for hiding this comment

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

this lgtm! think the build error is a pretty simple fix, so approving.

Wondering if we should have a followup that adds our hardcoded default servers to the dump file? Might be good to have them available out of the box like we do the default queries.

Copy link
Collaborator

@m-goggins m-goggins left a comment

Choose a reason for hiding this comment

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

Looks good to me as long as we update the table to pull the list of available FHIR servers from the table and the hard coded list like we do for the drop down or update the table to include all of the hard coded servers (even is just for the connectathon). Left a few other comments for consideration but nothing that should be blocking.

@nickclyde
Copy link
Member Author

Just fixed the build by moving all the hardcoded servers into the database via migration. Also cleaned up the FhirServers constant references. Also had to make the import of Modal a dynamic import to make the build work. Still have a few comments to address but I think this now covers almost all of the FHIR server configuration tickets in Linear!

Copy link
Collaborator

@robertandremitchell robertandremitchell left a comment

Choose a reason for hiding this comment

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

Looks good! a few formatting touch-ups, but I think they would be fine to tackle in a follow-up ticket if we want to get the bulk of this merged.

Screenshot 2025-01-13 at 8 28 30 AM
Screen.Recording.2025-01-13.at.8.23.09.AM.mov

@nickclyde
Copy link
Member Author

Good catch @robertandremitchell! Fixed those issues!

@nickclyde nickclyde merged commit d063847 into main Jan 13, 2025
11 checks passed
@nickclyde nickclyde deleted the nickclyde/fhir-server-config branch January 13, 2025 19:06
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.

6 participants