Feat(admin): in house labs config#6777
Conversation
abraun-ml
force-pushed
the
abraun/otr-2051-ehr-admin-in-house-labs-implementation
branch
2 times, most recently
from
ec6e03a to
160e05a
Compare
There was a problem hiding this comment.
Pull request overview
This is a large feature PR implementing admin configuration management for in-house laboratory tests. It adds backend zambdas for CRUD operations on in-house lab test definitions, refactors type naming for clarity between data entry and admin contexts, and provides a comprehensive frontend admin UI for managing test configurations. The PR includes proper type definitions, validation schemas using Zod, conversion functions between ActivityDefinition (FHIR format) and AdminInHouseLabItemDefinition (admin format), test coverage, and supporting UI components.
Changes:
- Added 4 new admin zambdas (list, add, get config, update) for managing in-house lab test definitions
- Refactored type system: renamed
TestItem→DataEntryTestItem,TestItemComponent→DataEntryComponentwith specific subtypes - Implemented bidirectional conversion functions between ActivityDefinition and AdminInHouseLabItemDefinition formats
- Created comprehensive admin UI with form components for test configuration, including CPT code selection, test components, and validation
- Added Zod validation schema for admin input validation
- Updated all references throughout codebase to use new renamed types
- Added test data and unit tests for conversion functions
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/zambdas/test/unit/in-house-lab.test.ts | Unit tests for bidirectional conversion between ActivityDefinition and AdminInHouseLabItemDefinition |
| packages/zambdas/test/data/in-house-lab-admin-test-config.ts | Test data with 6 test configurations covering different component types and features |
| packages/zambdas/src/ehr/lab/shared/in-house-labs.ts | Core conversion and helper functions for admin lab management |
| packages/zambdas/src/ehr/lab/in-house/admin-*/ | Four new zambdas for admin operations (list, add, get config, update) |
| packages/utils/lib/types/data/in-house/ | Type definitions and Zod schemas for admin in-house lab configuration |
| apps/ehr/src/features/visits/telemed/components/admin/in-house-labs/ | Complete admin UI implementation with forms, lists, and detail pages |
| apps/ehr/src/api/api.ts | API wrapper functions for the new admin zambdas |
| Various feature files | Updated imports/types to use new DataEntryTestItem and DataEntryComponent names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return { ...radioResultMap, ...tableResultMap }; | ||
| if (testDetails.labDetails.components.type !== 'empty') { | ||
| return testDetails.labDetails.components.components.reduce((acc: any, item) => { |
There was a problem hiding this comment.
Can components just be an array of DataEntryComponent ? and we can just include the type on each DataEntryComponent?
There was a problem hiding this comment.
Think my aversion here is mostly just naming though, components.components
There was a problem hiding this comment.
We caught up over huddle but just capturing here -- it was overly ambitious to attempt this refactor in this PR, so I am going to take all the comments about the data entry components and related and address them in a separate PR for better clarity
|
|
||
| interface ResultEntryFreeTextProps { | ||
| testItemComponent: TestItemComponent; | ||
| testItemComponent: DataEntryComponent; |
There was a problem hiding this comment.
Probably makes sense to rename testItemComponent too so things are a little more clear.
| | { | ||
| type: 'empty'; | ||
| components: undefined; | ||
| }; |
There was a problem hiding this comment.
maybe im just missing the need for it but i feel like this could be simplifier further by removing
{
type: 'empty';
components: undefined;
}
and mapping undefined to components if there are none (do we really have any tests where there actually are none?)
There was a problem hiding this comment.
I had a reason when I added this and now I don't remember why. But I will address this in the subsequent PR
| .filter((roleId) => roleId !== undefined); | ||
| }; | ||
|
|
||
| export const checkUserHasProvidedRoles = async ( |
There was a problem hiding this comment.
If you want to do zambda-level role enforcement, then it should be done in roles.json. make some roles not have access to the zambda function, then give explicit access to the zambda function to the roles that should have the access.
There was a problem hiding this comment.
Per our convo I am going to take out all the places where I do this and let these zambdas be wild west perms wise for this PR, and then in a separate PR will implement the correct role validation through roles.json for these and the other zambdas
alexwillingham
left a comment
alexwillingham
left a comment
There was a problem hiding this comment.
Looks generally very good. I left some notes. Re-request my review when you have responded / addressed them please and I will give the approval.
abraun-ml
force-pushed
the
abraun/otr-2051-ehr-admin-in-house-labs-implementation
branch
from
b3099a8 to
03edcb9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (item.result?.entry) acc[item.observationDefinitionId] = item.result.entry; | ||
| return acc; | ||
| }, {}); | ||
| } |
There was a problem hiding this comment.
When testDetails.labDetails.components.type === 'empty', the useMemo returns undefined implicitly. However, the old code would have returned an empty object {} when there were no radio or grouped components. This behavioral change could cause the form to fail to initialize correctly. The function should explicitly return {} when type is 'empty' to match the previous behavior.
| } | |
| } | |
| return {}; |
There was a problem hiding this comment.
Will handle in a separate PR
|
@alexwillingham the three commits after a9fb1ce "pr feedback" address the sorting bug you found, a tag bug I found, and an editing edge case to prevent editing old versions. Regarding the IaC disabling bug you found (where the disabled IaC managed AD disappears from the list)
|
i think this ok |
Design doc: https://docs.google.com/document/d/1YAwU_-oHrvD8JXz3wY4SztofKbEqb-ZbsK7eYuygBBQ/edit?tab=t.0#heading=h.x6a041rpf36e
Sorry this is a monster. You can roughly review by commit to help break it up.
One thing to note for future improvements: there are types that are 85% the same between the form we render in house lab test results in and the admin meta forms. It was too much of an overhaul for this already very tiny PR, so there is some duplication in the meantime.