Eng 433 scheduler for patient job #3997
Conversation
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MacBook-Pro.local>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MacBook-Pro.local>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MacBook-Pro.local>
Ref: ENG-281 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MacBook-Pro.local>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MacBook-Pro.local>
WalkthroughThis update introduces a comprehensive patient job scheduling and execution framework. It adds new fields to the patient job model, enables job scheduling, cancellation, and failure, and provides infrastructure for running jobs via SQS and Lambda. The API surface is expanded with endpoints for job management, and supporting types, handlers, and infrastructure stacks are implemented. Changes
Sequence Diagram(s)Patient Job Scheduling and Execution FlowsequenceDiagram
participant Client
participant API
participant DB as PatientJob DB
participant SQS as SQS Queue
participant Lambda as RunJob Lambda
Client->>API: POST /internal/patient/job (create job with runUrl, scheduledAt)
API->>DB: Insert patient job (with runUrl, scheduledAt)
alt scheduledAt is not set (immediate run)
API->>SQS: Enqueue job (id, cxId, runUrl)
end
SQS->>Lambda: Trigger event (job message)
Lambda->>API: POST runUrl (with jobId, cxId)
API->>DB: Update job status (processing/completed/failed)
Job Status Transition (Cancel/Fail)sequenceDiagram
participant Client
participant API
participant DB as PatientJob DB
Client->>API: POST /internal/patient/job/:jobId/cancel or /fail
API->>DB: Update job status (cancelled or failed), set cancelledAt/failedAt
API-->>Client: 200 OK (updated job)
Scheduled Job RunnersequenceDiagram
participant Scheduler
participant API
participant DB as PatientJob DB
participant SQS as SQS Queue
Scheduler->>API: POST /internal/patient/job/scheduler/start
API->>DB: Query jobs (status=waiting, scheduledBefore=now)
loop For each job with runUrl
API->>SQS: Enqueue job (id, cxId, runUrl)
end
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 error code ERR_SSL_WRONG_VERSION_NUMBER 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ 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 (
|
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
… eng-433-scheduler-for-patient-job Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-435 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 23
🔭 Outside diff range comments (3)
packages/core/src/command/job/patient/api/set-entry-status.ts (1)
31-38: 🛠️ Refactor suggestionReduce repetition – extract a tiny helper
completeJob,initializeJob,updateJobTotal, andsetJobEntryStatusall duplicate:
axios.create({ baseURL })- retry wrapper
logAxiosResponseA shared utility such as
postInternalJobEndpoint({ path, params })would cut ~30 lines per file and ensure consistent error handling.packages/api/src/command/job/patient/update/update-total.ts (1)
22-29:⚠️ Potential issueMissing guard against updating a running job – spec/doc mismatch
The JSDoc advertises:
“throws BadRequestError if the job total is already set and the job is running.”
The implementation only validates
total < 1; it never checks whether
a)job.totalis already set, or
b) the job is currently running.That means an in-flight job can have its counters silently reset, breaking every consumer that relies on the invariants
(successful + failed ≤ total).Suggested minimal fix:
@@ -const jobModel = await getPatientJobModelOrFail({ jobId, cxId }); +const jobModel = await getPatientJobModelOrFail({ jobId, cxId }); +const job = jobModel.dataValues; + +// Prevent mutating totals for jobs that are already in progress or completed +if (job.total !== null && ["running", "queued"].includes(job.status)) { + throw new BadRequestError( + `Cannot update total for a job in status ${job.status}` + ); +}Optionally refuse updates when
job.totalis already > 0 regardless of status if that better matches business rules.packages/shared/src/domain/job/job-status.ts (1)
18-20: 🛠️ Refactor suggestion
isJobDoneomitscancelledIn many systems a cancelled job is considered terminal.
If callers rely onisJobDone, they may endlessly poll a cancelled job.-export function isJobDone(status: JobStatus): boolean { - return status === "completed" || status === "failed"; +export function isJobDone(status: JobStatus): boolean { + return status === "completed" || status === "failed" || status === "cancelled"; }
♻️ Duplicate comments (2)
packages/core/src/command/job/patient/api/initialize-job.ts (1)
17-24: Same query-param construction caveat ascompleteJobReplicate the Axios
paramsrefactor here to stay consistent and avoid divergent URL building logic.packages/core/src/command/job/patient/api/update-job-total.ts (1)
21-29: Consistent param handling & logging concerns (see prior comments)Same points as in
completeJob/initializeJob:
- Replace manual query construction with Axios
params.- Re-evaluate logging of full
response.datafor PHI.
🧹 Nitpick comments (25)
packages/core/src/command/job/patient/api/complete-job.ts (1)
17-24: Avoid manual query-string assembly – leverage Axiosparamsfor correctness & readabilityManually concatenating
?${queryParams.toString()}works but is easy to break (e.g., double?, forgotten encoding, trailing&). Axios already handles this via itsparamsoption:-const queryParams = new URLSearchParams({ cxId }); -const completeJobUrl = `/internal/patient/job/${jobId}/complete?${queryParams.toString()}`; -... -const response = await executeWithNetworkRetries(async () => { - return api.post(completeJobUrl); -}); +const completeJobUrl = `/internal/patient/job/${jobId}/complete`; +... +const response = await executeWithNetworkRetries(async () => { + return api.post(completeJobUrl, undefined, { params: { cxId } }); +});Benefits: centralised encoding, reduced string-bashing, clearer intent.
packages/api/src/command/job/patient/status/initialize.ts (1)
24-34: Minor JSDoc mismatch – parameter nameThe code uses
cxId, but the docstring line still says “customer ID”. Consider clarifying (cxId - The customer (CX) ID). Small but keeps docs accurate.packages/api/src/routes/internal/medical/patient-jobs/index.ts (1)
6-8: Router export is fine – consider using a named export for consistencyMost route modules in the code-base appear to use
export defaultbut a few expose a named constant. Mixing the two styles can bite during refactors (e.g., when converting to ESM or re-exporting barrels).
If there is no established convention yet, consider switching to-const router = Router(); -... -export default router; +export const router = Router(); +...and importing it with
import { router as patientJobsRouter } from "./ehr"to keep everything explicit.
No functional changes required – this is purely a consistency nit.packages/api/src/shared/config.ts (1)
122-125: Wrapper now just forwards to core – consider de-duplicating sooner
getDBCredsis now a pure pass-through toCoreConfig.getDBCreds().
If nothing insideapicalls the core implementation directly, we are keeping two public APIs that must stay in sync./** @deprecated Use core's version of Config instead */ static getDBCreds(): string { - return CoreConfig.getDBCreds(); + // TODO: remove once callers migrate to CoreConfig + return CoreConfig.getDBCreds(); }Follow-up:
- Search for
Config.getDBCreds(usages and migrate them toCoreConfig.- Delete this wrapper in a subsequent clean-up PR to avoid bit-rot.
packages/infra/lib/shared/secrets.ts (1)
16-18: Nit: keep helper naming consistent with existing API
buildSecretstakes a genericscope: Construct; the newbuildSecrettakesnestedStack: Construct.
For symmetry (and to avoid a mental context-switch for callers) prefer the same parameter name & doc comment:-export function buildSecret(nestedStack: Construct, name: string): secret.ISecret { - return secret.Secret.fromSecretNameV2(nestedStack, name, name); +/** + * Build a single secret reference (sugar over buildSecrets for one entry). + */ +export function buildSecret(scope: Construct, name: string): secret.ISecret { + return secret.Secret.fromSecretNameV2(scope, name, name); }No functional change, purely readability.
packages/core/src/command/job/patient/api/shared.ts (1)
1-5: ExportJobIdto maximise reuse
JobIdis defined but not exported. Several downstream files may need the exact literal type instead of re-derivingPatientJob["id"].-type JobId = PatientJob["id"]; +export type JobId = PatientJob["id"];This avoids duplicating the alias elsewhere and keeps typings DRY.
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-direct.ts (1)
4-12: Minor style nit – destructure the argument for readabilityDestructuring avoids the
request.prefix noise and matches the functional-style guideline in the TS coding guide.- async runJob(request: RunJobRequest): Promise<void> { - await runJob({ - jobId: request.id, - cxId: request.cxId, - jobType: request.jobType, - }); + async runJob({ id: jobId, cxId, jobType }: RunJobRequest): Promise<void> { + await runJob({ jobId, cxId, jobType }); }packages/api/src/command/job/patient/scheduler/start-jobs.ts (1)
14-25: Fire-and-forget pattern may terminate execution prematurely
startJobsreturns as soon as the handler promise is created, not when the jobs are actually fetched/run. In a Lambda or short-lived process this risks the runtime finishing before work completes (even though you attach.catch). Consider awaiting (or explicitly documenting that early return is intentional).-export async function startJobs({...}: StartJobsParams): Promise<void> { +export async function startJobs({...}: StartJobsParams): Promise<void> { const handler = buildGetJobsHandler(); - handler - .getJobs({ runDate, cxId, patientId, jobType, status }) - .catch(processAsyncError("startPatientJobs")); + await handler + .getJobs({ runDate, cxId, patientId, jobType, status }) + .catch(processAsyncError("startPatientJobs")); }If “fire-and-forget” is desired (e.g., for an API
202 Acceptedsemantics), mark it with a comment and drop the unnecessaryasynckeyword to avoid confusion.packages/core/src/util/config.ts (1)
35-38: DB credentials accessor returns opaque string – clarify expected format
getDBCreds()surfaces a rawDB_CREDSstring. Call-sites likely need individual fields (host, user, pwd). Consider returning a parsed object (or at least document the expected JSON / connection-string format) to avoid ad-hoc parsing in multiple places.packages/shared/src/domain/job/patient-job.ts (1)
24-24: Consider using a generic forruntimeDatafor safer downstream usage
runtimeData: unknownforces consumers to cast, masking type errors. A generic type parameter keeps flexibility while preserving type-safety:-export type PatientJob = { +export type PatientJob<TRuntimeData = unknown> = { ... - runtimeData: unknown; + runtimeData: TRuntimeData | undefined; }This change is non-breaking if defaulted to
unknown, but gives callers the option to specify a shape.packages/api/src/command/job/patient/update/update-runtime-data.ts (2)
5-14: Docstring is misleading & parameter names are out of sync
- The description still says “Updates a patient job's total.” – should be “runtime data.”
@param runtimeDatadoes not exist in the signature; the actual key isdata.Keeping docs accurate prevents downstream confusion.
- * Updates a patient job's total. + * Updates a patient job's runtime data. ... - * @param runtimeData - The runtime data to update. + * @param data - The runtime data payload that will replace the existing value.
20-25: Return plain object instead of Sequelize instance for cleaner typing
jobModel.update()returns a Sequelize instance; returningdataValuesleaks an internal detail and strips any virtuals/getters. PreferupdatedJob.get({ plain: true })(ortoJSON()on newer Sequelize) to keep a clean POJO and consistent serialization.-const updatedJob = await jobModel.update(fieldsToUpdate); -return updatedJob.dataValues; +const [updatedJob] = await jobModel.update(fieldsToUpdate, { returning: true }); +return updatedJob.get({ plain: true });packages/infra/lib/api-stack/api-service.ts (1)
458-459: Grant invoke permission also forrunPatientJobLambdaThe task can invoke
getPatientJobsLambdabut notrunPatientJobLambda. If direct invocation is ever required (e.g., retries, back-fill scripts) the call will fail withAccessDenied.+jobAssets.runPatientJobLambda.grantInvoke(fargateService.taskDefinition.taskRole);packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job.ts (1)
3-5: Consider freezingRunJobRequestto avoid accidental mutationSince the same object may propagate through multiple layers, wrapping the request in
Readonly<...>(or freezing) enforces immutability per guidelines.-export type RunJobRequest = Pick<PatientJob, "id" | "cxId" | "jobType"> & - Partial<Pick<PatientJob, "paramsCx" | "paramsOps" | "data">>; +export type RunJobRequest = Readonly< + Pick<PatientJob, "id" | "cxId" | "jobType"> & + Partial<Pick<PatientJob, "paramsCx" | "paramsOps" | "data">> +>;packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-cloud.ts (1)
9-12: Optional DI enhancement for testabilityInjecting
SQSClientvia the constructor is great. To make unit tests deterministic, also expose the JSON stringify step so tests can assert the exact payload without duplicating logic.-private readonly sqsClient: SQSClient; +private readonly sqsClient: SQSClient; +protected stringify = JSON.stringify; // overridable for testingpackages/api/src/command/job/patient/status/cancel.ts (2)
7-18: Misleading JSDoc header & param descriptionsThe block states “Fails a patient job” while the function purpose is cancellation. Param docs follow the same pattern.
Update to prevent future confusion and inaccurate generated docs.
26-26: Log prefix does not match the action
updateJobTrackingneither conveys “cancel” nor matches the function name.
Consider a clearer prefix, e.g.cancelPatientJob.packages/core/src/command/job/patient/api/run-job.ts (1)
12-18: Comment header still says “complete”The function now runs a job, not completes it.
Please update comment & tag names to avoid misleading IntelliSense.packages/api/src/command/job/patient/status/fail.ts (2)
7-18: Docstrings copied from cancel handlerThe header says Cancels and params mention “cancelling”.
Please align wording with failure semantics.
26-26: Same log prefix issue as cancel handler
updateJobTrackingis generic—considerfailPatientJobfor traceability.packages/shared/src/domain/job/job-status.ts (1)
70-77: Status transition rule too restrictiveThe restriction
waiting → cancelleddisallows cancelling jobs stuck inprocessing.
If an operator needs to abort a long-running job this path won’t work.
Consider permittingprocessing → cancelledwhenforceStatusUpdateis true.packages/api/src/routes/internal/medical/patient-jobs/ehr.ts (2)
18-25: JSDoc path & parameter description don’t match actual routeThe JSDoc advertises
POST /internal/medical/patient-job/ehr/...and saysreq.query.jobId, but the implementation registers
/athenahealth-create-resource-diff-bundles/:jobId/run(and the Canvas variant) and extractsjobIdfromreq.params.Keep the documentation in sync or future maintainers will waste time debugging the “wrong” URL/param.
- * POST /internal/medical/patient-job/ehr/athenahealth-create-resource-diff-bundles/run + * POST /internal/medical/patient-jobs/ehr/athenahealth-create-resource-diff-bundles/:jobId/run ... - * @param req.query.jobId - The job ID. + * @param req.params.jobId - The job ID.
26-74: Duplicate handler bodies → extract a helper to reduce copy-pasteEverything inside the two
router.postcallbacks is identical except the EHR source enum value. Small refactor keeps this DRY and avoids divergence later:+function buildHandler(ehr: EhrSources) { + return asyncHandler(async (req: Request, res: Response) => { + const cxId = getFromQueryOrFail("cxId", req); + const jobId = getFrom("params").orFail("jobId", req); + const job = await initializePatientJob({ cxId, jobId }); + const { practiceId, ehrPatientId } = paramsOpsSchema.parse(job.paramsOps); + + await runJob({ + jobId, + ehr, + cxId, + practiceId, + metriportPatientId: job.patientId, + ehrPatientId, + }); + return res.sendStatus(httpStatus.OK); + }); +} + router.post( - "/athenahealth-create-resource-diff-bundles/:jobId/run", - requestLogger, - asyncHandler(async (...) => { ... }) + "/athenahealth-create-resource-diff-bundles/:jobId/run", + requestLogger, + buildHandler(EhrSources.athena) ); ... router.post( - "/canvas-create-resource-diff-bundles/:jobId/run", - requestLogger, - asyncHandler(async (...) => { ... }) + "/canvas-create-resource-diff-bundles/:jobId/run", + requestLogger, + buildHandler(EhrSources.canvas) );packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-cloud.ts (1)
24-32: Missing structured error context on retry failure
executeWithNetworkRetrieswill eventually throw after exhausting retries, but the current code doesn’t enrich the error with the lambda name/payload, which makes on-call debugging harder.await executeWithNetworkRetries(async () => - this.lambdaClient + this.lambdaClient .invoke({ FunctionName: this.getPatientJobsLambdaName, InvocationType: "Event", Payload: payload, }) .promise() -, `Invoke ${this.getPatientJobsLambdaName}` +).catch(err => { + err.message = `Invoke ${this.getPatientJobsLambdaName} failed – ${err.message}`; + err.payload = payload; + throw err; +});packages/lambdas/src/job/patient/run-job.ts (1)
49-54: Surface JSON-parse failures with more context
JSON.parsecan throw, but the resulting stack trace does not indicate which job failed. Wrap it so the job id (if extractable) or the full message is included in the thrownMetriportError’sadditionalInfo.- const bodyAsJson = JSON.parse(bodyString); + let bodyAsJson: unknown; + try { + bodyAsJson = JSON.parse(bodyString); + } catch (err) { + throw new MetriportError("Unable to parse SQS message body", { cause: err, additionalInfo: { bodySnippet: bodyString.slice(0, 200) } }); + }This will massively speed up triage when malformed messages appear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
packages/api/src/command/job/patient/create.ts(1 hunks)packages/api/src/command/job/patient/get.ts(2 hunks)packages/api/src/command/job/patient/scheduler/start-jobs.ts(1 hunks)packages/api/src/command/job/patient/status/cancel.ts(1 hunks)packages/api/src/command/job/patient/status/complete.ts(1 hunks)packages/api/src/command/job/patient/status/fail.ts(1 hunks)packages/api/src/command/job/patient/status/initialize.ts(1 hunks)packages/api/src/command/job/patient/update/set-entry-status.ts(1 hunks)packages/api/src/command/job/patient/update/update-runtime-data.ts(1 hunks)packages/api/src/command/job/patient/update/update-total.ts(1 hunks)packages/api/src/command/job/shared.ts(2 hunks)packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts(1 hunks)packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-job.ts(2 hunks)packages/api/src/external/ehr/shared/utils/job.ts(1 hunks)packages/api/src/models/patient-job.ts(3 hunks)packages/api/src/routes/internal/ehr/patient.ts(1 hunks)packages/api/src/routes/internal/medical/patient-job.ts(3 hunks)packages/api/src/routes/internal/medical/patient-jobs/ehr.ts(1 hunks)packages/api/src/routes/internal/medical/patient-jobs/index.ts(1 hunks)packages/api/src/sequelize/migrations/2025-06-10_00_add-patient-job-schedule.ts(1 hunks)packages/api/src/shared/config.ts(1 hunks)packages/core/src/command/job/patient/api/complete-job.ts(2 hunks)packages/core/src/command/job/patient/api/initialize-job.ts(2 hunks)packages/core/src/command/job/patient/api/run-job.ts(1 hunks)packages/core/src/command/job/patient/api/set-entry-status.ts(2 hunks)packages/core/src/command/job/patient/api/shared.ts(1 hunks)packages/core/src/command/job/patient/api/update-job-total.ts(2 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-cloud.ts(1 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-direct.ts(1 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-factory.ts(1 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs.ts(1 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-cloud.ts(1 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-direct.ts(1 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-factory.ts(1 hunks)packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job.ts(1 hunks)packages/core/src/external/ehr/api/job/shared.ts(0 hunks)packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/compute/ehr-compute-resource-diff-bundles-cloud.ts(1 hunks)packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/compute/ehr-compute-resource-diff-bundles.ts(1 hunks)packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/refresh/ehr-refresh-ehr-bundles-cloud.ts(1 hunks)packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/refresh/ehr-refresh-ehr-bundles.ts(1 hunks)packages/core/src/util/config.ts(2 hunks)packages/infra/config/env-config.ts(2 hunks)packages/infra/config/example.ts(1 hunks)packages/infra/lib/api-stack.ts(4 hunks)packages/infra/lib/api-stack/api-service.ts(6 hunks)packages/infra/lib/jobs/jobs-stack.ts(1 hunks)packages/infra/lib/jobs/types.ts(1 hunks)packages/infra/lib/secrets-stack.ts(1 hunks)packages/infra/lib/shared/secrets.ts(1 hunks)packages/infra/lib/surescripts/surescripts-stack.ts(1 hunks)packages/lambdas/src/job/patient/get-jobs.ts(1 hunks)packages/lambdas/src/job/patient/run-job.ts(1 hunks)packages/shared/src/common/response.ts(1 hunks)packages/shared/src/domain/job/job-status.ts(2 hunks)packages/shared/src/domain/job/patient-job.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/external/ehr/api/job/shared.ts
🧰 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/core/src/external/ehr/job/create-resource-diff-bundles/steps/refresh/ehr-refresh-ehr-bundles-cloud.tspackages/api/src/routes/internal/ehr/patient.tspackages/core/src/external/ehr/job/create-resource-diff-bundles/steps/compute/ehr-compute-resource-diff-bundles-cloud.tspackages/core/src/external/ehr/job/create-resource-diff-bundles/steps/refresh/ehr-refresh-ehr-bundles.tspackages/core/src/command/job/patient/api/complete-job.tspackages/core/src/command/job/patient/api/initialize-job.tspackages/api/src/command/job/patient/update/update-total.tspackages/api/src/command/job/patient/status/initialize.tspackages/core/src/external/ehr/job/create-resource-diff-bundles/steps/compute/ehr-compute-resource-diff-bundles.tspackages/api/src/command/job/patient/status/complete.tspackages/core/src/command/job/patient/api/set-entry-status.tspackages/infra/lib/secrets-stack.tspackages/infra/lib/surescripts/surescripts-stack.tspackages/api/src/command/job/patient/update/set-entry-status.tspackages/api/src/routes/internal/medical/patient-jobs/index.tspackages/infra/lib/shared/secrets.tspackages/core/src/command/job/patient/api/shared.tspackages/api/src/shared/config.tspackages/infra/lib/jobs/types.tspackages/api/src/command/job/patient/scheduler/start-jobs.tspackages/api/src/sequelize/migrations/2025-06-10_00_add-patient-job-schedule.tspackages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-factory.tspackages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-factory.tspackages/lambdas/src/job/patient/run-job.tspackages/core/src/command/job/patient/api/update-job-total.tspackages/infra/config/example.tspackages/core/src/command/job/patient/job/start-jobs/steps/run/run-job.tspackages/api/src/command/job/patient/update/update-runtime-data.tspackages/shared/src/domain/job/job-status.tspackages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-cloud.tspackages/infra/config/env-config.tspackages/lambdas/src/job/patient/get-jobs.tspackages/api/src/external/ehr/shared/utils/job.tspackages/core/src/util/config.tspackages/api/src/command/job/shared.tspackages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-direct.tspackages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-cloud.tspackages/api/src/command/job/patient/status/fail.tspackages/api/src/command/job/patient/status/cancel.tspackages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-job.tspackages/shared/src/common/response.tspackages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs.tspackages/shared/src/domain/job/patient-job.tspackages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.tspackages/api/src/models/patient-job.tspackages/infra/lib/api-stack/api-service.tspackages/core/src/command/job/patient/api/run-job.tspackages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-direct.tspackages/api/src/routes/internal/medical/patient-jobs/ehr.tspackages/infra/lib/api-stack.tspackages/infra/lib/jobs/jobs-stack.tspackages/api/src/command/job/patient/create.tspackages/api/src/command/job/patient/get.tspackages/api/src/routes/internal/medical/patient-job.ts
🧠 Learnings (1)
packages/api/src/routes/internal/medical/patient-job.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 (20)
packages/core/src/command/job/patient/api/complete-job.ts (1)
packages/shared/src/common/response.ts (1)
logAxiosResponse(17-23)
packages/core/src/command/job/patient/api/initialize-job.ts (1)
packages/shared/src/common/response.ts (1)
logAxiosResponse(17-23)
packages/core/src/command/job/patient/api/set-entry-status.ts (1)
packages/shared/src/common/response.ts (1)
logAxiosResponse(17-23)
packages/core/src/command/job/patient/api/shared.ts (1)
packages/shared/src/domain/job/patient-job.ts (1)
PatientJob(4-26)
packages/api/src/command/job/patient/scheduler/start-jobs.ts (2)
packages/api/src/command/job/shared.ts (1)
StartJobsParams(3-9)packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-factory.ts (1)
buildGetJobsHandler(6-13)
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-factory.ts (3)
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job.ts (1)
RunJobHandler(6-8)packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-direct.ts (1)
RunJobDirect(4-12)packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-cloud.ts (1)
RunJobCloud(6-19)
packages/core/src/command/job/patient/api/update-job-total.ts (1)
packages/shared/src/common/response.ts (1)
logAxiosResponse(17-23)
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job.ts (1)
packages/shared/src/domain/job/patient-job.ts (1)
PatientJob(4-26)
packages/api/src/command/job/patient/update/update-runtime-data.ts (3)
packages/api/src/command/job/shared.ts (1)
UpdateJobRuntimeDataParams(45-49)packages/shared/src/domain/job/patient-job.ts (1)
PatientJob(4-26)packages/api/src/command/job/patient/get.ts (1)
getPatientJobModelOrFail(27-31)
packages/api/src/external/ehr/shared/utils/job.ts (1)
packages/shared/src/interface/external/ehr/source.ts (1)
EhrSource(9-9)
packages/api/src/command/job/shared.ts (1)
packages/shared/src/domain/job/job-status.ts (1)
JobStatus(4-4)
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-direct.ts (2)
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job.ts (2)
RunJobHandler(6-8)RunJobRequest(3-4)packages/core/src/command/job/patient/api/run-job.ts (1)
runJob(19-39)
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-cloud.ts (1)
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job.ts (2)
RunJobHandler(6-8)RunJobRequest(3-4)
packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-job.ts (4)
packages/api/src/external/ehr/shared/utils/job.ts (1)
getCreateResourceDiffBundlesJobType(33-35)packages/api/src/command/job/patient/get.ts (1)
getLatestPatientJob(71-92)packages/shared/src/index.ts (1)
BadRequestError(40-40)packages/api/src/command/job/patient/create.ts (1)
createPatientJob(12-41)
packages/shared/src/common/response.ts (1)
packages/shared/src/index.ts (1)
MetriportError(41-41)
packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs.ts (1)
packages/shared/src/domain/job/patient-job.ts (1)
PatientJob(4-26)
packages/infra/lib/api-stack/api-service.ts (1)
packages/infra/lib/jobs/types.ts (1)
JobsAssets(4-8)
packages/infra/lib/api-stack.ts (1)
packages/infra/lib/jobs/jobs-stack.ts (1)
JobsNestedStack(69-209)
packages/infra/lib/jobs/jobs-stack.ts (4)
packages/infra/lib/shared/settings.ts (2)
LambdaSettings(35-35)QueueAndLambdaSettings(5-33)packages/infra/lib/shared/secrets.ts (1)
buildSecret(16-18)packages/infra/lib/shared/lambda-scheduled.ts (1)
createScheduledLambda(28-61)packages/infra/lib/jobs/types.ts (1)
JobsAssets(4-8)
packages/api/src/routes/internal/medical/patient-job.ts (9)
packages/terminology/src/util.ts (1)
asyncHandler(6-22)packages/shared/src/domain/job/job-status.ts (2)
isValidJobStatus(6-14)JobStatus(4-4)packages/api/src/command/job/patient/get.ts (1)
getPatientJobs(40-62)packages/shared/src/common/date.ts (1)
buildDayjs(86-88)packages/api/src/command/job/patient/scheduler/start-jobs.ts (1)
startJobs(14-25)packages/api/src/routes/schemas/uuid.ts (1)
getUUIDFrom(25-30)packages/api/src/command/job/patient/status/fail.ts (1)
failPatientJob(19-46)packages/api/src/command/job/patient/status/cancel.ts (1)
cancelPatientJob(19-48)packages/api/src/command/job/patient/update/update-runtime-data.ts (1)
updatePatientJobRuntimeData(15-26)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (26)
packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/refresh/ehr-refresh-ehr-bundles-cloud.ts (1)
5-5: Consistent import path for shared utilities.
The import forcreateSqsGroupIdhas been updated to reference the consolidatedsharedmodule, aligning with the other diff bundle steps.packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/compute/ehr-compute-resource-diff-bundles-cloud.ts (1)
5-5: Aligned import for shared utilities.
Switching thecreateSqsGroupIdimport to"../../shared"maintains consistency with the refactored module structure.packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/compute/ehr-compute-resource-diff-bundles.ts (1)
1-1: Update type import to centralized shared module.
TheCreateResourceDiffBundlesBaseRequesttype now imports from the consolidated"../../shared"path, matching other step modules.packages/core/src/external/ehr/job/create-resource-diff-bundles/steps/refresh/ehr-refresh-ehr-bundles.ts (1)
1-1: Centralize base request type import.
ImportingCreateResourceDiffBundlesBaseRequestfrom"../../shared"keeps shared types in one place.packages/api/src/routes/internal/ehr/patient.ts (1)
7-7: Updated import for patient job status command.
ThesetPatientJobEntryStatusfunction now correctly points to theupdatesubmodule, reflecting the restructured command directory.packages/core/src/command/job/patient/api/complete-job.ts (1)
23-23: Double-check PHI/PII exposure in debug logs
logAxiosResponsedumpsresponse.dataverbatim. For patient-level APIs this may include protected health information. Ensure:
- The
debuglogger is disabled or redacted in production.- No log aggregation sink stores raw payloads.
If not guaranteed, consider masking or selectively logging.
packages/core/src/command/job/patient/api/initialize-job.ts (1)
23-23: Validate success criteria explicitly
validateAndLogResponsepreviously enforced a schema / status check. Now we only log. Although Axios rejects non-2xx responses, any contract validation (e.g., required fields) is now skipped. If that validation was important, re-introduce it (e.g., zod schema).packages/api/src/command/job/patient/update/update-total.ts (1)
2-3: Import-path tweak LGTMThe relative-path correction (
../shared→../../shared) and move ofgetPatientJobModelOrFailare correct and compile-time safe.packages/api/src/command/job/patient/status/complete.ts (1)
4-5: Path update looks goodThe import-path changes keep the module layout coherent with the new folder structure. No behavioural impact.
packages/infra/config/env-config.ts (1)
285-289: Newjobssection is mandatory – could break existing config filesMaking the
jobsobject required means all non-updatedenv-config.tsvariants (sandbox, staging forks, developer-specific overrides) will fail type-checking and at runtime.
If that disruption is intentional, ignore this comment.
Otherwise mark it optional with sensible defaults:- jobs: { + jobs?: { roUsername: string; startPatientJobsSchedulerScheduleExpression: string; startPatientJobsSchedulerUrl: string; };and update consuming code to handle the undefined case gracefully.
packages/api/src/command/job/patient/update/set-entry-status.ts (1)
2-7: Import path updates look correctImports now point to the shared domain package and the refactored folder layout; build should resolve them without issues. No further action required.
packages/infra/lib/surescripts/surescripts-stack.ts (1)
14-14: CentralisingbuildSecretis the right move – good cleanupImporting the helper from
../shared/secretsremoves duplication across stacks and keeps secret handling consistent. No additional concerns here.packages/infra/config/example.ts (1)
231-235: Ensure type definitions & downstream stacks includejobsblock
jobsis added only in the example file. Confirm that:
EnvConfigNonSandbox(and any related runtime validation) now includes thejobsproperty.SecretsStack,JobsNestedStack, etc., read these new fields from typed config rather thanany, otherwise compilation will break.packages/api/src/external/ehr/shared/utils/job.ts (1)
14-21: Addition looks correct and aligns with new workflow
RunCreateResourceDiffBundlesJobParamscaptures the extra identifiers (jobId,metriportPatientId) needed at run time. No issues spotted.packages/infra/lib/jobs/types.ts (1)
4-8: Type-alias looks good and follows naming conventions
JobsAssetsprovides a clear contract for the CDK stack, and the alias fits the project’s naming/style guidelines. No concerns here.packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-factory.ts (1)
6-13: Ensure required env vars exist in every non-dev deployment
Config.getPatientJobsLambdaName()will throw at runtime ifGET_PATIENT_JOBS_LAMBDA_NAMEis missing. That’s correct behaviour, but please double-check that:
- The var is injected in all CI/CD targets (staging / prod / sandbox).
- Integration tests cover a mis-configured environment so failures surface early.
If that’s already covered, feel free to ignore.
packages/shared/src/domain/job/patient-job.ts (1)
32-53: Constant mapping looks correctAll new columns are mapped consistently to their snake_case counterparts.
packages/infra/lib/api-stack/api-service.ts (2)
29-30: Missing dependency check on upstream stack
JobsAssetsis imported but the calling stacks must now always provide it. Verify everycreateAPIServiceinvocation, including tests and non-prod stacks, passes this argument to avoid compilation failures.
524-528: Queue permission looks goodGranting
sendaccess to the run-job queue is correct and consistent with other queues in the file. 👍packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-cloud.ts (1)
13-18: Potential Date serialization issue
JSON.stringifyconvertsDateobjects to ISO strings. If downstream consumers expect JSDateinstances, they’ll instead receive strings, which may cause subtle bugs. Clarify the contract inRunJobRequestor explicitly transform dates.packages/api/src/command/job/patient/status/fail.ts (1)
30-34: Retried fail requests are not idempotentAnalogous to cancellation, re-failing an already failed job throws.
Evaluate whether idempotency is desirable for public endpoints.packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/start-job.ts (1)
41-55:❌ Incorrect review comment
Concurrency guard ignores
scheduledjobsThe “only one job at a time” check filters by statuses
waiting | processing, but scheduled jobs may also clash if they execute soon after. Consider includingscheduled(or whatever status you use) to truly prevent duplicates.Would adding it break any legitimate flow?
Search for all status values in patient-job lifecycle to confirm:
🏁 Script executed:
#!/bin/bash # list status literals in shared domain rg -o $'\"(scheduled|waiting|processing)\"' -n packages/shared | sort -uLength of output: 2693
🏁 Script executed:
#!/bin/bash # Locate and display the first 200 lines of the job-status definition job_status_file=$(rg -l 'job-status.ts' -n packages/shared | head -n1) echo "Inspecting: $job_status_file" sed -n '1,200p' "$job_status_file"Length of output: 170
#!/bin/bash set -e # Find the job-status definition file file=$(find packages/shared -type f -name 'job-status.ts' | head -n1) if [[ -z "$file" ]]; then echo "ERROR: job-status.ts not found" >&2 exit 1 fi echo "Inspecting: $file" echo "----- First 200 lines -----" head -n200 "$file" echo echo "----- All status literals -----" grep -Eo "['\"][a-zA-Z]+['\"]" "$file" | sort -u
Ignore suggestion to include “scheduled”; no such status exists
A search acrosspackages/sharedshows only “waiting” and “processing” literals insrc/domain/job/job-status.ts. There isn’t a “scheduled” status defined, so the current guard is correct and no filter change is needed.Likely an incorrect or invalid review comment.
packages/api/src/command/job/patient/get.ts (2)
33-62: Well-implemented filtering with proper ORM usageThe new
ListPatientJobsParamstype andgetPatientJobsfunction are well-designed. The implementation correctly uses Sequelize's ORM features with parameterized queries, avoiding SQL injection risks.
64-77: Good separation of concerns with specific type namingThe type rename from
ListPatientJobsParamstoListLatestPatientJobsParamsimproves code clarity by distinguishing between the general job listing and latest job retrieval use cases.packages/infra/lib/jobs/jobs-stack.ts (1)
74-111: Well-structured infrastructure with proper security practicesThe stack implementation follows AWS CDK best practices:
- Enables termination protection for production safety
- Uses read-only database credentials from Secrets Manager
- Properly configures Lambda functions with appropriate permissions
packages/api/src/routes/internal/medical/patient-job.ts (1)
43-68: Well-implemented endpoint with proper validationThe GET endpoint properly validates the status parameter and safely parses date inputs using
buildDayjs.
packages/api/src/sequelize/migrations/2025-06-10_00_add-patient-job-schedule.ts
Outdated
Show resolved
Hide resolved
packages/api/src/sequelize/migrations/2025-06-10_00_add-patient-job-schedule.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/job/patient/job/start-jobs/steps/run/run-job-factory.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-direct.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-direct.ts
Outdated
Show resolved
Hide resolved
packages/core/src/command/job/patient/job/start-jobs/steps/get/get-jobs-direct.ts
Outdated
Show resolved
Hide resolved
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/command/job/patient/command/run-job/run-job-cloud.ts (1)
8-8: Consider dependency injection for the SQS client.The SQS client is instantiated directly within the class, which reduces testability and flexibility. Consider injecting the SQS client through the constructor to improve testability and follow dependency inversion principles.
export class RunJobCloud implements RunJobHandler { - private readonly sqsClient: SQSClient = new SQSClient({ region: Config.getAWSRegion() }); + private readonly sqsClient: SQSClient; - constructor(private readonly runJobQueueUrl: string) {} + constructor( + private readonly runJobQueueUrl: string, + sqsClient?: SQSClient + ) { + this.sqsClient = sqsClient ?? new SQSClient({ region: Config.getAWSRegion() }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/command/job/patient/command/run-job/run-job-cloud.ts(1 hunks)packages/core/src/util/config.ts(1 hunks)packages/infra/lib/api-stack.ts(5 hunks)packages/infra/lib/api-stack/api-service.ts(5 hunks)packages/infra/lib/surescripts/surescripts-stack.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/util/config.ts
- packages/infra/lib/surescripts/surescripts-stack.ts
- packages/infra/lib/api-stack/api-service.ts
- packages/infra/lib/api-stack.ts
🧰 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/core/src/command/job/patient/command/run-job/run-job-cloud.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/core/src/command/job/patient/command/run-job/run-job-cloud.ts (1)
12-21: LGTM with proper error handling via network retries.The method implementation follows good practices:
- Uses async/await as per guidelines
- Implements proper JSON serialization
- Uses network retry mechanism for reliability
- Follows functional programming style with single responsibility
- Uses meaningful parameter names
The retry mechanism adequately handles transient failures, and the FIFO queue configuration with message grouping ensures proper job ordering.
packages/core/src/command/job/patient/command/run-job/run-job-cloud.ts
Outdated
Show resolved
Hide resolved
| const msg = "Failed to run some scheduled patient jobs"; | ||
| capture.message(msg, { |
There was a problem hiding this comment.
I don't think we need to log it unless there's some extra data not going to sentry. I know sending to sentry isn't bullet-proof but should be sufficient for what we need.
There was a problem hiding this comment.
Logging is super errors is super useful when we're debugging stuff in Cloudwatch. If I don't see a long on CWL I assume the code worked.
packages/core/src/command/job/patient/command/run-job/run-job-cloud.ts
Outdated
Show resolved
Hide resolved
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
… eng-433-scheduler-for-patient-job Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
| const msg = "Failed to run some scheduled patient jobs"; | ||
| capture.message(msg, { |
| * | ||
| * @param jobId - The job ID. | ||
| * @param cxId - The customer ID. | ||
| * @param reason - The reason for cancelling the job. |
There was a problem hiding this comment.
hypernit: The reason for failing the job.
There was a problem hiding this comment.
Good catch. Done.
| * @param req.query.cxId - The CX ID. | ||
| * @param req.query.jobId - The job ID. |
There was a problem hiding this comment.
these are no longer query params
There was a problem hiding this comment.
Good catch. Done.
| return res.sendStatus(httpStatus.OK); | ||
| }) | ||
| ); | ||
|
|
||
| /** | ||
| * POST /internal/patient/job/:jobId/update-total |
There was a problem hiding this comment.
btw, this is wrong. The route is /internal/patient/job/:jobId/total
There was a problem hiding this comment.
Good catch. Done.
| })); | ||
| log( | ||
| `${msg}. Cause: ${errors | ||
| .map(error => `\n${error.id} ${error.cxId} ${error.error}`) |
There was a problem hiding this comment.
nit: cxId and patientId can go on the log prefix since its a param for the fn, right?
There was a problem hiding this comment.
Is this due to an outdated pull from develop? Can you merge/rebase to make sure, please
There was a problem hiding this comment.
It's the result of shift+option+O
Ref: ENG-281 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
There was a problem hiding this comment.
AFAICT, this isn't used anywhere. Let's remove it.
There was a problem hiding this comment.
The route it's pointing to doesnt even exist
There was a problem hiding this comment.
/**
* POST /internal/patient/job/:jobId/total
*
* Updates the total of the job.
* @param req.query.cxId - The CX ID.
* @param req.params.jobId - The job ID.
* @param req.query.total - The total number of entries to process.
* @returns 200 OK
*/
router.post(
"/:jobId/total",
requestLogger,
asyncHandler(async (req: Request, res: Response) => {
const cxId = getUUIDFrom("query", req, "cxId").orFail();
const jobId = getFrom("params").orFail("jobId", req);
const total = getFromQueryOrFail("total", req);
if (isNaN(+total)) {
throw new BadRequestError("Total must be a number");
}
await updatePatientJobTotal({
jobId,
cxId,
total: +total,
});
return res.sendStatus(httpStatus.OK);
})
);
in patient-job.ts route file.
There was a problem hiding this comment.
removed the core command.
Ref: ENG-433 Ref: #1040 Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
… eng-433-scheduler-for-patient-job Signed-off-by: Thomas Yopes <thomasyopes@Thomass-MBP.attlocal.net>
Issues:
Dependencies
Description
Testing
/internal/patient/job/startRelease Plan
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Infrastructure
Bug Fixes
Chores