From 37b1a1fe4c4824a5a5fc27c2b2b161256e69e57a Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Wed, 4 Mar 2026 16:51:27 -0800 Subject: [PATCH 01/25] 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/25] 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/25] 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/25] 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 @@ -
+
<% end %> + + <% if @student_enrollment.present? %> +
+

Staff Notes for <%= @request.user.name %>

+
+
+ <%= @student_enrollment.notes.present? ? simple_format(@student_enrollment.notes) : content_tag(:span, 'No notes yet.', class: 'text-muted') %> +
+ +
+
+ +
+ + +
+
+
+ <% end %> +
<% if @request.status == 'pending' %> diff --git a/config/routes.rb b/config/routes.rb index 2a0448d9..bab878b2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -50,6 +50,7 @@ resources :user_to_courses, only: [] do member do patch :toggle_allow_extended_requests + patch :update_notes end end resource :form_setting, only: [:edit, :update] diff --git a/db/migrate/20260404000001_add_notes_to_user_to_courses.rb b/db/migrate/20260404000001_add_notes_to_user_to_courses.rb new file mode 100644 index 00000000..e2d569ec --- /dev/null +++ b/db/migrate/20260404000001_add_notes_to_user_to_courses.rb @@ -0,0 +1,5 @@ +class AddNotesToUserToCourses < ActiveRecord::Migration[7.2] + def change + add_column :user_to_courses, :notes, :text + end +end diff --git a/features/step_definitions/student_notes_steps.rb b/features/step_definitions/student_notes_steps.rb new file mode 100644 index 00000000..04f5406e --- /dev/null +++ b/features/step_definitions/student_notes_steps.rb @@ -0,0 +1,14 @@ +# Click "Show" link for a request by assignment name +When(/^I click "Show" for the request for "([^"]*)"$/) do |assignment_name| + request = Request.joins(:assignment).find_by(assignments: { name: assignment_name }) + raise "No request found for assignment #{assignment_name}" unless request + + visit course_request_path(@course, request) +end + +# Set notes on the student's enrollment in the course +Given(/^the student for the course has notes "([^"]*)"$/) do |notes_text| + student = User.joins(:user_to_courses).find_by(user_to_courses: { course: @course, role: 'student' }) + enrollment = UserToCourse.find_by(user: student, course: @course, role: 'student') + enrollment.update!(notes: notes_text) +end diff --git a/features/student_notes.feature b/features/student_notes.feature new file mode 100644 index 00000000..a2bbeeb8 --- /dev/null +++ b/features/student_notes.feature @@ -0,0 +1,43 @@ +Feature: Student Notes + +Background: + Given a course exists + And I'm logged in as a teacher + When I go to the Course page + And I enable "Homework 1" + +Scenario: Teacher sees notes section on request detail page + Given I'm logged in as a student + And I go to the Course page + And I click New for "Homework 1" in the "assignments-table" + And I fill in "request[reason]" with "Need more time" + And I press "Submit Request" + Then I log in as a Teacher + And I go to the Requests page + And I click "Show" for the request for "Homework 1" + Then I should see "Staff Notes for" + And I should see "No notes yet." + +Scenario: Teacher can save notes for a student + Given I'm logged in as a student + And I go to the Course page + And I click New for "Homework 1" in the "assignments-table" + And I fill in "request[reason]" with "Need more time" + And I press "Submit Request" + Then I log in as a Teacher + And the student for the course has notes "Student has DSP accommodations." + And I go to the Requests page + And I click "Show" for the request for "Homework 1" + Then I should see "Student has DSP accommodations." + +Scenario: Student does not see staff notes on their request page + Given I'm logged in as a student + And I go to the Course page + And I click New for "Homework 1" in the "assignments-table" + And I fill in "request[reason]" with "Need more time" + And I press "Submit Request" + Given the student for the course has notes "Internal staff note" + And I go to the Course page + And I follow "Show" + Then I should not see "Staff Notes for" + And I should not see "Internal staff note" diff --git a/spec/controllers/requests_controller_spec.rb b/spec/controllers/requests_controller_spec.rb index 723cac10..8aa88c98 100644 --- a/spec/controllers/requests_controller_spec.rb +++ b/spec/controllers/requests_controller_spec.rb @@ -78,6 +78,26 @@ get :show, params: { course_id: course.id, id: request.id } expect(response).to render_template('requests/student_show') end + + it 'assigns @student_enrollment for instructor view' do + session[:user_id] = instructor.canvas_uid + UserToCourse.create!(user: instructor, course: course, role: 'teacher') + instructor.lms_credentials.create!( + lms_name: 'canvas', + token: 'fake_token', + refresh_token: 'fake_refresh_token', + expire_time: 1.hour.from_now + ) + + get :show, params: { course_id: course.id, id: request.id } + expect(assigns(:student_enrollment)).to be_present + expect(assigns(:student_enrollment).user).to eq(user) + end + + it 'does not assign @student_enrollment for student view' do + get :show, params: { course_id: course.id, id: request.id } + expect(assigns(:student_enrollment)).to be_nil + end end describe 'GET #new' do diff --git a/spec/controllers/user_to_courses_controller_spec.rb b/spec/controllers/user_to_courses_controller_spec.rb index eaaa61e7..38f28830 100644 --- a/spec/controllers/user_to_courses_controller_spec.rb +++ b/spec/controllers/user_to_courses_controller_spec.rb @@ -144,4 +144,88 @@ end end end + + describe 'PATCH #update_notes' do + context 'when user is an instructor' do + before do + UserToCourse.create!(user: instructor, course: course, role: 'teacher') + student_enrollment + session[:user_id] = instructor.canvas_uid + instructor.lms_credentials.create!( + lms_name: 'canvas', + token: 'fake_token', + refresh_token: 'fake_refresh_token', + expire_time: 1.hour.from_now + ) + end + + it 'successfully saves notes' do + patch :update_notes, params: { + course_id: course.id, + id: student_enrollment.id, + notes: 'Student has DSP accommodations for extra time.' + } + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['success']).to be true + expect(student_enrollment.reload.notes).to eq('Student has DSP accommodations for extra time.') + end + + it 'successfully clears notes' do + student_enrollment.update!(notes: 'Old notes') + + patch :update_notes, params: { + course_id: course.id, + id: student_enrollment.id, + notes: '' + } + + expect(response).to have_http_status(:ok) + expect(student_enrollment.reload.notes).to eq('') + end + + it 'returns the saved notes in the response' do + patch :update_notes, params: { + course_id: course.id, + id: student_enrollment.id, + notes: 'OKed 3-day extensions for all assignments.' + } + + expect(response.parsed_body['notes']).to eq('OKed 3-day extensions for all assignments.') + end + end + + context 'when user is a student' do + before do + student_enrollment + session[:user_id] = student_user.canvas_uid + student_user.lms_credentials.create!( + lms_name: 'canvas', + token: 'fake_token', + refresh_token: 'fake_refresh_token', + expire_time: 1.hour.from_now + ) + end + + it 'returns forbidden status' do + patch :update_notes, params: { + course_id: course.id, + id: student_enrollment.id, + notes: 'Should not be allowed' + } + + expect(response).to have_http_status(:forbidden) + end + + it 'does not update the notes' do + patch :update_notes, params: { + course_id: course.id, + id: student_enrollment.id, + notes: 'Should not be allowed' + } + + expect(student_enrollment.reload.notes).to be_nil + end + end + end end From fa3a1d6164c7360f1b9e9d8dcd6324fac87b9283 Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Sun, 5 Apr 2026 14:42:06 -0700 Subject: [PATCH 06/25] Add updated schema.rb for student notes migration --- db/schema.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 0dd989fe..5a7d3147 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,9 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_02_02_000001) do - create_schema "hypershield" - +ActiveRecord::Schema[7.2].define(version: 2026_04_04_000001) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -162,7 +160,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" @@ -171,6 +168,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 @@ -212,6 +210,7 @@ t.datetime "updated_at", null: false t.boolean "removed", default: false, null: false t.boolean "allow_extended_requests", default: false, null: false + t.text "notes" t.index ["course_id"], name: "index_user_to_courses_on_course_id" t.index ["user_id"], name: "index_user_to_courses_on_user_id" end @@ -235,6 +234,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" From 1e4fce3795cb983b9c41fa99681bbe0ee36e0de6 Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Sun, 5 Apr 2026 16:41:10 -0700 Subject: [PATCH 07/25] fixed tests --- app/controllers/session_controller.rb | 2 +- app/models/lms_credential.rb | 3 ++- app/models/user.rb | 3 +-- features/step_definitions/student_notes_steps.rb | 4 ++-- features/student_notes.feature | 10 ++++------ spec/controllers/application_controller_spec.rb | 2 +- .../controllers/concerns/token_refreshable_spec.rb | 2 +- .../controllers/course_settings_controller_spec.rb | 4 ++-- spec/controllers/courses_controller_spec.rb | 6 +++--- spec/controllers/requests_controller_spec.rb | 14 +++++++------- .../controllers/user_to_courses_controller_spec.rb | 12 ++++++------ spec/factories/lms_credential.rb | 2 +- spec/features/accessibility_spec.rb | 4 ++-- spec/models/course_spec.rb | 2 +- spec/models/lms_credential_spec.rb | 2 +- spec/models/request_spec.rb | 2 +- spec/models/user_spec.rb | 10 +++++----- 17 files changed, 41 insertions(+), 43 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 81fd512c..597901e1 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -115,7 +115,7 @@ def update_user_credential(user, token) ) else user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: token.token, refresh_token: token.refresh_token, expire_time: Time.zone.at(token.expires_at) diff --git a/app/models/lms_credential.rb b/app/models/lms_credential.rb index aa895068..7bd3fb0e 100644 --- a/app/models/lms_credential.rb +++ b/app/models/lms_credential.rb @@ -26,10 +26,11 @@ class LmsCredential < ApplicationRecord # Belongs to a User belongs_to :user + belongs_to :lms, optional: true # Encryption for tokens encrypts :token, :refresh_token # LMS must exist - validates :lms_name, presence: true + validates :lms_id, presence: true end diff --git a/app/models/user.rb b/app/models/user.rb index 9cac5f45..c58e49b5 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: 1) end def token_expired? diff --git a/features/step_definitions/student_notes_steps.rb b/features/step_definitions/student_notes_steps.rb index 04f5406e..f3d28f9a 100644 --- a/features/step_definitions/student_notes_steps.rb +++ b/features/step_definitions/student_notes_steps.rb @@ -1,5 +1,5 @@ -# Click "Show" link for a request by assignment name -When(/^I click "Show" for the request for "([^"]*)"$/) do |assignment_name| +# Navigate to the request show page for a given assignment +When(/^I view the request for "([^"]*)"$/) do |assignment_name| request = Request.joins(:assignment).find_by(assignments: { name: assignment_name }) raise "No request found for assignment #{assignment_name}" unless request diff --git a/features/student_notes.feature b/features/student_notes.feature index a2bbeeb8..b4c3cd6d 100644 --- a/features/student_notes.feature +++ b/features/student_notes.feature @@ -13,8 +13,7 @@ Scenario: Teacher sees notes section on request detail page And I fill in "request[reason]" with "Need more time" And I press "Submit Request" Then I log in as a Teacher - And I go to the Requests page - And I click "Show" for the request for "Homework 1" + And I view the request for "Homework 1" Then I should see "Staff Notes for" And I should see "No notes yet." @@ -26,8 +25,7 @@ Scenario: Teacher can save notes for a student And I press "Submit Request" Then I log in as a Teacher And the student for the course has notes "Student has DSP accommodations." - And I go to the Requests page - And I click "Show" for the request for "Homework 1" + And I view the request for "Homework 1" Then I should see "Student has DSP accommodations." Scenario: Student does not see staff notes on their request page @@ -37,7 +35,7 @@ Scenario: Student does not see staff notes on their request page And I fill in "request[reason]" with "Need more time" And I press "Submit Request" Given the student for the course has notes "Internal staff note" - And I go to the Course page - And I follow "Show" + And I go to the Requests page + And I click View for "Homework 1" in the "requests-table" Then I should not see "Staff Notes for" And I should not see "Internal staff note" diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index cb795dab..09a9ef16 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -15,7 +15,7 @@ def test_auth let(:user) do User.create!(email: 'test@example.com', canvas_uid: '123').tap do |u| 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..4ce6d4c3 100644 --- a/spec/controllers/concerns/token_refreshable_spec.rb +++ b/spec/controllers/concerns/token_refreshable_spec.rb @@ -22,7 +22,7 @@ def current_user let(:user) do User.create!(email: 'test@example.com', canvas_uid: '123').tap do |u| 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..c76a1fbc 100644 --- a/spec/controllers/course_settings_controller_spec.rb +++ b/spec/controllers/course_settings_controller_spec.rb @@ -7,7 +7,7 @@ before do 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 +190,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..15ed29d4 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -11,7 +11,7 @@ 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 +118,7 @@ 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) + 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 +167,7 @@ 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) + 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 8aa88c98..83a853bc 100644 --- a/spec/controllers/requests_controller_spec.rb +++ b/spec/controllers/requests_controller_spec.rb @@ -1,8 +1,8 @@ require 'rails_helper' RSpec.describe RequestsController, type: :controller do - let(:user) { User.create!(email: 'student@example.com', canvas_uid: '123', name: 'Student') } - let(:instructor) { User.create!(email: 'instructor@example.com', canvas_uid: '566', name: 'Instructor') } + let(:user) { User.create!(email: 'student@example.com', canvas_uid: 'student-uid-123', name: 'Student') } + let(:instructor) { User.create!(email: 'instructor@example.com', canvas_uid: 'instructor-uid-566', name: 'Instructor') } let(:course) { create(:course, :with_staff, course_name: 'Test Course', canvas_id: '456', course_code: 'TST101') } let(:teacher_course) { Course.create!(course_name: 'Instructor Course', canvas_id: '999', course_code: 'INST101') } let(:assignment) do @@ -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 @@ -83,7 +83,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: 'fake_token', refresh_token: 'fake_refresh_token', expire_time: 1.hour.from_now @@ -334,7 +334,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 @@ -461,7 +461,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 @@ -692,7 +692,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/controllers/user_to_courses_controller_spec.rb b/spec/controllers/user_to_courses_controller_spec.rb index 38f28830..e290499c 100644 --- a/spec/controllers/user_to_courses_controller_spec.rb +++ b/spec/controllers/user_to_courses_controller_spec.rb @@ -13,7 +13,7 @@ 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 @@ -68,7 +68,7 @@ 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 @@ -102,7 +102,7 @@ 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 @@ -126,7 +126,7 @@ 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 @@ -152,7 +152,7 @@ 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 @@ -200,7 +200,7 @@ 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 diff --git a/spec/factories/lms_credential.rb b/spec/factories/lms_credential.rb index 7dff4782..949f8264 100644 --- a/spec/factories/lms_credential.rb +++ b/spec/factories/lms_credential.rb @@ -1,7 +1,7 @@ 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 } diff --git a/spec/features/accessibility_spec.rb b/spec/features/accessibility_spec.rb index 106827b4..3f614ea2 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..d69e0794 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -41,7 +41,7 @@ course = described_class.create!(canvas_id: 'canvas_123', course_name: 'Test', course_code: 'TEST101') user = User.create!(email: 'test@example.com', canvas_uid: '123') 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..672b1ebd 100644 --- a/spec/models/lms_credential_spec.rb +++ b/spec/models/lms_credential_spec.rb @@ -43,7 +43,7 @@ def self.mock_get_service(token, refresh_token) let!(:credential) do described_class.create!( user: user, - lms_name: 'ExampleLMS', + lms_id: 1, username: 'testuser', password: 'testpassword', token: 'sensitive_token', diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb index 24bcb0eb..6df9a740 100644 --- a/spec/models/request_spec.rb +++ b/spec/models/request_spec.rb @@ -71,7 +71,7 @@ before do 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a929e1d7..d4c27b65 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -32,7 +32,7 @@ context 'when the token is still valid' do before do user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -47,7 +47,7 @@ context 'when the token is expired' do before do user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'expired_token', refresh_token: 'refresh_token', expire_time: 1.hour.ago @@ -65,7 +65,7 @@ it 'returns the correct credentials for a user' do user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -82,7 +82,7 @@ context 'when token does not expire soon' do before do user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -97,7 +97,7 @@ context 'when token expires soon and is refreshed' do let(:credential) do user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'stale_token', refresh_token: 'refresh_token', expire_time: 5.minutes.from_now From c457642693ef087cba3a4f9973c45729d3c3984b Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Mon, 20 Apr 2026 12:43:02 -0700 Subject: [PATCH 08/25] removed duplicate filer courses method after rebasing --- app/controllers/courses_controller.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index da987165..5667bd26 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -124,11 +124,6 @@ def filter_by_semester(courses, semester) courses.select { |c| c.dig('term', 'name') == semester } end - # Filters Canvas API course hashes by their term name - def filter_by_semester(courses, semester) - courses.select { |c| c.dig('term', 'name') == semester } - end - # TODO: This should be moved to the Canvas Facade def filter_courses(courses, roles, exclude_ids = []) missing_enrollments = courses.select { |course| course['enrollments'].blank? } From 404b44982345fba537ca8739da8a10ab07de8f51 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Thu, 23 Apr 2026 19:05:33 -0700 Subject: [PATCH 09/25] Restore filter_by_semester method lost during merge --- app/controllers/courses_controller.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index b9254fc9..d54955ab 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -133,6 +133,10 @@ def group_by_semester(user_to_courses) sorted_semesters.map { |semester| [ semester, grouped[semester] ] } end + def filter_by_semester(courses, semester) + courses.select { |c| c.dig('term', 'name') == semester } + end + # TODO: This should be moved to the Canvas Facade def filter_courses(courses, roles, exclude_ids = []) missing_enrollments = courses.select { |course| course['enrollments'].blank? } From 2f519434885d04171db2e15865dcbc7198f10c47 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 23 Apr 2026 16:06:46 -0700 Subject: [PATCH 10/25] Support Lead TA course role --- app/controllers/courses_controller.rb | 14 +++--- app/facades/canvas_facade.rb | 19 +++++++- .../controllers/enrollments_controller.js | 2 +- app/models/course.rb | 10 ++-- app/models/user_to_course.rb | 48 +++++++++++++++++-- app/views/courses/enrollments.html.erb | 4 +- app/views/courses/index.html.erb | 2 +- app/views/courses/new.html.erb | 5 +- spec/controllers/courses_controller_spec.rb | 38 +++++++++++++-- spec/facades/canvas_facade_spec.rb | 18 +++++++ spec/models/course_spec.rb | 20 ++++++++ spec/models/user_to_course_spec.rb | 41 ++++++++++++++++ 12 files changed, 195 insertions(+), 26 deletions(-) create mode 100644 spec/models/user_to_course_spec.rb diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index c5b0d03f..812c5fb7 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -5,7 +5,7 @@ class CoursesController < ApplicationController before_action :determine_user_role def index - teacher_courses = UserToCourse.includes(:course).where(user: @user, role: %w[teacher ta]) + teacher_courses = UserToCourse.includes(:course).where(user: @user, role: UserToCourse.staff_roles) @teacher_courses_by_semester = group_by_semester(teacher_courses) # Only show courses to students if extensions are enabled at the course level @@ -44,16 +44,14 @@ def new @courses = Course.fetch_courses(token) flash[:alert] = 'No courses found.' if @courses.empty? - # Collect unique semester names from Canvas term data for the filter dropdown @semesters = @courses.filter_map { |c| c.dig('term', 'name') }.uniq.sort @selected_semester = params[:semester] - teacher_enrollment_types = %w[teacher ta] # TODO: Add spec for when a course is created, but the user is not enrolled in it. # TODO: Why do some courses have empty enrollments? existing_canvas_ids = @user.courses.pluck(:canvas_id) - @courses_teacher = filter_courses(@courses, teacher_enrollment_types, existing_canvas_ids) - @courses_student = filter_courses(@courses, [ 'student' ], existing_canvas_ids) + @courses_teacher = filter_courses(@courses, UserToCourse.staff_roles, existing_canvas_ids) + @courses_student = filter_courses(@courses, [ UserToCourse::STUDENT_ROLE ], existing_canvas_ids) if @selected_semester.present? @courses_teacher = filter_by_semester(@courses_teacher, @selected_semester) @@ -68,7 +66,7 @@ def edit def create token = @user.lms_credentials.first.token - filter_courses(Course.fetch_courses(token), %w[teacher ta]) + filter_courses(Course.fetch_courses(token), UserToCourse.staff_roles) .select { |c| params[:courses]&.include?(c['id'].to_s) } .each { |course_api| Course.create_or_update_from_canvas(course_api, token, @user) } redirect_to courses_path, notice: 'Selected courses and their assignments have been imported successfully.' @@ -146,7 +144,9 @@ def filter_courses(courses, roles, exclude_ids = []) courses = courses - missing_enrollments - courses.select { |course| exclude_ids.include?(course['id'].to_s) } return [] if courses.empty? - courses.select { |course| course['enrollments'].any? { |e| roles.include?(e['type']) } } + courses.select do |course| + course['enrollments'].any? { |enrollment| roles.include?(UserToCourse.role_from_canvas_enrollment(enrollment)) } + end end def course_data_for_sync diff --git a/app/facades/canvas_facade.rb b/app/facades/canvas_facade.rb index 6d437afd..19e38689 100644 --- a/app/facades/canvas_facade.rb +++ b/app/facades/canvas_facade.rb @@ -1,4 +1,5 @@ require 'date' +require 'cgi' require 'faraday' require 'json' require 'ostruct' @@ -8,6 +9,9 @@ class CanvasFacade < LmsFacade class CanvasAPIError < LmsFacade::LmsAPIError; end CANVAS_URL = ENV.fetch('CANVAS_URL', nil) + CANVAS_CUSTOM_COURSE_ROLES = { + UserToCourse::LEAD_TA_ROLE => 'Lead TA' + }.freeze # Canvas instances can scope the flextensions developer key. # There must be one scope for each endpoint we can use. @@ -161,10 +165,10 @@ def get_all_course_users(course, role = nil) # sigh, manually construct query string until we tweak Faraday middleware # to include :url_encoded, then use `'enrollment_type[]' : list_or_string` query_string = 'per_page=100' - query_string += "&enrollment_type[]=#{role}" if role.is_a?(String) && role.present? + query_string += "&#{role_query_param(role)}" if role.is_a?(String) && role.present? if role.is_a?(Array) && role.present? # rubocop:disable Style/IfUnlessModifier - query_string += role.map { |r| "&enrollment_type[]=#{r}" }.join + query_string += role.map { |r| "&#{role_query_param(r)}" }.join end depaginate_response(@canvas_conn.get("courses/#{course.canvas_id}/users?#{query_string}")) @@ -189,6 +193,17 @@ def get_instructor_courses teacher_courses + ta_courses end + def role_query_param(role) + normalized_role = UserToCourse.normalize_role(role) + canvas_course_role = CANVAS_CUSTOM_COURSE_ROLES[normalized_role] + + if canvas_course_role + "enrollment_role=#{CGI.escape(canvas_course_role)}" + else + "enrollment_type[]=#{CGI.escape(normalized_role)}" + end + end + ## # Gets a specified course that the authorized user has access to. # diff --git a/app/javascript/controllers/enrollments_controller.js b/app/javascript/controllers/enrollments_controller.js index cd03aecb..cad253ac 100644 --- a/app/javascript/controllers/enrollments_controller.js +++ b/app/javascript/controllers/enrollments_controller.js @@ -11,7 +11,7 @@ export default class extends Controller { if (!DataTable.isDataTable('#enrollments-table')) { // Define a custom sorting function for the Role column DataTable.ext.type.order['role-pre'] = function (data) { - const rolePriority = { teacher: 4, ta: 2, student: 3 }; + const rolePriority = { teacher: 4, leadta: 3, "lead ta": 3, ta: 2, student: 1 }; if (typeof data !== 'string') { data = String(data).trim(); } diff --git a/app/models/course.rb b/app/models/course.rb index 9f4ac9b7..f9ce422a 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -87,8 +87,8 @@ def enabled_assignments # Or is user.staff_role?(course) or user.student_role?(course) better? def user_role(user) roles = UserToCourse.where(user_id: user.id, course_id: id).pluck(:role) - return 'instructor' if roles.include?('teacher') || roles.include?('ta') - return 'student' if roles.include?('student') + return 'instructor' if roles.intersect?(UserToCourse.staff_roles) + return 'student' if roles.include?(UserToCourse::STUDENT_ROLE) nil end @@ -118,11 +118,11 @@ def assignments end def students - user_to_courses.where(role: 'student').map(&:user) + user_to_courses.where(role: UserToCourse::STUDENT_ROLE).map(&:user) end def instructors - user_to_courses.where(role: 'teacher').map(&:user) + user_to_courses.where(role: UserToCourse::TEACHER_ROLE).map(&:user) end def staff_users @@ -257,7 +257,7 @@ def sync_users_from_canvas(user, roles = [ 'student' ]) end def sync_all_enrollments_from_canvas(user) - sync_users_from_canvas(user, [ 'teacher', 'ta', 'student' ]) + sync_users_from_canvas(user, UserToCourse.roles) end def regenerate_readonly_api_token_if_blank diff --git a/app/models/user_to_course.rb b/app/models/user_to_course.rb index abb63fa2..33b983a5 100644 --- a/app/models/user_to_course.rb +++ b/app/models/user_to_course.rb @@ -23,6 +23,16 @@ # # TODO: In the future we should name this CourseEnrollment class UserToCourse < ApplicationRecord + STUDENT_ROLE = 'student'.freeze + TEACHER_ROLE = 'teacher'.freeze + TA_ROLE = 'ta'.freeze + LEAD_TA_ROLE = 'leadta'.freeze + STAFF_ROLES = [ TEACHER_ROLE, TA_ROLE, LEAD_TA_ROLE ].freeze + COURSE_ADMIN_ROLES = [ TEACHER_ROLE, LEAD_TA_ROLE ].freeze + ROLE_LABELS = { + LEAD_TA_ROLE => 'Lead TA' + }.freeze + # Associations belongs_to :user belongs_to :course @@ -39,18 +49,48 @@ def staff? end def course_admin? - role == 'teacher' || role == 'leadta' + UserToCourse.course_admin_roles.include?(role) end def student? - role == 'student' + role == STUDENT_ROLE + end + + def display_role + UserToCourse.display_role(role) end def self.roles - [ 'student' ] + UserToCourse.staff_roles + [ STUDENT_ROLE ] + UserToCourse.staff_roles end def self.staff_roles - %w[teacher ta leadta] + STAFF_ROLES + end + + def self.course_admin_roles + COURSE_ADMIN_ROLES + end + + def self.normalize_role(role) + role.to_s.downcase.gsub(/[^a-z]/, '') + end + + def self.role_from_canvas_enrollment(enrollment) + return nil unless enrollment + + normalized_role = normalize_role(enrollment['role'] || enrollment[:role]) + return LEAD_TA_ROLE if normalized_role == LEAD_TA_ROLE + + normalized_type = normalize_role(enrollment['type'] || enrollment[:type]) + roles.include?(normalized_type) ? normalized_type : nil + end + + def self.staff_enrollment?(enrollment) + staff_roles.include?(role_from_canvas_enrollment(enrollment)) + end + + def self.display_role(role) + ROLE_LABELS.fetch(role.to_s, role.to_s.capitalize) end end diff --git a/app/views/courses/enrollments.html.erb b/app/views/courses/enrollments.html.erb index 80750df6..cf2ee520 100644 --- a/app/views/courses/enrollments.html.erb +++ b/app/views/courses/enrollments.html.erb @@ -51,7 +51,7 @@ <%= enrollment.user.student_id %> <%= enrollment.user.email %> - <%= enrollment.role.downcase.capitalize %> + <%= enrollment.display_role %> <% if @is_course_admin %> @@ -89,4 +89,4 @@ <% end %>
-
\ 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 11/25] 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 12/25] 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 13/25] 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 14/25] 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 15/25] 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 16/25] 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 17/25] 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 18/25] 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 19/25] 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 20/25] 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 21/25] 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 22/25] 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 From 1286916538a8494cb747efaaacecae61d7dd0e6a Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 22:07:12 -0700 Subject: [PATCH 23/25] Fixed ruby lint --- app/models/lms_credential.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/lms_credential.rb b/app/models/lms_credential.rb index e976e78b..1ac24488 100644 --- a/app/models/lms_credential.rb +++ b/app/models/lms_credential.rb @@ -25,11 +25,7 @@ # fk_rails_... (user_id => users.id) # class LmsCredential < ApplicationRecord - # Belongs to a User belongs_to :user - belongs_to :lms, optional: true - - # Belongs to an LMS belongs_to :lms # Encryption for tokens From b429fcbf51d283f4c5d1670be3cb8a12f699d07c Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 22:16:52 -0700 Subject: [PATCH 24/25] Fixed Rspec/ clearing users issue and ruby lint --- app/models/lms_credential.rb | 3 --- spec/controllers/courses_controller_spec.rb | 5 +++++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/lms_credential.rb b/app/models/lms_credential.rb index 1ac24488..60ec5e0f 100644 --- a/app/models/lms_credential.rb +++ b/app/models/lms_credential.rb @@ -30,7 +30,4 @@ class LmsCredential < ApplicationRecord # Encryption for tokens encrypts :token, :refresh_token - - # LMS must exist - validates :lms_id, presence: true end diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index 9a887216..ef571ae3 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -372,6 +372,7 @@ context 'when user is a TA (staff but not course admin)' do before do + UserToCourse.where(user: user, course: course).destroy_all UserToCourse.create!(user: user, course: course, role: 'ta') end @@ -390,6 +391,10 @@ end context 'when user is a student' do + before do + UserToCourse.where(user: user, course: course, role: 'teacher').destroy_all + end + it 'redirects with access denied' do get :enrollments, params: { id: course.id } expect(response).to redirect_to(courses_path) From 91827158cf47d86b030db9efc78aff1cacfc28c5 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 22:52:51 -0700 Subject: [PATCH 25/25] Fixed set_enrollment --- app/controllers/user_to_courses_controller.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/user_to_courses_controller.rb b/app/controllers/user_to_courses_controller.rb index 9607cecc..72bd98c5 100644 --- a/app/controllers/user_to_courses_controller.rb +++ b/app/controllers/user_to_courses_controller.rb @@ -17,8 +17,6 @@ def toggle_allow_extended_requests end def update_notes - @enrollment = @course.user_to_courses.find(params[:id]) - if @enrollment.update(notes: params[:notes]) render json: { success: true, notes: @enrollment.notes }, status: :ok else @@ -43,7 +41,7 @@ def set_course end def set_enrollment - @enrollment = UserToCourse.find(params[:id]) + @enrollment = @course.user_to_courses.find(params[:id]) end def authorize_instructor!