Closed robertandremitchell closed 5 days ago
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.
My preference would be to re-use the same constants we already have, especially the ones for
Concept
andValueSet
, 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
forValueSet
, 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.
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.
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.
I am confused haha. Want to discuss in parking lot or jump on a call?
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.
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 utilize ValueSet/Concept interfaces specific to this work. I had initially started by adding news ones in query-building.ts
to limit to just fields we needed to display in
My Queries` page, then when a create is selected to edit, it would call in all the valueset/concepts needed to present that page in the edit-conditions page. However, after discussing with Marcelle, we thought it made sense to use the full, existing ValueSet and Concept data to avoid duplication and ensure the click to edit button moves faster since all data will be present.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 include aquery_data
JSONB 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 and assumes all valuesets in a query will be included.Related Issue
Fixes #117
Additional Information
Anything else the review team should know?
Checklist