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

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

Conversation

robertandremitchell
Copy link
Collaborator

@robertandremitchell robertandremitchell commented Oct 31, 2024

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

  • ex: “Check out the whitespace on the page, as well as the dropdowns in the form. Here is the corresponding Figma link. You can ignore everything else. Also, the button at the bottom doesn’t work now”

Additional Information

Anything else the review team should know?

Checklist

  • If this code affects the other scrum team, have they been notified? (In Slack, as reviewers, etc.)

Copy link

linear bot commented Oct 31, 2024

@robertandremitchell robertandremitchell changed the title Adding initialization work for API of customize query workflow Adding initialization work to display conditions.category and build queries Oct 31, 2024
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.

LGTM!

Think an API endpoint would be good. Could see a world where someone might query that in a script they're developing against our API, in which case having it accessible would be helpful. Could always remove it if we don't end up needing it

@m-goggins
Copy link
Collaborator

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.

Copy link
Collaborator

@bamader bamader left a comment

Choose a reason for hiding this comment

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

💪

@robertandremitchell
Copy link
Collaborator Author

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
Copy link
Collaborator

fzhao99 commented Nov 1, 2024

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
Copy link
Collaborator Author

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.

@robertandremitchell robertandremitchell merged commit 695963b into main Nov 4, 2024
5 checks passed
@robertandremitchell robertandremitchell deleted the robertamitchell/que-26-expose-category-information-to-the-frontend branch November 4, 2024 16:22
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.

4 participants