Skip to content

Improve Request#Show page for instructors with review information#455

Open
cycomachead wants to merge 2 commits into
mainfrom
cycomachead/151-improve-request-show-instructor/1
Open

Improve Request#Show page for instructors with review information#455
cycomachead wants to merge 2 commits into
mainfrom
cycomachead/151-improve-request-show-instructor/1

Conversation

@cycomachead

Copy link
Copy Markdown
Contributor

Improve Request#Show Page for Instructors with Review Information

Surfaces the context an instructor needs to make an informed approval/denial decision on an extension request, and cleans up the underlying role-checking pattern across the app.

Changes

New instructor review context on Request#Show:

  • Student email (mailto link) and an "Extended requests allowed" badge
  • Request submission timestamp with a before/after-deadline badge
  • Direct LMS link to the assignment
  • Projected late due date that will be applied on approval (for pending requests)
  • Student's full extension history in the course (assignment, requested date, days, submission date, status) in a compact table with approved/pending/denied summary counts
  • Decision provenance for already-decided requests (who processed it and when)

RequestReviewPresenter (app/presenters/request_review_presenter.rb) — encapsulates all the review-context data computation (student history, status counts, enrollment, projected dates) so the controller stays thin and the view reads from a single @review object.

_student_extension_history partial — the history table extracted into its own partial for clarity.

staff_user? helper — replaces every @role == 'instructor' permission check across controllers and views with a semantic, memoized predicate that delegates to Course#course_staff?. Available in both controllers and views via helper_method.

Security fix in AssignmentsController#toggle_enabled — previously trusted a client-supplied role param to determine authorization. Now uses the server-side staff_user? check exclusively.

Testing

  • Added spec/presenters/request_review_presenter_spec.rb (8 examples) covering all presenter methods including edge cases (no enrollment, submitted after deadline, status counts excluding current request).
  • Added controller specs for the instructor show action asserting template rendering and correct @review presenter assignment.
  • Added a regression test asserting AssignmentsController ignores a client-supplied role: 'instructor' param.
  • Added #staff_user? specs covering staff-true, student-false, nil guards, and memoization correctness.
  • Fixed a latent test isolation bug: the users factory generated bare numeric canvas_uid sequences ("1", "2", …) that would eventually collide with hardcoded UIDs in specs. Prefixed the sequence ("canvas-uid-#{n}") to prevent collisions across full suite runs.
  • Fixed course_settings_controller_spec enrollments that used a fictitious role: 'instructor' (not a real DB role); switched to role: 'teacher'.

Full suite: 446 examples, 0 failures.

Documentation

No additional documentation required.

Checklist

  • Name of branch corresponds to story

Superconductor Ticket Implementation | App Preview | Guided Review

- Introduce `RequestReviewPresenter` to encapsulate student history and approval metadata
- Add `_student_extension_history` partial to display prior requests and status counts
- Surface student email, LMS links, and projected late due dates on the show page
- Replace string-based `@role == 'instructor'` checks with a cached `staff_user?` helper
- Fix a security vulnerability in `AssignmentsController` by enforcing server-side permission checks
- Resolve a latent test isolation bug by prefixing factory `canvas_uid` sequences

Co-authored-by: Claude Code <noreply@anthropic.com>
@cycomachead

Copy link
Copy Markdown
Contributor Author

This is good. Will merge after we add student notes and display those.

@cycomachead cycomachead mentioned this pull request Jul 2, 2026
20 tasks
@cycomachead

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved in 5262d4e. The merge brought in all changes from origin/main (including the Enrollment rename, student notes feature, security audit changes, and more) and resolved 12 conflicted files:

  • Controllers: Kept staff_user? helper while adopting main's require_course_staff! name; used @assignment.course from main; removed redundant inline auth guards now covered by before_action
  • instructor_show view: Combined both our extension history/provenance section and main's student notes section — both features are now present
  • Presenter: Updated UserToCourseEnrollment throughout RequestReviewPresenter and all affected specs
  • Specs: Replaced allow_any_instance_of auth mocks with real Enrollment.create! records (main's approach); combined our RequestReviewPresenter assertions with main's @student_enrollment assertions in the show spec
  • request_service.rb: Deleted (removed in main)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants