EEDFIAL-375 update job payload to slack #5
Conversation
rtavernaea
commented
Jan 5, 2026
rtavernaea
commented
edandylytics
left a comment
edandylytics
left a comment
There was a problem hiding this comment.
I think we can get this there pretty quickly. There are a number of comments, but really just three main themes:
- Let's tighten up the typing. There's a prisma generator that'll help with that. That will catch some of the cases where this code might bomb out.
- Refactoring the
resourceErrorsgetter a bit should allow us to simplify things - Let's optimize the summary for concision and scan-ability. It's OK if it doesn't read like a sentence.
| const allProcessedRecordsFailed = | ||
| runSummary.studentAssessments.records_processed === | ||
| runSummary.studentAssessments.records_failed; | ||
| const unmatchedStudentsInfo = run.unmatchedStudentsInfo as any; |
There was a problem hiding this comment.
Let's use the prisma generator to type this more specifically. The as any causes us to miss the case where unmatchedStudentsInfo can be null.
edandylytics
left a comment
edandylytics
left a comment
There was a problem hiding this comment.
Submitting with some notes from our pairing
edandylytics
left a comment
edandylytics
left a comment
There was a problem hiding this comment.
This is looking good. Just a few small things and we're there.
app/models/src/dtos/job.dto.ts
Outdated
| resource: reportableFailedResource, | ||
| failed: this.resourceSummaries?.[reportableFailedResource]?.failed ?? 0, | ||
| total: | ||
| this.resourceSummaries?.[reportableFailedResource]?.success! + |
There was a problem hiding this comment.
Why do we need to assert the presence of success?
There was a problem hiding this comment.
Oh hm that may have been an ai autocomplete that was unintentional. But i think it's because resourceSummaries is possibly undefined. I can change this line to
this.resourceSummaries?.[reportableFailedResource].success ?? 0 +
edandylytics
left a comment
edandylytics
left a comment
There was a problem hiding this comment.
Seems we lost the PrismaJson types at some point. Once those are back in and the app builds, should be good to go
edandylytics
left a comment
edandylytics
left a comment
There was a problem hiding this comment.
Looks good. Let's get this in!
