CDCgov / dibbs-query-connector

A FHIR client allowing public health agencies to query health care organizations directly or via a TEFCA QHIN
Creative Commons Zero v1.0 Universal
4 stars 0 forks source link

Adding initialization work to display conditions.category and build queries #103

Closed robertandremitchell closed 2 weeks ago

robertandremitchell commented 3 weeks ago

PULL REQUEST

Summary

Adds an API that pulls the condition data.

I think we need conditions.ts for two purposes in the workflow:

  1. Display the conditions.name grouped by conditions.category on this page: https://www.figma.com/design/Iuw9me6kYftBF4WTCEsCZz/Query-Connector?node-id=475-18708&node-type=frame&t=QNaDph9YAIwc5BOa-0
  2. Use the condition.id(s) on that page to query for ValueSets to display when advancing to the next page. Should be able to use a loop of existing code in database-service.ts in order pull valuesets and concepts.

I think the main open question is whether we want to use API calls or continue to use database calls. I keep reading arguments for both. I think I lean toward actually just wanting to add a database query that pulls conditions data directly to avoid adding an additional API layer, but I am not sure if we should take this step to initialize an API. Curious others' thoughts.

Related Issue

Fixes https://linear.app/skylight-cdc/issue/QUE-26/expose-category-information-to-the-frontend

Acceptance Criteria

Please copy the acceptance criteria from your ticket and paste it here for your reviewer(s)

For frontend PR’s - include a description (including anything that’s out of scope) for what you want the designers to review

Additional Information

Anything else the review team should know?

Checklist

linear[bot] commented 3 weeks ago

QUE-26 Expose category information to the frontend

m-goggins commented 3 weeks ago

I might be missing some of the context here but an API call seems like overkill at this point. The goal is just to get the conditions that currently exist in the database and display them to users in two different ways, right? Even if we add an API, we'll still be making a database call to complete the request so I'd just go with the DB call for now.

Something else that comes to mind (and you probably have already thought of it) but as we move forward to actually rendering the conditions on the page you describe, we should thinking carefully about how often the db call is made. That is, can we smartly cache/store the results so it's not making the call each time the page loads.

robertandremitchell commented 3 weeks ago

I might be missing some of the context here but an API call seems like overkill at this point. The goal is just to get the conditions that currently exist in the database and display them to users in two different ways, right? Even if we add an API, we'll still be making a database call to complete the request so I'd just go with the DB call for now.

Something else that comes to mind (and you probably have already thought of it) but as we move forward to actually rendering the conditions on the page you describe, we should thinking carefully about how often the db call is made. That is, can we smartly cache/store the results so it's not making the call each time the page loads.

the main issue I'm reading up on is whether it starts to approach having your frontend making queries/updates to the database is generally frowned upon and that to build the queries, we're gonna have users doing lots of transactions where APIs give us a little more protection.

https://www.angularminds.com/blog/why-you-need-an-api-layer-and-how-to-build-it-in-react https://konstantinmb.medium.com/from-request-to-database-understanding-the-three-layer-architecture-in-api-development-1c44c973c7af

I tend to agree though that I think it would be easier to do a generic conditions call and format into JSONs the bits of data we need as continue to build this out, so in theory this first step of the work to display the selectable conditions should be written to be limited to one transaction.

fzhao99 commented 3 weeks ago

the main issue I'm reading up on is whether it starts to approach having your frontend making queries/updates to the database is generally frowned upon and that to build the queries, we're gonna have users doing lots of transactions where APIs give us a little more protection.

Will weigh in that this is a little bit different in Next.js land where the whole point of the framework is to allow for server-side code to be easily integrated into our frontend logic within the same programming paradigm. In some cases, (including this one depending on how we handle the caching concerns Marcelle is making) there would actually be optimizations calling the code through server actions rather than through the API. See this writeup for a succinct example and this reddit thread if you want to wade into the internet debate

Think we should ask the question of whether we want an API endpoint from a product / feature perspective (ie is it useful for our users or us in the future for building things out) rather than a technical concerns perspective. The earlier opinion I offered for the API option obviously comes out in one direction, but don't hold that too strongly since thing this would be a fringe endpoint either way.

robertandremitchell commented 2 weeks ago

intending to merge now but raise in 11/4 parking lot this question of implementing the API since we still need the database-service.ts update regardless of if we end up removing/changing the API aspect of this PR.