Refactor RequestsController and delegate logic to RequestService#477
Merged
Merged
Conversation
Refactored `RequestsController` to improve maintainability and remove unnecessary indirection. Deleted `RequestService` and moved its logic into idiomatic controller filters and model methods. Key changes: - Removed `RequestService` and folded logic into `before_action` filters. - Delegated business logic to models, including `Course#extensions_enabled?` and `Request#approve_by`. - Replaced `@role` string comparisons with model-based permission checks like `course_staff?` and `course_student?`. - Added a `pending` scope and `pending?` helper to `Request`. - Improved test coverage for the `export` endpoint, access control, and staff-only actions (+21 examples). Co-authored-by: Claude Code <noreply@anthropic.com>
- Consolidate staff-only route protection in `require_course_staff` - Move `new_for_student` to a private method with staff-only rendering - Implement a top-level enrollment guard in `RequestsController#new` - Update `Course#extensions_enabled?` to fail closed if settings are nil - Remove redundant LMS presence checks in request initialization - Add test coverage for student access to staff-only creation routes Co-authored-by: Claude Code <noreply@anthropic.com>
…tor-requests-controller/1 Reconcile the RequestsController refactor with main: - Adopt main's Course#requests_enabled? (drop the branch's duplicate extensions_enabled?) and main's argument-free prepare_* helpers. - Keep the branch's controller rewrite (RequestService removed, model- delegated access checks) and the deterministic Course#canvas_id fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Move CSV export logic to Requests::ExportsController to unify before_actions in RequestsController. - Replace custom authentication and staff checks with shared ApplicationController methods. - Update `ensure_instructor_role` to use `course_staff?` instead of `@role`. - Add `require_course_membership` filter to prevent unauthorized access to request actions. - Narrow LMS check in `new` to specifically verify Canvas integration. Co-authored-by: Claude Code <noreply@anthropic.com>
The branch's merge of main left three UserToCourse.create! calls in the requests controller spec; the model was renamed to Enrollment, so these referenced a constant that no longer exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CSV is used by Request.to_csv, not the exports controller (which only calls it), so the require belongs with the model. Co-Authored-By: Claude Opus 4.8 <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.
General Info
Changes
Refactors
RequestsControllerto remove unnecessary indirection, delegate business logic to models, and improve test coverage.RequestService— a set of class methods with zero test coverage that each returned a redirect hash the controller then re-interpreted — has been deleted entirely, with its logic folded into idiomaticbefore_actionfilters and model methods.Key changes:
RequestService: Logic moved into controller filters that redirect directly, delegating decisions to models rather than bouncing through a service hash.Course#extensions_enabled?,Request#pending?,Request.pendingscope, andRequest#approve_by(acting_user)encapsulate logic that was previously scattered across the controller and duplicated in places.@rolestring comparisons: Permission checks now ask the course directly (course_staff?/course_student?) instead of comparing a@rolestring.@roleis still assigned byApplicationController#set_coursesince the shared view layer still depends on it.before_actionchain:set_course_role_from_settings+authenticate_coursecollapsed into the existingApplicationController#set_course+ a smallset_form_settingsfilter.Request#approve_by: Consolidates theassignment.lms_facade.from_user(...)→approve(...)pattern that was duplicated in both the singleapproveaction and the mass-approve helper.Notable finding: The
new_for_studentaction has no route and is only reachable as a rendered template fromnew. Left as-is since route changes are out of scope, but worth a follow-up.Testing
Added 21 new examples covering previously untested paths:
exportendpoint had zero coverage — added tests for valid token, invalid token, missing token, non-existent course, and status filtering.approve,reject, andmass_approve.newaction: staff renders the on-behalf-of form; users with no course role are redirected.Course#extensions_enabled?,Request#pending?,Request.pending, andRequest#approve_by.Full suite went from 479 → 500 examples, 0 failures.
Documentation
No documentation changes required.
Checklist
Superconductor Ticket Implementation | App Preview | Guided Review