build: conversiontype required #3884
Conversation
Part of eng-231 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
WalkthroughThis change enforces that the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Gateway
participant Service
Client->>API Gateway: POST /patient/:id/consolidated/query?conversionType=json
API Gateway->>Service: Validate required conversionType (json/html/pdf)
Service-->>API Gateway: Processed consolidated data (format: conversionType)
API Gateway-->>Client: Response with data or signed URL
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
🧬 Code Graph Analysis (1)packages/core/src/command/consolidated/consolidated-get.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (14)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -496,7 +496,7 @@ export class MetriportMedicalApi { | |||
| resources?: readonly ResourceTypeForConsolidation[], | |||
| dateFrom?: string, | |||
| dateTo?: string, | |||
| conversionType?: string, | |||
| conversionType = "json", | |||
There was a problem hiding this comment.
Reason im setting a default value here is because if i make this required without one then it needs to go above the optional params due to our linter. To avoid the breaking change on the sdk for all cxs since this isnt an object will just make the conversionType default to json so the API will always receive a conversionType even if they dont explictly set one.
There was a problem hiding this comment.
Sounds good. Let's just update the TSDoc, please? It's saying defaults to JSON in the body of the WH request.
Part of eng-231 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/_snippets/consolidated-query.mdx(1 hunks)fern/definition/medical/fhir.yml(3 hunks)packages/api-sdk/src/medical/client/metriport.ts(1 hunks)packages/api-sdk/src/medical/models/fhir.ts(1 hunks)packages/api/src/command/medical/patient/__tests__/consolidated-get.test.ts(2 hunks)packages/api/src/command/medical/patient/__tests__/store-query-cmd.ts(2 hunks)packages/api/src/command/medical/patient/consolidated-get.ts(2 hunks)packages/api/src/routes/internal/medical/patient.ts(2 hunks)packages/api/src/routes/medical/patient.ts(2 hunks)packages/core/src/command/consolidated/get-snapshot.ts(1 hunks)packages/shared/src/interface/internal/consolidated.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/api/src/command/medical/patient/__tests__/consolidated-get.test.tspackages/api/src/command/medical/patient/__tests__/store-query-cmd.tspackages/core/src/command/consolidated/get-snapshot.tspackages/api/src/routes/medical/patient.tspackages/api-sdk/src/medical/models/fhir.tspackages/shared/src/interface/internal/consolidated.tspackages/api/src/routes/internal/medical/patient.tspackages/api-sdk/src/medical/client/metriport.tspackages/api/src/command/medical/patient/consolidated-get.ts
🧠 Learnings (2)
packages/api/src/routes/medical/patient.ts (1)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
packages/api/src/routes/internal/medical/patient.ts (1)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
🧬 Code Graph Analysis (4)
packages/api/src/command/medical/patient/__tests__/consolidated-get.test.ts (1)
packages/api/src/command/medical/patient/consolidated-get.ts (1)
getCurrentConsolidatedProgress(169-189)
packages/core/src/command/consolidated/get-snapshot.ts (1)
packages/api-sdk/src/medical/models/fhir.ts (1)
ConsolidationConversionType(21-21)
packages/shared/src/interface/internal/consolidated.ts (1)
packages/api-sdk/src/medical/models/fhir.ts (1)
consolidationConversionType(20-20)
packages/api/src/command/medical/patient/consolidated-get.ts (1)
packages/api-sdk/src/medical/models/fhir.ts (1)
ConsolidationConversionType(21-21)
🪛 GitHub Actions: Fern Check
fern/definition/medical/fhir.yml
[error] 1-1: Unexpected path parameter "conversionType" in service -> endpoints -> startConsolidatedQuery -> examples[0] -> path-parameters.
[error] 1-1: Example is missing required query parameter "conversionType" in service -> endpoints -> startConsolidatedQuery -> examples[0] -> query-parameters.
🔇 Additional comments (17)
docs/_snippets/consolidated-query.mdx (1)
21-23: Field correctly marked as required.The documentation has been updated to indicate that
conversionTypeis now a required field, which aligns with the PR goal of making this parameter mandatory across the codebase.packages/api-sdk/src/medical/models/fhir.ts (1)
27-27: Schema updated to make conversionType required.The change correctly removes optionality from the
conversionTypefield in the validation schema, properly enforcing it as a required parameter with values constrained to the allowed enum types.packages/shared/src/interface/internal/consolidated.ts (1)
8-8: Internal schema updated to require conversionType.This change properly aligns the internal schema validation with the PR objective of making
conversionTypea required field, ensuring consistency across different parts of the application.packages/core/src/command/consolidated/get-snapshot.ts (1)
17-17: Type definition updated to require conversionType.The type definition for
ConsolidatedSnapshotRequestAsyncnow correctly marksconversionTypeas a required property, maintaining type safety and consistency with the other changes in this PR.packages/api/src/command/medical/patient/__tests__/store-query-cmd.ts (2)
38-38: Added requiredconversionTypefield to test dataThe addition of the
"json"conversion type to the test data is consistent with making this field required. This matches the schema changes in the PR that make this field mandatory across the codebase.
61-61: Added default value forconversionTypein factory functionGood implementation of a fallback value with the nullish coalescing operator, following the coding guidelines for providing default values. This ensures backwards compatibility while making the field effectively required.
packages/api/src/command/medical/patient/__tests__/consolidated-get.test.ts (2)
33-35: Updated test to include requiredconversionTypeparameterThis test update correctly reflects the new requirement for
conversionTypeto be explicitly specified. The change maintains test coverage while adapting to the updated function signature.
72-74: Added requiredconversionTypefield to test parametersThis change properly updates the test to include the now-required
conversionTypeparameter, keeping test coverage intact while adapting to the updated API contract.packages/api/src/routes/internal/medical/patient.ts (2)
673-674: Updated JSDoc to documentconversionTypeas requiredThe JSDoc update correctly indicates that the
conversionTypeparameter is now required, and clarifies the accepted values include"json"along with"pdf"and"html". This provides clear documentation for API users.
687-688: Enforced requiredconversionTypeparameterThe implementation now properly requires the
conversionTypeparameter by usingorFail()instead ofoptional(). This change enforces the new contract consistently with the documentation.fern/definition/medical/fhir.yml (2)
12-20: Reformatted API documentation for clarityThe documentation updates maintain the same meaning while improving readability through better line breaks and formatting.
36-39: MadeconversionTypea required query parameterThe API specification now correctly defines
conversionTypeas a required query parameter, adding proper documentation that explains it accepts"json","html", or"pdf".packages/api/src/routes/medical/patient.ts (2)
291-293: Documentation properly updated to reflect required parameterThe JSDoc comment has been correctly updated to indicate that
conversionTypeis now a required parameter, with clear documentation about the accepted values and the webhook payload behavior.
310-313: Implementation correctly enforces required parameterThe implementation has been updated to use
orFailinstead ofoptionalwhen retrieving theconversionTypeparameter, ensuring it's always provided. The value is then correctly validated using the schema.packages/api-sdk/src/medical/client/metriport.ts (1)
499-499: Good approach to backward compatibilitySetting a default value of "json" for
conversionTypeis a smart way to make the parameter required without breaking existing client code. This ensures the API always receives a valid value even if clients don't explicitly set one.packages/api/src/command/medical/patient/consolidated-get.ts (2)
277-277: Simplified condition assuming required parameterThe condition has been simplified by removing the redundant existence check for
conversionType, which is appropriate since this parameter is now required throughout the codebase.
446-446: Function signature updated to reflect required parameterThe function signature now correctly indicates that
conversionTypeis a required parameter, aligning with other changes in the codebase that make this field mandatory.
Part of eng-231 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
|
Forgot to mention, but when we change docs it's a good idea to put some screenshots to help reviewers to see how it renders w/o having to run it locally. |
|
|
Part of eng-231 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
Ref: #000 - api@1.27.4-alpha.0 - @metriport/api-sdk@14.14.4-alpha.0 - @metriport/carequality-cert-runner@1.18.4-alpha.0 - @metriport/carequality-sdk@1.6.4-alpha.0 - @metriport/commonwell-cert-runner@1.26.4-alpha.0 - @metriport/commonwell-jwt-maker@1.24.4-alpha.0 - @metriport/commonwell-sdk@5.9.2-alpha.0 - @metriport/core@1.24.4-alpha.0 - @metriport/ihe-gateway-sdk@0.19.4-alpha.0 - infrastructure@1.22.4-alpha.0 - mllp-server@0.3.4-alpha.0 - @metriport/shared@0.23.4-alpha.0 - utils@1.25.4-alpha.0 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
Ref: #000 - api@1.27.5-alpha.0 - @metriport/api-sdk@14.14.5-alpha.0 - @metriport/carequality-cert-runner@1.18.5-alpha.0 - @metriport/carequality-sdk@1.6.5-alpha.0 - @metriport/commonwell-cert-runner@1.26.5-alpha.0 - @metriport/commonwell-jwt-maker@1.24.5-alpha.0 - @metriport/commonwell-sdk@5.9.3-alpha.0 - @metriport/core@1.24.5-alpha.0 - @metriport/ihe-gateway-sdk@0.19.5-alpha.0 - infrastructure@1.22.5-alpha.0 - mllp-server@0.3.5-alpha.0 - @metriport/shared@0.23.5-alpha.0 - utils@1.25.5-alpha.0 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
packages/api-sdk/package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@metriport/api-sdk", | |||
| "version": "14.14.3", | |||
| "version": "14.14.5-alpha.0", | |||
There was a problem hiding this comment.
I think this should be major, it's a breaking change since one can't trigger consolidated w/o conversion type.
| <ResponseField name="conversionType" type="string" required> | ||
| The format of the resulting data. Can be one of `pdf`, `html`, or `json`. | ||
| </ResponseField> |
There was a problem hiding this comment.
Wouldn't be better to move this upwards close to the other required fields?
Part of eng-231 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
Ref: #000 - @metriport/api-sdk@15.0.0-alpha.0 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
Part of eng-231 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
Part of eng-231 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
Part of eng-231
Issues:
Dependencies
Description
Testing
Check each PR.
Release Plan
Will create the alpha packages once approved as to not add more files to the review process
Summary by CodeRabbit
New Features
conversionTypeparameter is now required when requesting consolidated patient data and supports"json","html", or"pdf"formats.Bug Fixes
conversionTypefield across all relevant endpoints, request schemas, and internal logic.Documentation
conversionTypeparameter and its supported values.Tests
conversionTypeparameter.conversionType.