SP26 #1: LMS ID Refactor + Email Notifications Settings

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

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')

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

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)

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);

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

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

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

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 <select> and blank <input> 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?
 

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.

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

app/models/lms_credential.rb

@@ -5,14 +5,14 @@
 #
 #  id               :bigint           not null, primary key
 #  expire_time      :datetime
-#  lms_name         :string
 #  password         :string
 #  refresh_token    :string
 #  token            :string
 #  username         :string
 #  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

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