feat(api): discharge requery behind a ff #4101
Conversation
Part of ENG-539 Signed-off-by: Ramil Garipov <ramil@metriport.com>
WalkthroughA new feature flag, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FeatureFlags
participant JobCreator
Caller->>FeatureFlags: isDischargeRequeryFeatureFlagEnabledForCx(cxId)
alt Feature flag enabled
FeatureFlags-->>Caller: true
Caller->>JobCreator: createDischargeRequeryJob(props)
JobCreator->>JobCreator: Cancel existing waiting jobs
JobCreator->>JobCreator: Schedule new discharge requery job
JobCreator-->>Caller: void
else Feature flag disabled
FeatureFlags-->>Caller: false
Caller-->>JobCreator: (No job creation)
end
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 error code ERR_SSL_WRONG_VERSION_NUMBER ✨ 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 (
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.ts
Did you find this useful? React with a 👍 or 👎 |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.ts (1)
24-36: Update function documentation to reflect new behavior.The JSDoc comment still mentions "returns The newly created patient job" but the function now returns
void. Update the documentation to reflect the current behavior./** * Creates a new discharge requery job for a patient. * * The job is scheduled to run at a later time based on the number of remaining attempts and a scheduling map. * * If there is already a waiting job for the patient, it will be cancelled and a new one will be created, * with the remaining attempts set to the largest of the two jobs, and the scheduledAt set to the earliest of the two. * This ensures that the job will be run as soon as possible, but not more than once at a time, while also * preserving a paper trail for the previous job. * + * Note: Job creation is gated by the discharge requery feature flag. If the feature flag is disabled + * for the customer, no job will be created. * * @param props - The parameters for the new discharge requery job. - * @returns The newly created patient job. + * @returns void */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.ts(3 hunks)packages/core/src/command/feature-flags/domain-ffs.ts(1 hunks)packages/core/src/command/feature-flags/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/core/src/command/feature-flags/types.tspackages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.tspackages/core/src/command/feature-flags/domain-ffs.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - 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
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/core/src/command/feature-flags/types.tspackages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.tspackages/core/src/command/feature-flags/domain-ffs.ts
🧬 Code Graph Analysis (1)
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.ts (1)
packages/core/src/command/feature-flags/domain-ffs.ts (1)
isDischargeRequeryFeatureFlagEnabledForCx(258-261)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/core/src/command/feature-flags/types.ts (1)
39-39: LGTM! Feature flag addition follows established pattern.The new feature flag is properly added to the schema and follows the same naming convention and type structure as other customer-based feature flags.
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.ts (3)
1-1: LGTM! Proper import for feature flag utility.The import correctly brings in the feature flag checking function from the core domain module.
44-45: LGTM! Proper feature flag gating implementation.The early return pattern correctly implements the feature flag check, ensuring no job is created when the flag is disabled for the customer.
100-100: LGTM! Return statement matches new function signature.The return statement correctly reflects the new
voidreturn type.packages/core/src/command/feature-flags/domain-ffs.ts (1)
255-261: LGTM! Feature flag functions follow established pattern.Both functions correctly implement the standard pattern used throughout this file:
getCxsWithDischargeRequeryFeatureFlagretrieves the list of enabled customer IDsisDischargeRequeryFeatureFlagEnabledForCxchecks membership for a specific customer IDThe implementation is consistent with existing feature flag functions and follows the established coding conventions.
| @@ -37,10 +37,12 @@ export const dischargeRequeryJobType = "discharge-requery"; | |||
| */ | |||
| export async function createDischargeRequeryJob( | |||
| props: CreateDischargeRequeryParams | |||
| ): Promise<PatientJob> { | |||
| ): Promise<void> { | |||
There was a problem hiding this comment.
🧩 Verification correct
Breaking change: Verify impact on function callers.
The return type changed from Promise<PatientJob> to Promise<void>, which is a breaking change. Ensure all callers of this function have been updated to handle the new return type.
🏁 Script executed:
#!/bin/bash
# Description: Find all callers of createDischargeRequeryJob to verify impact
# Expected: All callers should be updated to handle void return type
# Search for function calls to createDischargeRequeryJob
rg -A 5 -B 2 "createDischargeRequeryJob\(" --type tsLength of output: 5995
Breaking change: Verify impact on function callers.
The return type changed from Promise<PatientJob> to Promise<void>, which is a breaking change. Ensure all callers of this function have been updated to handle the new return type.
#!/bin/bash
# Description: Find all callers of createDischargeRequeryJob to verify impact
# Expected: All callers should be updated to handle void return type
# Search for function calls to createDischargeRequeryJob
rg -A 5 -B 2 "createDischargeRequeryJob\(" --type ts🤖 Prompt for AI Agents
In
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/create.ts
at line 40, the function's return type was changed from Promise<PatientJob> to
Promise<void>, which is a breaking change. You need to search the entire
codebase for all callers of this function and verify that they have been updated
to handle the new void return type properly. Use the suggested ripgrep command
or equivalent to find all call sites and review their handling of the function's
result, updating them as necessary to avoid runtime errors or type mismatches.
Part of ENG-539 Signed-off-by: Ramil Garipov <ramil@metriport.com>
Part of ENG-539 Signed-off-by: Ramil Garipov <ramil@metriport.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/__tests__/create.test.ts (1)
49-69: Add test coverage for disabled feature flag scenario.All current tests assume the feature flag is enabled (
mockResolvedValue(true)), but there's no test case for when the feature flag is disabled. Based on the PR objectives, when the FF is disabled, no job should be created.Add a test case to verify the behavior when the feature flag is disabled:
+ it("should not create a job when feature flag is disabled", async () => { + isFeatureFlagEnabledMock.mockResolvedValue(false); + getPatientJobMock.mockResolvedValue([]); + + await createDischargeRequeryJob({ + cxId, + patientId, + }); + + expect(isFeatureFlagEnabledMock).toHaveBeenCalledWith(cxId); + expect(createPatientJobMock).not.toHaveBeenCalled(); + expect(getPatientJobMock).not.toHaveBeenCalled(); + expect(cancelPatientJobMock).not.toHaveBeenCalled(); + });Additionally, consider adding assertions to existing tests to verify the feature flag is called with the correct
cxId:expect(createPatientJobMock).toHaveBeenCalledWith( expect.objectContaining({ cxId, patientId, jobType: "discharge-requery", scheduledAt: dayjs(mockDate).add(5, "minutes").toDate(), paramsOps: { remainingAttempts: defaultRemainingAttempts, }, }) ); + expect(isFeatureFlagEnabledMock).toHaveBeenCalledWith(cxId);Also applies to: 71-101, 103-133
🧹 Nitpick comments (1)
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/__tests__/create.test.ts (1)
51-51: Consider setting a default mock behavior in beforeEach.Each test manually sets
isFeatureFlagEnabledMock.mockResolvedValue(true). Consider setting this as the default behavior inbeforeEachto avoid repetition and ensure consistent test setup.isFeatureFlagEnabledMock = jest.spyOn( domainFfsModule, "isDischargeRequeryFeatureFlagEnabledForCx" ); + isFeatureFlagEnabledMock.mockResolvedValue(true); // Default behaviorThen remove the individual
mockResolvedValue(true)calls from tests that expect the default behavior, keeping them only where you need different behavior (like the disabled feature flag test).Also applies to: 72-72, 104-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/__tests__/create.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*`: Use eslint to enforce code style Use prettier to format code Max column ...
**/*: Use eslint to enforce code style
Use prettier to format code
Max column length is 100 chars
Multi-line comments use /** */
Top-level comments go after the import (save pre-import to basic file header, like license)
Move literals to constants declared after imports when possible
File names: kebab-case
📄 Source: CodeRabbit Inference Engine (.cursorrules)
List of files the instruction was applied to:
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/__tests__/create.test.ts
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - 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
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/__tests__/create.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/api/src/command/medical/patient/patient-monitoring/discharge-requery/__tests__/create.test.ts (1)
2-2: LGTM: Feature flag mock setup follows existing patterns.The import and mock setup for the feature flag functionality is well-structured and consistent with the existing test patterns.
Also applies to: 15-15, 36-40
Part of ENG-539
Issues:
Description
Testing
Release Plan
Summary by CodeRabbit
New Features
Refactor