Closed robertandremitchell closed 1 month ago
Looks fine to me as is, though I would probably be in the camp of just using INSERT statements to make that temporary table. I have no problem exposing this file publicly, but we probably aren't looking to do so. Mounting and S3 both seem like overkill to me, but that's more a stylistic and preference thing, I think--I would rather our DB product not establish the pattern of bringing in externally mounted volumes. I think the seeding/creation should be as self-contained as possible. I don't feel strongly either way, and I don't think anything here is blocking though!
I'm torn, I think I lean toward a bunch of INSERT
statements also being fine. I think I'd probably want to add a script that can parse this CSV to write those INSERT
s each time a new eRSD drops for some future-proofing (or, preferably, convince APHL to surface this data in the eRSD...).
happy to look into that option if we think it's cleaner.
@bamader definitely agree that ultimately all database creation should be handled in a single place (the work you and @m-goggins are doing). I totally forgot about this when reviewing. I'd propose that we move this along so that @robertandremitchell and @fzhao99 aren't blocked on their query building work, but eventually have this along with all other seeding handled at the app layer.
@DanPaseltiner That sounds good to me--approving with the idea that we revisit the conversation during broader DB creation!
@bamader definitely agree that ultimately all database creation should be handled in a single place (the work you and @m-goggins are doing). I totally forgot about this when reviewing. I'd propose that we move this along so that @robertandremitchell and @fzhao99 aren't blocked on their query building work, but eventually have this along with all other seeding handled at the app layer.
Makes sense to me. I've created a new issue in the backlog so that we don't lose track of that work (https://linear.app/skylight-cdc/issue/QUE-43/refactor-conditionscategory-migration), but I will work on getting this merged shortly (darn CI tests).
Thank you both for your review!
requesting a re-review since switching to INSERT
over CSV. everything else should be the same.
PULL REQUEST
Summary
This adds a new column to the
conditions
table,category
. This is an organizational structure that we need to organize our conditions to display like so: https://www.figma.com/design/Iuw9me6kYftBF4WTCEsCZz/Query-Connector?node-id=475-18708&node-type=frame&t=Lcg6TdahnUvYPBsR-0These are the categories we would end up with:
the three missing are ones we added. Here are my thoughts on how to handle:
Cancer (Leukemia)
, which, probably fairly reasonably, could be coded to coded to be included in theCancer
category.Birth Defects and Infant Disorders
? I don't see an immediately obvious one we would nest Newborn Screening under, otherwise. I can see an argument for it being standalone.a few backend considerations: right now, we are just using INSERT statements to create the temporary
category_data
table that needs to be merged withconditions
.Other ideas:
Related Issue
Fixes #68
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