Non-ODS years selectable if crossyear matching enabled OR roster file exists#67
Draft
edandylytics wants to merge 11 commits into
Draft
Non-ODS years selectable if crossyear matching enabled OR roster file exists#67edandylytics wants to merge 11 commits into
edandylytics wants to merge 11 commits into
Conversation
A no-ODS year with no roster file should be selectable at job creation when the partner has cross-year matching enabled (EDU can supply the roster), and still rejected when the toggle is off. Tests fail until resolveJobDestination reads the partner toggle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
resolveJobDestination now reads the partner's crossYearMatchingEnabled flag. A no-ODS year is valid if a roster file exists OR the toggle is on; the S3 existence check is short-circuited when the toggle is on. Partner setting only — no creds/connection check, per spec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For a no-ODS year, hasRoster should be true when the partner toggle is on even with no S3 file (and the S3 check should be skipped). ODS years stay null. Fails until the toggle is folded into hasRoster. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hasRoster for a no-ODS year is now true when crossYearMatchingEnabled is on, short-circuiting the S3 doesFileExist call. ODS years stay null. No DTO change — hasRoster's shape is unchanged, only its computation, so the crossYearMatchingEnabled flag never reaches the FE. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On a no-ODS job with cross-year matching available, the executor payload should carry appUrls.roster and omit rosterFilePath (a dangling S3 pointer). Fails until rosterFilePath is gated on crossYearMatchAvailable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When crossYearMatchAvailable is true the executor pulls the roster from EDU via appUrls.roster, so the S3 rosterFilePath would be a dangling pointer. Omit it. The executor only reads rosterFilePath in its non-cross-year branch, so the absent key is safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hasRoster now folds in the cross-year toggle, so all FE gating (already keyed off hasRoster === true) is correct with no logic change. Drop "file" from user-facing strings — a roster may come from EDU or an S3 file, and the FE shouldn't reason about the source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a roster-source precedence note (ODS -> EDU -> S3 file) and the partner-setting-only selectability/green gate to AGENTS.md. The executor already prefers EDU over the S3 file (merged in #60); no executor change in this work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR 3 makes no-ODS roster availability source-agnostic (S3 file or EDU), but two reachable FE surfaces still implied an S3 roster file was required. Make the copy source-neutral so it stays accurate when the roster comes from EDU: - SetupRequiredPage: "Roster File Required" -> "Roster Required" and drop "file" from the body + contact-support message. - UnmatchedStudents: no-ODS branch "the roster file" -> "the roster". Also add a positive smoke test for the external API v1 job-create path. Both job-create paths (internal POST /jobs and external v1) flow through the shared resolveJobDestination, but only the internal path had a toggle-on/no-file coverage test. Add the analogous test for the v1 endpoint: no-ODS year, no roster file, partner cross-year matching enabled -> 201 with odsId=null/sendToOds=false. Passes against the behavior already on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The EDU-precedence bullet said "no app change in PR 3", but PR 3 does change the app (selectability + rosterFilePath omission). The point being made is that the executor is unchanged — it already prefers EDU over the S3 file (merged in #60). Reword to match the kickoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Post-review polish: - Rename the GetTenantSchoolYearConfigDto field hasRoster -> hasNonOdsRoster. ODS years already have an ODS-fetched roster, so "hasRoster" was ambiguous; the field specifically reports whether a no-ODS year has a roster available (S3 file or EDU). Stays null for ODS years. Updated the controller, FE consumers (JobCreatePage, OdsConfigsPage, routes/index), and the integration tests. - Status copy "roster loaded" / "roster not loaded" -> "roster available" / "roster not available", and the JobCreate suffix "(no roster loaded)" -> "(no roster available)". "Available" reads better for a status that may be satisfied by EDU rather than a loaded file. - Drop transient cross-references from code comments and AGENTS.md (the "(see project brief)" notes and the "PR 3" mention) so they don't rot. Co-Authored-By: Claude Opus 4.8 (1M context) <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 updates the app to allow users and API clients to submit jobs for non-ODS years when either:
This is a matter of:
resolveJobDestinationto takepartner.crossYearMatchingEnabledinto account when determining whether a job can be sent to a given no-ODS year.partner.crossYearMatchingEnabledwhen submitting jobsA couple design decisions:
partner.crossYearMatchingEnabledand not that we have EDU creds on file or an active connection. Having EDU creds is a condition for togglingpartner.crossYearMatchingEnabledtotruein the first place. It's possible someone could delete the AWS secret and not update the partner setting... but that's not really a case I think we need to handle.canConnectcheck (or similar) forresolveJobDestination. On submitting a job, it could be good to confirm we really do have creds so we can present an error if for some reason they're not there or not available..../school-year-config/tenanthas a single combined flag (hasNonOdsRoster) that indicates a year has a roster, either from a file or EDU. Exposing how the roster is sourced doesn't seem helpful for this endpoint. It's just meant to answer the question "can non-ods jobs be submitted to this year?"