Repository navigation
App/match review display - #142
Open
edandylytics wants to merge 39 commits into
Open
edandylytics wants to merge 39 commits into
edandylytics wants to merge 39 commits into
Conversation
GET /jobs/:jobId/student-match-results returns every group of input details for the job, each with the result of every run that searched for it and that result's suggestions in ordinal order: one call, three queries under Prisma's nested include, each on a job-first key. It sits with the job's other reads, behind the same tenant ownership guard and metatenant read privilege. Prisma names the relations after their tables, so the service renames them to results and suggestions before serializing. Result ids travel as strings, since JSON has no bigint, and scores as numbers. Both conversions use @type: a @Transform would run only after class-transformer tried to copy the Prisma Decimal with new Decimal(), which throws. The details and roster JSON are returned as stored. Tests seed through the real Executor callback, so they cover what a run posts and what a reviewer reads, with auth first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Job reads now count the job's student input details (studentsToMatchCount), and a successful run with any becomes "complete with errors", like the older unmatched-IDs case. That shows on the job page and in the jobs list, and lets the job be marked resolved. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Red: job reads don't carry idMatchingMode, and a fuzzy background job with reported students shows complete with errors and can be resolved. In background mode the ID-based run already delivered and review is read-only, so the students it reports say nothing about the job's outcome. The existing status tests now seed a fuzzy job, since that's the mode they describe. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Green: GetJobDto exposes the job's idMatchingMode, and hasStudentsToMatch requires fuzzy. A fuzzy background job's ID-based run has already delivered and its review is read-only, so its reported students no longer make it complete with errors or resolvable. The mode is the job's snapshot, so switching a partner to fuzzy doesn't change old background jobs. The frontend also needs the mode to show background results as read-only. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Red: the spec doesn't compile, since JobsService has no
studentMatchReviewLimit, and the endpoint still returns a bare array.
The response becomes { count, students }. Past the review limit it
returns the count with students: null: that many students to match
means something is wrong with the file, not a queue anyone will
review, so there's no point fetching them. The object also leaves room
for the job-level review summary later PRs add.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Green: GET /jobs/:jobId/student-match-results returns { count, students }.
Past MATCH_REVIEW_STUDENT_LIMIT it counts and returns students: null
without fetching them.
Genuine review queues are under 10, a few hundred at most; thousands
means the file needs fixing. Measured locally with ten suggestions per
student: 200 students is 0.7 MB in about 0.3 s, and 1,000 is 3.5 MB in
about 1.6 s, so the limit is set where the response is still workable.
Under the limit the count is taken from the rows fetched, so it can't
disagree with the students returned if results arrive in between.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Red: a tenant user without an admin role can read a background-mode job's match results. Background mode exists for admins to check IDRS's suggestions before a partner switches to fuzzy, so partner admins and support users can read its results, including support users across tenants, and other users can't. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Green: new privilege job.match-results.background.read, held by PartnerAdmin and SupportUser. The read endpoint refuses a background-mode job's results without it. Fuzzy jobs are unchanged. The check depends on the job's mode, so it's in the handler rather than a route decorator. It uses the job the middleware already loaded, and runs after the tenant ownership guard, so a support user's metatenant read still works. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
AGENTS.md now describes the { count, students } response, the
1,000-student limit and why there's no pagination, and how a job's
matching mode decides who reads its results and whether they affect its
status. The read paragraphs move below the ingestion callback's, so
"this endpoint" there refers to the callback again.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Put each comment on the field it explains, and describe studentsToMatchCount in terms of students, matching the response. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Guards the next commit, which moves the list's count out of Prisma's _count and has to put each count back on the right job. This passes before and after: the change is to how the count is queried, not what it returns. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Prisma emits _count as a LEFT JOIN to a GROUP BY over all of student_input_details. For one job, Postgres pushes the job's id into the subquery and uses the primary key. For the jobs list it can't push the tenant filter in, so every list load aggregated the whole table: 16 ms at 100k rows, growing linearly with what will be Runway's largest tables. findAll now runs one groupBy on student_input_details for the listed job ids, which uses the primary key and is bounded by the tenant's jobs (about 2 ms for 20 jobs among 100k rows). The single-job reads keep _count. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review B1: the existing test posted students, runs and suggestions in the order it expected back, so it passed with every orderBy removed. The new test stores each level out of order: the later run reports first, corr-2 before corr-1. Suggestions are stored directly, ordinal 1 first, since the callback numbers them in the order it stores them. With the orderBy clauses removed it fails. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review C1 and O1: only background mode's access was tested, so widening the admin-only rule to every non-fuzzy job would have passed, and locked tenant users out of id_based jobs, the only mode in use in production. The tests now show any tenant user reading fuzzy and id_based results, a support user reading fuzzy results across tenants, and background mode's admin-only rule, under one describe block for access by mode. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review C2: only the resolve test showed a fuzzy job's students turning its status to complete with errors. The job and list reads were tested only in background mode, where status stays success, so they'd have passed if those reads never changed status at all. The fuzzy test and the background one now share a helper that rebuilds the job from both reads, as the frontend does. Removing hasStudentsToMatch from the status, or the list's count, now fails them. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review C3: studentsToMatchCount now comes from Prisma's _count for one job and from JobsService.countStudentsToMatch for the list, and a read that supplies neither defaults to 0, silently showing a fuzzy job as success. The DTO comment was stale about where the API reads it. It and AGENTS.md now say what a job read must supply, and which source to use. Review O2: the background gate read req.job?., so a missing job would have skipped the check. It now refuses with a 500 instead. The guard already makes that unreachable, but an auth check should deny by construction. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Couldn't fail: "counts none for a job with no match results". Prisma always returns _count for one job, so dropping the include still read 0; the list's zero is pinned by "counts each listed job's own students". Duplicates removed: - the fuzzy row of the any-user access matrix, the same user, tenant and role as the shape test; the ID-based case stays on its own - fuzzy cross-tenant support-user access, and background support-user access in its own tenant, both implied by background cross-tenant access - the single-job and list count test, covered by the per-job list count and the background count - "carries the job's matching mode", implied by the status test, since the frontend computes status from the JSON - background resolve refusal, implied by the background status staying success, since resolve reads the same getter Also drops an unused tenant B job, takes "in order" off the shape test (the ordering test owns ordering) and renames the resolve and background tests. Mutations checked, each failing the intended test: dropping the route's @AllowMetatenant, the background privilege check or either role's privilege; widening admin-only to every non-fuzzy mode; each orderBy; the limit and its off-by-one; _count on the job read and on resolve; the list's count and its per-job keying; status ignoring the mode or the students; idMatchingMode not exposed; id and score conversion. Dropping countStudentsToMatch's where fails nothing, as expected: it bounds what is read, not what is counted. 22 tests before, 15 after. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Corrections: - AGENTS.md said the frontend reads the match results endpoint; nothing in the frontend calls it yet. - The fuzzy bullet implied only fuzzy jobs' results are open to any user; id_based jobs are too. The count note moved out of that bullet, since every mode reports it. - hasStudentsToMatch gave "review is read-only" as a reason background mode leaves status alone; fuzzy review is read-only too. The reason is that the ID-based run already delivered. - "BIGINT, which JSON can't carry" now says JSON.stringify refuses the BigInt Prisma returns. - The resolve test's comment ignored that a resolved job is also changeable. Removed repetition: the review limit's rationale (now in AGENTS.md and on MATCH_REVIEW_STUDENT_LIMIT only), background mode's purpose (AGENTS.md only; the controller keeps why the check isn't a decorator), _count's cost (on countStudentsToMatch only), the 3.5 MB estimate (on the constant only), and the DTO file header that restated the response shape. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
Every non-public endpoint should eventually carry its own privilege, so the match results read gets job.match-results.read, granted to every role. No real role lacks it, so the test takes it from the session. Fails to compile: job.match-results.read is not yet a PrivilegeKey. With the key in place but the route unguarded, it gets 200 instead of 403. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A first step toward every non-public endpoint having its own privilege. Every role has it, so no one loses access; the background-mode privilege still narrows fuzzy background jobs to admins. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The jobs list was its only caller, and from there it wasn't clear why the count is a separate query rather than an included _count. With the query and its reason at the call site, it is. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The privilege, not the variable, decides who reads background results; "admin" was a concept the check does not use. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The order is stable but opaque to a reviewer, so the client picks its own. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Executor and IDRS keep the input file's order, so the order students were first reported is file order, which a reviewer can relate to. Correlation id order is stable but meaningless. corr-1 is first reported by the later run, so sorting on its first result by run, rather than its earliest result, also fails. Fails: students come back by correlation id, [corr-1, corr-2, corr-3] instead of [corr-2, corr-1, corr-3]. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Result ids follow the order the Executor sent each batch, which is the input file's order, so sorting by each student's earliest result id lists students as they appear in the file. Sorted in JS because Prisma can't order by an aggregate over a relation; at most 1,000 students, it's cheap. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
GetJobDto read Prisma's _count through a @Transform, so the DTO knew how the queries were built. Elsewhere the caller shapes its input to the DTO, so the reads now map _count into studentsToMatchCount, as the jobs list already did with its groupBy. The field is optional on DtoableJob: reads that don't feed a status (start, the run-complete event) don't need a count. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
hasUnmatchedStudents and hasStudentsToMatch read alike but mean different things: IDs that ID-based matching couldn't find, which the user fixes in the file, and fuzzy-mode students waiting for a reviewer's match decision. The names now say which mode each belongs to, and each says its remedy. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The event builds a GetJobDto only for these, and the next commits stop it using the DTO. Nothing tested them, so this guards the move; it passes before and after. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The event built a GetJobDto only for its resource-error getters and input params. The getters' logic is now getResourceErrors and getResourceSummaries, which the DTO's getters call, and the event calls getResourceErrors on the run and reads input params from the job. Moved unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The frontend navigates on success without reading the response, and the job it serialized had no runs, so its status was always null. Like resolve, it now returns nothing, which leaves toGetJobDto to reads that show a status. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Status depends on the count, and a read that left it out showed a fuzzy job with students to match as success. Now that only reads that show a status build the DTO, the count can be required, so leaving it out fails to compile instead. jobs.spec typed its seeded jobs as DtoableJob, though none is a DTO input; they're typed from seedJob now. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
prisma.d.ts imported JobInputParamDto from @edanalytics/runway-models, which does not exist. skipLibCheck hides the error in a .d.ts, so JobInputParams was silently any. Importing from @edanalytics/models makes it real; the job factory's cast to JsonArray is no longer needed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The template column was untyped JSON, so the run-complete event built a GetJobTemplateDto just to read reportResources. Annotated as JobTemplate, the shape the column stores, the event reads it from the job. Writes keep storing instanceToPlain of the DTO, cast because instanceToPlain returns Record<string, any>. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nothing checked the stored template, and the next commit changes how it is written. The expected JSON is spelled out from the fixture rather than built with the same helpers. Passes before and after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
instanceToPlain returns Record<string, any>, which needed a cast to the typed template column. The DTO instance already holds only its exposed fields and Prisma serializes it to the same JSON, as the stored-template test confirms, so it is written directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No summary and an empty one mean different things, and the job view tells them apart, so callers decide what a missing summary means: the job DTO's getter returns undefined, and getResourceErrors finds no errors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
edandylytics
commented
Oct 6, 2026
| const res = await this.jobService.startJob(updatedJob, this.prisma); | ||
| if (res.result === 'JOB_STARTED') { | ||
| return toGetJobDto(updatedJob); | ||
| return; |
Collaborator
Author
There was a problem hiding this comment.
The frontend didn't do anything with this return value.
Collaborator
Author
There was a problem hiding this comment.
Updates to this file are just related to the refactor to move away from using DTOs just to access a getter.
edandylytics
commented
Oct 6, 2026
| expect(job?.sendToOds).toBe(true); | ||
| }); | ||
|
|
||
| it('stores the template from the bundle', async () => { |
Collaborator
Author
There was a problem hiding this comment.
We updated how the bundle was saved on creating the job -- it's no long converted from the DTO to a plain object. That was untested, so added a test that the DTO was handled appropriately on being converted to Postgres JSON.
Reverts 77b48fa and 6f662f0. The Executor builds candidates with earthmover's group_by, keyed on the hash that becomes the correlation id, so it sends them in correlation id order, not file order. "First reported" just recreated correlation id order with a sort, and its test passed without the sort because rows came back from the table already in that order. Correlation id order is only for stability, and the comment and AGENTS.md now say so. A reviewer works through a few students, so file order isn't worth more. The restored test fails without the orderBy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains the backend components and DTO updates to populate the fuzzy matching views