Admin screen for Cross-year ID Matching#65
Conversation
|
|
||
| @Module({ | ||
| imports: [], | ||
| imports: [EarthbeamApiModule], |
There was a problem hiding this comment.
This is where the EDU service lives -- probably a better place for it, but not something I want to tackle right now
PR 2 introduces admin endpoints for the cross-year matching toggle. Privileges and role mapping land first so the controller wire-up has something to authorize against.
Red step: tests describe the read endpoint's auth gating, partner-scoped column reflection, and eduCredsExist coming from canConnect.
Returns the partner's crossYearMatchingEnabled and an eduCredsExist indicator derived from EduSnowflakePoolService.canConnect, so the FE can render the toggle and gate the enable action when creds are missing without ever seeing the creds themselves. Wires PartnersModule against EarthbeamApiModule for the pool service; the module now exports EduSnowflakePoolService.
Covers auth gating, the partner-scoped column write, the no-gate-on-creds policy, partner isolation, and invalid-body rejection. The PUT handler itself shipped with the prior commit alongside the GET handler — keeping the test file as a single round-trip surface kept the controller wire-up coherent, so the red/green pair here lands as one commit rather than two.
New FE section + partner-config queries. Toggle is disabled when EDU creds are missing AND it's currently off — preventing partner admins from enabling a feature that can't function while still allowing them to turn it back off if creds disappear after the fact. EDU connection state is shown as a badge alongside a note about cred rotation requiring an app restart for now (the optional refresh-pool endpoint is deferred).
Reverses the original "FE-only gate" decision: backend now rejects crossYearMatchingEnabled=true with 400 when canConnect is false. Disabling is always allowed, so an admin can still turn the feature off if creds disappear after the fact. Reason for the change: FE-only gating is bypassable via direct API call, and canConnect short-circuits on the pool cache so the extra check is effectively free on the hot path.
Mirrors the school-year config pane's interaction model: view-only by
default with an "edit" affordance, save/cancel footer while editing,
unsaved-changes blocker, and a confirm-changes modal. View mode shows
an enabled/disabled badge in place of the switch.
Reframes the section as "partner-wide configuration" with cross-year
matching ("source roster from EDU") as its first setting, in
anticipation of more partner-level toggles landing here later.
EDU connection status is rendered inline with the icon-chip + label
treatment from the ODS Configs page so admins encounter the same
visual vocabulary across surfaces.
Helptext picks up the missing-creds requirement when applicable, so
admins see why the toggle is unavailable without needing to read the
backend error.
generalError was set on mutation failure but never cleared, so a transient "something went wrong" banner could linger after a successful retry — the section stays mounted across saves, unlike SchoolYearConfigEditForm. Clear it when entering edit mode and at the start of each save attempt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lock GET to the exact { crossYearMatchingEnabled, eduCredsExist } shape
so an accidental partner-field or secret leak is caught, and assert the
session partner's row actually flipped in the scoping test (previously
it only checked the other partner stayed put, so a no-op would pass).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
the help text grew long enough to warrant hiding the detail behind a show more/less disclosure, keeping only the one-line summary visible. the disclosure button carries aria-expanded/aria-controls and the collapsed region is display:none when closed, so screen readers only announce it when expanded. convert the toggle row to FormControl/FormLabel/FormHelperText so the switch gets its accessible name and description wired by chakra instead of by hand. the EDU status block keeps an explicit id so the switch's aria-describedby still references it (chakra merges it with the helper text id). move the "EDU connection required" warning out of the help text to sit directly under the EDU status badge, where it's both more discoverable and still part of the switch description. add a plain FormControl theme variant that strips the container chrome (bg/padding/radius/focus glow) for controls that supply their own layout, like this toggle row nested in a contentBox. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
the admin section will host additional partner-wide settings beyond the cross-year matching toggle, so name it for the broader role. the cross-year-specific aria ids stay as-is since they describe that specific control. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mirror the school-year-config stale-write pattern so a partner admin can't silently clobber a concurrent change as more settings land in this section. GET /partners/config returns the partner row's modifiedOn in x-config-modified-at; PUT requires x-if-config-modified-at and 409s on mismatch (or a missing header), returning lastModifiedOn/ lastModifiedBy. no migration needed — partner already carries modified_on/modified_by_id, maintained by the update_meta trigger. the FE captures the header on read and sends it back on save, surfacing a distinct "reload and try again" message on conflict. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
edandylytics
force-pushed
the
app/cross-year-2
branch
from
a07ed3b to
b6a7b3e
Compare
the optimistic-concurrency check was more machinery than a single boolean toggle warrants. revert it for now and leave a TODO on the PUT handler so we revisit concurrency before this section gains more settings, when last-write-wins would actually risk silent clobbers. deferring the choice of approach too — the school-year-config header/409 pattern doesn't obviously map onto a one-row partner update.
| @Authorize('partner-config.read') | ||
| @Get('config') | ||
| async getConfig(@TenantDecorator() tenant: Tenant) { | ||
| const partner = await this.prisma.partner.findUniqueOrThrow({ |
There was a problem hiding this comment.
I'm trying to move away from findUniqueOrThrow generally, but leaving this in since if we don't find the partner for the session tenant, that's rightly a 500, not a 404.
the four as-PartnerAdmin GET cases overlapped — the exact-shape check
already covered the true cases of the column and creds assertions.
collapse to two cases (both-true, both-false), each asserting the full
{ crossYearMatchingEnabled, eduCredsExist } body so the no-field-leak
guard stays, plus the canConnect-called-with-partnerId check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
drop the subsequent-GET test (PUT-writes-row + GET-reflects-column
already cover the round trip transitively), and fold the basic update
test into the partner-scoping test — acting as partner X proves the
write both lands on the session's partner and leaves others untouched,
which subsumes the plain A-update plus { status: 'ok' }. move the X test
to the end so the partner-A cases stay next to the beforeEach that sets
them up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- inline the one-off switchSx onto the Switch
- rename the modal disclosure to isModalOpen/onModalOpen/onModalClose so
it's distinguishable from the help disclosure
- make the edit buffer a draftConfig: PutPartnerConfigDto | null instead
of a fake { crossYearMatchingEnabled: false } default; derive a
non-null draft (falling back to the saved config) for rendering, and
null draftConfig whenever leaving edit mode so the type honestly
signals when an edit is in flight
- drop the now-redundant sync effect (the draft fallback covers it)
- comment the beforeunload guard
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bhaugeea
left a comment
bhaugeea
left a comment
There was a problem hiding this comment.
Looks good! Just a couple questions.
| where: { id: tenant.partnerId }, | ||
| select: { crossYearMatchingEnabled: true }, | ||
| }); | ||
| const eduCredsExist = await this.eduPool.canConnect(tenant.partnerId); |
There was a problem hiding this comment.
I wonder whether this caller really wants the same failure-swallowing behavior as the job-payload assembly caller. If those failures are not actually real anyway, no biggie. But just in theory, I do wonder.
There was a problem hiding this comment.
It's a good question! Just to make sure we're thinking about the same thing, the failure swallowing scenario is when there's some error retrieving the secret from AWS. This is reported as false when really it's an indeterminate scenario: we don't know whether we've got EDU creds. Is that the one?
I think this is fine. If we're having issues communicating with AWS, then Runway cannot, in fact, connect to EDU using creds stored in the secret. It'd be helpful to know that the reason is an AWS error and not simply missing EDU creds... but I think it's OK not to handle that. It's a one time switch, fails in the right direction, and either a reload fixes it or there's some bigger AWS situation to figure out.
Your question is prompting me to think this through, so let me know if that doesn't sound right.
There was a problem hiding this comment.
Yeah that's the failure I'm talking about, if it's the one the comment in the helper is talking about. I think the two things that caused me to pause were (a) eduCredsExist is a bit of a misnomer for canConnect given the error-swallowing, and (b) I wasn't sure whether the two features wanted the same thing. There are a couple places in UM where a given error is swallowed by a normal feature but exposed by a related admin feature.
There was a problem hiding this comment.
I do think it's fine to leave though! Just wanted to confirm.
There was a problem hiding this comment.
It can also be tough to draw a line between errors that are real and ones that aren't. Thinking rigidly and simplistically, it's sort of confusing and wrong to say that the AWS error is real for the purpose of swallowing it and carrying on with the job processing, but not real for the purpose of saying EDU creds exist. But allowing for more nuance I'm happy to accept that it's "sort of" real, and we want to swallow it for job processing but pretend it doesn't exist for other purposes.
There was a problem hiding this comment.
I agree with you on the misnomer. I'll rename the variable.
For this feature, I think the canConnect behavior is OK. If anything, I see us iterating more on how to handle failures in job processing. Currently, we'll fall back to not using EDU if EDU is unavailable and still attempt to process the job... which I think is the right move while we're introducing this functionality, but I could see us later preferring to treat a bad EDU connection as a fatal error and have some automated retry, at least for transient errors. But I don't think we'll get a read on that until we see it in prod.
Address review nits on the integration tests: - Drop the dead canConnect mock default in the GET beforeEach; every test in that block already sets the value it exercises, so the baseline only read as a misleading default. - Remove the verbose header comment on "updates only the session partner's row" (the name and assertions speak for themselves) and leave a one-line note on the load-bearing partner-A untouched assertion instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The single ConfirmChangesModal was being puppeted into two unrelated dialogs via a modalMode flag — "confirm these changes" and "you have unsaved changes, leave?" — with every prop ternaried on the mode and the state machinery duplicated across both Admin forms. Extract a generic ConfirmModal shell. ConfirmChangesModal becomes a thin wrapper over it that owns the change-list rendering; the unsaved-changes dialog just uses ConfirmModal directly with literal copy (not worth its own wrapper — it only presets two strings). Both forms now drive two separate useDisclosure instances, dropping modalMode and all the JSX ternaries. Behavior is unchanged. Addresses Bjorn's review note on the wonky dual-purpose modal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The PartnerConfig confirm dialog described the setting as "source roster from EDU" while the form labels it "Cross-year roster for ID matching". Use the same wording so the confirmation reads consistently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The cancel button reused the "unsaved changes — leave without saving?" dialog, which was confusing: cancelling stays on the page and discards edits, it doesn't navigate away. Give it its own "discard changes" dialog and reserve the leave dialog for the navigation blocker. Each exit now has its own useDisclosure and confirm handler, so the pendingLeaveAction discriminator and the branching in handleLeaveConfirm are gone. Applied to both Admin edit forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The partner-config read field is sourced straight from EduSnowflakePoolService.canConnect, so name it to mirror that primitive rather than implying a separate "do creds exist" check. Renamed across the DTO, controller, FE consumer, and integration tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bhaugeea
left a comment
bhaugeea
left a comment
There was a problem hiding this comment.
Looks great! Works well. Tests well.
2 participants
This PR adds a toggle to the Admin Settings page to enable cross year ID matching.
I tried to adhere to conventions from the school year config pane above: click "edit" to make changes, a modal that confirms changes, a modal that warns on navigation with unsaved changes. There's an exception: no stale-write check. That's because (a) it's not necessary with just this one toggle and (b) I'm not sure I want to take the pattern used in school year configs as a model, where it leans on the last modified date. We'll add this when we extend this pane to other settings.
Admin users can toggle
crossYearMatchingEnabledto true only if EDU creds exist for the partner. Of course, the creds could be deleted later and, in that case, the setting would still be enabled, which is, I think, fine -- we check both the existence of creds and the setting when processing jobs.To test locally, you can comment out
EDU_SNOWFLAKE_USERNAMEin .env and that'll give you the no connection state.Some other odds and ends:
FormControlparadigm from Chakra so are wired manually into the input'saria-describedby, and get combined with the references from theFormControl:plainkey to the themes so I could use form controls without inheriting styles. There's a larger refactor to be done, but not in this PR