diff --git a/app/controllers/course_settings_controller.rb b/app/controllers/course_settings_controller.rb index 08263b99..9cd34d83 100644 --- a/app/controllers/course_settings_controller.rb +++ b/app/controllers/course_settings_controller.rb @@ -75,7 +75,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/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index ede42f1e..a3950e79 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -6,10 +6,10 @@ class CoursesController < ApplicationController before_action :set_pending_request_count def index - @staff_enrollments = Enrollment.includes(:course) + staff_enrollments = Enrollment.includes(:course) .where(user: current_user, role: Enrollment.staff_roles) .keep_highest_role - @staff_enrollments_by_semester = group_by_semester(@staff_enrollments) + @staff_enrollments_by_semester = group_by_semester(staff_enrollments) # Only show courses to students if extensions are enabled at the course level student_courses = Enrollment.includes(course: :course_settings).where(user: current_user, role: 'student') diff --git a/app/controllers/enrollments_controller.rb b/app/controllers/enrollments_controller.rb index 5dc67ca1..fd0108b0 100644 --- a/app/controllers/enrollments_controller.rb +++ b/app/controllers/enrollments_controller.rb @@ -1,25 +1,37 @@ class EnrollmentsController < ApplicationController - before_action :authenticate_user + # `authenticated!` from ApplicationController runs before this filter before_action :set_course - before_action :ensure_course_admin + before_action :ensure_course_staff! + before_action :set_enrollment def toggle_allow_extended_requests - @enrollment = @course.enrollments.find(params[:id]) - 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_content + render json: { + success: false, + errors: @enrollment.errors.full_messages, + redirect_to: courses_path + }, status: :unprocessable_content end end private - def ensure_course_admin - enrollment = @course.enrollments.find_by(user: @user) - return if enrollment&.course_admin? + def set_enrollment + @enrollment = @course.enrollments.find(params[:id]) + end + + # ApplicationController#ensure_instructor_role would redirect with a flash, + # which breaks the JSON fetch from the course-settings UI. Respond with 403 + # JSON so the client can surface the failure inline. + def ensure_course_staff! + return if @course.staff_user?(current_user) - render json: { error: 'You must be an instructor or Lead TA.', redirect_to: course_path(@course) }, status: :forbidden + render json: { + success: false, + error: 'Forbidden', + redirect_to: courses_path + }, status: :forbidden end end diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index d770f92a..ccb25260 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -54,12 +54,11 @@ def omniauth_callback # dev provider doesnt have real credentials so its stubbed expires_at = creds.expires_at || 30.days.from_now.to_i - refresh_token = creds.refresh_token || 'none' access_token = OAuth2::AccessToken.new( OAuth2::Client.new('', ''), # client never used – stub creds.token, - refresh_token: refresh_token, + refresh_token: creds.refresh_token, expires_at: expires_at ) @@ -189,7 +188,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/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/jobs/pending_requests_notification_job.rb b/app/jobs/pending_requests_notification_job.rb new file mode 100644 index 00000000..0d5d4eaf --- /dev/null +++ b/app/jobs/pending_requests_notification_job.rb @@ -0,0 +1,15 @@ +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" + + PendingRequestsMailer.send_pending_request_notifications(cs, pending_count, requests_url) + end + end +end diff --git a/app/mailers/pending_requests_mailer.rb b/app/mailers/pending_requests_mailer.rb new file mode 100644 index 00000000..8f5b9e2f --- /dev/null +++ b/app/mailers/pending_requests_mailer.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +# Templates and delivery helper for the "you have N pending extension requests" +# digest email that PendingRequestsNotificationJob sends. Wraps EmailService so +# the job doesn't need to know the template strings or mapping shape. +class PendingRequestsMailer + SUBJECT_TEMPLATE = '{{pending_count}} Pending Extension Request{{plural}} - {{course_code}}' + + BODY_TEMPLATE = <<~BODY + Hello, + + You have {{pending_count}} pending extension request{{plural}} in {{course_name}} ({{course_code}}). + + Please review them at: {{requests_url}} + + Thank you, + Flextensions + BODY + + def self.send_pending_request_notifications(course_settings, pending_count, requests_url) + course = course_settings.course + default_from = ENV.fetch('DEFAULT_FROM_EMAIL') + + EmailService.send_email( + to: course_settings.pending_notification_email, + from: default_from, + reply_to: course_settings.reply_email.presence || default_from, + subject_template: SUBJECT_TEMPLATE, + body_template: BODY_TEMPLATE, + 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 diff --git a/app/models/course.rb b/app/models/course.rb index 6aafc843..2f47b647 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -147,13 +147,6 @@ def student_user?(user) enrollments.where(user_id: user.id).any?(&:student?) end - # TODO: This doesn't make sense actually. - # A course can be linked to many LMSs. - # def lms_facade - # course_to_lms = CourseToLms.find_by(id: course_to_lms_id) - # Lms.facade_class(course_to_lms.lms_id) - # end - def canvas_id external_course_id_for(CANVAS_LMS_ID) end diff --git a/app/models/course_settings.rb b/app/models/course_settings.rb index c2b1524b..34974710 100644 --- a/app/models/course_settings.rb +++ b/app/models/course_settings.rb @@ -17,6 +17,8 @@ # gradescope_course_url :string # max_auto_approve :integer default(0) # min_hours_before_deadline :integer default(0), not null +# pending_notification_email :string +# pending_notification_frequency :string # reply_email :string # slack_webhook_url :string # created_at :datetime not null @@ -34,7 +36,6 @@ # rubocop:enable Layout/LineLength class CourseSettings < ApplicationRecord - # TODO: Remove the db default text, and use an AR validation. DEFAULT_EMAIL_TEMPLATE = <<~LIQUID.freeze Hello {{student_name}}, @@ -51,16 +52,31 @@ class CourseSettings < ApplicationRecord {{course_name}} Staff LIQUID - belongs_to :course + VALID_NOTIFICATION_FREQUENCIES = %w[daily weekly].freeze - # Courses and settings are 1:1; the course_id unique index enforces this at - # the database level. + belongs_to :course validates :course_id, uniqueness: true + # Empty submissions become "" — coerce to nil so + # `allow_nil` behaves as expected and unset rows compare equal. + normalizes :pending_notification_frequency, :pending_notification_email, with: ->(v) { v.presence } + before_save :ensure_system_user_for_auto_approval + # Clear a stored email when notifications are turned off, so re-enabling + # doesn't silently reuse a stale address. + 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: URI::MailTo::EMAIL_REGEXP }, + 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/app/models/enrollment.rb b/app/models/enrollment.rb index 3872325f..98c80b4b 100644 --- a/app/models/enrollment.rb +++ b/app/models/enrollment.rb @@ -36,11 +36,9 @@ class Enrollment < ApplicationRecord # user holds more than one in the same course. ROLE_PRIORITY = [ STUDENT_ROLE, TA_ROLE, LEAD_TA_ROLE, TEACHER_ROLE ].freeze - # Associations belongs_to :user belongs_to :course - # Validations # NOTE: Validations are skipped when a User is created by SyncUsersFromCanvasJob # You should update that job if these validations become complex. # In the meantime, we can trust that the data coming from Canvas is valid. @@ -60,7 +58,7 @@ def student? end def display_role - ROLE_LABELS.fetch(role, role.capitalize) + Enrollment.display_role(role) end # Rank of this enrollment's role; higher wins. Unknown roles rank lowest. diff --git a/app/models/lms.rb b/app/models/lms.rb index 98ba63de..8fa66da3 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 946fc01b..b46b9969 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,18 +21,16 @@ # # 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 :lms # Encryption for tokens encrypts :token, :refresh_token - # LMS must exist - validates :lms_name, presence: true - # Whether the access token expires within the given buffer (so callers can # refresh it before starting a batch of API calls). def expires_soon?(buffer = 15.minutes) @@ -72,12 +70,12 @@ def refresh! # Flextensions under Settings > Approved Integrations, or the developer # key/scopes changed (which invalidates every token derived from the # key). It can never work again, so remove it rather than retry it. - Rails.logger.warn "Removing revoked #{lms_name} credential for user #{user_id} (invalid_grant)" + Rails.logger.warn "Removing revoked #{lms.lms_name} credential for user #{user_id} (invalid_grant)" destroy else - Rails.logger.error "Failed to refresh #{lms_name} token for user #{user_id}: #{e.message}" + Rails.logger.error "Failed to refresh #{lms.lms_name} token for user #{user_id}: #{e.message}" Rails.error.report(e, handled: true, - context: { component: 'token_refresh', lms: lms_name, user_id: user_id }) + context: { component: 'token_refresh', lms: lms.lms_name, user_id: user_id }) end nil end diff --git a/app/models/user.rb b/app/models/user.rb index d3547e14..c73bf711 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -38,9 +38,8 @@ class User < ApplicationRecord has_many :enrollments has_many :courses, through: :enrollments - # 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 # Returns a Canvas access token that is valid for at least the next few diff --git a/app/views/courses/edit.html.erb b/app/views/courses/edit.html.erb index e1854a52..8502e5ec 100644 --- a/app/views/courses/edit.html.erb +++ b/app/views/courses/edit.html.erb @@ -240,6 +240,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.requests_enabled? %> diff --git a/app/views/home/index.html.erb b/app/views/home/index.html.erb index f356c5f5..15c6b6d4 100644 --- a/app/views/home/index.html.erb +++ b/app/views/home/index.html.erb @@ -23,7 +23,7 @@
<%= link_to 'Login', '/auth/canvas', class: 'btn btn-primary', id: 'login-button-index' %> - <% if Rails.env.development? %> + <% if Rails.env.local? %> <%= link_to 'Developer Login', '/auth/developer', class: 'btn btn-secondary ms-2', id: 'dev-login-button' %> <% end %>
diff --git a/db/api_spec_seeds.rb b/db/api_spec_seeds.rb index 5333421a..b52de76c 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/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 797ad985..c88b00d4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -106,6 +106,8 @@ t.string "gradescope_course_url" t.integer "max_auto_approve", default: 0 t.integer "min_hours_before_deadline", default: 0, null: false + t.string "pending_notification_email" + t.string "pending_notification_frequency" t.string "reply_email" t.string "slack_webhook_url" t.datetime "updated_at", null: false @@ -268,7 +270,7 @@ t.datetime "created_at", null: false t.datetime "expire_time" t.string "external_user_id" - t.string "lms_name" + t.bigint "lms_id" t.string "password" t.string "refresh_token" t.string "token" @@ -333,6 +335,7 @@ add_foreign_key "faultline_error_occurrences", "faultline_error_groups", column: "error_group_id" add_foreign_key "faultline_request_profiles", "faultline_request_traces", column: "request_trace_id", on_delete: :cascade 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/lib/tasks/lms_credentials.rake b/lib/tasks/lms_credentials.rake index 91ff8022..be4b131a 100644 --- a/lib/tasks/lms_credentials.rake +++ b/lib/tasks/lms_credentials.rake @@ -6,7 +6,7 @@ namespace :lms_credentials do 'every token derived from the key. Usage: bin/rails "lms_credentials:purge[canvas]"' task :purge, [ :lms_name ] => :environment do |_task, args| lms_name = args[:lms_name] || 'canvas' - count = LmsCredential.where(lms_name: lms_name).delete_all + count = LmsCredential.joins(:lms).where('LOWER(lmss.lms_name) = ?', lms_name.downcase).delete_all puts "Deleted #{count} #{lms_name} credential(s). " \ 'Users will re-authenticate automatically on their next visit; auto-approval in each ' \ 'course resumes once one of its staff members has logged in.' diff --git a/lib/tasks/notifications.rake b/lib/tasks/notifications.rake new file mode 100644 index 00000000..978c5291 --- /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/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index f70502d5..b19b2272 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 3d341a35..68376ffd 100644 --- a/spec/controllers/concerns/token_refreshable_spec.rb +++ b/spec/controllers/concerns/token_refreshable_spec.rb @@ -22,8 +22,9 @@ def current_user let(:expire_time) { 1.hour.from_now } 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: expire_time diff --git a/spec/controllers/course_settings_controller_spec.rb b/spec/controllers/course_settings_controller_spec.rb index 7c9c453e..63019ce1 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 @@ -112,6 +113,74 @@ end end + describe 'pending notification params' do + before do + session[:user_id] = instructor.canvas_uid + Enrollment.create!(user: instructor, course: course, role: 'teacher') + course.course_settings.update!(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 @@ -181,7 +250,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 797a4586..4cb99c60 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -8,10 +8,11 @@ let(:course_settings) { course.course_settings.tap { |cs| cs.update!(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 Enrollment.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 @@ -289,7 +290,8 @@ 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(canvas_courses) end @@ -390,7 +392,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) CourseToLms.create!(course: course, lms_id: 1) end diff --git a/spec/controllers/enrollments_controller_spec.rb b/spec/controllers/enrollments_controller_spec.rb index f07342f0..95047104 100644 --- a/spec/controllers/enrollments_controller_spec.rb +++ b/spec/controllers/enrollments_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 @@ Enrollment.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 diff --git a/spec/controllers/requests_controller_spec.rb b/spec/controllers/requests_controller_spec.rb index 9bcd5837..9482df5e 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 @@ -374,7 +374,7 @@ session[:user_id] = instructor.canvas_uid Enrollment.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 @@ -507,10 +507,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 Enrollment.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 @@ -739,7 +740,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/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 5ae31b2a..ca65dc4f 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -233,14 +233,14 @@ expect(enrollment.role).to eq('student') end - it 'stores fake refresh token for developer provider' do + it 'stores credentials for developer provider with nil refresh token' do get :omniauth_callback, params: { provider: 'developer' } user = User.find_by(canvas_uid: 'test@example.com') creds = user.lms_credentials.first - expect(creds.refresh_token).to be_present expect(creds.token).to be_present + expect(creds.refresh_token).to be_nil end end end 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 3ce21aa1..3fd56575 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/jobs/pending_requests_notification_job_spec.rb b/spec/jobs/pending_requests_notification_job_spec.rb new file mode 100644 index 00000000..54aa990c --- /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 diff --git a/spec/jobs/sync_all_course_assignments_job_spec.rb b/spec/jobs/sync_all_course_assignments_job_spec.rb index 600749a8..8dbe98af 100644 --- a/spec/jobs/sync_all_course_assignments_job_spec.rb +++ b/spec/jobs/sync_all_course_assignments_job_spec.rb @@ -128,20 +128,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 diff --git a/spec/models/course_settings_spec.rb b/spec/models/course_settings_spec.rb index 8fbdc837..fe9f216e 100644 --- a/spec/models/course_settings_spec.rb +++ b/spec/models/course_settings_spec.rb @@ -16,6 +16,8 @@ # gradescope_course_url :string # max_auto_approve :integer default(0) # min_hours_before_deadline :integer default(0), not null +# pending_notification_email :string +# pending_notification_frequency :string # reply_email :string # slack_webhook_url :string # created_at :datetime not null @@ -192,6 +194,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 = 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) # rubocop:disable Rails/SkipsModelValidations + + results = described_class.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' diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index ad0a0013..0d3a1c9d 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -111,7 +111,7 @@ def create_staff(email, canvas_uid, role, with_credentials: true) user = User.create!(email: email, canvas_uid: canvas_uid) if with_credentials user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'valid_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -135,8 +135,9 @@ def create_staff(email, canvas_uid, role, with_credentials: true) end it 'ignores non-Canvas credentials' do + other_lms = Lms.find_or_create_by(id: 3) { |l| l.lms_name = 'other_lms'; l.use_auth_token = true } user = create_staff('other-lms@example.com', '126', 'ta', with_credentials: false) - user.lms_credentials.create!(lms_name: 'other_lms', token: 't', refresh_token: 'r', expire_time: 1.hour.from_now) + user.lms_credentials.create!(lms: other_lms, token: 't', refresh_token: 'r', expire_time: 1.hour.from_now) expect(course.staff_user_for_auto_approval).to be_nil end @@ -158,14 +159,14 @@ def create_staff(email, canvas_uid, role, with_credentials: true) idle_ta = User.create!(email: 'idle_ta@example.com', canvas_uid: '127') idle_ta.lms_credentials.create!( - lms_name: 'canvas', token: 'stale', refresh_token: 'stale', + lms_id: 1, token: 'stale', refresh_token: 'stale', expire_time: 6.months.ago, updated_at: 6.months.ago ) Enrollment.create!(user: idle_ta, course: course, role: 'ta') active_teacher = User.create!(email: 'active_teacher@example.com', canvas_uid: '128') active_teacher.lms_credentials.create!( - lms_name: 'canvas', token: 'fresh', refresh_token: 'fresh', + lms_id: 1, token: 'fresh', refresh_token: 'fresh', expire_time: 1.hour.from_now ) Enrollment.create!(user: active_teacher, course: course, role: 'teacher') diff --git a/spec/models/lms_credential_spec.rb b/spec/models/lms_credential_spec.rb index 9356d9c3..2ea579f2 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', @@ -76,8 +78,9 @@ def self.mock_get_service(token, refresh_token) describe '#expires_soon?' do let(:user) { User.create!(email: 'expiry@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: 'canvas', token: 't', refresh_token: 'r', expire_time: expire_time) + described_class.create!(user: user, lms: lms, token: 't', refresh_token: 'r', expire_time: expire_time) end context 'when the token expires within the buffer' do @@ -107,10 +110,11 @@ def self.mock_get_service(token, refresh_token) describe '#refresh!' do let(:user) { User.create!(email: 'refresh@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: 'canvas', + lms: lms, token: 'old_token', refresh_token: 'old_refresh', expire_time: 5.minutes.from_now diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb index 726ec31a..696d0f4a 100644 --- a/spec/models/request_spec.rb +++ b/spec/models/request_spec.rb @@ -71,7 +71,7 @@ before do Enrollment.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 8c51d1e2..3ccb4cd1 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -24,8 +24,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 @@ -47,8 +48,9 @@ context 'when the user only has non-Canvas credentials' do before do + other_lms = Lms.find_or_create_by(id: 3) { |lms| lms.lms_name = 'other_lms'; lms.use_auth_token = true } user.lms_credentials.create!( - lms_name: 'other_lms', + lms: other_lms, token: 'other_token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now @@ -62,8 +64,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 @@ -79,7 +82,7 @@ context 'when token expires soon' do before do user.lms_credentials.create!( - lms_name: 'canvas', + lms_id: 1, token: 'stale_token', refresh_token: 'refresh_token', expire_time: 5.minutes.from_now diff --git a/spec/tasks/lms_credentials_rake_spec.rb b/spec/tasks/lms_credentials_rake_spec.rb index 8cbce321..30400301 100644 --- a/spec/tasks/lms_credentials_rake_spec.rb +++ b/spec/tasks/lms_credentials_rake_spec.rb @@ -12,9 +12,14 @@ after { task.reenable } def create_credential(email, lms_name) + # The seeds loaded in rails_helper insert Lms rows with explicit ids, which + # leaves the lmss id sequence behind the existing rows — so create with an + # explicit id too rather than relying on the sequence. + lms = Lms.find_by('LOWER(lms_name) = ?', lms_name.downcase) || + Lms.create!(id: Lms.maximum(:id).to_i + 1, lms_name: lms_name, use_auth_token: true) user = User.create!(email: email, canvas_uid: email) user.lms_credentials.create!( - lms_name: lms_name, + lms: lms, token: 'token', refresh_token: 'refresh_token', expire_time: 1.hour.from_now diff --git a/spec/tasks/notifications_rake_spec.rb b/spec/tasks/notifications_rake_spec.rb new file mode 100644 index 00000000..f8d2c818 --- /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 # rubocop:disable RSpec/DescribeClass + before(:all) do + Rails.application.load_tasks unless Rake::Task.task_defined?('notifications:send_pending_digests') + 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 diff --git a/utils/canvas_sandbox/test_approval_flows.rb b/utils/canvas_sandbox/test_approval_flows.rb index bee0514a..133c1954 100644 --- a/utils/canvas_sandbox/test_approval_flows.rb +++ b/utils/canvas_sandbox/test_approval_flows.rb @@ -76,7 +76,7 @@ def find_or_create_user(attrs) end def give_canvas_credentials(user) - cred = user.lms_credentials.find_or_initialize_by(lms_name: 'canvas') + cred = user.lms_credentials.find_or_initialize_by(lms: Lms.CANVAS_LMS) cred.update!(token: TOKEN, refresh_token: 'none', expire_time: 1.year.from_now) end @@ -225,7 +225,7 @@ def verify_in_canvas(scenario, assignment, student, request) stale_staff = find_or_create_user(STALE_STAFF) stale_staff.lms_credentials.destroy_all stale_staff.lms_credentials.create!( - lms_name: 'canvas', + lms: Lms.CANVAS_LMS, token: 'expired-and-unrefreshable', refresh_token: 'revoked-refresh-token', expire_time: 1.minute.from_now