From 37b1a1fe4c4824a5a5fc27c2b2b161256e69e57a Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Wed, 4 Mar 2026 16:51:27 -0800 Subject: [PATCH 01/74] 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/74] 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/74] 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/74] 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/74] 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/74] 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 af2a3d3326aab30914f28754710859ab604aa9c9 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Tue, 7 Apr 2026 17:48:53 -0700 Subject: [PATCH 08/74] Async sync enrollments and sync assignments, added UI to indicate pending async calls --- Gemfile | 1 + Gemfile.lock | 14 +++ app/controllers/courses_controller.rb | 12 +- app/controllers/session_controller.rb | 2 + .../controllers/assignment_controller.js | 64 +++++++---- .../controllers/enrollments_controller.js | 67 +++++++---- app/models/course.rb | 4 +- app/views/courses/enrollments.html.erb | 6 +- app/views/courses/instructor_show.html.erb | 6 +- config/application.rb | 1 + config/routes.rb | 2 + db/migrate/20260407004259_create_good_jobs.rb | 105 ++++++++++++++++++ db/schema.rb | 93 +++++++++++++++- 13 files changed, 321 insertions(+), 56 deletions(-) create mode 100644 db/migrate/20260407004259_create_good_jobs.rb diff --git a/Gemfile b/Gemfile index d0c7ea6e..089afc3b 100644 --- a/Gemfile +++ b/Gemfile @@ -70,6 +70,7 @@ gem 'lograge' # gem 'blazer' gem 'hypershield' +gem 'good_job', '~> 4.0' #### Frontend related tools # The original asset pipeline for Rails [https://github.com/rails/sprockets-rails] diff --git a/Gemfile.lock b/Gemfile.lock index 92b2be3c..b9ef2bf1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -189,6 +189,8 @@ GEM dumb_delegator (1.1.0) erb (6.0.2) erubi (1.13.1) + et-orbi (1.4.0) + tzinfo factory_bot (6.5.6) activesupport (>= 6.1.0) factory_bot_rails (6.5.1) @@ -215,8 +217,18 @@ GEM sassc (~> 2.0) formatador (1.2.3) reline + fugit (1.12.1) + et-orbi (~> 1.4) + raabro (~> 1.4) globalid (1.3.0) activesupport (>= 6.1) + good_job (4.14.2) + activejob (>= 6.1.0) + activerecord (>= 6.1.0) + concurrent-ruby (>= 1.3.1) + fugit (>= 1.11.0) + railties (>= 6.1.0) + thor (>= 1.0.0) guard (2.20.1) formatador (>= 0.2.4) listen (>= 2.7, < 4.0) @@ -382,6 +394,7 @@ GEM public_suffix (7.0.5) puma (7.2.0) nio4r (~> 2.0) + raabro (1.4.0) racc (1.8.1) rack (3.2.6) rack-protection (4.2.1) @@ -628,6 +641,7 @@ DEPENDENCIES faraday faraday-cookie_jar font-awesome-sass + good_job (~> 4.0) guard-rspec hypershield importmap-rails diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index c5b0d03f..411b442c 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -1,6 +1,6 @@ class CoursesController < ApplicationController before_action :authenticate_user - before_action :set_course, only: %i[show edit sync_assignments sync_enrollments enrollments delete] + before_action :set_course, only: %i[show edit sync_assignments sync_enrollments sync_status enrollments delete] before_action :set_pending_request_count before_action :determine_user_role @@ -89,6 +89,16 @@ def sync_enrollments render json: { message: 'Users synced successfully.' }, status: :ok end + def sync_status + course_to_lms = @course.course_to_lms(1) + return render json: { error: 'LMS connection not found.' }, status: :not_found unless course_to_lms + + render json: { + roster_synced_at: course_to_lms.recent_roster_sync&.dig('synced_at'), + assignments_synced_at: course_to_lms.recent_assignment_sync&.dig('synced_at') + }, status: :ok + end + def enrollments @side_nav = 'enrollments' return redirect_to courses_path, alert: 'You do not have access to this page.' unless @role == 'instructor' diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 81fd512c..14d181e3 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -65,6 +65,8 @@ def omniauth_callback redirect_to courses_path, notice: "Logged in! Welcome, #{user_data['name']}!" rescue StandardError => e + puts(e.message) + puts(e) Rails.logger.error("OmniAuth callback error: #{e.message}") redirect_to root_path, alert: 'Authentication failed. Invalid credentials.' end diff --git a/app/javascript/controllers/assignment_controller.js b/app/javascript/controllers/assignment_controller.js index 8112cfb5..8d7cbae1 100644 --- a/app/javascript/controllers/assignment_controller.js +++ b/app/javascript/controllers/assignment_controller.js @@ -5,7 +5,7 @@ import "datatables.net-responsive-bs5"; // Connects to data-controller="assignment" export default class extends Controller { - static targets = ["checkbox"] + static targets = ["checkbox", "syncBtn", "syncLabel", "syncSpinner"] static values = { courseId: Number } connect() { @@ -64,31 +64,47 @@ export default class extends Controller { } } - sync(event) { - const button = event.currentTarget; - button.disabled = true; + async sync() { + const button = this.syncBtnTarget; + const label = this.syncLabelTarget; + const spinner = this.syncSpinnerTarget; const courseId = this.courseIdValue; const token = document.querySelector('meta[name="csrf-token"]').content; - fetch(`/courses/${courseId}/sync_assignments`, { - method: "POST", - headers: { - "Content-Type": "application/json", - "X-CSRF-Token": token, - }, - }) - .then((response) => { - if (!response.ok) { - throw new Error("Failed to sync assignments."); - } - return response.json(); - }) - .then((data) => { - flash("notice", data.message || "Assignments synced successfully."); - location.reload(); - }) - .catch((error) => { - flash("alert", error.message || "An error occurred while syncing assignments."); - location.reload(); + + button.disabled = true; + label.textContent = "Syncing..."; + spinner.classList.remove("d-none"); + + try { + const statusBefore = await fetch(`/courses/${courseId}/sync_status`).then(r => r.json()); + const beforeTs = statusBefore.assignments_synced_at; + + const response = await fetch(`/courses/${courseId}/sync_assignments`, { + method: "POST", + headers: { "Content-Type": "application/json", "X-CSRF-Token": token }, }); + + if (!response.ok) throw new Error(`Failed to sync assignments. ${response.status}`); + + await this._pollUntilDone(courseId, "assignments_synced_at", beforeTs); + + flash("notice", "Assignments synced successfully."); + location.reload(); + } catch (error) { + flash("alert", error.message || "An error occurred while syncing assignments."); + button.disabled = false; + label.textContent = "Sync Assignments"; + spinner.classList.add("d-none"); + } + } + + async _pollUntilDone(courseId, key, beforeTs, intervalMs = 1000, timeoutMs = 60000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + await new Promise(resolve => setTimeout(resolve, intervalMs)); + const status = await fetch(`/courses/${courseId}/sync_status`).then(r => r.json()); + if (status[key] && status[key] !== beforeTs) return; + } + throw new Error("Sync timed out. Please refresh the page."); } } diff --git a/app/javascript/controllers/enrollments_controller.js b/app/javascript/controllers/enrollments_controller.js index cd03aecb..145d73b2 100644 --- a/app/javascript/controllers/enrollments_controller.js +++ b/app/javascript/controllers/enrollments_controller.js @@ -4,7 +4,7 @@ import "datatables.net-responsive"; import "datatables.net-responsive-bs5"; export default class extends Controller { - static targets = ["checkbox"] + static targets = ["checkbox", "syncBtn", "syncLabel", "syncSpinner"] static values = { courseId: Number } connect() { @@ -76,30 +76,49 @@ export default class extends Controller { window.dispatchEvent(new CustomEvent('flash', { detail: { type: type, message: message } })); } - sync() { - const button = event.currentTarget; - button.disabled = true; + async sync() { + const button = this.syncBtnTarget; + const label = this.syncLabelTarget; + const spinner = this.syncSpinnerTarget; const courseId = this.courseIdValue; - const token = document.querySelector('meta[name="csrf-token"]').content; fetch(`/courses/${courseId}/sync_enrollments`, { - method: "POST", - headers: { - "Content-Type": "application/json", - "X-CSRF-Token": token, - }, - }) - .then((response) => { - if (!response.ok) { - throw new Error(`Failed to sync enrollments. ${response.status} - ${response.statusText}`); - } - return response.json(); - }) - .then((data) => { - flash("notice", data.message || "Enrollments synced successfully."); + const token = document.querySelector('meta[name="csrf-token"]').content; + + button.disabled = true; + label.textContent = "Syncing..."; + spinner.classList.remove("d-none"); + + try { + // Capture timestamp before sync so we can detect when job finishes + const statusBefore = await fetch(`/courses/${courseId}/sync_status`).then(r => r.json()); + const beforeTs = statusBefore.roster_synced_at; + + const response = await fetch(`/courses/${courseId}/sync_enrollments`, { + method: "POST", + headers: { "Content-Type": "application/json", "X-CSRF-Token": token }, + }); + + if (!response.ok) throw new Error(`Failed to sync enrollments. ${response.status}`); + + // Poll until synced_at changes + await this._pollUntilDone(courseId, "roster_synced_at", beforeTs); + + flash("notice", "Enrollments synced successfully."); location.reload(); - }) - .catch((error) => { + } catch (error) { flash("alert", error.message || "An error occurred while syncing enrollments."); - location.reload(); - }); - } + button.disabled = false; + label.textContent = "Sync Enrollments"; + spinner.classList.add("d-none"); + } + } + + async _pollUntilDone(courseId, key, beforeTs, intervalMs = 2000, timeoutMs = 60000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + await new Promise(resolve => setTimeout(resolve, intervalMs)); + const status = await fetch(`/courses/${courseId}/sync_status`).then(r => r.json()); + if (status[key] && status[key] !== beforeTs) return; + } + throw new Error("Sync timed out. Please refresh the page."); + } } diff --git a/app/models/course.rb b/app/models/course.rb index 9f4ac9b7..6e4aca1d 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -246,14 +246,14 @@ def sync_assignments(sync_user) return unless lms_links.any? lms_links.each do |course_to_lms| - SyncAllCourseAssignmentsJob.perform_now(course_to_lms.id, sync_user.id) + SyncAllCourseAssignmentsJob.perform_later(course_to_lms.id, sync_user.id) end end # Fetch users for a course and create/find their User and UserToCourse records # TODO: This may need to become a background job def sync_users_from_canvas(user, roles = [ 'student' ]) - SyncUsersFromCanvasJob.perform_now(id, user, roles) + SyncUsersFromCanvasJob.perform_later(id, user, roles) end def sync_all_enrollments_from_canvas(user) diff --git a/app/views/courses/enrollments.html.erb b/app/views/courses/enrollments.html.erb index 80750df6..70e22b2d 100644 --- a/app/views/courses/enrollments.html.erb +++ b/app/views/courses/enrollments.html.erb @@ -82,8 +82,10 @@
<% end %> diff --git a/app/views/courses/instructor_show.html.erb b/app/views/courses/instructor_show.html.erb index 3f0ed8ef..54d64376 100644 --- a/app/views/courses/instructor_show.html.erb +++ b/app/views/courses/instructor_show.html.erb @@ -53,8 +53,10 @@
diff --git a/config/application.rb b/config/application.rb index e298613f..9c4003f8 100644 --- a/config/application.rb +++ b/config/application.rb @@ -40,6 +40,7 @@ class Application < Rails::Application config.active_record.default_timezone = :utc config.time_zone = 'Pacific Time (US & Canada)' config.generators.system_tests = nil + config.active_job.queue_adapter = :good_job # We do not require the master key and insetad use environment variables # Review .env.example for required variables. diff --git a/config/routes.rb b/config/routes.rb index 2a0448d9..9886eed9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,6 +30,7 @@ member do post :sync_assignments post :sync_enrollments + get :sync_status get :enrollments delete :delete end @@ -68,4 +69,5 @@ # This is protected by `require_admin` via blazer.yml mount Blazer::Engine, at: "admin/blazer" + mount GoodJob::Engine, at: "admin/good_job" end diff --git a/db/migrate/20260407004259_create_good_jobs.rb b/db/migrate/20260407004259_create_good_jobs.rb new file mode 100644 index 00000000..e2e88bfb --- /dev/null +++ b/db/migrate/20260407004259_create_good_jobs.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +class CreateGoodJobs < ActiveRecord::Migration[7.2] + def change + # Uncomment for Postgres v12 or earlier to enable gen_random_uuid() support + # enable_extension 'pgcrypto' + + create_table :good_jobs, id: :uuid do |t| + t.text :queue_name + t.integer :priority + t.jsonb :serialized_params + t.datetime :scheduled_at + t.datetime :performed_at + t.datetime :finished_at + t.text :error + + t.timestamps + + t.uuid :active_job_id + t.text :concurrency_key + t.text :cron_key + t.uuid :retried_good_job_id + t.datetime :cron_at + + t.uuid :batch_id + t.uuid :batch_callback_id + + t.boolean :is_discrete + t.integer :executions_count + t.text :job_class + t.integer :error_event, limit: 2 + t.text :labels, array: true + t.uuid :locked_by_id + t.datetime :locked_at + end + + create_table :good_job_batches, id: :uuid do |t| + t.timestamps + t.text :description + t.jsonb :serialized_properties + t.text :on_finish + t.text :on_success + t.text :on_discard + t.text :callback_queue_name + t.integer :callback_priority + t.datetime :enqueued_at + t.datetime :discarded_at + t.datetime :finished_at + t.datetime :jobs_finished_at + end + + create_table :good_job_executions, id: :uuid do |t| + t.timestamps + + t.uuid :active_job_id, null: false + t.text :job_class + t.text :queue_name + t.jsonb :serialized_params + t.datetime :scheduled_at + t.datetime :finished_at + t.text :error + t.integer :error_event, limit: 2 + t.text :error_backtrace, array: true + t.uuid :process_id + t.interval :duration + end + + create_table :good_job_processes, id: :uuid do |t| + t.timestamps + t.jsonb :state + t.integer :lock_type, limit: 2 + end + + create_table :good_job_settings, id: :uuid do |t| + t.timestamps + t.text :key + t.jsonb :value + t.index :key, unique: true + end + + add_index :good_jobs, :scheduled_at, where: "(finished_at IS NULL)", name: :index_good_jobs_on_scheduled_at + add_index :good_jobs, [ :queue_name, :scheduled_at ], where: "(finished_at IS NULL)", name: :index_good_jobs_on_queue_name_and_scheduled_at + add_index :good_jobs, [ :active_job_id, :created_at ], name: :index_good_jobs_on_active_job_id_and_created_at + add_index :good_jobs, :concurrency_key, where: "(finished_at IS NULL)", name: :index_good_jobs_on_concurrency_key_when_unfinished + add_index :good_jobs, [ :concurrency_key, :created_at ], name: :index_good_jobs_on_concurrency_key_and_created_at + add_index :good_jobs, [ :cron_key, :created_at ], where: "(cron_key IS NOT NULL)", name: :index_good_jobs_on_cron_key_and_created_at_cond + add_index :good_jobs, [ :cron_key, :cron_at ], where: "(cron_key IS NOT NULL)", unique: true, name: :index_good_jobs_on_cron_key_and_cron_at_cond + add_index :good_jobs, [ :finished_at ], where: "finished_at IS NOT NULL", name: :index_good_jobs_jobs_on_finished_at_only + add_index :good_jobs, [ :priority, :created_at ], order: { priority: "DESC NULLS LAST", created_at: :asc }, + where: "finished_at IS NULL", name: :index_good_jobs_jobs_on_priority_created_at_when_unfinished + add_index :good_jobs, [ :priority, :created_at ], order: { priority: "ASC NULLS LAST", created_at: :asc }, + where: "finished_at IS NULL", name: :index_good_job_jobs_for_candidate_lookup + add_index :good_jobs, [ :batch_id ], where: "batch_id IS NOT NULL" + add_index :good_jobs, [ :batch_callback_id ], where: "batch_callback_id IS NOT NULL" + add_index :good_jobs, :job_class, name: :index_good_jobs_on_job_class + add_index :good_jobs, :labels, using: :gin, where: "(labels IS NOT NULL)", name: :index_good_jobs_on_labels + + add_index :good_job_executions, [ :active_job_id, :created_at ], name: :index_good_job_executions_on_active_job_id_and_created_at + add_index :good_jobs, [ :priority, :scheduled_at ], order: { priority: "ASC NULLS LAST", scheduled_at: :asc }, + where: "finished_at IS NULL AND locked_by_id IS NULL", name: :index_good_jobs_on_priority_scheduled_at_unfinished_unlocked + add_index :good_jobs, :locked_by_id, + where: "locked_by_id IS NOT NULL", name: "index_good_jobs_on_locked_by_id" + add_index :good_job_executions, [ :process_id, :created_at ], name: :index_good_job_executions_on_process_id_and_created_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 848df19c..9336c27c 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: 2026_03_06_000001) do +ActiveRecord::Schema[7.2].define(version: 2026_04_07_004259) do create_schema "hypershield" # These are extensions that must be enabled in order to support this database @@ -161,6 +161,97 @@ t.index ["course_id"], name: "index_form_settings_on_course_id" end + create_table "good_job_batches", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.text "description" + t.jsonb "serialized_properties" + t.text "on_finish" + t.text "on_success" + t.text "on_discard" + t.text "callback_queue_name" + t.integer "callback_priority" + t.datetime "enqueued_at" + t.datetime "discarded_at" + t.datetime "finished_at" + t.datetime "jobs_finished_at" + end + + create_table "good_job_executions", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.uuid "active_job_id", null: false + t.text "job_class" + t.text "queue_name" + t.jsonb "serialized_params" + t.datetime "scheduled_at" + t.datetime "finished_at" + t.text "error" + t.integer "error_event", limit: 2 + t.text "error_backtrace", array: true + t.uuid "process_id" + t.interval "duration" + t.index ["active_job_id", "created_at"], name: "index_good_job_executions_on_active_job_id_and_created_at" + t.index ["process_id", "created_at"], name: "index_good_job_executions_on_process_id_and_created_at" + end + + create_table "good_job_processes", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.jsonb "state" + t.integer "lock_type", limit: 2 + end + + create_table "good_job_settings", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.text "key" + t.jsonb "value" + t.index ["key"], name: "index_good_job_settings_on_key", unique: true + end + + create_table "good_jobs", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.text "queue_name" + t.integer "priority" + t.jsonb "serialized_params" + t.datetime "scheduled_at" + t.datetime "performed_at" + t.datetime "finished_at" + t.text "error" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.uuid "active_job_id" + t.text "concurrency_key" + t.text "cron_key" + t.uuid "retried_good_job_id" + t.datetime "cron_at" + t.uuid "batch_id" + t.uuid "batch_callback_id" + t.boolean "is_discrete" + t.integer "executions_count" + t.text "job_class" + t.integer "error_event", limit: 2 + t.text "labels", array: true + t.uuid "locked_by_id" + t.datetime "locked_at" + t.index ["active_job_id", "created_at"], name: "index_good_jobs_on_active_job_id_and_created_at" + t.index ["batch_callback_id"], name: "index_good_jobs_on_batch_callback_id", where: "(batch_callback_id IS NOT NULL)" + t.index ["batch_id"], name: "index_good_jobs_on_batch_id", where: "(batch_id IS NOT NULL)" + t.index ["concurrency_key", "created_at"], name: "index_good_jobs_on_concurrency_key_and_created_at" + t.index ["concurrency_key"], name: "index_good_jobs_on_concurrency_key_when_unfinished", where: "(finished_at IS NULL)" + t.index ["cron_key", "created_at"], name: "index_good_jobs_on_cron_key_and_created_at_cond", where: "(cron_key IS NOT NULL)" + t.index ["cron_key", "cron_at"], name: "index_good_jobs_on_cron_key_and_cron_at_cond", unique: true, where: "(cron_key IS NOT NULL)" + t.index ["finished_at"], name: "index_good_jobs_jobs_on_finished_at_only", where: "(finished_at IS NOT NULL)" + t.index ["job_class"], name: "index_good_jobs_on_job_class" + t.index ["labels"], name: "index_good_jobs_on_labels", where: "(labels IS NOT NULL)", using: :gin + t.index ["locked_by_id"], name: "index_good_jobs_on_locked_by_id", where: "(locked_by_id IS NOT NULL)" + t.index ["priority", "created_at"], name: "index_good_job_jobs_for_candidate_lookup", where: "(finished_at IS NULL)" + t.index ["priority", "created_at"], name: "index_good_jobs_jobs_on_priority_created_at_when_unfinished", order: { priority: "DESC NULLS LAST" }, where: "(finished_at IS NULL)" + t.index ["priority", "scheduled_at"], name: "index_good_jobs_on_priority_scheduled_at_unfinished_unlocked", where: "((finished_at IS NULL) AND (locked_by_id IS NULL))" + t.index ["queue_name", "scheduled_at"], name: "index_good_jobs_on_queue_name_and_scheduled_at", where: "(finished_at IS NULL)" + t.index ["scheduled_at"], name: "index_good_jobs_on_scheduled_at", where: "(finished_at IS NULL)" + end + create_table "lms_credentials", force: :cascade do |t| t.bigint "user_id" t.string "lms_name" From 556182ca0101f1fe5daa0f1df921ce72d4222dd8 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Tue, 7 Apr 2026 18:14:38 -0700 Subject: [PATCH 09/74] Added dotenv gem to run app locally --- Gemfile | 3 +++ Gemfile.lock | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/Gemfile b/Gemfile index 089afc3b..70f07eae 100644 --- a/Gemfile +++ b/Gemfile @@ -32,6 +32,9 @@ gem 'tzinfo-data', platforms: %i[windows jruby] # Reduces boot times through caching; required in config/boot.rb gem 'bootsnap', require: false +# Loads environment variables from .env (local dev/test only, not Heroku) +gem 'dotenv-rails', groups: [:development, :test] + # Alternative Canvas API. We probably don't need this. # Verify instances of `LMS::Canvas` gem 'lms-api' diff --git a/Gemfile.lock b/Gemfile.lock index b9ef2bf1..d6200315 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -185,6 +185,10 @@ GEM diff-lcs (1.6.2) docile (1.4.1) domain_name (0.6.20240107) + dotenv (3.2.0) + dotenv-rails (3.2.0) + dotenv (= 3.2.0) + railties (>= 6.1) drb (2.2.3) dumb_delegator (1.1.0) erb (6.0.2) @@ -637,6 +641,7 @@ DEPENDENCIES cucumber-rails database_cleaner-active_record debug + dotenv-rails factory_bot_rails faraday faraday-cookie_jar From f6a44345ede6d67e4826fb595b55e19967c88f35 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Tue, 7 Apr 2026 18:23:52 -0700 Subject: [PATCH 10/74] Add Procfile for Heroku web and worker dynos --- Procfile | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Procfile diff --git a/Procfile b/Procfile new file mode 100644 index 00000000..581b1f10 --- /dev/null +++ b/Procfile @@ -0,0 +1,2 @@ +web: bundle exec rails server -p $PORT +worker: bundle exec good_job start From 4a5bf8c0eed9c521e72325ce841f69f22853ab2f Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Thu, 9 Apr 2026 16:28:00 -0700 Subject: [PATCH 11/74] Added more RSpec and cucumber tests --- features/assignments.feature | 15 +- features/enrollments.feature | 13 ++ features/step_definitions/custom_steps.rb | 24 +++ spec/jobs/sync_users_from_canvas_job_spec.rb | 188 +++++++++++++++++++ 4 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 spec/jobs/sync_users_from_canvas_job_spec.rb diff --git a/features/assignments.feature b/features/assignments.feature index d6978f7f..61f07628 100644 --- a/features/assignments.feature +++ b/features/assignments.feature @@ -15,4 +15,17 @@ Feature: Course Assignments Then I log in as a student And I go to the Course page Then I should not see "Homework 1" - Then I should see "Homework 2" \ No newline at end of file + Then I should see "Homework 2" + + Scenario: Instructor sees the Sync Assignments button + Given I'm logged in as a teacher + When I go to the Course page + Then I should see a "Sync Assignments" button + + @javascript + Scenario: Clicking Sync Assignments disables the button and shows a spinner + Given I'm logged in as a teacher + When I go to the Course page + And I click the "Sync Assignments" button + Then the "Sync Assignments" button should be disabled + And I should see a loading spinner \ No newline at end of file diff --git a/features/enrollments.feature b/features/enrollments.feature index 8d16f577..4be7d1c0 100644 --- a/features/enrollments.feature +++ b/features/enrollments.feature @@ -35,3 +35,16 @@ Feature: Course Enrollments And I click the name link for student "User 3" Then I should be on the "Requests page" with param show_all=true And the requests table search should be filtered + + Scenario: Instructor sees the Sync Enrollments button + Given I'm logged in as a teacher + When I go to the Course Enrollments page + Then I should see a "Sync Enrollments" button + + @javascript + Scenario: Clicking Sync Enrollments disables the button and shows a spinner + Given I'm logged in as a teacher + When I go to the Course Enrollments page + And I click the "Sync Enrollments" button + Then the "Sync Enrollments" button should be disabled + And I should see a loading spinner diff --git a/features/step_definitions/custom_steps.rb b/features/step_definitions/custom_steps.rb index 651f5c7a..2a2bfb98 100644 --- a/features/step_definitions/custom_steps.rb +++ b/features/step_definitions/custom_steps.rb @@ -218,6 +218,30 @@ expect(page.current_path).to eq(expected_path) end +################### +# SYNC UI # +################### + +# Then I should see a "Sync Enrollments" button +Then(/^I should see a "([^"]*)" button$/) do |label| + expect(page).to have_button(label) +end + +# When I click the "Sync Enrollments" button +When(/^I click the "([^"]*)" button$/) do |label| + click_button(label) +end + +# Then the "Sync Enrollments" button should be disabled +Then(/^the "([^"]*)" button should be disabled$/) do |label| + expect(page).to have_button(label, disabled: true) +end + +# Then I should see a loading spinner +Then(/^I should see a loading spinner$/) do + expect(page).to have_css('.spinner-border:not(.d-none)', visible: true) +end + Given(/^I deny the request for "([^"]*)"$/) do |assignment_name| request = Request.joins(:assignment) .find_by(assignments: { name: assignment_name }, status: 'pending') diff --git a/spec/jobs/sync_users_from_canvas_job_spec.rb b/spec/jobs/sync_users_from_canvas_job_spec.rb new file mode 100644 index 00000000..c448ec89 --- /dev/null +++ b/spec/jobs/sync_users_from_canvas_job_spec.rb @@ -0,0 +1,188 @@ +require 'rails_helper' + +RSpec.describe SyncUsersFromCanvasJob, type: :job do + let(:course) { create(:course, :with_staff) } + let(:sync_user) { course.staff_users.first } + let(:canvas_facade_double) { instance_double(CanvasFacade) } + + before do + allow(sync_user).to receive(:ensure_fresh_canvas_token!).and_return('fake_token') + allow(CanvasFacade).to receive(:new).with('fake_token').and_return(canvas_facade_double) + end + + describe '#perform' do + context 'user upsert' do + let(:canvas_user) { create(:user) } + let(:canvas_data) do + [{ 'id' => canvas_user.canvas_uid, 'name' => canvas_user.name, + 'email' => canvas_user.email, 'sis_user_id' => canvas_user.student_id }] + end + + before do + allow(canvas_facade_double).to receive(:get_all_course_users).and_return(canvas_data) + end + + it 'creates new users from Canvas data when they do not exist locally' do + new_uid = 'brand-new-canvas-uid' + allow(canvas_facade_double).to receive(:get_all_course_users) + .and_return([{ 'id' => new_uid, 'name' => 'New Person', 'email' => 'new@example.com', 'sis_user_id' => 'S99' }]) + + expect { + described_class.perform_now(course.id, sync_user.id, 'student') + }.to change(User, :count).by(1) + + expect(User.find_by(canvas_uid: new_uid)).to have_attributes(name: 'New Person', email: 'new@example.com') + end + + it 'updates an existing user without creating a duplicate' do + canvas_user.update!(name: 'Old Name') + updated_data = [{ 'id' => canvas_user.canvas_uid, 'name' => 'New Name', + 'email' => canvas_user.email, 'sis_user_id' => canvas_user.student_id }] + allow(canvas_facade_double).to receive(:get_all_course_users).and_return(updated_data) + + expect { + described_class.perform_now(course.id, sync_user.id, 'student') + }.not_to change(User, :count) + + expect(canvas_user.reload.name).to eq('New Name') + end + + it 'skips users with a blank email' do + allow(canvas_facade_double).to receive(:get_all_course_users) + .and_return([{ 'id' => 'no-email', 'name' => 'No Email', 'email' => '', 'sis_user_id' => nil }]) + + expect { + described_class.perform_now(course.id, sync_user.id, 'student') + }.not_to change(User, :count) + end + end + + context 'enrollment creation' do + let(:student1) { create(:user) } + let(:student2) { create(:user) } + let(:canvas_data) do + [student1, student2].map do |u| + { 'id' => u.canvas_uid, 'name' => u.name, 'email' => u.email, 'sis_user_id' => u.student_id } + end + end + + before do + allow(canvas_facade_double).to receive(:get_all_course_users).and_return(canvas_data) + end + + it 'creates UserToCourse enrollments for synced users' do + expect { + described_class.perform_now(course.id, sync_user.id, 'student') + }.to change(UserToCourse, :count).by(2) + + expect(UserToCourse.exists?(user: student1, course: course, role: 'student')).to be true + expect(UserToCourse.exists?(user: student2, course: course, role: 'student')).to be true + end + + it 'assigns the correct role to enrollments' do + described_class.perform_now(course.id, sync_user.id, 'ta') + + expect(UserToCourse.find_by(user: student1, course: course).role).to eq('ta') + end + + it 'does not duplicate enrollments on re-run' do + described_class.perform_now(course.id, sync_user.id, 'student') + + expect { + described_class.perform_now(course.id, sync_user.id, 'student') + }.not_to change(UserToCourse, :count) + end + end + + context 'role-based removal' do + let(:remaining) { create(:user) } + let(:removed) { create(:user) } + + before do + create(:user_to_course, user: removed, course: course, role: 'student') + allow(canvas_facade_double).to receive(:get_all_course_users) + .and_return([{ 'id' => remaining.canvas_uid, 'name' => remaining.name, + 'email' => remaining.email, 'sis_user_id' => remaining.student_id }]) + end + + it 'removes enrollments for users no longer returned by Canvas' do + expect { + described_class.perform_now(course.id, sync_user.id, 'student') + }.to change(UserToCourse, :count).by(-1) + + expect(UserToCourse.exists?(user: removed, course: course)).to be false + end + + it 'does not remove enrollments for other roles' do + teacher = create(:user) + create(:user_to_course, user: teacher, course: course, role: 'teacher') + + described_class.perform_now(course.id, sync_user.id, 'student') + + expect(UserToCourse.exists?(user: teacher, course: course, role: 'teacher')).to be true + end + end + + context 'multiple roles' do + let(:student) { create(:user) } + let(:ta) { create(:user) } + + before do + allow(canvas_facade_double).to receive(:get_all_course_users).with(anything, 'student') + .and_return([{ 'id' => student.canvas_uid, 'name' => student.name, 'email' => student.email, 'sis_user_id' => student.student_id }]) + allow(canvas_facade_double).to receive(:get_all_course_users).with(anything, 'ta') + .and_return([{ 'id' => ta.canvas_uid, 'name' => ta.name, 'email' => ta.email, 'sis_user_id' => ta.student_id }]) + end + + it 'syncs each role independently' do + described_class.perform_now(course.id, sync_user.id, %w[student ta]) + + expect(UserToCourse.exists?(user: student, course: course, role: 'student')).to be true + expect(UserToCourse.exists?(user: ta, course: course, role: 'ta')).to be true + end + end + + context 'return value and persistence' do + let(:new_student) { create(:user) } + + before do + allow(canvas_facade_double).to receive(:get_all_course_users) + .and_return([{ 'id' => new_student.canvas_uid, 'name' => new_student.name, + 'email' => new_student.email, 'sis_user_id' => new_student.student_id }]) + end + + it 'returns results keyed by role with added/removed/updated counts' do + result = described_class.perform_now(course.id, sync_user.id, 'student') + + expect(result[:student]).to include(added: 1, removed: 0, updated: 0) + end + + it 'includes a synced_at timestamp' do + result = described_class.perform_now(course.id, sync_user.id, 'student') + + expect(result[:synced_at]).to be_within(1.second).of(Time.current) + end + + it 'persists results to course_to_lms.recent_roster_sync' do + described_class.perform_now(course.id, sync_user.id, 'student') + + expect(course.course_to_lms(1).reload.recent_roster_sync).to include('student' => include('added' => 1)) + end + end + + context 'when Canvas returns a non-array response' do + before do + allow(canvas_facade_double).to receive(:get_all_course_users).and_return({ 'error' => 'unauthorized' }) + end + + it 'returns zeroed counts without raising' do + result = nil + expect { + result = described_class.perform_now(course.id, sync_user.id, 'student') + }.not_to raise_error + + expect(result[:student]).to eq(added: 0, removed: 0, updated: 0) + end + end + end +end From e807177c27dd179da492a01ab601fcc3be70f0e1 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Fri, 10 Apr 2026 03:47:30 -0700 Subject: [PATCH 12/74] Add RSpec and Cucumber tests for async syncs --- Gemfile | 2 +- app/controllers/session_controller.rb | 2 - .../controllers/enrollments_controller.js | 2 +- config/environments/test.rb | 5 ++ features/step_definitions/custom_steps.rb | 15 +++++- .../sync_all_course_assignments_job_spec.rb | 49 +++++++++++++----- spec/jobs/sync_users_from_canvas_job_spec.rb | 50 +++++++++++-------- spec/models/course_spec.rb | 9 ++-- 8 files changed, 87 insertions(+), 47 deletions(-) diff --git a/Gemfile b/Gemfile index 70f07eae..1657433d 100644 --- a/Gemfile +++ b/Gemfile @@ -33,7 +33,7 @@ gem 'tzinfo-data', platforms: %i[windows jruby] gem 'bootsnap', require: false # Loads environment variables from .env (local dev/test only, not Heroku) -gem 'dotenv-rails', groups: [:development, :test] +gem 'dotenv-rails', groups: [ :development, :test ] # Alternative Canvas API. We probably don't need this. # Verify instances of `LMS::Canvas` diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 14d181e3..81fd512c 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -65,8 +65,6 @@ def omniauth_callback redirect_to courses_path, notice: "Logged in! Welcome, #{user_data['name']}!" rescue StandardError => e - puts(e.message) - puts(e) Rails.logger.error("OmniAuth callback error: #{e.message}") redirect_to root_path, alert: 'Authentication failed. Invalid credentials.' end diff --git a/app/javascript/controllers/enrollments_controller.js b/app/javascript/controllers/enrollments_controller.js index 145d73b2..6e25b3b2 100644 --- a/app/javascript/controllers/enrollments_controller.js +++ b/app/javascript/controllers/enrollments_controller.js @@ -81,7 +81,7 @@ export default class extends Controller { const label = this.syncLabelTarget; const spinner = this.syncSpinnerTarget; const courseId = this.courseIdValue; - const token = document.querySelector('meta[name="csrf-token"]').content; + const token = document.querySelector('meta[name="csrf-token"]')?.content || ''; button.disabled = true; label.textContent = "Syncing..."; diff --git a/config/environments/test.rb b/config/environments/test.rb index c9f0436f..b43794c5 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -66,6 +66,11 @@ # Raise error when a before_action's only/except options reference missing actions config.action_controller.raise_on_missing_callback_actions = true + # Disable GoodJob's in-process worker so enqueued jobs do not execute during tests. + # Without this, GoodJob runs jobs in background threads, causing sync operations to + # complete before Capybara can observe transient UI states (spinner, disabled button). + config.good_job.execution_mode = :external + # Set up default encryption keys for the test environment config.active_record.encryption.primary_key = 'test-primary-key-1234567890abcdef' config.active_record.encryption.deterministic_key = 'test-deterministic-key-1234567890abcdef' diff --git a/features/step_definitions/custom_steps.rb b/features/step_definitions/custom_steps.rb index 2a2bfb98..e1b33e54 100644 --- a/features/step_definitions/custom_steps.rb +++ b/features/step_definitions/custom_steps.rb @@ -229,12 +229,23 @@ # When I click the "Sync Enrollments" button When(/^I click the "([^"]*)" button$/) do |label| + # csrf_meta_tags renders nothing when allow_forgery_protection = false (test env). + # Inject a placeholder token so JS fetch handlers don't throw before disabling the button. + page.execute_script(<<~JS) + if (!document.querySelector('meta[name="csrf-token"]')) { + const m = document.createElement('meta'); + m.name = 'csrf-token'; + m.content = 'test-csrf-token'; + document.head.appendChild(m); + } + JS click_button(label) end # Then the "Sync Enrollments" button should be disabled -Then(/^the "([^"]*)" button should be disabled$/) do |label| - expect(page).to have_button(label, disabled: true) +# When sync starts, the label changes to "Syncing..." so we check for that text + disabled +Then(/^the "([^"]*)" button should be disabled$/) do |_label| + expect(page).to have_button("Syncing...", disabled: true) end # Then I should see a loading spinner diff --git a/spec/jobs/sync_all_course_assignments_job_spec.rb b/spec/jobs/sync_all_course_assignments_job_spec.rb index 079b0dd1..370a457c 100644 --- a/spec/jobs/sync_all_course_assignments_job_spec.rb +++ b/spec/jobs/sync_all_course_assignments_job_spec.rb @@ -131,19 +131,42 @@ end - # 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) - - assignment = Assignment.last - expect(assignment.name).to eq('HW1') + describe '#sync_assignment' do + let(:job) { described_class.new } + before { Assignment.where(course_to_lms: course_to_lms).destroy_all } + let(:lms_assignment) do + build_canvas_assignment('id' => 'a123', 'name' => 'HW1', 'due_at' => '2025-06-01T23:59:00Z', 'lock_at' => nil) + end + let(:results) { { added_assignments: 0, updated_assignments: 0, unchanged_assignments: 0 } } + + it 'creates a new assignment' do + expect { + job.sync_assignment(course_to_lms, lms_assignment, results) + }.to change(Assignment, :count).by(1) + + assignment = Assignment.find_by(external_assignment_id: 'a123') + expect(assignment).to have_attributes(name: 'HW1', due_date: DateTime.parse('2025-06-01T23:59:00Z')) + expect(results[:added_assignments]).to eq(1) + end + + it 'updates an existing assignment' do + create(:assignment, course_to_lms: course_to_lms, external_assignment_id: 'a123', name: 'Old Name') + + expect { + job.sync_assignment(course_to_lms, lms_assignment, results) + }.not_to change(Assignment, :count) + + expect(Assignment.find_by(external_assignment_id: 'a123').name).to eq('HW1') + expect(results[:updated_assignments]).to eq(1) + end + + it 'increments unchanged_assignments when nothing changed' do + create(:assignment, course_to_lms: course_to_lms, external_assignment_id: 'a123', + name: 'HW1', due_date: DateTime.parse('2025-06-01T23:59:00Z'), late_due_date: nil) + + job.sync_assignment(course_to_lms, lms_assignment, results) + + expect(results[:unchanged_assignments]).to eq(1) end end diff --git a/spec/jobs/sync_users_from_canvas_job_spec.rb b/spec/jobs/sync_users_from_canvas_job_spec.rb index c448ec89..bb75694c 100644 --- a/spec/jobs/sync_users_from_canvas_job_spec.rb +++ b/spec/jobs/sync_users_from_canvas_job_spec.rb @@ -14,8 +14,8 @@ context 'user upsert' do let(:canvas_user) { create(:user) } let(:canvas_data) do - [{ 'id' => canvas_user.canvas_uid, 'name' => canvas_user.name, - 'email' => canvas_user.email, 'sis_user_id' => canvas_user.student_id }] + [ { 'id' => canvas_user.canvas_uid, 'name' => canvas_user.name, + 'email' => canvas_user.email, 'sis_user_id' => canvas_user.student_id } ] end before do @@ -25,7 +25,7 @@ it 'creates new users from Canvas data when they do not exist locally' do new_uid = 'brand-new-canvas-uid' allow(canvas_facade_double).to receive(:get_all_course_users) - .and_return([{ 'id' => new_uid, 'name' => 'New Person', 'email' => 'new@example.com', 'sis_user_id' => 'S99' }]) + .and_return([ { 'id' => new_uid, 'name' => 'New Person', 'email' => 'new@example.com', 'sis_user_id' => 'S99' } ]) expect { described_class.perform_now(course.id, sync_user.id, 'student') @@ -36,8 +36,8 @@ it 'updates an existing user without creating a duplicate' do canvas_user.update!(name: 'Old Name') - updated_data = [{ 'id' => canvas_user.canvas_uid, 'name' => 'New Name', - 'email' => canvas_user.email, 'sis_user_id' => canvas_user.student_id }] + updated_data = [ { 'id' => canvas_user.canvas_uid, 'name' => 'New Name', + 'email' => canvas_user.email, 'sis_user_id' => canvas_user.student_id } ] allow(canvas_facade_double).to receive(:get_all_course_users).and_return(updated_data) expect { @@ -49,7 +49,7 @@ it 'skips users with a blank email' do allow(canvas_facade_double).to receive(:get_all_course_users) - .and_return([{ 'id' => 'no-email', 'name' => 'No Email', 'email' => '', 'sis_user_id' => nil }]) + .and_return([ { 'id' => 'no-email', 'name' => 'No Email', 'email' => '', 'sis_user_id' => nil } ]) expect { described_class.perform_now(course.id, sync_user.id, 'student') @@ -58,10 +58,10 @@ end context 'enrollment creation' do - let(:student1) { create(:user) } - let(:student2) { create(:user) } + let(:first_student) { create(:user) } + let(:second_student) { create(:user) } let(:canvas_data) do - [student1, student2].map do |u| + [ first_student, second_student ].map do |u| { 'id' => u.canvas_uid, 'name' => u.name, 'email' => u.email, 'sis_user_id' => u.student_id } end end @@ -75,14 +75,14 @@ described_class.perform_now(course.id, sync_user.id, 'student') }.to change(UserToCourse, :count).by(2) - expect(UserToCourse.exists?(user: student1, course: course, role: 'student')).to be true - expect(UserToCourse.exists?(user: student2, course: course, role: 'student')).to be true + expect(UserToCourse.exists?(user: first_student, course: course, role: 'student')).to be true + expect(UserToCourse.exists?(user: second_student, course: course, role: 'student')).to be true end it 'assigns the correct role to enrollments' do described_class.perform_now(course.id, sync_user.id, 'ta') - expect(UserToCourse.find_by(user: student1, course: course).role).to eq('ta') + expect(UserToCourse.find_by(user: first_student, course: course).role).to eq('ta') end it 'does not duplicate enrollments on re-run' do @@ -101,11 +101,14 @@ before do create(:user_to_course, user: removed, course: course, role: 'student') allow(canvas_facade_double).to receive(:get_all_course_users) - .and_return([{ 'id' => remaining.canvas_uid, 'name' => remaining.name, - 'email' => remaining.email, 'sis_user_id' => remaining.student_id }]) + .and_return([ { 'id' => remaining.canvas_uid, 'name' => remaining.name, + 'email' => remaining.email, 'sis_user_id' => remaining.student_id } ]) end it 'removes enrollments for users no longer returned by Canvas' do + # Also pre-enroll `remaining` so the job's insert doesn't offset the removal + create(:user_to_course, user: remaining, course: course, role: 'student') + expect { described_class.perform_now(course.id, sync_user.id, 'student') }.to change(UserToCourse, :count).by(-1) @@ -129,9 +132,9 @@ before do allow(canvas_facade_double).to receive(:get_all_course_users).with(anything, 'student') - .and_return([{ 'id' => student.canvas_uid, 'name' => student.name, 'email' => student.email, 'sis_user_id' => student.student_id }]) + .and_return([ { 'id' => student.canvas_uid, 'name' => student.name, 'email' => student.email, 'sis_user_id' => student.student_id } ]) allow(canvas_facade_double).to receive(:get_all_course_users).with(anything, 'ta') - .and_return([{ 'id' => ta.canvas_uid, 'name' => ta.name, 'email' => ta.email, 'sis_user_id' => ta.student_id }]) + .and_return([ { 'id' => ta.canvas_uid, 'name' => ta.name, 'email' => ta.email, 'sis_user_id' => ta.student_id } ]) end it 'syncs each role independently' do @@ -143,18 +146,20 @@ end context 'return value and persistence' do - let(:new_student) { create(:user) } + # Use a canvas_uid not in the DB so the job counts this as an add, not an update + let(:canvas_data) do + [ { 'id' => 'brand-new-uid-999', 'name' => 'New Student', 'email' => 'newstudent@example.com', 'sis_user_id' => 'S999' } ] + end before do - allow(canvas_facade_double).to receive(:get_all_course_users) - .and_return([{ 'id' => new_student.canvas_uid, 'name' => new_student.name, - 'email' => new_student.email, 'sis_user_id' => new_student.student_id }]) + allow(canvas_facade_double).to receive(:get_all_course_users).and_return(canvas_data) end it 'returns results keyed by role with added/removed/updated counts' do result = described_class.perform_now(course.id, sync_user.id, 'student') - expect(result[:student]).to include(added: 1, removed: 0, updated: 0) + # The job keys results with string role names + expect(result['student']).to include(added: 1, removed: 0, updated: 0) end it 'includes a synced_at timestamp' do @@ -181,7 +186,8 @@ result = described_class.perform_now(course.id, sync_user.id, 'student') }.not_to raise_error - expect(result[:student]).to eq(added: 0, removed: 0, updated: 0) + # The job keys results with string role names + expect(result['student']).to eq(added: 0, removed: 0, updated: 0) end end end diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index d85efb78..c3a9d17c 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -171,12 +171,9 @@ allow(user).to receive(:ensure_fresh_canvas_token!).and_return('fake_token') end - it 'creates user and user_to_course record' do - expect do - course.sync_users_from_canvas(user.id, 'student') - end.to change(User, :count).by(CANVAS_USERS.size).and( - change(UserToCourse, :count).by(CANVAS_USERS.size) - ) + it 'enqueues SyncUsersFromCanvasJob with the correct arguments' do + expect(SyncUsersFromCanvasJob).to receive(:perform_later).with(course.id, user.id, 'student') + course.sync_users_from_canvas(user.id, 'student') end end From 7701fabb6833ba40160904ef989916326e505e2d Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Fri, 10 Apr 2026 03:56:11 -0700 Subject: [PATCH 13/74] Fixed Ruby lint --- spec/jobs/sync_all_course_assignments_job_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/jobs/sync_all_course_assignments_job_spec.rb b/spec/jobs/sync_all_course_assignments_job_spec.rb index 370a457c..6f195486 100644 --- a/spec/jobs/sync_all_course_assignments_job_spec.rb +++ b/spec/jobs/sync_all_course_assignments_job_spec.rb @@ -133,12 +133,13 @@ describe '#sync_assignment' do let(:job) { described_class.new } - before { Assignment.where(course_to_lms: course_to_lms).destroy_all } let(:lms_assignment) do build_canvas_assignment('id' => 'a123', 'name' => 'HW1', 'due_at' => '2025-06-01T23:59:00Z', 'lock_at' => nil) end let(:results) { { added_assignments: 0, updated_assignments: 0, unchanged_assignments: 0 } } + before { Assignment.where(course_to_lms: course_to_lms).destroy_all } + it 'creates a new assignment' do expect { job.sync_assignment(course_to_lms, lms_assignment, results) From 17a375f9104a4b330aa869fc9cdc15113a8e33a5 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Mon, 13 Apr 2026 22:19:45 -0700 Subject: [PATCH 14/74] Changed polling time to 1s --- app/javascript/controllers/assignment_controller.js | 2 +- app/javascript/controllers/enrollments_controller.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/javascript/controllers/assignment_controller.js b/app/javascript/controllers/assignment_controller.js index 8d7cbae1..7b44a4ff 100644 --- a/app/javascript/controllers/assignment_controller.js +++ b/app/javascript/controllers/assignment_controller.js @@ -76,7 +76,7 @@ export default class extends Controller { spinner.classList.remove("d-none"); try { - const statusBefore = await fetch(`/courses/${courseId}/sync_status`).then(r => r.json()); + const statusBefore = await this._fetchJson(`/courses/${courseId}/sync_status`); const beforeTs = statusBefore.assignments_synced_at; const response = await fetch(`/courses/${courseId}/sync_assignments`, { diff --git a/app/javascript/controllers/enrollments_controller.js b/app/javascript/controllers/enrollments_controller.js index 6e25b3b2..b506cae4 100644 --- a/app/javascript/controllers/enrollments_controller.js +++ b/app/javascript/controllers/enrollments_controller.js @@ -112,7 +112,7 @@ export default class extends Controller { } } - async _pollUntilDone(courseId, key, beforeTs, intervalMs = 2000, timeoutMs = 60000) { + async _pollUntilDone(courseId, key, beforeTs, intervalMs = 1000, timeoutMs = 60000) { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { await new Promise(resolve => setTimeout(resolve, intervalMs)); From 25d7799b909a457c8287b646318b21be42887dbe Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Mon, 20 Apr 2026 11:22:36 -0700 Subject: [PATCH 15/74] Added initializer for good_job --- config/initializers/good_job.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 config/initializers/good_job.rb diff --git a/config/initializers/good_job.rb b/config/initializers/good_job.rb new file mode 100644 index 00000000..3c827270 --- /dev/null +++ b/config/initializers/good_job.rb @@ -0,0 +1,17 @@ +Rails.application.config.to_prepare do + GoodJob::ApplicationController.class_eval do + def current_user + @current_user ||= User.find_by(canvas_uid: session[:user_id]) + end + + before_action :require_admin + + def require_admin + if current_user.nil? + redirect_to '/', alert: 'You must be logged in.' + elsif !current_user.admin? + redirect_to '/', alert: 'You are not authorized to view this page.' + end + end + end +end From 5396841537e8afb036b3c295a51450b5a74e873d Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Mon, 20 Apr 2026 11:46:39 -0700 Subject: [PATCH 16/74] scoping lograge current_user to app controllers --- config/initializers/lograge.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/lograge.rb b/config/initializers/lograge.rb index 25af3a01..1a3bc987 100644 --- a/config/initializers/lograge.rb +++ b/config/initializers/lograge.rb @@ -25,7 +25,7 @@ config.lograge.custom_payload do |controller| { request_id: controller.request.uuid, - user_id: controller.current_user.try(:id) + user_id: controller.is_a?(ApplicationController) ? controller.current_user.try(:id) : nil } end From f416d983b2bce93ed1c87502ea2516ff6256d87e Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Mon, 20 Apr 2026 13:01:15 -0700 Subject: [PATCH 17/74] Retrigger CI From 071c68bd20f90927181d2a5ccce9247ed4f94be7 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Mon, 20 Apr 2026 13:23:01 -0700 Subject: [PATCH 18/74] Fix undefined _fetchJson call in assignment_controller sync --- app/javascript/controllers/assignment_controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/javascript/controllers/assignment_controller.js b/app/javascript/controllers/assignment_controller.js index 7b44a4ff..8d7cbae1 100644 --- a/app/javascript/controllers/assignment_controller.js +++ b/app/javascript/controllers/assignment_controller.js @@ -76,7 +76,7 @@ export default class extends Controller { spinner.classList.remove("d-none"); try { - const statusBefore = await this._fetchJson(`/courses/${courseId}/sync_status`); + const statusBefore = await fetch(`/courses/${courseId}/sync_status`).then(r => r.json()); const beforeTs = statusBefore.assignments_synced_at; const response = await fetch(`/courses/${courseId}/sync_assignments`, { From 883565649389d4f433663b61a55aa422466436c8 Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Tue, 21 Apr 2026 15:18:18 -0700 Subject: [PATCH 19/74] Added late days column --- app/controllers/courses_controller.rb | 1 + app/models/request.rb | 13 +++++ app/views/courses/enrollments.html.erb | 2 + spec/controllers/courses_controller_spec.rb | 5 ++ spec/models/request_spec.rb | 59 +++++++++++++++++++++ 5 files changed, 80 insertions(+) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index da987165..91fc0b8a 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -89,6 +89,7 @@ def enrollments @enrollments = @course.user_to_courses.includes(:user) @is_course_admin = @course.course_admin?(@user) + @approved_late_days = Request.total_approved_late_days_by_user(@course) end def delete diff --git a/app/models/request.rb b/app/models/request.rb index 2d69d957..ee1ba4fa 100644 --- a/app/models/request.rb +++ b/app/models/request.rb @@ -50,6 +50,19 @@ class Request < ApplicationRecord where(user: user, course: course, status: 'approved', auto_approved: true) } + # Returns { user_id => total_approved_late_days } for all users in a course + # only the widest approved extension is counted. + def self.total_approved_late_days_by_user(course) + rows = where(course: course, status: 'approved') + .joins(:assignment) + .group(:user_id, :assignment_id) + .pluck(:user_id, Arel.sql('MAX(requested_due_date::date - assignments.due_date::date)')) + + rows.each_with_object(Hash.new(0)) do |(user_id, max_days), totals| + totals[user_id] += max_days + end + end + # Class methods def self.merge_date_and_time!(request_params) return unless request_params[:requested_due_date].present? && request_params[:due_time].present? diff --git a/app/views/courses/enrollments.html.erb b/app/views/courses/enrollments.html.erb index 80750df6..b4966a6f 100644 --- a/app/views/courses/enrollments.html.erb +++ b/app/views/courses/enrollments.html.erb @@ -13,6 +13,7 @@ Student ID Email Role + Approved Days <% if @is_course_admin %> Extended Requests? @@ -52,6 +53,7 @@ <%= enrollment.user.student_id %> <%= enrollment.user.email %> <%= enrollment.role.downcase.capitalize %> + <%= enrollment.student? ? @approved_late_days.fetch(enrollment.user_id, 0) : '' %> <% if @is_course_admin %> diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index 4eb701c0..c9cb9d83 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -294,6 +294,11 @@ get :enrollments, params: { id: course.id } expect(assigns(:is_course_admin)).to be true end + + it 'assigns @approved_late_days' do + get :enrollments, params: { id: course.id } + expect(assigns(:approved_late_days)).to be_a(Hash) + end end context 'when user is a TA (staff but not course admin)' do diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb index 24bcb0eb..8afeea6f 100644 --- a/spec/models/request_spec.rb +++ b/spec/models/request_spec.rb @@ -922,4 +922,63 @@ request.send_email_response end end + + describe '.total_approved_late_days_by_user' do + let(:assignment2) do + Assignment.create!( + name: 'Assignment 2', + course_to_lms_id: course.course_to_lms(1).id, + external_assignment_id: 'ext2', + enabled: true, + due_date: 5.days.from_now + ) + end + + it 'sums approved days across different assignments' do + described_class.create!(user: user, course: course, assignment: assignment, + reason: 'r', status: 'approved', + requested_due_date: assignment.due_date + 2.days) + described_class.create!(user: user, course: course, assignment: assignment2, + reason: 'r', status: 'approved', + requested_due_date: assignment2.due_date + 3.days) + + result = described_class.total_approved_late_days_by_user(course) + expect(result[user.id]).to eq(5) + end + + it 'takes the wider window when multiple requests exist for the same assignment' do + described_class.create!(user: user, course: course, assignment: assignment, + reason: 'r', status: 'approved', + requested_due_date: assignment.due_date + 2.days) + described_class.create!(user: user, course: course, assignment: assignment, + reason: 'r', status: 'approved', + requested_due_date: assignment.due_date + 5.days) + + result = described_class.total_approved_late_days_by_user(course) + expect(result[user.id]).to eq(5) + end + + it 'does not count denied requests' do + described_class.create!(user: user, course: course, assignment: assignment, + reason: 'r', status: 'denied', + requested_due_date: assignment.due_date + 3.days) + + result = described_class.total_approved_late_days_by_user(course) + expect(result[user.id]).to eq(0) + end + + it 'does not count pending requests' do + described_class.create!(user: user, course: course, assignment: assignment, + reason: 'r', status: 'pending', + requested_due_date: assignment.due_date + 3.days) + + result = described_class.total_approved_late_days_by_user(course) + expect(result[user.id]).to eq(0) + end + + it 'returns 0 for users with no approved requests' do + result = described_class.total_approved_late_days_by_user(course) + expect(result[user.id]).to eq(0) + end + end end From 00149c788337fa79f53c2206ca79fe20c7a41e1e Mon Sep 17 00:00:00 2001 From: Noah Nizamian Date: Tue, 21 Apr 2026 15:30:39 -0700 Subject: [PATCH 20/74] fix cucumber tests --- app/javascript/controllers/enrollments_controller.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/javascript/controllers/enrollments_controller.js b/app/javascript/controllers/enrollments_controller.js index cd03aecb..5c4c070f 100644 --- a/app/javascript/controllers/enrollments_controller.js +++ b/app/javascript/controllers/enrollments_controller.js @@ -26,9 +26,9 @@ export default class extends Controller { responsive: true, pageLength: 500, lengthMenu: [[-1, 25, 50, 100, 500], ["All", 25, 50, 100, 500]], - columns: document.querySelectorAll('#enrollments-table thead th').length === 5 - ? [null, null, null, { orderDataType: 'role-pre' }, null] - : [null, null, null, { orderDataType: 'role-pre' }], + columns: document.querySelectorAll('#enrollments-table thead th').length === 6 + ? [null, null, null, { orderDataType: 'role-pre' }, null, null] + : [null, null, null, { orderDataType: 'role-pre' }, null], order: [[3, 'des'], [0, 'asc']] // Sort Role first, then Name }); } From c457642693ef087cba3a4f9973c45729d3c3984b Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Mon, 20 Apr 2026 12:43:02 -0700 Subject: [PATCH 21/74] 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 22/74] 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 805d9c867f201293ccec31bf656f8b6351c5d910 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Thu, 23 Apr 2026 19:23:41 -0700 Subject: [PATCH 23/74] Resolved merge conflict sync_assignment method --- .../sync_all_course_assignments_job_spec.rb | 74 ++++++++++++++----- 1 file changed, 55 insertions(+), 19 deletions(-) diff --git a/spec/jobs/sync_all_course_assignments_job_spec.rb b/spec/jobs/sync_all_course_assignments_job_spec.rb index 6f195486..0fc2cde9 100644 --- a/spec/jobs/sync_all_course_assignments_job_spec.rb +++ b/spec/jobs/sync_all_course_assignments_job_spec.rb @@ -133,39 +133,75 @@ describe '#sync_assignment' do let(:job) { described_class.new } - let(:lms_assignment) do - build_canvas_assignment('id' => 'a123', 'name' => 'HW1', 'due_at' => '2025-06-01T23:59:00Z', 'lock_at' => nil) + let(:results) do + { + added_assignments: 0, + updated_assignments: 0, + unchanged_assignments: 0, + deleted_assignments: 0 + } end - let(:results) { { added_assignments: 0, updated_assignments: 0, unchanged_assignments: 0 } } - before { Assignment.where(course_to_lms: course_to_lms).destroy_all } + it 'creates a new assignment and updates results' do + lms_assignment = build_canvas_assignment( + 'id' => 'a123', 'name' => 'HW1', + 'due_at' => '2025-01-15T23:59:00Z', 'lock_at' => '2025-01-20T23:59:00Z' + ) - it 'creates a new assignment' do - expect { - job.sync_assignment(course_to_lms, lms_assignment, results) - }.to change(Assignment, :count).by(1) + expect do + job.send(:sync_assignment, course_to_lms, lms_assignment, results) + end.to change { Assignment.where(course_to_lms_id: course_to_lms.id).count }.by(1) - assignment = Assignment.find_by(external_assignment_id: 'a123') - expect(assignment).to have_attributes(name: 'HW1', due_date: DateTime.parse('2025-06-01T23:59:00Z')) + assignment = Assignment.find_by(course_to_lms_id: 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' do - create(:assignment, course_to_lms: course_to_lms, external_assignment_id: 'a123', name: 'Old Name') + it 'updates an existing assignment and updates results' do + existing_assignment = create(:assignment, + course_to_lms: course_to_lms, + external_assignment_id: 'a123', + name: 'Old HW Name', + due_date: DateTime.parse('2025-01-10T23:59:00Z') + ) - expect { - job.sync_assignment(course_to_lms, lms_assignment, results) - }.not_to change(Assignment, :count) + 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, course_to_lms, lms_assignment, results) + end.not_to change { Assignment.where(course_to_lms_id: course_to_lms.id).count } - expect(Assignment.find_by(external_assignment_id: 'a123').name).to eq('HW1') + 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 it 'increments unchanged_assignments when nothing changed' do - create(:assignment, course_to_lms: course_to_lms, external_assignment_id: 'a123', - name: 'HW1', due_date: DateTime.parse('2025-06-01T23:59:00Z'), late_due_date: nil) + create(:assignment, + course_to_lms: course_to_lms, + external_assignment_id: 'a123', + name: 'HW1', + due_date: DateTime.parse('2025-06-01T23:59:00Z'), + late_due_date: nil + ) + + lms_assignment = build_canvas_assignment( + 'id' => 'a123', 'name' => 'HW1', + 'due_at' => '2025-06-01T23:59:00Z', 'lock_at' => nil + ) - job.sync_assignment(course_to_lms, lms_assignment, results) + job.send(:sync_assignment, course_to_lms, lms_assignment, results) expect(results[:unchanged_assignments]).to eq(1) end From 3a1010eb6f1e8e3f98984816398b629d0f19d443 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Thu, 23 Apr 2026 19:40:08 -0700 Subject: [PATCH 24/74] Fix rubocop: remove extra empty line at block body end --- spec/jobs/sync_all_course_assignments_job_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/jobs/sync_all_course_assignments_job_spec.rb b/spec/jobs/sync_all_course_assignments_job_spec.rb index 24ae9393..e6d87d51 100644 --- a/spec/jobs/sync_all_course_assignments_job_spec.rb +++ b/spec/jobs/sync_all_course_assignments_job_spec.rb @@ -221,7 +221,6 @@ expect(results[:added_assignments]).to eq(0) expect(results[:updated_assignments]).to eq(1) expect(results[:unchanged_assignments]).to eq(0) - end it 'increments unchanged_assignments when nothing changed' do From 2f519434885d04171db2e15865dcbc7198f10c47 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 23 Apr 2026 16:06:46 -0700 Subject: [PATCH 25/74] 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 26/74] 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 27/74] 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 28/74] 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 29/74] 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 30/74] 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 31/74] 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 32/74] 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 b69af4be21d87a3c8b7e014e91951883ee624bea Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 17:39:50 -0700 Subject: [PATCH 33/74] Updated gemfile for the app to work locally and on heroku --- Gemfile | 7 +++++++ Gemfile.lock | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/Gemfile b/Gemfile index d0c7ea6e..7987e6e5 100644 --- a/Gemfile +++ b/Gemfile @@ -32,6 +32,9 @@ gem 'tzinfo-data', platforms: %i[windows jruby] # Reduces boot times through caching; required in config/boot.rb gem 'bootsnap', require: false +# Loads environment variables from .env (local dev/test only, not Heroku) +gem 'dotenv-rails', groups: [ :development, :test ] + # Alternative Canvas API. We probably don't need this. # Verify instances of `LMS::Canvas` gem 'lms-api' @@ -60,6 +63,9 @@ gem 'strong_migrations' # Logging Customization gem 'lograge' +# Environment variable management +gem 'dotenv-rails', require: 'dotenv/load' + # Use Active Storage for file uploads [https://guides.rubyonrails.org/active_storage_overview.html] # gem "activestorage", "~> 7.0.0" @@ -70,6 +76,7 @@ gem 'lograge' # gem 'blazer' gem 'hypershield' +gem 'good_job', '~> 4.0' #### Frontend related tools # The original asset pipeline for Rails [https://github.com/rails/sprockets-rails] diff --git a/Gemfile.lock b/Gemfile.lock index 20ad5914..f28804e8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -185,10 +185,16 @@ GEM diff-lcs (1.6.2) docile (1.4.1) domain_name (0.6.20240107) + dotenv (3.2.0) + dotenv-rails (3.2.0) + dotenv (= 3.2.0) + railties (>= 6.1) drb (2.2.3) dumb_delegator (1.1.0) erb (6.0.2) erubi (1.13.1) + et-orbi (1.4.0) + tzinfo factory_bot (6.5.6) activesupport (>= 6.1.0) factory_bot_rails (6.5.1) @@ -215,8 +221,18 @@ GEM sassc (~> 2.0) formatador (1.2.3) reline + fugit (1.12.1) + et-orbi (~> 1.4) + raabro (~> 1.4) globalid (1.3.0) activesupport (>= 6.1) + good_job (4.14.2) + activejob (>= 6.1.0) + activerecord (>= 6.1.0) + concurrent-ruby (>= 1.3.1) + fugit (>= 1.11.0) + railties (>= 6.1.0) + thor (>= 1.0.0) guard (2.20.1) formatador (>= 0.2.4) listen (>= 2.7, < 4.0) @@ -382,6 +398,7 @@ GEM public_suffix (7.0.5) puma (7.2.0) nio4r (~> 2.0) + raabro (1.4.0) racc (1.8.1) rack (3.2.6) rack-protection (4.2.1) @@ -624,10 +641,12 @@ DEPENDENCIES cucumber-rails database_cleaner-active_record debug + dotenv-rails factory_bot_rails faraday faraday-cookie_jar font-awesome-sass + good_job (~> 4.0) guard-rspec hypershield importmap-rails From 72e667761ad1c283b3b0afd87857b6fe5d6e3ea7 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 21:29:03 -0700 Subject: [PATCH 34/74] 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 35/74] 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 36/74] 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 37/74] 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 38/74] 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 39/74] 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 40/74] 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 41/74] 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! From e21a7d63658a3e012a238c705782bd025b3e099d Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 23:11:03 -0700 Subject: [PATCH 42/74] Fixed negative days issue --- app/models/request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/request.rb b/app/models/request.rb index ee1ba4fa..eade0f62 100644 --- a/app/models/request.rb +++ b/app/models/request.rb @@ -56,7 +56,7 @@ def self.total_approved_late_days_by_user(course) rows = where(course: course, status: 'approved') .joins(:assignment) .group(:user_id, :assignment_id) - .pluck(:user_id, Arel.sql('MAX(requested_due_date::date - assignments.due_date::date)')) + .pluck(:user_id, Arel.sql('GREATEST(0, MAX(requested_due_date::date - assignments.due_date::date))')) rows.each_with_object(Hash.new(0)) do |(user_id, max_days), totals| totals[user_id] += max_days From 0665f6a6b4801ffe49a6f654648761f9d0a53d93 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 23:21:31 -0700 Subject: [PATCH 43/74] Trigger CI From 6ef391617833bddc07700d1881435a7e475a06b0 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 23:21:51 -0700 Subject: [PATCH 44/74] Trigger CI From 897bb9afa5d4181bee6762d875258d33cc1d11aa Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Wed, 29 Apr 2026 23:45:38 -0700 Subject: [PATCH 45/74] Fix course spec to use perform_later --- spec/models/course_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index b499deae..6d55bef8 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -247,7 +247,7 @@ 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]) + expect(SyncUsersFromCanvasJob).to receive(:perform_later).with(course.id, 999, %w[student teacher ta leadta]) course.sync_all_enrollments_from_canvas(999) end From 8fe182ad26388997f75cb1f6d73332c0e34db09d Mon Sep 17 00:00:00 2001 From: Igor Moyzeson Date: Fri, 24 Apr 2026 23:03:47 -0700 Subject: [PATCH 46/74] added 2 buttons to allow all assignments to be enabled or disables --- Gemfile | 2 +- Gemfile.lock | 2 +- app/controllers/courses_controller.rb | 12 +++++- .../controllers/assignment_controller.js | 38 ++++++++++++++++++- app/views/courses/instructor_show.html.erb | 17 +++++++-- config/routes.rb | 1 + 6 files changed, 64 insertions(+), 8 deletions(-) diff --git a/Gemfile b/Gemfile index 7987e6e5..11fd5788 100644 --- a/Gemfile +++ b/Gemfile @@ -161,4 +161,4 @@ end # Staging only gems. # group :staging do -# end +# end \ No newline at end of file diff --git a/Gemfile.lock b/Gemfile.lock index d6200315..59aee7fd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -226,7 +226,7 @@ GEM raabro (~> 1.4) globalid (1.3.0) activesupport (>= 6.1) - good_job (4.14.2) + good_job (4.18.2) activejob (>= 6.1.0) activerecord (>= 6.1.0) concurrent-ruby (>= 1.3.1) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index f197a305..b6906d1b 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -1,6 +1,6 @@ class CoursesController < ApplicationController before_action :authenticate_user - before_action :set_course, only: %i[show edit sync_assignments sync_enrollments enrollments delete] + before_action :set_course, only: %i[show edit sync_assignments sync_enrollments bulk_update_assignments enrollments delete] before_action :set_pending_request_count before_action :determine_user_role @@ -79,6 +79,16 @@ def sync_assignments render json: { message: 'Assignments synced successfully.' }, status: :ok end + def bulk_update_assignments + return render json: { error: 'Course not found.' }, status: :not_found unless @course + return render json: { error: 'You do not have permission.' }, status: :forbidden unless @role == 'instructor' + + enabled = ActiveModel::Type::Boolean.new.cast(params[:enabled]) + Assignment.where(course_to_lms_id: CourseToLms.where(course_id: @course.id).select(:id)) + .update_all(enabled: enabled) + render json: { success: true }, status: :ok + end + def sync_enrollments return render json: { error: 'Course not found.' }, status: :not_found unless @course return render json: { error: 'You do not have permission.' }, status: :forbidden unless @is_course_admin diff --git a/app/javascript/controllers/assignment_controller.js b/app/javascript/controllers/assignment_controller.js index 8112cfb5..04e724ce 100644 --- a/app/javascript/controllers/assignment_controller.js +++ b/app/javascript/controllers/assignment_controller.js @@ -6,7 +6,7 @@ import "datatables.net-responsive-bs5"; // Connects to data-controller="assignment" export default class extends Controller { static targets = ["checkbox"] - static values = { courseId: Number } + static values = { courseId: Number, bulkUrl: String } connect() { this.checkboxTargets.forEach((checkbox) => { @@ -64,6 +64,42 @@ export default class extends Controller { } } + async bulkUpdate(enabled) { + const url = this.bulkUrlValue; + const token = document.querySelector('meta[name="csrf-token"]').content; + + try { + const response = await fetch(url, { + method: "PATCH", + headers: { + "Content-Type": "application/json", + "X-CSRF-Token": token, + }, + body: JSON.stringify({ enabled }), + }); + + if (!response.ok) throw new Error("Failed to update assignments."); + + document.querySelectorAll(".assignment-enabled-switch").forEach((cb) => { + cb.checked = enabled; + }); + } catch (error) { + flash("alert", error.message || "An error occurred."); + } + } + + enableAll(event) { + const button = event.currentTarget; + button.disabled = true; + this.bulkUpdate(true).finally(() => { button.disabled = false; }); + } + + disableAll(event) { + const button = event.currentTarget; + button.disabled = true; + this.bulkUpdate(false).finally(() => { button.disabled = false; }); + } + sync(event) { const button = event.currentTarget; button.disabled = true; diff --git a/app/views/courses/instructor_show.html.erb b/app/views/courses/instructor_show.html.erb index 3f0ed8ef..1be8b3e3 100644 --- a/app/views/courses/instructor_show.html.erb +++ b/app/views/courses/instructor_show.html.erb @@ -49,11 +49,20 @@
-
+
+ +
diff --git a/config/routes.rb b/config/routes.rb index bab878b2..e4d270a1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,6 +30,7 @@ member do post :sync_assignments post :sync_enrollments + patch :bulk_update_assignments get :enrollments delete :delete end From 065812c679642bfd5e69dc3e2db559586f17e3de Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Fri, 24 Apr 2026 23:43:20 -0700 Subject: [PATCH 47/74] Fixed Ruby lint and replaced updateAll method in bulkAssignmentUpdate to fix warning --- Gemfile | 2 +- app/controllers/courses_controller.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 11fd5788..7987e6e5 100644 --- a/Gemfile +++ b/Gemfile @@ -161,4 +161,4 @@ end # Staging only gems. # group :staging do -# end \ No newline at end of file +# end diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index b6906d1b..ccdd0509 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -85,7 +85,7 @@ def bulk_update_assignments enabled = ActiveModel::Type::Boolean.new.cast(params[:enabled]) Assignment.where(course_to_lms_id: CourseToLms.where(course_id: @course.id).select(:id)) - .update_all(enabled: enabled) + .each { |a| a.update!(enabled: enabled) } render json: { success: true }, status: :ok end From fb2046eade19a96c0b9addb1bc96ea9582a3fbbf Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Sat, 25 Apr 2026 00:05:26 -0700 Subject: [PATCH 48/74] Fixed Datatable DOM issue selecting only current page --- .../controllers/assignment_controller.js | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/app/javascript/controllers/assignment_controller.js b/app/javascript/controllers/assignment_controller.js index 04e724ce..db9b980c 100644 --- a/app/javascript/controllers/assignment_controller.js +++ b/app/javascript/controllers/assignment_controller.js @@ -80,9 +80,19 @@ export default class extends Controller { if (!response.ok) throw new Error("Failed to update assignments."); - document.querySelectorAll(".assignment-enabled-switch").forEach((cb) => { - cb.checked = enabled; - }); + const dt = DataTable.isDataTable('#assignments-table') + ? new DataTable('#assignments-table') + : null; + if (dt) { + dt.rows().nodes().each((node) => { + const cb = node.querySelector('.assignment-enabled-switch'); + if (cb) cb.checked = enabled; + }); + } else { + document.querySelectorAll(".assignment-enabled-switch").forEach((cb) => { + cb.checked = enabled; + }); + } } catch (error) { flash("alert", error.message || "An error occurred."); } From c5d961a109114c697242245dd7e7a8ebb4cdd684 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Thu, 30 Apr 2026 00:10:12 -0700 Subject: [PATCH 49/74] Fix lms_name to id --- app/controllers/courses_controller.rb | 5 +++-- app/controllers/session_controller.rb | 4 ++-- app/models/lms_credential.rb | 1 - 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index ccdd0509..cc0a4b77 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -84,8 +84,9 @@ def bulk_update_assignments return render json: { error: 'You do not have permission.' }, status: :forbidden unless @role == 'instructor' enabled = ActiveModel::Type::Boolean.new.cast(params[:enabled]) - Assignment.where(course_to_lms_id: CourseToLms.where(course_id: @course.id).select(:id)) - .each { |a| a.update!(enabled: enabled) } + scope = Assignment.where(course_to_lms_id: CourseToLms.where(course_id: @course.id).select(:id)) + scope = scope.where.not(due_date: nil) if enabled + scope.update_all(enabled: enabled) render json: { success: true }, status: :ok end diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 1cd5317c..55d156fc 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -135,14 +135,14 @@ def update_user_credential(user, token) user.lms_credentials.first.update( token: token.token, refresh_token: token.refresh_token, - expire_time: Time.zone.at(token.expires_at) + expire_time: Time.zone.at(token.expires_at || 30.days.from_now.to_i) ) else user.lms_credentials.create!( lms_id: Lms.CANVAS_LMS.id, token: token.token, refresh_token: token.refresh_token, - expire_time: Time.zone.at(token.expires_at) + expire_time: Time.zone.at(token.expires_at || 30.days.from_now.to_i) ) end end diff --git a/app/models/lms_credential.rb b/app/models/lms_credential.rb index 60ec5e0f..966b0eb6 100644 --- a/app/models/lms_credential.rb +++ b/app/models/lms_credential.rb @@ -28,6 +28,5 @@ class LmsCredential < ApplicationRecord belongs_to :user belongs_to :lms - # Encryption for tokens encrypts :token, :refresh_token end From a52a4eb5a4591fac1548654051ad4b9ede5cad79 Mon Sep 17 00:00:00 2001 From: Anthony Dam Date: Thu, 30 Apr 2026 00:12:13 -0700 Subject: [PATCH 50/74] Disable RuboCop for intentional update_all --- app/controllers/courses_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index cc0a4b77..4bbda59b 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -86,7 +86,7 @@ def bulk_update_assignments enabled = ActiveModel::Type::Boolean.new.cast(params[:enabled]) scope = Assignment.where(course_to_lms_id: CourseToLms.where(course_id: @course.id).select(:id)) scope = scope.where.not(due_date: nil) if enabled - scope.update_all(enabled: enabled) + scope.update_all(enabled: enabled) # rubocop:disable Rails/SkipsModelValidations render json: { success: true }, status: :ok end From af656f608fa5b65158c06733f6d490c3f85339cd Mon Sep 17 00:00:00 2001 From: Igor Moyzeson Date: Fri, 24 Apr 2026 23:03:47 -0700 Subject: [PATCH 51/74] added 2 buttons to allow all assignments to be enabled or disables --- Gemfile | 2 +- Gemfile.lock | 2 +- app/controllers/courses_controller.rb | 12 +++++- .../controllers/assignment_controller.js | 38 ++++++++++++++++++- app/views/courses/instructor_show.html.erb | 13 ++++++- config/routes.rb | 1 + 6 files changed, 63 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index 7987e6e5..11fd5788 100644 --- a/Gemfile +++ b/Gemfile @@ -161,4 +161,4 @@ end # Staging only gems. # group :staging do -# end +# end \ No newline at end of file diff --git a/Gemfile.lock b/Gemfile.lock index d6200315..59aee7fd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -226,7 +226,7 @@ GEM raabro (~> 1.4) globalid (1.3.0) activesupport (>= 6.1) - good_job (4.14.2) + good_job (4.18.2) activejob (>= 6.1.0) activerecord (>= 6.1.0) concurrent-ruby (>= 1.3.1) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 719ce1b3..0e8bec48 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -1,6 +1,6 @@ class CoursesController < ApplicationController before_action :authenticate_user - before_action :set_course, only: %i[show edit sync_assignments sync_enrollments sync_status enrollments delete] + before_action :set_course, only: %i[show edit sync_assignments sync_enrollments sync_status bulk_update_assignments enrollments delete] before_action :set_pending_request_count before_action :determine_user_role @@ -79,6 +79,16 @@ def sync_assignments render json: { message: 'Assignments synced successfully.' }, status: :ok end + def bulk_update_assignments + return render json: { error: 'Course not found.' }, status: :not_found unless @course + return render json: { error: 'You do not have permission.' }, status: :forbidden unless @role == 'instructor' + + enabled = ActiveModel::Type::Boolean.new.cast(params[:enabled]) + Assignment.where(course_to_lms_id: CourseToLms.where(course_id: @course.id).select(:id)) + .update_all(enabled: enabled) + render json: { success: true }, status: :ok + end + def sync_enrollments return render json: { error: 'Course not found.' }, status: :not_found unless @course return render json: { error: 'You do not have permission.' }, status: :forbidden unless @is_course_admin diff --git a/app/javascript/controllers/assignment_controller.js b/app/javascript/controllers/assignment_controller.js index 8d7cbae1..70b84c99 100644 --- a/app/javascript/controllers/assignment_controller.js +++ b/app/javascript/controllers/assignment_controller.js @@ -6,7 +6,7 @@ import "datatables.net-responsive-bs5"; // Connects to data-controller="assignment" export default class extends Controller { static targets = ["checkbox", "syncBtn", "syncLabel", "syncSpinner"] - static values = { courseId: Number } + static values = { courseId: Number, bulkUrl: String } connect() { this.checkboxTargets.forEach((checkbox) => { @@ -64,6 +64,42 @@ export default class extends Controller { } } + async bulkUpdate(enabled) { + const url = this.bulkUrlValue; + const token = document.querySelector('meta[name="csrf-token"]').content; + + try { + const response = await fetch(url, { + method: "PATCH", + headers: { + "Content-Type": "application/json", + "X-CSRF-Token": token, + }, + body: JSON.stringify({ enabled }), + }); + + if (!response.ok) throw new Error("Failed to update assignments."); + + document.querySelectorAll(".assignment-enabled-switch").forEach((cb) => { + cb.checked = enabled; + }); + } catch (error) { + flash("alert", error.message || "An error occurred."); + } + } + + enableAll(event) { + const button = event.currentTarget; + button.disabled = true; + this.bulkUpdate(true).finally(() => { button.disabled = false; }); + } + + disableAll(event) { + const button = event.currentTarget; + button.disabled = true; + this.bulkUpdate(false).finally(() => { button.disabled = false; }); + } + async sync() { const button = this.syncBtnTarget; const label = this.syncLabelTarget; diff --git a/app/views/courses/instructor_show.html.erb b/app/views/courses/instructor_show.html.erb index 54d64376..48812a1b 100644 --- a/app/views/courses/instructor_show.html.erb +++ b/app/views/courses/instructor_show.html.erb @@ -49,7 +49,18 @@
-
+
+ +