From 37b1a1fe4c4824a5a5fc27c2b2b161256e69e57a Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Wed, 4 Mar 2026 16:51:27 -0800 Subject: [PATCH 01/19] LMS credentials now uses LMS ID instead of LMS name --- app/controllers/session_controller.rb | 2 +- app/models/lms.rb | 25 +++++++++++------- app/models/lms_credential.rb | 7 +++-- app/models/user.rb | 3 +-- db/api_spec_seeds.rb | 2 +- ..._refactor_lms_credentials_to_use_lms_id.rb | 26 +++++++++++++++++++ ...0001_validate_lms_credentials_lms_id_fk.rb | 5 ++++ db/schema.rb | 5 ++-- .../application_controller_spec.rb | 3 ++- .../concerns/token_refreshable_spec.rb | 3 ++- .../course_settings_controller_spec.rb | 5 ++-- spec/controllers/courses_controller_spec.rb | 9 ++++--- spec/controllers/requests_controller_spec.rb | 6 ++--- spec/factories/lms.rb | 13 +++++++++- spec/factories/lms_credential.rb | 9 ++++++- spec/features/accessibility_spec.rb | 4 +-- spec/models/course_spec.rb | 3 ++- spec/models/lms_credential_spec.rb | 6 +++-- spec/models/request_spec.rb | 3 ++- spec/models/user_spec.rb | 15 +++++++---- 20 files changed, 113 insertions(+), 41 deletions(-) create mode 100644 db/migrate/20260304000000_refactor_lms_credentials_to_use_lms_id.rb create mode 100644 db/migrate/20260304000001_validate_lms_credentials_lms_id_fk.rb diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 75815b0f..b87348ab 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -138,7 +138,7 @@ def update_user_credential(user, token) ) else user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: Lms.CANVAS_LMS.id, token: token.token, refresh_token: token.refresh_token, expire_time: Time.zone.at(token.expires_at) diff --git a/app/models/lms.rb b/app/models/lms.rb index 08aeee1e..ce2662b2 100644 --- a/app/models/lms.rb +++ b/app/models/lms.rb @@ -17,23 +17,28 @@ class Lms < ApplicationRecord # Relationship with Assignment has_many :assignments + # Relationship with LmsCredential + has_many :lms_credentials, dependent: :destroy + # Singleton instances for each LMS def self.CANVAS_LMS - @canvas_lms ||= find_or_create_by( + @canvas_lms ||= find_by(id: 1) || find_or_create_by!( id: 1, - lms_name: 'Canvas', - lms_base_url: ENV.fetch('CANVAS_URL', ''), - use_auth_token: true - ) + lms_name: 'Canvas' + ) do |lms| + lms.lms_base_url = ENV.fetch('CANVAS_URL', '') + lms.use_auth_token = true + end end def self.GRADESCOPE_LMS - @gradescope_lms ||= find_or_create_by( + @gradescope_lms ||= find_by(id: 2) || find_or_create_by!( id: 2, - lms_name: 'Gradescope', - lms_base_url: 'https://www.gradescope.com', - use_auth_token: false - ) + lms_name: 'Gradescope' + ) do |lms| + lms.lms_base_url = 'https://www.gradescope.com' + lms.use_auth_token = false + end end # Map a linked LMS to the appropriate API facade which can be used to post extension requests diff --git a/app/models/lms_credential.rb b/app/models/lms_credential.rb index aa895068..7480a5e5 100644 --- a/app/models/lms_credential.rb +++ b/app/models/lms_credential.rb @@ -5,7 +5,6 @@ # # id :bigint not null, primary key # expire_time :datetime -# lms_name :string # password :string # refresh_token :string # token :string @@ -13,6 +12,7 @@ # created_at :datetime not null # updated_at :datetime not null # external_user_id :string +# lms_id :bigint # user_id :bigint # # Indexes @@ -21,15 +21,18 @@ # # Foreign Keys # +# fk_rails_... (lms_id => lmss.id) # fk_rails_... (user_id => users.id) # class LmsCredential < ApplicationRecord # Belongs to a User belongs_to :user + # Belongs to an LMS + belongs_to :lms + # Encryption for tokens encrypts :token, :refresh_token # LMS must exist - validates :lms_name, presence: true end diff --git a/app/models/user.rb b/app/models/user.rb index 9cac5f45..6695a9a7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -38,9 +38,8 @@ class User < ApplicationRecord has_many :user_to_courses has_many :courses, through: :user_to_courses - # TODO: We should probably use lms_id over lms_name def canvas_credentials - lms_credentials.find_by(lms_name: 'canvas') + lms_credentials.find_by(lms_id: Lms.CANVAS_LMS.id) end def token_expired? diff --git a/db/api_spec_seeds.rb b/db/api_spec_seeds.rb index 3e99bc2b..50748302 100644 --- a/db/api_spec_seeds.rb +++ b/db/api_spec_seeds.rb @@ -50,7 +50,7 @@ test_lms_credential = LmsCredential.create!({ user_id: test_user.id, - lms_name: "canvas", + lms_id: canvas.id, token: "test token", external_user_id: "44444", }) diff --git a/db/migrate/20260304000000_refactor_lms_credentials_to_use_lms_id.rb b/db/migrate/20260304000000_refactor_lms_credentials_to_use_lms_id.rb new file mode 100644 index 00000000..d267a21a --- /dev/null +++ b/db/migrate/20260304000000_refactor_lms_credentials_to_use_lms_id.rb @@ -0,0 +1,26 @@ +class RefactorLmsCredentialsToUseLmsId < ActiveRecord::Migration[7.1] + def change + # Add lms_id column as foreign key without validation + add_column :lms_credentials, :lms_id, :bigint + add_foreign_key :lms_credentials, :lmss, column: :lms_id, validate: false + + # Migrate existing data: populate lms_id based on lms_name + reversible do |dir| + dir.up do + safety_assured do + execute <<-SQL + UPDATE lms_credentials + SET lms_id = lmss.id + FROM lmss + WHERE LOWER(lms_credentials.lms_name) = LOWER(lmss.lms_name) + SQL + end + end + end + + # Remove lms_name column (after data migration) + safety_assured do + remove_column :lms_credentials, :lms_name, :string + end + end +end diff --git a/db/migrate/20260304000001_validate_lms_credentials_lms_id_fk.rb b/db/migrate/20260304000001_validate_lms_credentials_lms_id_fk.rb new file mode 100644 index 00000000..a80bca51 --- /dev/null +++ b/db/migrate/20260304000001_validate_lms_credentials_lms_id_fk.rb @@ -0,0 +1,5 @@ +class ValidateLmsCredentialsLmsIdFk < ActiveRecord::Migration[7.1] + def change + validate_foreign_key :lms_credentials, :lmss + end +end diff --git a/db/schema.rb b/db/schema.rb index 52c429cc..b9dea08c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_10_01_192900) do +ActiveRecord::Schema[7.2].define(version: 2026_03_04_000001) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -159,7 +159,6 @@ create_table "lms_credentials", force: :cascade do |t| t.bigint "user_id" - t.string "lms_name" t.string "username" t.string "password" t.string "token" @@ -168,6 +167,7 @@ t.datetime "updated_at", null: false t.string "external_user_id" t.datetime "expire_time" + t.bigint "lms_id" t.index ["user_id"], name: "index_lms_credentials_on_user_id" end @@ -232,6 +232,7 @@ add_foreign_key "extensions", "assignments" add_foreign_key "extensions", "users", column: "last_processed_by_id" add_foreign_key "form_settings", "courses" + add_foreign_key "lms_credentials", "lmss" add_foreign_key "lms_credentials", "users" add_foreign_key "requests", "assignments" add_foreign_key "requests", "courses" diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index cb795dab..2a44ea6b 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -14,8 +14,9 @@ def test_auth let(:user) do User.create!(email: 'test@example.com', canvas_uid: '123').tap do |u| + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } u.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now diff --git a/spec/controllers/concerns/token_refreshable_spec.rb b/spec/controllers/concerns/token_refreshable_spec.rb index 599d8e23..4a7f308a 100644 --- a/spec/controllers/concerns/token_refreshable_spec.rb +++ b/spec/controllers/concerns/token_refreshable_spec.rb @@ -21,8 +21,9 @@ def current_user let(:user) do User.create!(email: 'test@example.com', canvas_uid: '123').tap do |u| + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } u.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 10.minutes.from_now diff --git a/spec/controllers/course_settings_controller_spec.rb b/spec/controllers/course_settings_controller_spec.rb index c6ac03a8..7d273e79 100644 --- a/spec/controllers/course_settings_controller_spec.rb +++ b/spec/controllers/course_settings_controller_spec.rb @@ -6,8 +6,9 @@ let(:course) { Course.create!(course_name: 'Test Course', canvas_id: '123') } before do + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } instructor.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now @@ -190,7 +191,7 @@ before do session[:user_id] = student.canvas_uid student.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'student_token', refresh_token: 'student_refresh_token', expire_time: 1.hour.from_now diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index 39629b9d..08c2b8ee 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -8,10 +8,11 @@ let(:course_settings) { CourseSettings.create!(course: course, enable_extensions: true) } before do + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } session[:user_id] = user.canvas_uid UserToCourse.create!(user: user, course: course, role: 'student') user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now @@ -118,7 +119,8 @@ describe 'GET #new' do before do # Create a fake LMS credential with a token - user.lms_credentials.create!(lms_name: 'canvas', token: 'fake_token', expire_time: 1.hour.from_now) + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } + user.lms_credentials.create!(lms_id: 1, token: 'fake_token', expire_time: 1.hour.from_now) allow(Course).to receive(:fetch_courses).and_return([ { @@ -167,7 +169,8 @@ describe 'GET #enrollments' do before do # Create LMS credentials so user has a token - user.lms_credentials.create!(lms_name: 'canvas', token: 'fake_token', expire_time: 1.hour.from_now) + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } + user.lms_credentials.create!(lms_id: 1, token: 'fake_token', expire_time: 1.hour.from_now) # Add user as a teacher so they are allowed to view enrollments UserToCourse.create!(user: user, course: course, role: 'teacher') diff --git a/spec/controllers/requests_controller_spec.rb b/spec/controllers/requests_controller_spec.rb index 810033d2..d8efd148 100644 --- a/spec/controllers/requests_controller_spec.rb +++ b/spec/controllers/requests_controller_spec.rb @@ -29,7 +29,7 @@ CourseToLms.create!(course:, lms_id: 1) user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now @@ -294,7 +294,7 @@ session[:user_id] = instructor.canvas_uid UserToCourse.create!(user: instructor, course: course, role: 'teacher') instructor.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'instructor_token', refresh_token: 'instructor_refresh', expire_time: 1.hour.from_now @@ -494,7 +494,7 @@ # Create credentials for user user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now diff --git a/spec/factories/lms.rb b/spec/factories/lms.rb index c5c6a006..95bd90e3 100644 --- a/spec/factories/lms.rb +++ b/spec/factories/lms.rb @@ -1,7 +1,18 @@ FactoryBot.define do factory :lms do - id { 1 } # Explicitly set id to 1 (must be hardcoded) + sequence(:id) { |n| n } lms_name { 'Canvas' } use_auth_token { true } + + trait :canvas do + id { 1 } + lms_name { 'Canvas' } + end + + trait :gradescope do + id { 2 } + lms_name { 'Gradescope' } + use_auth_token { false } + end end end diff --git a/spec/factories/lms_credential.rb b/spec/factories/lms_credential.rb index 7dff4782..4e3b097d 100644 --- a/spec/factories/lms_credential.rb +++ b/spec/factories/lms_credential.rb @@ -1,9 +1,16 @@ FactoryBot.define do factory :lms_credential do association :user - lms_name { 'canvas' } + lms_id { 1 } token { 'fake_token' } refresh_token { 'fake_refresh_token' } expire_time { 1.hour.from_now } + + before(:create) do |credential| + # Ensure the LMS with id: 1 exists + unless Lms.exists?(id: 1) + Lms.create!(id: 1, lms_name: 'Canvas', use_auth_token: true) + end + end end end diff --git a/spec/features/accessibility_spec.rb b/spec/features/accessibility_spec.rb index 824f448b..8ef91245 100644 --- a/spec/features/accessibility_spec.rb +++ b/spec/features/accessibility_spec.rb @@ -165,8 +165,8 @@ def wait_for_page_to_load # Set a default wait time Capybara.default_max_wait_time = 3 - create(:lms_credential, user: teacher1, lms_name: 'canvas') - create(:lms_credential, user: student1, lms_name: 'canvas') + create(:lms_credential, user: teacher1, lms_id: 1) + create(:lms_credential, user: student1, lms_id: 1) stub_request(:get, %r{#{ENV.fetch('CANVAS_URL')}/api/v1/courses/.*}) .to_return(status: 200, body: [].to_json) diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index 7d30c67d..accc1140 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -40,8 +40,9 @@ it 'returns the correct user for auto approval' do course = described_class.create!(canvas_id: 'canvas_123', course_name: 'Test', course_code: 'TEST101') user = User.create!(email: 'test@example.com', canvas_uid: '123') + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now diff --git a/spec/models/lms_credential_spec.rb b/spec/models/lms_credential_spec.rb index 3a845caf..121115dc 100644 --- a/spec/models/lms_credential_spec.rb +++ b/spec/models/lms_credential_spec.rb @@ -5,7 +5,6 @@ # # id :bigint not null, primary key # expire_time :datetime -# lms_name :string # password :string # refresh_token :string # token :string @@ -13,6 +12,7 @@ # created_at :datetime not null # updated_at :datetime not null # external_user_id :string +# lms_id :bigint # user_id :bigint # # Indexes @@ -21,6 +21,7 @@ # # Foreign Keys # +# fk_rails_... (lms_id => lmss.id) # fk_rails_... (user_id => users.id) # require 'rails_helper' @@ -40,10 +41,11 @@ def self.mock_get_service(token, refresh_token) RSpec.describe LmsCredential, type: :model do describe 'Token Encryption' do let(:user) { User.create!(email: 'test@example.com') } + let!(:lms) { Lms.find_or_create_by(id: 1) { |lms| lms.lms_name = 'Canvas'; lms.use_auth_token = true } } let!(:credential) do described_class.create!( user: user, - lms_name: 'ExampleLMS', + lms_id: lms.id, username: 'testuser', password: 'testpassword', token: 'sensitive_token', diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb index 9e4156f3..51fe8a35 100644 --- a/spec/models/request_spec.rb +++ b/spec/models/request_spec.rb @@ -70,8 +70,9 @@ before do UserToCourse.create!(user: user, course: course, role: 'student') + create(:lms) user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a929e1d7..feabdbc3 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -31,8 +31,9 @@ context 'when the token is still valid' do before do + Lms.find_or_create_by(id: 1) { |lms| lms.lms_name = 'Canvas'; lms.use_auth_token = true } user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -46,8 +47,9 @@ context 'when the token is expired' do before do + Lms.find_or_create_by(id: 1) { |lms| lms.lms_name = 'Canvas'; lms.use_auth_token = true } user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'expired_token', refresh_token: 'refresh_token', expire_time: 1.hour.ago @@ -64,8 +66,9 @@ let(:user) { described_class.create!(email: 'test@example.com', canvas_uid: '123') } it 'returns the correct credentials for a user' do + Lms.find_or_create_by(id: 1) { |lms| lms.lms_name = 'Canvas'; lms.use_auth_token = true } user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -81,8 +84,9 @@ context 'when token does not expire soon' do before do + Lms.find_or_create_by(id: 1) { |lms| lms.lms_name = 'Canvas'; lms.use_auth_token = true } user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -96,8 +100,9 @@ context 'when token expires soon and is refreshed' do let(:credential) do + Lms.find_or_create_by(id: 1) { |lms| lms.lms_name = 'Canvas'; lms.use_auth_token = true } user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'stale_token', refresh_token: 'refresh_token', expire_time: 5.minutes.from_now From d2bbb10ac851ab832dfadf409693b59ba97edc44 Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Thu, 5 Mar 2026 15:52:26 -0800 Subject: [PATCH 02/19] Implement UserToCoursesController#toggle_allow_extended_requests - 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 --- app/controllers/user_to_courses_controller.rb | 50 +++++++++++++++---- app/models/user_to_course.rb | 4 ++ .../user_to_courses_controller_spec.rb | 12 +++-- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/app/controllers/user_to_courses_controller.rb b/app/controllers/user_to_courses_controller.rb index 70451135..d06d414b 100644 --- a/app/controllers/user_to_courses_controller.rb +++ b/app/controllers/user_to_courses_controller.rb @@ -1,21 +1,49 @@ class UserToCoursesController < ApplicationController - before_action :authenticate_user + before_action :authenticate_user! before_action :set_course + before_action :set_enrollment + before_action :authorize_instructor! def toggle_allow_extended_requests - @enrollment = @course.user_to_courses.find(params[:id]) - - unless @role == 'instructor' - Rails.logger.error "Role #{@role} does not have permission to toggle allow_extended_requests" - flash.now[:alert] = 'You do not have permission to perform this action.' - return render json: { redirect_to: course_path(@course) }, status: :forbidden - end - if @enrollment.update(allow_extended_requests: params[:allow_extended_requests]) render json: { success: true }, status: :ok else - flash[:alert] = "Failed to update enrollment: #{@enrollment.errors.full_messages.to_sentence}" - render json: { redirect_to: course_path(@course) }, status: :unprocessable_entity + render json: { + success: false, + errors: @enrollment.errors.full_messages, + redirect_to: courses_path + }, status: :unprocessable_entity + end + end + + private + + def authenticate_user! + user_id = session[:user_id] + @current_user = User.find_by(canvas_uid: user_id) if user_id + redirect_to root_path unless @current_user + end + + def set_course + @course = Course.find_by(id: params[:course_id]) + unless @course + flash[:alert] = 'Course not found.' + redirect_to courses_path + end + end + + def set_enrollment + @enrollment = UserToCourse.find(params[:id]) + end + + def authorize_instructor! + user_to_course = UserToCourse.find_by(user: @current_user, course: @course) + unless user_to_course&.teacher? + render json: { + success: false, + error: 'Forbidden', + redirect_to: courses_path + }, status: :forbidden end end end diff --git a/app/models/user_to_course.rb b/app/models/user_to_course.rb index abb63fa2..822fb8af 100644 --- a/app/models/user_to_course.rb +++ b/app/models/user_to_course.rb @@ -46,6 +46,10 @@ def student? role == 'student' end + def teacher? + role == 'teacher' + end + def self.roles [ 'student' ] + UserToCourse.staff_roles end diff --git a/spec/controllers/user_to_courses_controller_spec.rb b/spec/controllers/user_to_courses_controller_spec.rb index eaaa61e7..90c53c2f 100644 --- a/spec/controllers/user_to_courses_controller_spec.rb +++ b/spec/controllers/user_to_courses_controller_spec.rb @@ -9,11 +9,12 @@ describe 'PATCH #toggle_allow_extended_requests' do context 'when user is an instructor' do before do + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } UserToCourse.create!(user: instructor, course: course, role: 'teacher') student_enrollment session[:user_id] = instructor.canvas_uid instructor.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now @@ -65,10 +66,11 @@ context 'when user is a student' do before do + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } student_enrollment session[:user_id] = student_user.canvas_uid student_user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now @@ -99,10 +101,11 @@ context 'when course does not exist' do before do + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } student_enrollment session[:user_id] = instructor.canvas_uid instructor.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now @@ -123,10 +126,11 @@ context 'when enrollment does not exist' do before do + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } UserToCourse.create!(user: instructor, course: course, role: 'teacher') session[:user_id] = instructor.canvas_uid instructor.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now From 6f04fe7b5fea4d5a571ce16d96c6edfd0afca396 Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Sun, 29 Mar 2026 14:19:48 -0700 Subject: [PATCH 03/19] patch + rspec tests --- app/jobs/sync_all_course_assignments_job.rb | 16 ++- lib/lmss/base_assignment.rb | 1 + lib/lmss/canvas/assignment.rb | 7 +- .../sync_all_course_assignments_job_spec.rb | 105 ++++++++++++++++-- 4 files changed, 116 insertions(+), 13 deletions(-) diff --git a/app/jobs/sync_all_course_assignments_job.rb b/app/jobs/sync_all_course_assignments_job.rb index d39f9aff..cec3ebd1 100644 --- a/app/jobs/sync_all_course_assignments_job.rb +++ b/app/jobs/sync_all_course_assignments_job.rb @@ -47,8 +47,10 @@ def sync_assignment(course_to_lms, lms_assignment, results) # Use shared LmsAssignment to populate Assignment assignment.name = lms_assignment.name - assignment.due_date = lms_assignment.due_date - assignment.late_due_date = lms_assignment.late_due_date + unless preserve_existing_dates?(assignment, lms_assignment) + assignment.due_date = lms_assignment.due_date + assignment.late_due_date = lms_assignment.late_due_date + end assignment.external_assignment_id = lms_assignment.id if assignment.new_record? @@ -60,4 +62,14 @@ def sync_assignment(course_to_lms, lms_assignment, results) end assignment.save! end + + private + + def preserve_existing_dates?(assignment, lms_assignment) + return false if assignment.new_record? + return false unless lms_assignment.is_a?(Lmss::Canvas::Assignment) + return false if lms_assignment.base_date_present? + + assignment.due_date.present? || assignment.late_due_date.present? + end end diff --git a/lib/lmss/base_assignment.rb b/lib/lmss/base_assignment.rb index 26c02376..1ecc38da 100644 --- a/lib/lmss/base_assignment.rb +++ b/lib/lmss/base_assignment.rb @@ -4,5 +4,6 @@ def id = raise(NotImplementedError) def name = raise(NotImplementedError) def due_date = raise(NotImplementedError) def late_due_date = raise(NotImplementedError) + def base_date_present? = false end end diff --git a/lib/lmss/canvas/assignment.rb b/lib/lmss/canvas/assignment.rb index 4b273238..07d284a1 100644 --- a/lib/lmss/canvas/assignment.rb +++ b/lib/lmss/canvas/assignment.rb @@ -1,15 +1,20 @@ module Lmss module Canvas class Assignment < BaseAssignment - attr_reader :id, :name, :due_date, :late_due_date + attr_reader :id, :name, :due_date, :late_due_date, :base_date def initialize(data) @id = data['id'] @name = data['name'] + @base_date = data['base_date'] @due_date = extract_date_field(data, 'due_at') @late_due_date = extract_date_field(data, 'lock_at') end + def base_date_present? + @base_date.is_a?(Hash) && @base_date.any? + end + private def extract_date_field(assignment_data, field_name) diff --git a/spec/jobs/sync_all_course_assignments_job_spec.rb b/spec/jobs/sync_all_course_assignments_job_spec.rb index 079b0dd1..976e2c83 100644 --- a/spec/jobs/sync_all_course_assignments_job_spec.rb +++ b/spec/jobs/sync_all_course_assignments_job_spec.rb @@ -114,6 +114,43 @@ end end + context 'when Canvas omits base_date metadata' do + let(:canvas_assignments) do + [ + Lmss::Canvas::Assignment.new( + 'id' => '123', + 'name' => 'Assignment 1', + 'due_at' => '2025-01-30T23:59:00Z', + 'lock_at' => '2025-02-05T23:59:00Z', + 'base_date' => nil + ) + ] + end + + it 'preserves existing due dates for Canvas assignments' do + existing_assignment = create(:assignment, + course_to_lms: course_to_lms, + external_assignment_id: '123', + due_date: DateTime.parse('2025-01-15T23:59:00Z'), + late_due_date: DateTime.parse('2025-01-20T23:59:00Z') + ) + + described_class.perform_now(course_to_lms.id, sync_user.id) + + existing_assignment.reload + expect(existing_assignment.due_date).to eq(DateTime.parse('2025-01-15T23:59:00Z')) + expect(existing_assignment.late_due_date).to eq(DateTime.parse('2025-01-20T23:59:00Z')) + end + + it 'still sets dates for newly imported assignments' do + described_class.perform_now(course_to_lms.id, sync_user.id) + + assignment = Assignment.find_by(external_assignment_id: '123') + expect(assignment.due_date).to eq(DateTime.parse('2025-01-30T23:59:00Z')) + expect(assignment.late_due_date).to eq(DateTime.parse('2025-02-05T23:59:00Z')) + end + end + context 'when sync_user is not staff' do let(:student_user) { course.students.first } @@ -130,20 +167,68 @@ end end + describe '#sync_assignment' do + let(:job) { described_class.new } + let(:results) do + { + added_assignments: 0, + updated_assignments: 0, + unchanged_assignments: 0, + deleted_assignments: 0 + } + end + + it 'creates a new assignment and updates results' do + target_course_to_lms = course_to_lms + + lms_assignment = build_canvas_assignment( + 'id' => 'a123', + 'name' => 'HW1', + 'due_at' => '2025-01-15T23:59:00Z', + 'lock_at' => '2025-01-20T23:59:00Z' + ) - # THIS MUST BE REWRITTEN - # This was moved from Course.sync_assignment - # It is now a helper method within the job. - describe '.sync_assignment' do - it 'creates or updates an assignment' do - pending 'moved from course_spec and should be rewritten' - assignment_data = { 'id' => 'a123', 'name' => 'HW1', 'due_at' => 1.day.from_now.to_s } expect do - described_class.sync_assignment(course_to_lms, assignment_data) - end.to change(Assignment, :count).by(1) + job.send(:sync_assignment, target_course_to_lms, lms_assignment, results) + end.to change { Assignment.where(course_to_lms_id: target_course_to_lms.id).count }.by(1) - assignment = Assignment.last + assignment = Assignment.find_by(course_to_lms_id: target_course_to_lms.id, external_assignment_id: 'a123') expect(assignment.name).to eq('HW1') + expect(assignment.due_date).to eq(DateTime.parse('2025-01-15T23:59:00Z')) + expect(assignment.late_due_date).to eq(DateTime.parse('2025-01-20T23:59:00Z')) + expect(results[:added_assignments]).to eq(1) + expect(results[:updated_assignments]).to eq(0) + expect(results[:unchanged_assignments]).to eq(0) + end + + it 'updates an existing assignment and updates results' do + target_course_to_lms = course_to_lms + + existing_assignment = create(:assignment, + course_to_lms: target_course_to_lms, + external_assignment_id: 'a123', + name: 'Old HW Name', + due_date: DateTime.parse('2025-01-10T23:59:00Z') + ) + + lms_assignment = build_canvas_assignment( + 'id' => 'a123', + 'name' => 'HW1 Updated', + 'due_at' => '2025-01-25T23:59:00Z', + 'lock_at' => nil + ) + + expect do + job.send(:sync_assignment, target_course_to_lms, lms_assignment, results) + end.not_to change { Assignment.where(course_to_lms_id: target_course_to_lms.id).count } + + existing_assignment.reload + expect(existing_assignment.name).to eq('HW1 Updated') + expect(existing_assignment.due_date).to eq(DateTime.parse('2025-01-25T23:59:00Z')) + expect(existing_assignment.late_due_date).to be_nil + expect(results[:added_assignments]).to eq(0) + expect(results[:updated_assignments]).to eq(1) + expect(results[:unchanged_assignments]).to eq(0) end end From acfe1552aeab32b225fda7bfe620134224ba0ff2 Mon Sep 17 00:00:00 2001 From: Sarj-Basim Al Harbi <123058662+ba88im@users.noreply.github.com> Date: Wed, 1 Apr 2026 17:23:22 -0700 Subject: [PATCH 04/19] Hide checkbox and status columns on mobile requests page 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) --- app/views/requests/instructor_index.html.erb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/views/requests/instructor_index.html.erb b/app/views/requests/instructor_index.html.erb index 4274f8ef..4f6a8d54 100644 --- a/app/views/requests/instructor_index.html.erb +++ b/app/views/requests/instructor_index.html.erb @@ -31,7 +31,7 @@ id="requests-table"> - + Actions - Name - Assignment + Name + Assignment Student ID Requested At Original Due Date - Requested Due Date + Requested Due Date # of Days - Status + Status @@ -143,7 +143,7 @@ -
+
-
\ No newline at end of file + diff --git a/app/views/courses/index.html.erb b/app/views/courses/index.html.erb index b1323b6e..09a2c713 100644 --- a/app/views/courses/index.html.erb +++ b/app/views/courses/index.html.erb @@ -33,7 +33,7 @@ <%= user_to_course.course.course_name %> <%= user_to_course.course.course_code %> - <%= user_to_course.role.capitalize %> + <%= user_to_course.display_role %> <% end %> <% end %> diff --git a/app/views/courses/new.html.erb b/app/views/courses/new.html.erb index a6585801..dfd1a870 100644 --- a/app/views/courses/new.html.erb +++ b/app/views/courses/new.html.erb @@ -46,7 +46,10 @@ <%= course.dig("term", "name") %> <%= course["course_code"] %> <%= course["name"] %> - <%= course['enrollments'].find { |enrollment| %w[teacher ta].include?(enrollment['type']) }['type'].capitalize %> + + <% staff_enrollment = course['enrollments'].find { |enrollment| UserToCourse.staff_enrollment?(enrollment) } %> + <%= UserToCourse.display_role(UserToCourse.role_from_canvas_enrollment(staff_enrollment)) %> + <% end %> diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index 383e1b05..cc3a98e5 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -29,6 +29,14 @@ expect(response).to render_template(:index) end + it 'includes Lead TA enrollments in staff courses' do + UserToCourse.create!(user: user, course: student_course, role: 'leadta') + + get :index + + expect(assigns(:teacher_courses).map(&:role)).to include('leadta') + end + context 'semester grouping' do let(:spring_course) { Course.create!(course_name: 'Spring Course', canvas_id: 'sp1', course_code: 'SP101', semester: 'Spring 2026') } let(:fall_course) { Course.create!(course_name: 'Fall Course', canvas_id: 'fa1', course_code: 'FA101', semester: 'Fall 2025') } @@ -112,6 +120,21 @@ expect(response).to redirect_to(courses_path) expect(flash[:notice]).to eq('Selected courses and their assignments have been imported successfully.') end + + it 'imports courses where the user is enrolled with the Canvas Lead TA role' do + lead_ta_course = { + 'id' => '999', + 'name' => 'Lead TA Canvas Course', + 'course_code' => 'LTA101', + 'enrollments' => [ { 'type' => 'ta', 'role' => 'Lead TA' } ] + } + allow(Course).to receive(:fetch_courses).and_return([ lead_ta_course ]) + allow(Course).to receive(:create_or_update_from_canvas) + + post :create, params: { courses: [ '999' ] } + + expect(Course).to have_received(:create_or_update_from_canvas).with(lead_ta_course, 'fake_token', user) + end end describe 'POST #sync_assignments' do @@ -139,6 +162,14 @@ headers: { 'Authorization' => 'Bearer fake_token' } ).to_return(status: 200, body: '[]', headers: {}) end + stub_request(:get, "#{ENV.fetch('CANVAS_URL', nil)}/api/v1/courses/456/users") + .with( + query: { + 'enrollment_role' => 'Lead TA', + 'per_page' => '100' + }, + headers: { 'Authorization' => 'Bearer fake_token' } + ).to_return(status: 200, body: '[]', headers: {}) end context 'when user is a teacher (course admin)' do @@ -215,7 +246,7 @@ 'id' => '103', 'name' => 'Test Course 103', 'course_code' => 'TC103', - 'enrollments' => [ { 'type' => 'teacher' } ], + 'enrollments' => [ { 'type' => 'ta', 'role' => 'Lead TA' } ], 'term' => { 'name' => 'Fall 2025' } }, { @@ -246,8 +277,9 @@ expect(assigns(:courses_student)).not_to be_empty # Teacher course should be categorized correctly - teacher_course = assigns(:courses_teacher).first - expect(teacher_course['enrollments'].first['type']).to eq('teacher') + teacher_course_roles = assigns(:courses_teacher).map { |canvas_course| canvas_course['enrollments'].first } + expect(teacher_course_roles).to include(hash_including('type' => 'teacher')) + expect(teacher_course_roles).to include(hash_including('role' => 'Lead TA')) # Student course should be categorized correctly student_course = assigns(:courses_student).first diff --git a/spec/facades/canvas_facade_spec.rb b/spec/facades/canvas_facade_spec.rb index 94a966a3..f935f019 100644 --- a/spec/facades/canvas_facade_spec.rb +++ b/spec/facades/canvas_facade_spec.rb @@ -169,6 +169,24 @@ end end + describe '#get_all_course_users' do + let(:course) { instance_double(Course, canvas_id: course_id) } + + it 'uses enrollment_type for built-in Canvas roles' do + stubs.get("courses/#{course_id}/users?per_page=100&enrollment_type[]=ta") { [ 200, {}, '[]' ] } + + expect(facade.get_all_course_users(course, 'ta')).to eq([]) + stubs.verify_stubbed_calls + end + + it 'uses enrollment_role for the custom Lead TA Canvas role' do + stubs.get("courses/#{course_id}/users?per_page=100&enrollment_role=Lead+TA") { [ 200, {}, '[]' ] } + + expect(facade.get_all_course_users(course, 'leadta')).to eq([]) + stubs.verify_stubbed_calls + end + end + describe 'get_course' do before do stubs.get("courses/#{course_id}") { [ 200, {}, '{}' ] } diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index d85efb78..6446230c 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -54,6 +54,16 @@ end end + describe '#user_role' do + it 'treats leadta enrollments as instructors' do + course = described_class.create!(canvas_id: 'canvas_leadta', course_name: 'Test', course_code: 'TEST101') + user = User.create!(email: 'leadta@example.com', canvas_uid: 'leadta_123') + UserToCourse.create!(user: user, course: course, role: 'leadta') + + expect(course.user_role(user)).to eq('instructor') + end + end + describe '.fetch_courses' do it 'returns parsed JSON if response is successful' do stub_request(:get, %r{api/v1/courses}) @@ -235,6 +245,16 @@ end end + describe '#sync_all_enrollments_from_canvas' do + let!(:course) { described_class.create!(canvas_id: 'canvas_all_roles', course_name: 'User Sync', course_code: 'USYNC') } + + it 'syncs every supported internal role, including leadta' do + expect(SyncUsersFromCanvasJob).to receive(:perform_now).with(course.id, 999, %w[student teacher ta leadta]) + + course.sync_all_enrollments_from_canvas(999) + end + end + describe '.create_or_update_from_canvas' do let(:user) { create(:user, id: 999, canvas_uid: 'u1', name: 'User 1', email: 'user1@example.com') } diff --git a/spec/models/user_to_course_spec.rb b/spec/models/user_to_course_spec.rb new file mode 100644 index 00000000..29aef034 --- /dev/null +++ b/spec/models/user_to_course_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe UserToCourse, type: :model do + describe 'Lead TA role support' do + it 'treats leadta as a supported staff role' do + user_to_course = build(:user_to_course, role: 'leadta') + + expect(described_class.staff_roles).to include('leadta') + expect(described_class.roles).to include('leadta') + expect(user_to_course).to be_staff + end + + it 'treats leadta as a course admin role' do + user_to_course = build(:user_to_course, role: 'leadta') + + expect(user_to_course).to be_course_admin + end + end + + describe '.role_from_canvas_enrollment' do + it 'normalizes Canvas Lead TA custom role enrollments' do + enrollment = { 'type' => 'ta', 'role' => 'Lead TA' } + + expect(described_class.role_from_canvas_enrollment(enrollment)).to eq('leadta') + end + + it 'falls back to the Canvas enrollment type for built-in roles' do + enrollment = { 'type' => 'teacher' } + + expect(described_class.role_from_canvas_enrollment(enrollment)).to eq('teacher') + end + end + + describe '#display_role' do + it 'formats leadta as Lead TA' do + user_to_course = build(:user_to_course, role: 'leadta') + + expect(user_to_course.display_role).to eq('Lead TA') + end + end +end From ec48c398fb094c2f1821e83f0c9455ac26987637 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 6 Apr 2026 14:07:08 -0700 Subject: [PATCH 08/19] Add pending notification columns to course_settings --- ...5234_add_pending_notification_to_course_settings.rb | 10 ++++++++++ db/schema.rb | 5 ++++- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260406175234_add_pending_notification_to_course_settings.rb diff --git a/db/migrate/20260406175234_add_pending_notification_to_course_settings.rb b/db/migrate/20260406175234_add_pending_notification_to_course_settings.rb new file mode 100644 index 00000000..fceca5e9 --- /dev/null +++ b/db/migrate/20260406175234_add_pending_notification_to_course_settings.rb @@ -0,0 +1,10 @@ +class AddPendingNotificationToCourseSettings < ActiveRecord::Migration[7.2] + def change + safety_assured do + change_table :course_settings, bulk: true do |t| + t.string :pending_notification_frequency, default: nil + t.string :pending_notification_email, default: nil + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 848df19c..51f871e5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,9 +10,10 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_03_06_000001) do +ActiveRecord::Schema[7.2].define(version: 2026_04_06_175234) do create_schema "hypershield" + # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -105,6 +106,8 @@ t.boolean "enable_gradescope", default: false t.string "gradescope_course_url" t.boolean "extend_late_due_date", default: true, null: false + t.string "pending_notification_frequency" + t.string "pending_notification_email" t.index ["course_id"], name: "index_course_settings_on_course_id" end From 7b06bea9f7ee649cb6d1724c00b3a977f910c4e0 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 6 Apr 2026 14:07:21 -0700 Subject: [PATCH 09/19] Add pending notification validations, normalization, and scope to CourseSettings --- app/models/course_settings.rb | 13 ++++ spec/factories/course_settings.rb | 10 +++ spec/models/course_settings_spec.rb | 102 ++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+) diff --git a/app/models/course_settings.rb b/app/models/course_settings.rb index 10488a4c..a999a20c 100644 --- a/app/models/course_settings.rb +++ b/app/models/course_settings.rb @@ -49,12 +49,25 @@ class CourseSettings < ApplicationRecord {{course_name}} Staff LIQUID + VALID_NOTIFICATION_FREQUENCIES = %w[daily weekly].freeze + belongs_to :course + before_validation -> { self.pending_notification_frequency = nil if pending_notification_frequency.blank? } + before_validation -> { self.pending_notification_email = nil if pending_notification_email.blank? } before_save :ensure_system_user_for_auto_approval + before_save -> { self.pending_notification_email = nil if pending_notification_frequency.nil? } validate :gradescope_url_is_valid, if: :enable_gradescope? + validates :pending_notification_frequency, inclusion: { in: VALID_NOTIFICATION_FREQUENCIES }, allow_nil: true + validates :pending_notification_email, presence: true, format: { with: /\A[^@\s]+@[^@\s]+\z/ }, + if: -> { pending_notification_frequency.present? } after_save :create_or_update_gradescope_link + scope :with_pending_notifications, ->(frequency) { + where(pending_notification_frequency: frequency) + .where.not(pending_notification_email: [nil, '']) + } + def automatic_approval_enabled? return false unless enable_extensions? diff --git a/spec/factories/course_settings.rb b/spec/factories/course_settings.rb index 2abddc97..682c96f6 100644 --- a/spec/factories/course_settings.rb +++ b/spec/factories/course_settings.rb @@ -36,5 +36,15 @@ association :course enable_extensions { true } auto_approve_days { 0 } + + trait :with_daily_notifications do + pending_notification_frequency { 'daily' } + pending_notification_email { 'instructor@example.com' } + end + + trait :with_weekly_notifications do + pending_notification_frequency { 'weekly' } + pending_notification_email { 'instructor@example.com' } + end end end diff --git a/spec/models/course_settings_spec.rb b/spec/models/course_settings_spec.rb index 31cd24e8..94848fa6 100644 --- a/spec/models/course_settings_spec.rb +++ b/spec/models/course_settings_spec.rb @@ -177,6 +177,108 @@ end end + describe 'pending notification validations' do + context 'pending_notification_frequency' do + it 'accepts nil' do + course_settings.pending_notification_frequency = nil + expect(course_settings).to be_valid + end + + it 'accepts "daily"' do + course_settings.pending_notification_frequency = 'daily' + course_settings.pending_notification_email = 'test@example.com' + expect(course_settings).to be_valid + end + + it 'accepts "weekly"' do + course_settings.pending_notification_frequency = 'weekly' + course_settings.pending_notification_email = 'test@example.com' + expect(course_settings).to be_valid + end + + it 'rejects "monthly"' do + course_settings.pending_notification_frequency = 'monthly' + course_settings.pending_notification_email = 'test@example.com' + expect(course_settings).not_to be_valid + expect(course_settings.errors[:pending_notification_frequency]).to be_present + end + end + + context 'pending_notification_email' do + it 'is required when frequency is set' do + course_settings.pending_notification_frequency = 'daily' + course_settings.pending_notification_email = nil + expect(course_settings).not_to be_valid + expect(course_settings.errors[:pending_notification_email]).to be_present + end + + it 'validates email format when frequency is set' do + course_settings.pending_notification_frequency = 'daily' + course_settings.pending_notification_email = 'not-an-email' + expect(course_settings).not_to be_valid + expect(course_settings.errors[:pending_notification_email]).to be_present + end + + it 'accepts a valid email when frequency is set' do + course_settings.pending_notification_frequency = 'daily' + course_settings.pending_notification_email = 'instructor@berkeley.edu' + expect(course_settings).to be_valid + end + + it 'is not required when frequency is nil' do + course_settings.pending_notification_frequency = nil + course_settings.pending_notification_email = nil + expect(course_settings).to be_valid + end + end + + context 'normalization' do + it 'normalizes empty string frequency to nil' do + course_settings.pending_notification_frequency = '' + course_settings.valid? + expect(course_settings.pending_notification_frequency).to be_nil + end + + it 'normalizes empty string email to nil' do + course_settings.pending_notification_email = '' + course_settings.valid? + expect(course_settings.pending_notification_email).to be_nil + end + + it 'clears email when frequency is set to nil on save' do + course_settings.pending_notification_frequency = 'daily' + course_settings.pending_notification_email = 'test@example.com' + course_settings.save! + + course_settings.pending_notification_frequency = nil + course_settings.save! + course_settings.reload + + expect(course_settings.pending_notification_email).to be_nil + end + end + end + + describe '.with_pending_notifications' do + it 'returns records matching the given frequency with an email set' do + course_settings.update!(pending_notification_frequency: 'daily', pending_notification_email: 'a@example.com') + + other_course = create(:course, canvas_id: 'other_123', course_name: 'Other', course_code: 'OTHER101') + other_course.course_settings.update!(pending_notification_frequency: 'weekly', pending_notification_email: 'b@example.com') + + results = CourseSettings.with_pending_notifications('daily') + expect(results).to include(course_settings) + expect(results).not_to include(other_course.course_settings) + end + + it 'excludes records with nil email' do + course_settings.update_columns(pending_notification_frequency: 'daily', pending_notification_email: nil) + + results = CourseSettings.with_pending_notifications('daily') + expect(results).not_to include(course_settings) + end + end + describe '#extract_gradescope_course_id' do it 'extracts course ID from valid URL' do url = 'https://www.gradescope.com/courses/123456' From 6d13725cdd1fcb4bc2dc0fe1db61fdf324b85b63 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 6 Apr 2026 14:07:27 -0700 Subject: [PATCH 10/19] Add PendingRequestsNotificationJob to send digest emails --- app/jobs/pending_requests_notification_job.rb | 32 +++++++ .../pending_requests_notification_job_spec.rb | 86 +++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 app/jobs/pending_requests_notification_job.rb create mode 100644 spec/jobs/pending_requests_notification_job_spec.rb diff --git a/app/jobs/pending_requests_notification_job.rb b/app/jobs/pending_requests_notification_job.rb new file mode 100644 index 00000000..f94faa22 --- /dev/null +++ b/app/jobs/pending_requests_notification_job.rb @@ -0,0 +1,32 @@ +class PendingRequestsNotificationJob < ApplicationJob + queue_as :default + + def perform(frequency) + CourseSettings.with_pending_notifications(frequency).includes(:course).find_each do |cs| + course = cs.course + pending_count = Request.where(course_id: course.id, status: 'pending').count + next if pending_count.zero? + + requests_url = "#{ENV.fetch('APP_HOST', nil)}/courses/#{course.id}/requests" + + EmailService.send_email( + to: cs.pending_notification_email, + from: ENV.fetch('DEFAULT_FROM_EMAIL'), + reply_to: cs.reply_email.presence || ENV.fetch('DEFAULT_FROM_EMAIL'), + subject_template: '{{pending_count}} Pending Extension Request{{plural}} - {{course_code}}', + body_template: "Hello,\n\nYou have {{pending_count}} pending extension request{{plural}} " \ + "in {{course_name}} ({{course_code}}).\n\n" \ + "Please review them at: {{requests_url}}\n\n" \ + "Thank you,\nFlextensions", + mapping: { + 'pending_count' => pending_count.to_s, + 'plural' => pending_count == 1 ? '' : 's', + 'course_name' => course.course_name, + 'course_code' => course.course_code, + 'requests_url' => requests_url + }, + deliver_later: false + ) + end + end +end diff --git a/spec/jobs/pending_requests_notification_job_spec.rb b/spec/jobs/pending_requests_notification_job_spec.rb new file mode 100644 index 00000000..3b368b61 --- /dev/null +++ b/spec/jobs/pending_requests_notification_job_spec.rb @@ -0,0 +1,86 @@ +require 'rails_helper' + +RSpec.describe PendingRequestsNotificationJob, type: :job do + let(:course) { create(:course, canvas_id: 'notif_123', course_name: 'CS 101', course_code: 'CS101') } + let(:student) { create(:user, canvas_uid: 'stu_notif_1', email: 'student_notif@example.com', name: 'Student') } + let(:lms) { Lms.first } + let(:course_to_lms) { CourseToLms.create!(course: course, lms: lms, external_course_id: 'ext_123') } + let(:assignment) do + Assignment.create!( + name: 'HW1', + course_to_lms: course_to_lms, + due_date: 3.days.from_now, + external_assignment_id: 'asgn_notif_1', + enabled: true + ) + end + + before do + ActionMailer::Base.delivery_method = :test + ActionMailer::Base.deliveries.clear + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with('DEFAULT_FROM_EMAIL').and_return('flextensions@berkeley.edu') + allow(ENV).to receive(:fetch).with('APP_HOST', nil).and_return('http://localhost:3000') + end + + describe '#perform' do + it 'sends email when course has matching frequency and pending requests' do + course.course_settings.update!(pending_notification_frequency: 'daily', pending_notification_email: 'prof@example.com') + Request.create!(course: course, assignment: assignment, user: student, status: 'pending', + reason: 'Need more time', requested_due_date: 5.days.from_now) + + expect { described_class.perform_now('daily') }.to change { ActionMailer::Base.deliveries.count }.by(1) + + mail = ActionMailer::Base.deliveries.last + expect(mail.to).to eq(['prof@example.com']) + expect(mail.subject).to include('1 Pending Extension Request') + expect(mail.subject).to include('CS101') + expect(mail.body.encoded).to include("http://localhost:3000/courses/#{course.id}/requests") + end + + it 'skips courses with zero pending requests' do + course.course_settings.update!(pending_notification_frequency: 'daily', pending_notification_email: 'prof@example.com') + + expect { described_class.perform_now('daily') }.not_to(change { ActionMailer::Base.deliveries.count }) + end + + it 'only sends to courses matching the given frequency' do + course.course_settings.update!(pending_notification_frequency: 'weekly', pending_notification_email: 'prof@example.com') + Request.create!(course: course, assignment: assignment, user: student, status: 'pending', + reason: 'Need more time', requested_due_date: 5.days.from_now) + + expect { described_class.perform_now('daily') }.not_to(change { ActionMailer::Base.deliveries.count }) + end + + it 'pluralizes correctly for multiple pending requests' do + course.course_settings.update!(pending_notification_frequency: 'daily', pending_notification_email: 'prof@example.com') + 2.times do |i| + Request.create!(course: course, assignment: assignment, + user: create(:user, canvas_uid: "stu_multi_#{i}", email: "stu_multi_#{i}@example.com"), + status: 'pending', reason: 'Need time', requested_due_date: 5.days.from_now) + end + + described_class.perform_now('daily') + + mail = ActionMailer::Base.deliveries.last + expect(mail.subject).to include('2 Pending Extension Requests') + end + + it 'sends separate emails to multiple courses' do + course.course_settings.update!(pending_notification_frequency: 'daily', pending_notification_email: 'prof1@example.com') + Request.create!(course: course, assignment: assignment, user: student, status: 'pending', + reason: 'Need time', requested_due_date: 5.days.from_now) + + other_course = create(:course, canvas_id: 'notif_456', course_name: 'CS 201', course_code: 'CS201') + other_ctlms = CourseToLms.create!(course: other_course, lms: lms, external_course_id: 'ext_456') + other_assignment = Assignment.create!(name: 'HW2', course_to_lms: other_ctlms, due_date: 3.days.from_now, + external_assignment_id: 'asgn_notif_2', enabled: true) + other_course.course_settings.update!(pending_notification_frequency: 'daily', pending_notification_email: 'prof2@example.com') + other_student = create(:user, canvas_uid: 'stu_notif_2', email: 'stu_notif_2@example.com') + Request.create!(course: other_course, assignment: other_assignment, user: other_student, status: 'pending', + reason: 'Need time', requested_due_date: 5.days.from_now) + + expect { described_class.perform_now('daily') }.to change { ActionMailer::Base.deliveries.count }.by(2) + end + end +end From 48778d89e398dbc7eaf87e6123dafa10bdd3e099 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 6 Apr 2026 14:07:34 -0700 Subject: [PATCH 11/19] Permit pending notification params in CourseSettingsController --- app/controllers/course_settings_controller.rb | 4 +- .../course_settings_controller_spec.rb | 69 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/app/controllers/course_settings_controller.rb b/app/controllers/course_settings_controller.rb index 4a2cfe07..10116d88 100644 --- a/app/controllers/course_settings_controller.rb +++ b/app/controllers/course_settings_controller.rb @@ -72,7 +72,9 @@ def course_settings_params :email_subject, :email_template, :enable_slack_webhook_url, - :slack_webhook_url + :slack_webhook_url, + :pending_notification_frequency, + :pending_notification_email ) end diff --git a/spec/controllers/course_settings_controller_spec.rb b/spec/controllers/course_settings_controller_spec.rb index c6ac03a8..79143970 100644 --- a/spec/controllers/course_settings_controller_spec.rb +++ b/spec/controllers/course_settings_controller_spec.rb @@ -118,6 +118,75 @@ end end + describe 'pending notification params' do + before do + session[:user_id] = instructor.canvas_uid + UserToCourse.create!(user: instructor, course: course, role: 'instructor') + allow_any_instance_of(Course).to receive(:user_role).with(instructor).and_return('instructor') + CourseSettings.create!(course: course, enable_extensions: true) + end + + it 'persists pending notification settings' do + post :update, params: { + course_id: course.id, + course_settings: { + pending_notification_frequency: 'daily', + pending_notification_email: 'prof@berkeley.edu' + }, + tab: 'general' + } + + settings = CourseSettings.find_by(course_id: course.id) + expect(settings.pending_notification_frequency).to eq('daily') + expect(settings.pending_notification_email).to eq('prof@berkeley.edu') + end + + it 'normalizes blank frequency to nil' do + post :update, params: { + course_id: course.id, + course_settings: { + pending_notification_frequency: '', + pending_notification_email: '' + }, + tab: 'general' + } + + settings = CourseSettings.find_by(course_id: course.id) + expect(settings.pending_notification_frequency).to be_nil + end + + it 'clears stored email when frequency is set to blank' do + settings = CourseSettings.find_by(course_id: course.id) + settings.update!(pending_notification_frequency: 'daily', pending_notification_email: 'prof@berkeley.edu') + + post :update, params: { + course_id: course.id, + course_settings: { + pending_notification_frequency: '', + pending_notification_email: '' + }, + tab: 'general' + } + + settings.reload + expect(settings.pending_notification_frequency).to be_nil + expect(settings.pending_notification_email).to be_nil + end + + it 'shows validation errors for invalid email with frequency set' do + post :update, params: { + course_id: course.id, + course_settings: { + pending_notification_frequency: 'daily', + pending_notification_email: 'not-an-email' + }, + tab: 'general' + } + + expect(flash[:alert]).to include('Failed to update course settings:') + end + end + describe 'pending requests count' do let(:assignment) do # Create necessary related objects for Request From ace581cf6e0c683373dc42c2ec66067463ec598e Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 6 Apr 2026 14:07:42 -0700 Subject: [PATCH 12/19] Add rake task for sending pending request digest emails --- lib/tasks/notifications.rake | 9 +++++++++ spec/tasks/notifications_rake_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 lib/tasks/notifications.rake create mode 100644 spec/tasks/notifications_rake_spec.rb diff --git a/lib/tasks/notifications.rake b/lib/tasks/notifications.rake new file mode 100644 index 00000000..35da861f --- /dev/null +++ b/lib/tasks/notifications.rake @@ -0,0 +1,9 @@ +namespace :notifications do + desc 'Send pending request digest emails (usage: rake notifications:send_pending_digests[daily])' + task :send_pending_digests, [:frequency] => :environment do |_t, args| + frequency = args[:frequency] + abort 'Usage: rake notifications:send_pending_digests[daily|weekly]' unless %w[daily weekly].include?(frequency) + + PendingRequestsNotificationJob.perform_now(frequency) + end +end diff --git a/spec/tasks/notifications_rake_spec.rb b/spec/tasks/notifications_rake_spec.rb new file mode 100644 index 00000000..4579d8ae --- /dev/null +++ b/spec/tasks/notifications_rake_spec.rb @@ -0,0 +1,21 @@ +require 'rails_helper' +require 'rake' + +RSpec.describe 'notifications:send_pending_digests' do + before(:all) do + Rails.application.load_tasks + end + + it 'invokes PendingRequestsNotificationJob with valid frequency' do + expect(PendingRequestsNotificationJob).to receive(:perform_now).with('daily') + Rake::Task['notifications:send_pending_digests'].reenable + Rake::Task['notifications:send_pending_digests'].invoke('daily') + end + + it 'aborts with usage message for invalid frequency' do + Rake::Task['notifications:send_pending_digests'].reenable + expect { + Rake::Task['notifications:send_pending_digests'].invoke('monthly') + }.to raise_error(SystemExit) + end +end From b07c24c17c205c1d2ad69703991ed18277fcf13e Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 6 Apr 2026 14:07:47 -0700 Subject: [PATCH 13/19] Add notification frequency dropdown and email field to course settings UI --- .../controllers/course_settings_controller.js | 12 +++++++- app/views/courses/edit.html.erb | 28 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/app/javascript/controllers/course_settings_controller.js b/app/javascript/controllers/course_settings_controller.js index 19a4ff03..7ada1020 100644 --- a/app/javascript/controllers/course_settings_controller.js +++ b/app/javascript/controllers/course_settings_controller.js @@ -1,12 +1,13 @@ import { Controller } from "@hotwired/stimulus" export default class extends Controller { - static targets = ["emailField", "tab", "gradescopeField", "slackWebhookField"]; + static targets = ["emailField", "tab", "gradescopeField", "slackWebhookField", "pendingNotificationEmail"]; connect() { this.toggleEmailFields(); this.toggleSlackWebhookField(); this.toggleGradescopeFields(); + this.togglePendingNotificationEmail(); const gradescopeToggle = document.getElementById('enable-gradescope'); if (gradescopeToggle) { @@ -53,6 +54,15 @@ export default class extends Controller { } } + togglePendingNotificationEmail() { + const frequencySelect = document.getElementById('pending-notification-frequency'); + const emailField = document.getElementById('pending-notification-email'); + + if (frequencySelect && emailField) { + emailField.disabled = !frequencySelect.value; + } + } + updateUrlParam(event) { const tabName = event.currentTarget.dataset.tab; const url = new URL(window.location); diff --git a/app/views/courses/edit.html.erb b/app/views/courses/edit.html.erb index 9214779e..f19b3fde 100644 --- a/app/views/courses/edit.html.erb +++ b/app/views/courses/edit.html.erb @@ -211,6 +211,34 @@ +
+ +
+ <%= select_tag 'course_settings[pending_notification_frequency]', + options_for_select( + [['No notifications', ''], ['Daily', 'daily'], ['Once weekly (Fridays)', 'weekly']], + @course.course_settings&.pending_notification_frequency + ), + class: 'form-select', + id: 'pending-notification-frequency', + data: { action: 'change->course-settings#togglePendingNotificationEmail' } %> +
+
+ +
+ +
+ <%= email_field_tag 'course_settings[pending_notification_email]', + @course.course_settings&.pending_notification_email, + class: 'form-control', + id: 'pending-notification-email', + placeholder: 'instructor@berkeley.edu', + data: { course_settings_target: 'pendingNotificationEmail' }, + disabled: @course.course_settings&.pending_notification_frequency.blank? %> +
Weekly notifications are sent on Fridays at 5:00 PM PT.
+
+
+
<% if @course.course_settings&.enable_extensions %> From 8a8984a5a9aaf3fa10109b6d1e11c0befa68e600 Mon Sep 17 00:00:00 2001 From: Alex Date: Sun, 26 Apr 2026 14:05:03 -0700 Subject: [PATCH 14/19] Fix RuboCop offenses on email-notifications branch --- app/models/course_settings.rb | 2 +- lib/tasks/notifications.rake | 2 +- spec/jobs/pending_requests_notification_job_spec.rb | 2 +- spec/models/course_settings_spec.rb | 6 +++--- spec/tasks/notifications_rake_spec.rb | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/course_settings.rb b/app/models/course_settings.rb index a999a20c..0ef7cf6c 100644 --- a/app/models/course_settings.rb +++ b/app/models/course_settings.rb @@ -65,7 +65,7 @@ class CourseSettings < ApplicationRecord scope :with_pending_notifications, ->(frequency) { where(pending_notification_frequency: frequency) - .where.not(pending_notification_email: [nil, '']) + .where.not(pending_notification_email: [ nil, '' ]) } def automatic_approval_enabled? diff --git a/lib/tasks/notifications.rake b/lib/tasks/notifications.rake index 35da861f..978c5291 100644 --- a/lib/tasks/notifications.rake +++ b/lib/tasks/notifications.rake @@ -1,6 +1,6 @@ namespace :notifications do desc 'Send pending request digest emails (usage: rake notifications:send_pending_digests[daily])' - task :send_pending_digests, [:frequency] => :environment do |_t, args| + task :send_pending_digests, [ :frequency ] => :environment do |_t, args| frequency = args[:frequency] abort 'Usage: rake notifications:send_pending_digests[daily|weekly]' unless %w[daily weekly].include?(frequency) diff --git a/spec/jobs/pending_requests_notification_job_spec.rb b/spec/jobs/pending_requests_notification_job_spec.rb index 3b368b61..54aa990c 100644 --- a/spec/jobs/pending_requests_notification_job_spec.rb +++ b/spec/jobs/pending_requests_notification_job_spec.rb @@ -32,7 +32,7 @@ expect { described_class.perform_now('daily') }.to change { ActionMailer::Base.deliveries.count }.by(1) mail = ActionMailer::Base.deliveries.last - expect(mail.to).to eq(['prof@example.com']) + expect(mail.to).to eq([ 'prof@example.com' ]) expect(mail.subject).to include('1 Pending Extension Request') expect(mail.subject).to include('CS101') expect(mail.body.encoded).to include("http://localhost:3000/courses/#{course.id}/requests") diff --git a/spec/models/course_settings_spec.rb b/spec/models/course_settings_spec.rb index 94848fa6..711a894a 100644 --- a/spec/models/course_settings_spec.rb +++ b/spec/models/course_settings_spec.rb @@ -266,15 +266,15 @@ other_course = create(:course, canvas_id: 'other_123', course_name: 'Other', course_code: 'OTHER101') other_course.course_settings.update!(pending_notification_frequency: 'weekly', pending_notification_email: 'b@example.com') - results = CourseSettings.with_pending_notifications('daily') + results = described_class.with_pending_notifications('daily') expect(results).to include(course_settings) expect(results).not_to include(other_course.course_settings) end it 'excludes records with nil email' do - course_settings.update_columns(pending_notification_frequency: 'daily', pending_notification_email: nil) + course_settings.update_columns(pending_notification_frequency: 'daily', pending_notification_email: nil) # rubocop:disable Rails/SkipsModelValidations - results = CourseSettings.with_pending_notifications('daily') + results = described_class.with_pending_notifications('daily') expect(results).not_to include(course_settings) end end diff --git a/spec/tasks/notifications_rake_spec.rb b/spec/tasks/notifications_rake_spec.rb index 4579d8ae..638d1c4e 100644 --- a/spec/tasks/notifications_rake_spec.rb +++ b/spec/tasks/notifications_rake_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' require 'rake' -RSpec.describe 'notifications:send_pending_digests' do +RSpec.describe 'notifications:send_pending_digests' do # rubocop:disable RSpec/DescribeClass before(:all) do Rails.application.load_tasks end From 72e667761ad1c283b3b0afd87857b6fe5d6e3ea7 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 21:29:03 -0700 Subject: [PATCH 15/19] re-run CI From 7f86619de41ec34230a08a3ce23063884dd52449 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 21:29:23 -0700 Subject: [PATCH 16/19] re-run CI From 42f7d9fe508bb9cfe84a2b600249018974115bae Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 21:30:06 -0700 Subject: [PATCH 17/19] re-run CI From d1b503a3497509641f10668ce478c1b18d07304a Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 21:36:49 -0700 Subject: [PATCH 18/19] Fix authorize_instructor to include also TAs and leadTAs --- app/controllers/user_to_courses_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/user_to_courses_controller.rb b/app/controllers/user_to_courses_controller.rb index d06d414b..e339fb7d 100644 --- a/app/controllers/user_to_courses_controller.rb +++ b/app/controllers/user_to_courses_controller.rb @@ -38,7 +38,7 @@ def set_enrollment def authorize_instructor! user_to_course = UserToCourse.find_by(user: @current_user, course: @course) - unless user_to_course&.teacher? + unless user_to_course&.course_admin? render json: { success: false, error: 'Forbidden', From 76bb6525eeeda4a9881c367daa12fb8695e106b6 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 21:40:03 -0700 Subject: [PATCH 19/19] Fix rubycop expecting lms id --- app/controllers/user_to_courses_controller.rb | 2 +- spec/controllers/requests_controller_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/controllers/user_to_courses_controller.rb b/app/controllers/user_to_courses_controller.rb index e339fb7d..1702c256 100644 --- a/app/controllers/user_to_courses_controller.rb +++ b/app/controllers/user_to_courses_controller.rb @@ -12,7 +12,7 @@ def toggle_allow_extended_requests success: false, errors: @enrollment.errors.full_messages, redirect_to: courses_path - }, status: :unprocessable_entity + }, status: :unprocessable_content end end diff --git a/spec/controllers/requests_controller_spec.rb b/spec/controllers/requests_controller_spec.rb index 7ac50ad6..b0f7078f 100644 --- a/spec/controllers/requests_controller_spec.rb +++ b/spec/controllers/requests_controller_spec.rb @@ -438,10 +438,11 @@ end before do + Lms.find_or_create_by(id: 1) { |l| l.lms_name = 'Canvas'; l.use_auth_token = true } session[:user_id] = instructor.canvas_uid UserToCourse.create!(user: instructor, course: course, role: 'teacher') instructor.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'instructor_token', refresh_token: 'instructor_refresh', expire_time: 1.hour.from_now