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 Custom Queries backend #133

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

robertandremitchell
Copy link
Collaborator

@robertandremitchell robertandremitchell commented Nov 8, 2024

PULL REQUEST

Summary

This adds a backend function that calls the query data to display eventually on the My Queries page.

For now, we opted to use the existing query table. We could limit it to the five out-of-the-box queries we offer in demo; i may do that in the next PR which actually puts up the front-end but tinker with how the page looks when there are 20+, 30+ custom queries saved.

Right now, this PR is not taking into account user permissions; for now, it is displaying all queries based on feedback we should not yet integrate that work Nick and Katie are doing on that front.

I also opted to create new Query/ValueSet/Concept interfaces specific to this work. I had initially started by editing the existing ones in constants.ts to have a lot of optional fields, but that felt potentially unwieldy. Open to the idea, but it seemed like we'd have to then build in more guardrails elsewhere to ensure that those interfaces had sufficient data elsewhere.

Only other thing to note is that we are also punting on displaying which conditions comprise each query based on notes here: https://linear.app/skylight-cdc/issue/QUE-53/build-out-my-queries-backend-function. Basically, to implement, we will need to refactor part of the query work to ensure we can tie queries to conditions selected to valuesets included/unincluded to concepts included/unincluded. As of now, our workflow only ties queries to valuesets to concepts included/unincluded. This will impact our functionality on the My Queries to display conditions accurately as well as edit existing queries from that page.

Related Issue

Fixes #117

Additional Information

Anything else the review team should know?

Checklist

  • Descriptive Pull Request title
  • Link to relevant issues
  • Provide necessary context for design reviewers
  • Update documentation

Copy link

linear bot commented Nov 8, 2024

@m-goggins
Copy link
Collaborator

m-goggins commented Nov 12, 2024

My preference would be to re-use the same constants we already have, especially the ones for Concept and ValueSet, to ensure consistency across all the different work. It looks like the ones you created are pared down versions of the existing ones (and did not add any additional components), but I imagine that once we build out the front end of this work, we would want some of the components that you removed, e.g., author for ValueSet, right?

Can you say more about the guardrails you're imagining we'd need to implement elsewhere? I might be missing some of the context for this.

@robertandremitchell
Copy link
Collaborator Author

My preference would be to re-use the same constants we already have, especially the ones for Concept and ValueSet, to ensure consistency across all the different work. It looks like the ones you created are pared down versions of the existing ones (and did not add any additional components), but I imagine that once we build out the front end of this work, we would want some of the components that you removed, e.g., author for ValueSet, right?

Can you say more about the guardrails you're imagining we'd need to implement elsewhere? I might be missing some of the context for this.

Hm, so I think using all the fields like author means we would have to preserve them in some way in the query table, which we currently don't. I think I envisioned using the different data we save about saved queries (valueset_id and concept_id) to link back to their respective tables to then display the name, author, etc. so at that point, you would resume using ValueSet and Concept.

I think the main thing i was worried about was making a bunch of the fields in ValueSet and Concept optional, such that when we get to the point where users are uploading their own ValueSet and Concept we will have to add checks to make sure they those fields we don't need here like name/author and make them a requirement in the upload/UI. Maybe that's fine, since we'll need to build in error messaging for bad uploads/updates of user-generated condition/valuesets/concepts anyway.

So, 🤷🏽 i think I'm good to just update the existing if that's preferred.

@m-goggins
Copy link
Collaborator

Why would we have to preserve them in the query table?

I think the main thing i was worried about was making a bunch of the fields in ValueSet and Concept optional, such that when we get to the point where users are uploading their own ValueSet and Concept we will have to add checks to make sure they those fields we don't need here like name/author and make them a requirement in the upload/UI.

I thought we would want to keep the fields as required and add the checks since those fields make up a ValueSet and lend credibility to them when used, e.g., a valueset from CSTE is more trustworthy than a valueset from DIBBs.

@robertandremitchell
Copy link
Collaborator Author

Why would we have to preserve them in the query table?

I think the main thing i was worried about was making a bunch of the fields in ValueSet and Concept optional, such that when we get to the point where users are uploading their own ValueSet and Concept we will have to add checks to make sure they those fields we don't need here like name/author and make them a requirement in the upload/UI.

I thought we would want to keep the fields as required and add the checks since those fields make up a ValueSet and lend credibility to them when used, e.g., a valueset from CSTE is more trustworthy than a valueset from DIBBs.

Because there are more required fields in Concept / ValueSet etc that we don't preserve when using those interfaces into the query table. I think the only way around that is to make fields like author optional, but i could be wrong. happy to look again.

@m-goggins
Copy link
Collaborator

I am confused haha. Want to discuss in parking lot or jump on a call?

@robertandremitchell
Copy link
Collaborator Author

I am confused haha. Want to discuss in parking lot or jump on a call?

yeah, happy to chat about it. I pushed a change where the error generated on lines 660 onward try to demonstrate the issue with just using ValueSet/Concept without further change. But free most of the day to chat through it if there's a better way forward.

@robertandremitchell robertandremitchell marked this pull request as draft November 14, 2024 16:02
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.

Build out "My Queries" backend function
2 participants