[fix] Facilitators can only drop out, not defer#2564
Merged
Conversation
Contributor
WalkthroughThis PR prevents facilitators from deferring courses while keeping deferral available to participants. It adds a server-side guard rejecting facilitator deferral requests, updates DropoutModal to query registrations and disable the Deferral option for facilitators, changes the course-row action label to "Drop out" for facilitators, and updates mocks, tests, and Storybook accordingly. ChangesFacilitator deferral restriction
Sequence Diagram(s)sequenceDiagram
participant User
participant DropoutModal
participant courseRegistrations
participant dropoutMutation
participant dropoutOrDeferral
participant courseRegistrationTable
User->>DropoutModal: open modal
DropoutModal->>courseRegistrations: getAll()
courseRegistrations-->>DropoutModal: [{ role: "Facilitator" } | ...]
DropoutModal->>DropoutModal: compute allowDeferral (false if Facilitator)
User->>DropoutModal: submit form (type "Deferral" or "Drop out")
DropoutModal->>dropoutMutation: mutate(input)
dropoutMutation->>dropoutOrDeferral: invoke backend mutation
dropoutOrDeferral->>courseRegistrationTable: fetch registration
courseRegistrationTable-->>dropoutOrDeferral: registration{ role }
dropoutOrDeferral->>dropoutOrDeferral: if role === "Facilitator" && type === "Deferral" -> throw FORBIDDEN
alt allowed
dropoutOrDeferral->>dropoutMutation: success
dropoutMutation-->>DropoutModal: success
else forbidden
dropoutOrDeferral-->>dropoutMutation: TRPCError(FORBIDDEN)
dropoutMutation-->>DropoutModal: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/website/src/components/courses/DropoutModal.tsx`:
- Around line 77-82: When courseRegistrations.getAll is still loading
registrations, allowDeferral currently defaults to true and exposes the Deferral
option; change allowDeferral to be false while registrations is undefined and
compute effectiveType only once the registrations are loaded. Concretely, update
allowDeferral to something like: const allowDeferral = registrations ?
registrations.find(r => r.id === applicantId)?.role !== 'Facilitator' : false;
and set effectiveType to undefined while registrations is undefined (e.g., const
effectiveType: DropoutType | undefined = registrations ? (allowDeferral ?
dropoutType : 'Drop out') : undefined) so isDeferral (isDeferral = effectiveType
=== 'Deferral') and the UI won't show/select Deferral until the applicant role
is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23af9eed-1134-45f1-9191-b223e8394620
📒 Files selected for processing (8)
apps/website/src/__tests__/testUtils.tsapps/website/src/components/courses/DropoutModal.stories.tsxapps/website/src/components/courses/DropoutModal.test.tsxapps/website/src/components/courses/DropoutModal.tsxapps/website/src/components/my-courses/CourseListRow.test.tsxapps/website/src/components/my-courses/useCourseListRow.tsxapps/website/src/server/routers/dropout.test.tsapps/website/src/server/routers/dropout.ts
Contributor
Greptile SummaryThis PR restricts facilitators to drop-out only (no deferral), enforcing the business rule both server-side (a new
Confidence Score: 5/5Safe to merge; the deferral restriction is correctly enforced at both the UI layer and the server, and the incidental TypeScript fix is mechanical. All changed paths are well-tested, the server-side guard makes the restriction tamper-proof independent of client state, and the only notable gap is a cosmetic banner that remains visible to facilitators with text that no longer applies to them. DropoutModal.tsx — the InformationBanner that promotes deferral is still shown unconditionally to facilitators. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (Facilitator)
participant M as DropoutModal
participant TR as tRPC: courseRegistrations.getAll
participant TD as tRPC: dropout.dropoutOrDeferral
U->>M: Opens "Drop out" modal
M->>TR: getAll()
TR-->>M: registrations[]
M->>M: "allowDeferral = role !== 'Facilitator' → false"
M->>U: Renders form (Deferral option disabled)
U->>M: Selects "Drop out of the course"
U->>M: Clicks Submit
M->>TD: "{ type: "Drop out", applicantId }"
TD->>TD: Fetch courseRegistration by id+email
TD->>TD: "role === 'Facilitator' && type === 'Deferral'? → No, proceed"
TD-->>M: Dropout record created
M-->>U: Success message
Note over TD: If type === 'Deferral' and role === 'Facilitator'
TD-->>M: FORBIDDEN error
Reviews (2): Last reviewed commit: "[fix] Facilitators can only drop out, no..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Will-Howard
force-pushed
the
wh-2551-facilitator-drop-not-defer-2026-05
branch
from
161edb0 to
2c364c1
Compare
Will-Howard
temporarily deployed
to
wh-2551-facilitator-drop-not-defer-2026-05 - bluedot-preview PR #2564
Render
Destroyed
Will-Howard
temporarily deployed
to
wh-2551-facilitator-drop-not-defer-2026-05 - bluedot-storybook-preview PR #2564
Render
Destroyed
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/website/src/components/courses/DropoutModal.tsx (1)
76-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve role before enabling/submitting deferral.
Deferral is still enabled while registrations are unresolved, and submission uses
dropoutTypedirectly, so facilitator flows can still hit a forbidden deferral path from client state.Suggested patch
- const { data: registrations } = trpc.courseRegistrations.getAll.useQuery(); - const allowDeferral = registrations?.find((r) => r.id === applicantId)?.role !== 'Facilitator'; - - const isDeferral = dropoutType === 'Deferral'; + const { data: registrations } = trpc.courseRegistrations.getAll.useQuery(); + const registration = registrations?.find((r) => r.id === applicantId); + const registrationsLoaded = registrations !== undefined; + const allowDeferral = registrationsLoaded + ? registration?.role !== 'Facilitator' + : false; + const effectiveType: DropoutType | undefined = registrationsLoaded + ? (allowDeferral ? dropoutType : 'Drop out') + : undefined; + + const isDeferral = effectiveType === 'Deferral'; @@ - const submitDisabled = !dropoutType + const submitDisabled = !effectiveType || dropoutMutation.isPending || (isDeferral && !effectiveTargetRoundId); @@ - if (!dropoutType) return; + if (!effectiveType) return; if (isDeferral && !effectiveTargetRoundId) return; @@ - type: dropoutType, + type: effectiveType, newRoundId: isDeferral ? effectiveTargetRoundId : undefined, @@ - value={dropoutType} + value={effectiveType}Also applies to: 208-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/components/courses/DropoutModal.tsx` around lines 76 - 78, The component enables and submits deferral before registrations are loaded causing facilitator users to bypass checks; update the logic around trpc.courseRegistrations.getAll.useQuery, registrations, allowDeferral and any submission handlers that reference dropoutType so they first ensure registrations is defined and the applicant's role is resolved (e.g., compute allowDeferral only when registrations !== undefined and applicantId matches a resolved registration), disable the deferral UI while registrations are loading, and additionally re-check the resolved role inside the submit handler (reject/abort submission or ignore dropoutType when the found registration.role === 'Facilitator') to prevent client-side forbidden flows.
🧹 Nitpick comments (1)
apps/website/src/components/courses/DropoutModal.test.tsx (1)
139-141: ⚡ Quick winAssert disabled behavior instead of a styling class.
This assertion is coupled to presentational CSS and can fail on harmless style changes; verifying non-selectability is more stable.
Suggested patch
- await waitFor(() => { - expect(within(listbox).getByRole('option', { name: 'Defer to a future round' })).toHaveClass('cursor-not-allowed'); - }); + const deferralOption = await within(listbox).findByRole('option', { name: 'Defer to a future round' }); + await user.click(deferralOption); + expect(screen.getByRole('button', { name: 'Submit' })).toBeDisabled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/website/src/components/courses/DropoutModal.test.tsx` around lines 139 - 141, The test currently asserts a presentational CSS class on the option fetched via within(listbox).getByRole('option', { name: 'Defer to a future round' }) which is brittle; change the assertion to verify non-selectability instead (e.g., assert the option is disabled or has aria-disabled="true" or is not enabled) so the test checks behavior not styling—update the assertion in DropoutModal.test.tsx to use toBeDisabled() or expect(...).toHaveAttribute('aria-disabled','true') against the same role lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/website/src/components/courses/DropoutModal.tsx`:
- Around line 76-78: The component enables and submits deferral before
registrations are loaded causing facilitator users to bypass checks; update the
logic around trpc.courseRegistrations.getAll.useQuery, registrations,
allowDeferral and any submission handlers that reference dropoutType so they
first ensure registrations is defined and the applicant's role is resolved
(e.g., compute allowDeferral only when registrations !== undefined and
applicantId matches a resolved registration), disable the deferral UI while
registrations are loading, and additionally re-check the resolved role inside
the submit handler (reject/abort submission or ignore dropoutType when the found
registration.role === 'Facilitator') to prevent client-side forbidden flows.
---
Nitpick comments:
In `@apps/website/src/components/courses/DropoutModal.test.tsx`:
- Around line 139-141: The test currently asserts a presentational CSS class on
the option fetched via within(listbox).getByRole('option', { name: 'Defer to a
future round' }) which is brittle; change the assertion to verify
non-selectability instead (e.g., assert the option is disabled or has
aria-disabled="true" or is not enabled) so the test checks behavior not
styling—update the assertion in DropoutModal.test.tsx to use toBeDisabled() or
expect(...).toHaveAttribute('aria-disabled','true') against the same role
lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be63325b-1a23-4611-9cf1-e907b943f05a
📒 Files selected for processing (8)
apps/website/src/__tests__/testUtils.tsapps/website/src/components/courses/DropoutModal.stories.tsxapps/website/src/components/courses/DropoutModal.test.tsxapps/website/src/components/courses/DropoutModal.tsxapps/website/src/components/my-courses/CourseListRow.test.tsxapps/website/src/components/my-courses/useCourseListRow.tsxapps/website/src/server/routers/dropout.test.tsapps/website/src/server/routers/dropout.ts
✅ Files skipped from review due to trivial changes (1)
- apps/website/src/components/my-courses/useCourseListRow.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/website/src/server/routers/dropout.ts
- apps/website/src/tests/testUtils.ts
- apps/website/src/server/routers/dropout.test.ts
- apps/website/src/components/courses/DropoutModal.stories.tsx
- apps/website/src/components/my-courses/CourseListRow.test.tsx
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.
Description
Facilitators could previously defer a course as well as drop out, through the shared "Drop or defer" flow on the Facilitated Courses page. The agreed rule (per Li-Lian in Slack) is that facilitators can only drop out, not defer.
Incidental fix:
createMockCourseRegistrationwas missing the facilitator-application columns added in #2563, which had lefttsc --noEmitred on master. Added them (all nullable) so typecheck passes.Issue
Fixes #2551
Developer checklist
Screenshot