SP26 #1: LMS ID Refactor + Email Notifications Settings#487
Open
cycomachead wants to merge 47 commits into
Open
SP26 #1: LMS ID Refactor + Email Notifications Settings#487cycomachead wants to merge 47 commits into
cycomachead wants to merge 47 commits into
Conversation
- Implemented user_to_courses_controller.rb with role-based authorization - PATCH endpoint to toggle allow_extended_requests on enrollments - Authorization: only teachers can toggle - Uses lms_id FK pattern from LMS credentials refactoring - Added teacher? method to UserToCourse model for role checking - Complete spec with 7 test scenarios (instructor, student, missing resources) - All tests passing: 365 examples, 0 failures, 80.88% coverage
Mass approve/ reject functionality and UI changes for request table PR reviewed and merged into Golden Repo. Thank you!
Adjust DataTables data-priority values so only Actions, Assignment, and Name columns remain visible on small screens. Bulk action buttons are also hidden on mobile via Bootstrap responsive utility classes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Added developer login + fake data for local testing
Incorrect due dates on Canvas assignments
…lean Mobile view: hide checkbox and status columns on requests page
Main rebase
Add instructor email notifications for pending requests
Support Lead TA course role
set_enrollment uses UserToCourse.find, which raises RecordNotFound when the id is bogus. Running it before authorize_instructor! leaks the existence/non-existence of enrollment ids to unauthorized callers and returns 404 instead of 403. Reorder so authorization runs first. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Storing the literal string 'none' as a refresh token caused later token-refresh attempts to send refresh_token=none to Canvas and fail with an opaque 4xx. Persist nil instead so refresh code paths can detect the absence and prompt the user to re-authenticate. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
The hand-rolled regex accepted strings like "a@b" with no TLD. URI::MailTo::EMAIL_REGEXP is RFC-aligned and ships with stdlib. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
# Conflicts: # app/controllers/courses_controller.rb # db/schema.rb
Main's aae5b25 switched the Canvas facade to override_assignment_dates=false, so top-level due_at/lock_at are now the base ("Everyone") dates and are trustworthy. The base_date branch no longer receives data, so the guard in SyncAllCourseAssignmentsJob triggered on every update and prevented Canvas date changes from propagating to existing assignments. Remove the guard, the associated base_date attr on Lmss::Canvas::Assignment, and the base_date_present? abstract method on Lmss::BaseAssignment. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Match the omniauth developer-provider gate in config/initializers/omniauth.rb (Rails.env.local?), so the button appears in test environments too and stays in lockstep with the provider registration. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Restore main's version of the table header markup — the PR's styling changes were out of scope for this branch. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
The '# LMS must exist' comment was left over after removing the validates :lms_name, presence: true line; belongs_to :lms already enforces this. UserToCourse#teacher? was never called from the codebase — staff? / course_admin? cover the existing use cases. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Replace the pair of before_validation lambdas with Rails 7.1's normalizes declaration, which handles the blank-to-nil coercion on assignment. Keep the before_save cascade that clears the email when frequency is turned off, but add a comment explaining why. The scope can now compare against nil only (no empty strings can persist). Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Remove the controller-local authenticate_user! and set_course overrides — ApplicationController's authenticated! before_action populates current_user, and its protected set_course does the same lookup. Replace the manual UserToCourse lookup + course_admin? check with Course#course_staff?(current_user). Note in a comment why we don't use the inherited ensure_instructor_role (JSON endpoint, redirect semantics don't fit). Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Pulls the subject/body templates out of the job into constants on a new mailer class (body uses a heredoc). The job now just computes the per-course values and delegates delivery. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
…w-only-pr-lms-id-fix/2
cycomachead
changed the title
cycomachead
changed the title
…w-only-pr-lms-id-fix/2 Resolve conflicts between the lms-id-fix branch and main's Rails 8.1 upgrade, UserToCourse->Enrollment rename, 1:1 course-settings refactor, and Canvas auto-approval reliability work: - Keep main's LmsCredential#expires_soon?/#refresh! (drop-dead-credential handling) and adapt them to the lms_id foreign key: the lms_name column is gone, so the presence validation is dropped (belongs_to :lms covers it) and log messages read the name via the lms association. - Port the lms_credentials:purge rake task and its spec to join lmss instead of filtering on the removed lms_name column. - EnrollmentsController: keep the branch's staff-authorization-before-load and JSON 403 shape, with main's class name and course-scoped lookup. - CourseSettings: keep both main's course_id uniqueness validation and the branch's pending-notification normalization/validations/scope; add the pending notification params to main's params.expect list. - Drop the course_settings factory (main deleted it; settings are now auto-created with each course) and update branch specs to mutate course.course_settings instead of creating rows/factories. - Adapt main's expanded staff-failover and token-refresh specs to create credentials via lms_id; drop the branch's tests for the removed User#token_expired?. - Regenerate db/schema.rb via migration replay on top of main's 8.1 dump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The suite seeds Lms rows (ids 1 and 2) with explicit ids, which leaves the lmss id sequence pointing at 1. Creating an Lms without an id in the purge task spec then collides with the seeded rows on a fresh CI database. Find the seeded row case-insensitively and create any extra LMS with an explicit id instead of relying on the sequence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main renumbered the rename migration to 20260701000001 and removed the 20260704000001 file; after merging main this branch carried both, and ActiveRecord's duplicate-migration-name check aborted every spec run. Delete the stale copy so the migration set matches main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ms-id-fix/2' into cycomachead/97-preview-only-pr-lms-id-fix/2
Contributor
Author
|
This will be deployed to staging. Notification emails will be sent very frequently on staging as a test |
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
5 participants
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
Syncs
maininto thelms-id-fixbranch. The core change in this branch replaces the string-basedlms_namecolumn onlms_credentialswith a proper foreign keylms_idreferencing thelmsstable, ensuring LMS credentials are associated by ID rather than a loosely-typed string.Key changes included in this branch (not from main):
lms_idFK tolms_credentials, migrates existing rows fromlms_namestring matching, then dropslms_nameLmsCredentialnowbelongs_to :lms;User#canvas_credentialsqueries bylms_id;Lms.CANVAS_LMS/GRADESCOPE_LMSsingletons use a safer find-then-create pattern to avoid overwriting existing recordslms_id: Lms.CANVAS_LMS.idinstead oflms_name: 'canvas'; stops fabricating a fake refresh token for the dev provider (storesnilinstead)PendingRequestsMailer,PendingRequestsNotificationJob, and rake task (notifications:send_pending_digests[daily|weekly]) for digest emails;CourseSettingsgainspending_notification_frequency/pending_notification_emailfields with validations, normalization, and awith_pending_notificationsscope; course settings UI exposes the new fieldsUserToCoursesController: Refactored authorization to return JSON 403 (instead of a redirect flash) so the course-settings fetch UI can surface errors inlinelms_id: 1and ensure the CanvasLmsrecord exists before creating credentials; new specs for the mailer, job, rake task, andCourseSettingsnotification validationsChanges pulled in from
main(documentation and dependency bumps only):docs/instructors.mdupdatesGemfile.lockbumps (bootsnap 1.18.4→1.24.6, annotaterb 4.14.0→4.23.0)Testing
Documentation
No new user-facing documentation required. The instructor-facing docs updated on
mainare included via the sync.Checklist
Superconductor Ticket Implementation | App Preview | Guided Review