From 673fcbc70f3fde712f4284eb0d83ecfb9a8ea7bf Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 16:12:49 +1000 Subject: [PATCH] feat: notifications --- app/api/api_root.rb | 2 + app/api/notifications_api.rb | 170 +++++++++++ app/api/task_comments_api.rb | 2 + app/mailers/notifications_mailer.rb | 20 ++ app/models/comments/task_comment.rb | 4 + app/models/comments/task_status_comment.rb | 4 - app/models/notification.rb | 268 ++++++++++++++++++ app/models/notification_preference.rb | 137 +++++++++ app/models/overseer_assessment.rb | 6 +- app/models/portfolio_evidence.rb | 13 +- app/models/project.rb | 1 + app/models/task.rb | 2 + app/models/tutor_note.rb | 1 + app/models/unit.rb | 24 +- app/models/user.rb | 11 + app/services/notification_group_builder.rb | 116 ++++++++ app/sidekiq/accept_overseer_job.rb | 1 + app/sidekiq/accept_submission_job.rb | 11 +- ...eate_pending_overseer_notifications_job.rb | 17 ++ app/sidekiq/notify_tutor_notes_job.rb | 2 +- app/sidekiq/poll_notification_digests_job.rb | 16 ++ app/sidekiq/prune_notifications_job.rb | 17 ++ .../send_immediate_notification_job.rb | 44 +++ app/sidekiq/send_notification_digest_job.rb | 56 ++++ .../notification_digest.html.erb | 100 +++++++ .../notification_digest.text.erb | 16 ++ config/schedule.yml | 12 + ...ifications_and_notification_preferences.rb | 54 ++++ db/schema.rb | 57 +++- test/api/comments/status_test.rb | 8 +- test/api/notifications_api_test.rb | 103 +++++++ test/factories/notifications.rb | 19 ++ test/mailers/unit_mail_test.rb | 12 +- test/models/notification_preference_test.rb | 111 ++++++++ test/models/notification_test.rb | 222 +++++++++++++++ test/sidekiq/notification_jobs_test.rb | 98 +++++++ 36 files changed, 1700 insertions(+), 57 deletions(-) create mode 100644 app/api/notifications_api.rb create mode 100644 app/models/notification.rb create mode 100644 app/models/notification_preference.rb create mode 100644 app/services/notification_group_builder.rb create mode 100644 app/sidekiq/create_pending_overseer_notifications_job.rb create mode 100644 app/sidekiq/poll_notification_digests_job.rb create mode 100644 app/sidekiq/prune_notifications_job.rb create mode 100644 app/sidekiq/send_immediate_notification_job.rb create mode 100644 app/sidekiq/send_notification_digest_job.rb create mode 100644 app/views/notifications_mailer/notification_digest.html.erb create mode 100644 app/views/notifications_mailer/notification_digest.text.erb create mode 100644 db/migrate/20260729043436_create_notifications_and_notification_preferences.rb create mode 100644 test/api/notifications_api_test.rb create mode 100644 test/factories/notifications.rb create mode 100644 test/models/notification_preference_test.rb create mode 100644 test/models/notification_test.rb create mode 100644 test/sidekiq/notification_jobs_test.rb diff --git a/app/api/api_root.rb b/app/api/api_root.rb index cb583d7a2c..760f2a038f 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -99,6 +99,7 @@ class ApiRoot < Grape::API mount UnitContentsApi mount UnitsApi mount TutorNotesApi + mount NotificationsApi mount D2lIntegrationApi::D2lApi mount D2lIntegrationApi::OauthPublicApi @@ -162,6 +163,7 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to DiscussionPromptsApi AuthenticationHelpers.add_auth_to OverseerStepsApi AuthenticationHelpers.add_auth_to TutorNotesApi + AuthenticationHelpers.add_auth_to NotificationsApi add_swagger_documentation \ base_path: nil, diff --git a/app/api/notifications_api.rb b/app/api/notifications_api.rb new file mode 100644 index 0000000000..e191437998 --- /dev/null +++ b/app/api/notifications_api.rb @@ -0,0 +1,170 @@ +# frozen_string_literal: true + +require 'grape' + +class NotificationsApi < Grape::API + helpers AuthenticationHelpers + + before do + authenticated? + end + + helpers do + def notification_scope + current_user + .received_notifications + .includes(:recipient, :unit, task: [:task_definition, { project: :user }]) + end + + def unread_group_count + current_user.received_notifications.unread.pluck(:task_id, :unit_id, :kind, :metadata).map do |task_id, unit_id, kind, metadata| + if task_id.present? + "task:#{task_id}" + elsif kind == 'tutor_note' + "tutor-notes:#{unit_id}:#{metadata['unit_role_id']}" + else + "unit:#{unit_id}:#{kind}" + end + end.uniq.count + end + + def serialize_preference(preference) + { + id: preference.id, + unit: { + id: preference.unit.id, + code: preference.unit.code, + name: preference.unit.name + }, + email_categories: preference.email_categories, + email_frequency: preference.email_frequency, + email_time: preference.email_time, + email_weekday: preference.email_weekday, + timezone: preference.timezone, + next_digest_at: preference.next_digest_at, + last_digest_at: preference.last_digest_at + } + end + + def accessible_unit_ids + project_units = current_user.projects.where(enrolled: true).select(:unit_id) + role_units = current_user.unit_roles.select(:unit_id) + preference_units = current_user.notification_preferences.select(:unit_id) + + Unit.where(id: project_units).or(Unit.where(id: role_units)).or(Unit.where(id: preference_units)).pluck(:id) + end + end + + desc 'Get grouped notifications for the current user' + params do + optional :state, type: String, values: %w[all unread read], default: 'all' + optional :unit_id, type: Integer + optional :kinds, type: Array[String], values: Notification::KINDS + optional :query, type: String + optional :page, type: Integer, default: 1, values: ->(value) { value.positive? } + optional :per_page, type: Integer, default: 25, values: 1..50 + end + get '/notifications' do + scope = notification_scope + scope = scope.where(unit_id: params[:unit_id]) if params[:unit_id] + scope = scope.where(kind: params[:kinds]) if params[:kinds].present? + + scope = + case params[:state] + when 'unread' + scope.unread + when 'read' + scope.recently_read + else + scope.where('notifications.read_at IS NULL OR notifications.read_at >= ?', 30.days.ago) + end + + groups = NotificationGroupBuilder.new(scope).groups + if params[:query].present? + query = params[:query].downcase + groups.select! do |group| + [ + group[:summary], + group.dig(:unit, :code), + group.dig(:unit, :name), + group.dig(:task, :abbreviation), + group.dig(:task, :name), + group.dig(:task, :student_name) + ].compact.any? { |value| value.to_s.downcase.include?(query) } + end + end + + page = params[:page] + per_page = params[:per_page] + total = groups.count + + { + groups: groups.slice((page - 1) * per_page, per_page) || [], + page: page, + per_page: per_page, + total: total, + unread_count: unread_group_count + } + end + + desc 'Get the grouped unread notification count for the current user' + get '/notifications/unread_count' do + { count: unread_group_count } + end + + desc 'Mark selected notifications as read' + params do + requires :notification_ids, type: Array[Integer] + end + put '/notifications/read' do + scope = current_user.received_notifications.where(id: params[:notification_ids]).unread + count = scope.count + Notification.mark_read(scope) + { count: count } + end + + desc 'Mark all notifications as read' + params do + optional :unit_id, type: Integer + end + put '/notifications/read_all' do + scope = current_user.received_notifications.unread + scope = scope.where(unit_id: params[:unit_id]) if params[:unit_id] + count = scope.count + Notification.mark_read(scope) + { count: count } + end + + desc 'Get per-unit notification email preferences' + get '/notification_preferences' do + preferences = Unit.where(id: accessible_unit_ids).order(:code).map do |unit| + NotificationPreference.for(current_user, unit) + end + + preferences.map { |preference| serialize_preference(preference) } + end + + desc 'Update notification email preferences for a unit' + params do + requires :email_categories, type: Array[String], values: Notification::KINDS + requires :email_frequency, type: String, values: NotificationPreference::FREQUENCIES + requires :email_time, type: String + requires :email_weekday, type: Integer + requires :timezone, type: String + end + put '/notification_preferences/:unit_id' do + unit_id = params[:unit_id].to_i + error!({ error: 'You do not have access to notification settings for this unit' }, 403) unless accessible_unit_ids.include?(unit_id) + + preference = NotificationPreference.for(current_user, Unit.find(unit_id)) + preference.update!( + email_categories: params[:email_categories], + email_frequency: params[:email_frequency], + email_time: params[:email_time], + email_weekday: params[:email_weekday], + timezone: params[:timezone] + ) + + serialize_preference(preference) + end +end diff --git a/app/api/task_comments_api.rb b/app/api/task_comments_api.rb index cfe2500a87..9ad1c8be57 100644 --- a/app/api/task_comments_api.rb +++ b/app/api/task_comments_api.rb @@ -141,6 +141,7 @@ class TaskCommentsApi < Grape::API # mark every comment type except for DiscussionComments so we don't mark it as read. comments_to_mark_as_read = comments.where("TYPE is null OR TYPE != 'DiscussionComment'") task.mark_comments_as_read(current_user, comments_to_mark_as_read) + Notification.mark_task_read(current_user, task) else result = [] end @@ -267,6 +268,7 @@ class TaskCommentsApi < Grape::API task_comment = task.comments.find(params[:id]) task_comment.mark_as_unread(current_user) + Notification.reopen_for_source(task_comment, current_user) SessionTracker.record_assessment_activity( action: 'mark-comment-unread', diff --git a/app/mailers/notifications_mailer.rb b/app/mailers/notifications_mailer.rb index f4b0d255ad..f06b1b40ce 100644 --- a/app/mailers/notifications_mailer.rb +++ b/app/mailers/notifications_mailer.rb @@ -83,6 +83,26 @@ def weekly_student_summary(project, summary_stats, did_revert_to_pass) mail(to: email_with_name, from: tutor_email, subject: subject) end + def notification_digest(preference, notifications) + return nil if preference.nil? || notifications.blank? + + add_general + @recipient = preference.user + @unit = preference.unit + @groups = NotificationGroupBuilder.new(notifications).groups + @notification_count = notifications.count + @notification_url = "#{@doubtfire_host}/notifications" + @sender = @unit.main_convenor_user + return nil if @sender.nil? + + subject = "#{@unit.code}: #{@notification_count} new #{'change'.pluralize(@notification_count)} across #{@groups.count} #{'notification'.pluralize(@groups.count)}" + mail( + to: %("#{@recipient.name}" <#{@recipient.email}>), + from: %("#{@sender.name}" <#{@sender.email}>), + subject: subject + ) + end + def discussion_deadline_approaching(task, sender, expiry_date) add_discussion_deadline_details(task, sender) @deadline = task.unit.formatted_discuss_timeout_date(expiry_date) diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index c74883d014..4b7ca794d4 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -16,6 +16,7 @@ class TaskComment < ApplicationRecord belongs_to :recipient, class_name: 'User', optional: false has_many :comments_read_receipts, class_name: 'CommentsReadReceipts', dependent: :destroy, inverse_of: :task_comment + has_many :notifications, as: :source, dependent: :destroy # Can optionally be a reply to a comment belongs_to :task_comment, optional: true @@ -33,6 +34,9 @@ class TaskComment < ApplicationRecord after_create do mark_as_read(self.user) end + after_create_commit do + Notification.create_for_task_comment(self) + end # Delete action - before dependent association before_destroy :delete_associated_files diff --git a/app/models/comments/task_status_comment.rb b/app/models/comments/task_status_comment.rb index a224279db8..4e896ea91a 100644 --- a/app/models/comments/task_status_comment.rb +++ b/app/models/comments/task_status_comment.rb @@ -5,10 +5,6 @@ class TaskStatusComment < TaskComment self.content_type = :status end - after_create do - mark_as_read(self.recipient) - end - def serialize(user) json = super(user) json[:recipient_read_time] = nil diff --git a/app/models/notification.rb b/app/models/notification.rb new file mode 100644 index 0000000000..3d518e1c2a --- /dev/null +++ b/app/models/notification.rb @@ -0,0 +1,268 @@ +# frozen_string_literal: true + +class Notification < ApplicationRecord + KINDS = %w[ + feedback_left + task_status_changed + overseer_failed + pdf_generation_failed + discuss_warning + discuss_expired + tutor_note + ].freeze + + DISCUSS_KINDS = %w[discuss_warning discuss_expired].freeze + INTERNAL_COMMENT_TYPES = %w[assessment checked_in discussed_in_class plan].freeze + + attribute :metadata, :json, default: -> { {} } + + belongs_to :recipient, class_name: 'User', inverse_of: :received_notifications + belongs_to :unit + belongs_to :project, optional: true + belongs_to :task, optional: true + belongs_to :actor, class_name: 'User', optional: true, inverse_of: :acted_notifications + belongs_to :source, polymorphic: true, optional: true + + validates :kind, inclusion: { in: KINDS } + validates :deduplication_key, presence: true, uniqueness: { scope: :recipient_id } + validate :metadata_is_an_object + + scope :unread, -> { where(read_at: nil) } + scope :recently_read, -> { where(read_at: 30.days.ago..) } + scope :email_pending, -> { unread.where(email_processed_at: nil) } + + before_validation :normalize_metadata + + def self.create_for_task_comment(comment) + kind = kind_for_comment(comment) + return if kind.nil? + + recipients_for_comment(comment).each do |recipient, recipient_task| + next if comment.read_by?(recipient) + + if kind == 'task_status_changed' + resolve_for(recipient: recipient, task: recipient_task, kinds: kind) + elsif kind == 'discuss_expired' + resolve_for(recipient: recipient, task: recipient_task, kinds: 'discuss_warning') + end + + notification = create_event( + recipient: recipient, + unit: recipient_task.unit, + project: recipient_task.project, + task: recipient_task, + actor: comment.user, + kind: kind, + source: comment, + deduplication_key: "task-comment:#{comment.id}:#{kind}", + metadata: metadata_for_comment(comment, recipient_task) + ) + + SendImmediateNotificationJob.perform_async(notification.id) if notification && DISCUSS_KINDS.include?(kind) + end + end + + def self.create_for_overseer(assessment) + latest_assessment = assessment.task.overseer_assessments.order(created_at: :desc, id: :desc).first + assessment_comment = assessment.latest_assessment_comment + return unless assessment == latest_assessment && assessment.failed? && assessment_comment.present? + + student_task_recipients(assessment.task).each do |recipient, recipient_task| + mark_read( + where( + recipient: recipient, + task: recipient_task, + kind: 'overseer_failed' + ).where.not(source: assessment).unread + ) + next if assessment_comment.read_by?(recipient) + + create_event( + recipient: recipient, + unit: recipient_task.unit, + project: recipient_task.project, + task: recipient_task, + actor: assessment.task.project.tutor_for(assessment.task.task_definition), + kind: 'overseer_failed', + source: assessment, + deduplication_key: "overseer-assessment:#{assessment.id}:failed", + metadata: { + email_not_before: ( + assessment.updated_at + OverseerAssessment.student_notification_grace_period + ).iso8601 + } + ) + end + end + + def self.create_pdf_failure(task) + version = task.file_uploaded_at&.to_i || task.updated_at.to_i + + student_task_recipients(task).each do |recipient, recipient_task| + create_event( + recipient: recipient, + unit: recipient_task.unit, + project: recipient_task.project, + task: recipient_task, + actor: task.project.tutor_for(task.task_definition), + kind: 'pdf_generation_failed', + source: task, + deduplication_key: "pdf-generation:#{task.id}:#{version}:failed", + metadata: {} + ) + end + end + + def self.create_for_tutor_note(tutor_note, recipient) + create_event( + recipient: recipient, + unit: tutor_note.unit_role.unit, + project: tutor_note.task&.project, + task: tutor_note.task, + actor: tutor_note.user, + kind: 'tutor_note', + source: tutor_note, + deduplication_key: "tutor-note:#{tutor_note.id}", + metadata: { + unit_role_id: tutor_note.unit_role_id, + tutor_note_id: tutor_note.id + } + ) + end + + def self.create_event(**attributes) + recipient = attributes.fetch(:recipient) + unit = attributes.fetch(:unit) + deduplication_key = attributes.fetch(:deduplication_key) + preference = NotificationPreference.for(recipient, unit) + + notification = find_or_initialize_by( + recipient: recipient, + deduplication_key: deduplication_key + ) + return notification if notification.persisted? + + notification.assign_attributes( + unit: unit, + project: attributes[:project], + task: attributes[:task], + actor: attributes[:actor], + kind: attributes.fetch(:kind), + source: attributes[:source], + metadata: attributes.fetch(:metadata, {}) + ) + notification.save! + notification.update!(email_processed_at: Time.current) if preference.email_frequency == 'off' + notification + rescue ActiveRecord::RecordNotUnique + find_by(recipient: recipient, deduplication_key: deduplication_key) + end + + def self.mark_read(relation, at: Time.current) + # A group must share one exact read timestamp so read history can reconstruct the group. + # rubocop:disable Rails/SkipsModelValidations + relation.update_all( + [ + 'read_at = ?, email_processed_at = COALESCE(email_processed_at, ?), updated_at = ?', + at, + at, + at + ] + ) + # rubocop:enable Rails/SkipsModelValidations + end + + def self.mark_task_read(recipient, task) + mark_read(where(recipient: recipient, task: task).unread) + end + + def self.reopen_for_source(source, recipient) + # Keep email_processed_at unchanged so manually reopening a comment never resends email. + # rubocop:disable Rails/SkipsModelValidations + where(source: source, recipient: recipient).update_all(read_at: nil, updated_at: Time.current) + # rubocop:enable Rails/SkipsModelValidations + end + + def self.resolve_task_kinds(task, kinds) + tasks = related_group_tasks(task) + mark_read(where(task: tasks, kind: Array(kinds)).unread) + end + + def self.resolve_for(recipient:, task:, kinds:) + mark_read(where(recipient: recipient, task: task, kind: Array(kinds)).unread) + end + + def email_ready?(at: Time.current) + email_not_before = metadata['email_not_before'] || metadata[:email_not_before] + return true if email_not_before.blank? + + Time.zone.parse(email_not_before) <= at + rescue ArgumentError, TypeError + true + end + + def self.kind_for_comment(comment) + case comment + when TaskStatusComment + return nil if student_actor?(comment) + + 'task_status_changed' + when DiscussTimeoutComment + comment.content_type == DiscussTimeoutComment.expired ? 'discuss_expired' : 'discuss_warning' + when AssessmentComment + nil + else + return nil if INTERNAL_COMMENT_TYPES.include?(comment.content_type) + + 'feedback_left' + end + end + + def self.metadata_for_comment(comment, recipient_task) + result = {} + result[:status] = comment.task_status.status_key if comment.is_a?(TaskStatusComment) + result[:deadline] = recipient_task.unit.discuss_timeout_expiry_date(recipient_task)&.iso8601 if comment.content_type == DiscussTimeoutComment.warning + result + end + + def self.recipients_for_comment(comment) + if !student_actor?(comment) && comment.task.group_task? && comment.task.group_submission.present? + student_task_recipients(comment.task) + else + [[comment.recipient, comment.task]] + end + end + + def self.student_task_recipients(task) + related_group_tasks(task).filter_map do |recipient_task| + student = recipient_task.project.student + [student, recipient_task] unless student.nil? + end + end + + def self.related_group_tasks(task) + return [task] unless task.group_task? && task.group_submission_id.present? + + Task.where(group_submission_id: task.group_submission_id).includes(project: :user).to_a + end + + def self.student_actor?(comment) + comment.user == comment.project.student || comment.task.role_for(comment.user).in?(%i[student group_member]) + end + + def metadata_is_an_object + errors.add(:metadata, 'must be a JSON object') unless metadata.is_a?(Hash) + end + + def normalize_metadata + self.metadata ||= {} + return unless metadata.is_a?(String) + + parsed = JSON.parse(metadata) + self.metadata = parsed if parsed.is_a?(Hash) + rescue JSON::ParserError + nil + end + + private_class_method :kind_for_comment, :metadata_for_comment, :recipients_for_comment, :student_actor? +end diff --git a/app/models/notification_preference.rb b/app/models/notification_preference.rb new file mode 100644 index 0000000000..e803515dbc --- /dev/null +++ b/app/models/notification_preference.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +class NotificationPreference < ApplicationRecord + FREQUENCIES = %w[off hourly daily weekly].freeze + TIME_FORMAT = /\A(?:[01]\d|2[0-3]):[0-5]\d\z/ + + attribute :email_categories, :json + + belongs_to :user + belongs_to :unit + + validates :email_frequency, inclusion: { in: FREQUENCIES } + validates :email_time, format: { with: TIME_FORMAT } + validates :email_weekday, inclusion: { in: 1..7 } + validates :timezone, presence: true + validate :valid_timezone + validate :valid_email_categories + + before_validation :apply_defaults + before_validation :normalize_email_categories + before_save :refresh_next_digest_at, if: :schedule_changed? + after_commit :process_pending_notifications_when_off, if: :saved_change_to_off? + + scope :due, -> { where.not(email_frequency: 'off').where(next_digest_at: ..Time.current) } + + def self.for(user, unit) + find_or_create_by!(user: user, unit: unit) do |preference| + preference.email_categories = default_categories(user) + preference.timezone = default_timezone(user, unit) + end + end + + def self.default_categories(user) + categories = ['tutor_note'] + categories.push('feedback_left', 'task_status_changed', 'discuss_warning', 'discuss_expired') if user.receive_feedback_notifications + categories.push('overseer_failed', 'pdf_generation_failed') if user.receive_task_notifications + categories.uniq + end + + def self.default_timezone(user, unit) + return Time.zone.name if unit.unit_roles.exists?(user_id: user.id) + + Project.find_by(user: user, unit: unit)&.campus&.timezone.presence || Time.zone.name + end + + def email_enabled_for?(kind) + email_frequency != 'off' && email_categories.include?(kind) + end + + def advance_digest!(from: Time.current) + update!(last_digest_at: from, next_digest_at: next_occurrence(from)) + end + + def next_occurrence(from = Time.current) + return nil if email_frequency == 'off' + + zone = timezone_object + local_from = from.in_time_zone(zone) + return (local_from.beginning_of_hour + 1.hour).utc if email_frequency == 'hourly' + + hour, minute = email_time.split(':').map(&:to_i) + candidate_date = local_from.to_date + + if email_frequency == 'weekly' + days_ahead = (email_weekday - candidate_date.cwday) % 7 + candidate_date += days_ahead.days + end + + candidate = zone.local(candidate_date.year, candidate_date.month, candidate_date.day, hour, minute) + candidate += email_frequency == 'weekly' ? 1.week : 1.day if candidate <= local_from + candidate.utc + end + + def timezone_object + ActiveSupport::TimeZone[timezone] || Time.zone + end + + private + + def apply_defaults + self.email_categories ||= self.class.default_categories(user) + self.email_frequency ||= 'weekly' + self.email_time ||= '09:00' + self.email_weekday ||= 1 + self.timezone ||= self.class.default_timezone(user, unit) + end + + def refresh_next_digest_at + self.next_digest_at = next_occurrence(Time.current) + end + + def schedule_changed? + next_digest_at.nil? || + will_save_change_to_email_frequency? || + will_save_change_to_email_time? || + will_save_change_to_email_weekday? || + will_save_change_to_timezone? + end + + def valid_timezone + errors.add(:timezone, 'must be a valid timezone') if timezone.present? && ActiveSupport::TimeZone[timezone].nil? + end + + def valid_email_categories + unless email_categories.is_a?(Array) + errors.add(:email_categories, 'must be an array') + return + end + + invalid = Array(email_categories) - Notification::KINDS + errors.add(:email_categories, "contains unsupported categories: #{invalid.join(', ')}") if invalid.any? + end + + def normalize_email_categories + return unless email_categories.is_a?(String) + + parsed = JSON.parse(email_categories) + self.email_categories = parsed if parsed.is_a?(Array) + rescue JSON::ParserError + nil + end + + def saved_change_to_off? + email_frequency == 'off' && saved_change_to_email_frequency? + end + + def process_pending_notifications_when_off + now = Time.current + # These are delivery-ledger updates and intentionally bypass callbacks. + # rubocop:disable Rails/SkipsModelValidations + user.received_notifications.where(unit: unit).email_pending.update_all( + email_processed_at: now, + updated_at: now + ) + # rubocop:enable Rails/SkipsModelValidations + end +end diff --git a/app/models/overseer_assessment.rb b/app/models/overseer_assessment.rb index d01a3210ed..0373bbc449 100644 --- a/app/models/overseer_assessment.rb +++ b/app/models/overseer_assessment.rb @@ -6,6 +6,7 @@ class OverseerAssessment < ApplicationRecord has_one :project, through: :task has_many :assessment_comments, as: :commentable, dependent: :destroy has_many :overseer_step_results, dependent: :destroy + has_many :notifications, as: :source, dependent: :destroy validates :status, presence: true validates :task_id, presence: true @@ -15,6 +16,10 @@ class OverseerAssessment < ApplicationRecord validates :submission_history_id, uniqueness: true validate :submission_history_matches_task + after_update_commit do + Notification.create_for_overseer(self) if saved_change_to_status? && failed? + end + enum :status, { pre_queued: 0, passed: 1, failed: 2 } def submission_history_matches_task @@ -43,7 +48,6 @@ def self.student_notification_grace_period AND student_read_receipts.user_id = projects.user_id SQL .where(status: statuses[:failed], student_notified_at: nil) - .where(users: { receive_task_notifications: true }) .where('overseer_assessments.updated_at <= ?', notification_cutoff) .where('student_read_receipts.id IS NULL') .where(<<~SQL.squish) diff --git a/app/models/portfolio_evidence.rb b/app/models/portfolio_evidence.rb index 65d1d481b6..76e64af6e2 100644 --- a/app/models/portfolio_evidence.rb +++ b/app/models/portfolio_evidence.rb @@ -62,6 +62,7 @@ def self.process_new_to_pdf(my_source) if success done[task.project] = [] if done[task.project].nil? done[task.project] << task + Notification.resolve_task_kinds(task, 'pdf_generation_failed') else add_error.call('Failed to convert your submission to pdf.') end @@ -70,16 +71,8 @@ def self.process_new_to_pdf(my_source) end end - errors.each do |project, tasks| - logger.debug "checking email for project #{project.id}" - next unless project.student.receive_task_notifications - - logger.info "emailing task notification to #{project.student.name}" - begin - PortfolioEvidenceMailer.task_pdf_failed(project, tasks).deliver - rescue StandardError => e - logger.error "Failed to send task pdf failed email for project #{project.id}!\n#{e.message}" - end + errors.each_value do |tasks| + tasks.each { |task| Notification.create_pdf_failure(task) } end end diff --git a/app/models/project.rb b/app/models/project.rb index b74cc84add..4d86b5567a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -31,6 +31,7 @@ class Project < ApplicationRecord has_many :comments, through: :tasks has_many :tutorial_enrolments, dependent: :destroy has_many :session_activities, dependent: :destroy + has_many :notifications, dependent: :destroy has_many :staff_notes, dependent: :destroy has_many :engagements, dependent: :destroy, inverse_of: :project diff --git a/app/models/task.rb b/app/models/task.rb index 9a535b5a13..21bddbcb12 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -141,6 +141,8 @@ def specific_permission_hash(role, perm_hash, _other) has_many :tii_submissions, dependent: :destroy has_many :test_attempts, dependent: :destroy has_many :session_activities, dependent: :destroy + has_many :notifications, dependent: :destroy + has_many :source_notifications, as: :source, class_name: 'Notification', dependent: :destroy delegate :unit, to: :project delegate :student, to: :project diff --git a/app/models/tutor_note.rb b/app/models/tutor_note.rb index dbc42645d7..ee7ad0eed4 100644 --- a/app/models/tutor_note.rb +++ b/app/models/tutor_note.rb @@ -3,6 +3,7 @@ class TutorNote < ApplicationRecord belongs_to :user belongs_to :task, optional: true belongs_to :reply_to, class_name: "TutorNote", optional: true + has_many :notifications, as: :source, dependent: :destroy def task_definition_id task&.task_definition&.id diff --git a/app/models/unit.rb b/app/models/unit.rb index a3757c62e3..6b4355af8c 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -181,6 +181,8 @@ def role_for(user) has_many :communication_set_schedules, through: :communication_sets, class_name: 'CommunicationSetSchedule' has_many :unit_content_sites, dependent: :destroy has_many :unit_content_links, dependent: :destroy + has_many :notifications, dependent: :destroy + has_many :notification_preferences, dependent: :destroy has_many :comments, through: :projects has_many :tasks, through: :projects @@ -320,7 +322,6 @@ def warn_discuss_timeout_task(task, actor, teaching_breaks: nil) raise ActiveRecord::Rollback if comment.blank? task.update!(notified_discuss_warning_at: Time.zone.now) - queue_discuss_timeout_email(task, actor, :approaching, expiry_date) created_comment = true end @@ -346,7 +347,6 @@ def expire_discuss_timeout_task(task, actor) raise ActiveRecord::Rollback end - queue_discuss_timeout_email(task, actor, :missed) created_comment = true end @@ -366,13 +366,6 @@ def formatted_discuss_timeout_date(date) "#{result} #{date.year}" end - def queue_discuss_timeout_email(task, actor, type, expiry_date = nil) - return unless send_notifications - return unless task.project.student.receive_feedback_notifications - - SendDiscussTimeoutEmailJob.perform_async(task.id, actor.id, type.to_s, expiry_date&.iso8601) - end - def detailed_name "#{name} #{teaching_period.present? ? teaching_period.detailed_name : start_date.strftime('%Y-%m-%d')}" end @@ -3101,19 +3094,6 @@ def update_task_status_from_csv(user, csv_str, success, _ignored, errors) end end - # send emails... - begin - done.each do |project, tasks| - logger.info "Checking feedback email for project #{project.id}" - if project.student.receive_feedback_notifications - logger.info "Emailing feedback notification to #{project.student.name}" - PortfolioEvidenceMailer.task_feedback_ready(project, tasks).deliver - end - end - rescue => e - logger.error "Failed to send emails from feedback submission. Rescued with error: #{e.message}" - end - true end diff --git a/app/models/user.rb b/app/models/user.rb index 67a50d1877..a393b69af1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -182,6 +182,17 @@ def token_for_text?(a_token, token_type) has_many :chip_usage, dependent: :destroy, inverse_of: :tutor, class_name: 'Feedback::ChipUsage' has_many :marking_sessions, dependent: :destroy + has_many :received_notifications, + class_name: 'Notification', + foreign_key: :recipient_id, + dependent: :destroy, + inverse_of: :recipient + has_many :acted_notifications, + class_name: 'Notification', + foreign_key: :actor_id, + dependent: :nullify, + inverse_of: :actor + has_many :notification_preferences, dependent: :destroy # Model validations/constraints validates :first_name, presence: true diff --git a/app/services/notification_group_builder.rb b/app/services/notification_group_builder.rb new file mode 100644 index 0000000000..ba50e4760d --- /dev/null +++ b/app/services/notification_group_builder.rb @@ -0,0 +1,116 @@ +# frozen_string_literal: true + +class NotificationGroupBuilder + SEVERITY_ORDER = { + 'critical' => 0, + 'warning' => 1, + 'normal' => 2 + }.freeze + + def initialize(notifications) + @notifications = notifications.to_a + end + + def groups + @notifications + .group_by { |notification| grouping_key(notification) } + .values + .map { |items| serialize(items) } + .sort_by { |group| [group[:read] ? 1 : 0, SEVERITY_ORDER.fetch(group[:severity]), -group[:latest_at].to_f] } + end + + private + + def grouping_key(notification) + state_key = notification.read_at ? "read:#{notification.read_at.to_f}" : 'unread' + return "#{state_key}:task:#{notification.task_id}" if notification.task_id.present? + + unit_role_id = notification.metadata['unit_role_id'] || notification.metadata[:unit_role_id] + return "#{state_key}:tutor-notes:#{unit_role_id}" if notification.kind == 'tutor_note' + + "#{state_key}:unit:#{notification.unit_id}:#{notification.kind}" + end + + def serialize(items) + latest = items.max_by(&:created_at) + task = latest.task + counts = items.each_with_object(Hash.new(0)) { |notification, result| result[notification.kind] += 1 } + latest_status = items + .select { |notification| notification.kind == 'task_status_changed' } + .max_by(&:created_at) + &.metadata + &.fetch('status', nil) + tutor_notes = items + .select { |notification| notification.kind == 'tutor_note' } + .sort_by { |notification| [notification.created_at, notification.id] } + + { + key: grouping_key(latest), + notification_ids: items.map(&:id), + tutor_note_notification_ids: tutor_notes.map(&:id), + unit: { + id: latest.unit.id, + code: latest.unit.code, + name: latest.unit.name + }, + task: task_details(task, latest.recipient), + counts: counts, + event_count: items.count, + latest_status: latest_status, + severity: severity_for(items), + read: items.all? { |notification| notification.read_at.present? }, + read_at: items.filter_map(&:read_at).max, + latest_at: latest.created_at, + tutor_note_ids: tutor_notes.filter_map { |notification| notification.metadata['tutor_note_id'] }, + tutor_note_unit_role_id: tutor_notes.first&.metadata&.fetch('unit_role_id', nil), + summary: summary_for(task, latest.recipient, counts, latest_status) + } + end + + def task_details(task, recipient) + return nil if task.nil? + + staff_view = task.project.student != recipient + + { + id: task.id, + project_id: task.project_id, + task_definition_id: task.task_definition_id, + abbreviation: task.task_definition.abbreviation, + name: task.task_definition.name, + staff_view: staff_view, + student_name: staff_view ? task.project.student.name : nil + } + end + + def severity_for(items) + kinds = items.map(&:kind) + return 'critical' if kinds.intersect?(%w[discuss_expired pdf_generation_failed]) + return 'warning' if kinds.intersect?(%w[discuss_warning overseer_failed tutor_note]) + + 'normal' + end + + def summary_for(task, recipient, counts, latest_status) + details = [] + details << 'discussion deadline missed' if counts['discuss_expired'].positive? + details << 'discussion deadline approaching' if counts['discuss_warning'].positive? + details << 'submission PDF generation failed' if counts['pdf_generation_failed'].positive? + details << 'automated assessment failed' if counts['overseer_failed'].positive? + details << pluralize(counts['tutor_note'], 'tutor note') if counts['tutor_note'].positive? + details << pluralize(counts['feedback_left'], 'new comment') if counts['feedback_left'].positive? + details << "status changed to #{latest_status.to_s.humanize}" if latest_status.present? + + subject = + if task + task.project.student == recipient ? task.task_definition.abbreviation : "#{task.task_definition.abbreviation} for #{task.project.student.name}" + else + 'Unit notification' + end + "#{subject} — #{details.to_sentence}" + end + + def pluralize(count, noun) + "#{count} #{count == 1 ? noun : noun.pluralize}" + end +end diff --git a/app/sidekiq/accept_overseer_job.rb b/app/sidekiq/accept_overseer_job.rb index 59ddd753fd..b818e2332a 100644 --- a/app/sidekiq/accept_overseer_job.rb +++ b/app/sidekiq/accept_overseer_job.rb @@ -91,6 +91,7 @@ def perform(task_id, _output_path, docker_image_name_tag, submission, assessment if steps_attempted == steps_passed && assessment_pass oa.update!(status: :passed) + Notification.resolve_task_kinds(task, 'overseer_failed') unless success_status.nil? # TODO: have an override status setting for the step? eg. if the task is overdue, let it remain overdue, otherwise use this task status task.update!(task_status: success_status) diff --git a/app/sidekiq/accept_submission_job.rb b/app/sidekiq/accept_submission_job.rb index eaaf9b380e..d0bc6c4fe7 100644 --- a/app/sidekiq/accept_submission_job.rb +++ b/app/sidekiq/accept_submission_job.rb @@ -30,14 +30,7 @@ def perform(task_id, user_id, accepted_tii_eula, test_submission) rescue StandardError => e logger.error e - # Send email to student if task pdf failed - if task.project.student.receive_task_notifications - begin - PortfolioEvidenceMailer.task_pdf_failed(task.project, [task]).deliver - rescue StandardError => e - logger.error "Failed to send task pdf failed email for project #{task.project.id}!\n#{e.message}" - end - end + Notification.create_pdf_failure(task) begin # Notify system admin @@ -60,6 +53,8 @@ def perform(task_id, user_id, accepted_tii_eula, test_submission) return end + Notification.resolve_task_kinds(task, 'pdf_generation_failed') + # Mark this task for moderation tutor_user = task.project.tutor_for(task.task_definition) if tutor_user && !test_submission diff --git a/app/sidekiq/create_pending_overseer_notifications_job.rb b/app/sidekiq/create_pending_overseer_notifications_job.rb new file mode 100644 index 0000000000..e5291ab28c --- /dev/null +++ b/app/sidekiq/create_pending_overseer_notifications_job.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class CreatePendingOverseerNotificationsJob + include Sidekiq::Job + + sidekiq_options lock: :until_executed, + lock_args_method: ->(_args) { ['create-pending-overseer-notifications'] }, + on_conflict: :reject, + retry: 1 + + def perform + OverseerAssessment.awaiting_student_failure_notification.find_each do |assessment| + Notification.create_for_overseer(assessment) + assessment.update!(student_notified_at: Time.current) + end + end +end diff --git a/app/sidekiq/notify_tutor_notes_job.rb b/app/sidekiq/notify_tutor_notes_job.rb index 194c6b4f35..971749690f 100644 --- a/app/sidekiq/notify_tutor_notes_job.rb +++ b/app/sidekiq/notify_tutor_notes_job.rb @@ -5,7 +5,7 @@ def perform(tutor_note_id, recipient_user_id) tutor_note = TutorNote.find(tutor_note_id) recipient = User.find(recipient_user_id) - TutorNoteMailer.notify_tutor_note(tutor_note, recipient).deliver + Notification.create_for_tutor_note(tutor_note, recipient) rescue StandardError => e Rails.logger.error("Failed to send tutor note email for TutorNote #{tutor_note_id} to User #{recipient_user_id}: #{e.class} - #{e.message}") end diff --git a/app/sidekiq/poll_notification_digests_job.rb b/app/sidekiq/poll_notification_digests_job.rb new file mode 100644 index 0000000000..54915e0d52 --- /dev/null +++ b/app/sidekiq/poll_notification_digests_job.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class PollNotificationDigestsJob + include Sidekiq::Job + + sidekiq_options lock: :until_executed, + lock_args_method: ->(_args) { ['poll-notification-digests'] }, + on_conflict: :reject, + retry: 1 + + def perform + NotificationPreference.due.find_each do |preference| + SendNotificationDigestJob.perform_async(preference.id) + end + end +end diff --git a/app/sidekiq/prune_notifications_job.rb b/app/sidekiq/prune_notifications_job.rb new file mode 100644 index 0000000000..5ed818dc6a --- /dev/null +++ b/app/sidekiq/prune_notifications_job.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class PruneNotificationsJob + include Sidekiq::Job + + sidekiq_options lock: :until_executed, + lock_args_method: ->(_args) { ['prune-notifications'] }, + on_conflict: :reject, + retry: 1 + + def perform + Notification + .where.not(read_at: nil) + .where(read_at: ...90.days.ago) + .delete_all + end +end diff --git a/app/sidekiq/send_immediate_notification_job.rb b/app/sidekiq/send_immediate_notification_job.rb new file mode 100644 index 0000000000..b10bef1b6b --- /dev/null +++ b/app/sidekiq/send_immediate_notification_job.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +class SendImmediateNotificationJob + include Sidekiq::Job + + sidekiq_options retry: 5 + + def perform(notification_id) + notification = Notification.find(notification_id) + return if notification.email_processed_at.present? + + preference = NotificationPreference.for(notification.recipient, notification.unit) + unless notification.read_at.nil? && + notification.unit.send_notifications && + preference.email_enabled_for?(notification.kind) + notification.update!(email_processed_at: Time.current) + return + end + + sender = notification.actor || notification.unit.main_convenor_user + if sender.nil? + notification.update!(email_processed_at: Time.current) + return + end + + mail = + if notification.kind == 'discuss_warning' + deadline = notification.metadata['deadline'] + NotificationsMailer.discussion_deadline_approaching( + notification.task, + sender, + deadline.present? ? Date.iso8601(deadline) : notification.unit.discuss_timeout_expiry_date(notification.task) + ) + else + NotificationsMailer.discussion_deadline_missed(notification.task, sender) + end + + mail.deliver_now + notification.update!( + email_processed_at: Time.current, + email_sent_at: Time.current + ) + end +end diff --git a/app/sidekiq/send_notification_digest_job.rb b/app/sidekiq/send_notification_digest_job.rb new file mode 100644 index 0000000000..e026d35dad --- /dev/null +++ b/app/sidekiq/send_notification_digest_job.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +class SendNotificationDigestJob + include Sidekiq::Job + + sidekiq_options lock: :until_executed, + lock_args_method: ->(args) { ["notification-digest:#{args.first}"] }, + on_conflict: :reject, + retry: 5 + + def perform(preference_id) + preference = NotificationPreference.find(preference_id) + return if preference.email_frequency == 'off' + + now = Time.current + pending = preference.user + .received_notifications + .where(unit: preference.unit) + .email_pending + .where.not(kind: Notification::DISCUSS_KINDS) + .includes(:recipient, :unit, task: [:task_definition, { project: :user }]) + ready_ids = pending.select { |notification| notification.email_ready?(at: now) }.map(&:id) + ready = Notification.where(id: ready_ids) + + disabled = ready.where.not(kind: preference.email_categories) + # These are immutable delivery-ledger updates and intentionally bypass callbacks. + # rubocop:disable Rails/SkipsModelValidations + disabled.update_all(email_processed_at: now, updated_at: now) + # rubocop:enable Rails/SkipsModelValidations + + enabled = ready + .where(kind: preference.email_categories) + .includes(:recipient, :unit, task: [:task_definition, { project: :user }]) + .to_a + unless preference.unit.send_notifications + # rubocop:disable Rails/SkipsModelValidations + Notification.where(id: enabled.map(&:id)).update_all(email_processed_at: now, updated_at: now) + # rubocop:enable Rails/SkipsModelValidations + preference.advance_digest!(from: now) + return + end + + if enabled.any? + NotificationsMailer.notification_digest(preference, enabled).deliver_now + # rubocop:disable Rails/SkipsModelValidations + Notification.where(id: enabled.map(&:id)).update_all( + email_processed_at: now, + email_sent_at: now, + updated_at: now + ) + # rubocop:enable Rails/SkipsModelValidations + end + + preference.advance_digest!(from: now) + end +end diff --git a/app/views/notifications_mailer/notification_digest.html.erb b/app/views/notifications_mailer/notification_digest.html.erb new file mode 100644 index 0000000000..6b54fb481e --- /dev/null +++ b/app/views/notifications_mailer/notification_digest.html.erb @@ -0,0 +1,100 @@ + + + + + + +
+

<%= @unit.name %> - Notification Summary

+

<%= @unit.code %>

+ +

Hi <%= @recipient.first_name %>,

+ +

+ You have <%= @notification_count %> new <%= 'change'.pluralize(@notification_count) %> + across <%= @groups.count %> <%= 'notification'.pluralize(@groups.count) %>. +

+ + <% @groups.each do |group| %> +
+ <%= group[:summary] %> + <% if group[:task] %> +

+ + View this task + +

+ <% end %> +
+ <% end %> + +

+ View all notifications and email settings +

+ +

+ Cheers,
+ The <%= @doubtfire_product_name %> Team on behalf of <%= @sender.name %> +

+
+ + + + diff --git a/app/views/notifications_mailer/notification_digest.text.erb b/app/views/notifications_mailer/notification_digest.text.erb new file mode 100644 index 0000000000..b01e60889c --- /dev/null +++ b/app/views/notifications_mailer/notification_digest.text.erb @@ -0,0 +1,16 @@ +Hi <%= @recipient.first_name %>, + +You have <%= @notification_count %> new <%= 'change'.pluralize(@notification_count) %> across <%= @groups.count %> <%= 'notification'.pluralize(@groups.count) %> in <%= @unit.code %>. + +<% @groups.each do |group| %> +- <%= group[:summary] %> +<% if group[:task] %> + <%= @doubtfire_host %>/projects/<%= group[:task][:project_id] %>/dashboard/<%= group[:task][:abbreviation] %><%= '?tutor=true' if group[:task][:staff_view] %> +<% end %> +<% end %> + +View all notifications and email settings: +<%= @notification_url %> + +Cheers, +The <%= @doubtfire_product_name %> Team diff --git a/config/schedule.yml b/config/schedule.yml index adb2a1f282..d591b65aaf 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -28,6 +28,18 @@ notify_discuss_timeout: cron: "every day at 8am" class: "NotifyDiscussTimeoutJob" +poll_notification_digests: + cron: "every 5 minutes" + class: "PollNotificationDigestsJob" + +create_pending_overseer_notifications: + cron: "every 5 minutes" + class: "CreatePendingOverseerNotificationsJob" + +prune_notifications: + cron: "every day at 2am" + class: "PruneNotificationsJob" + # archive_old_units: # cron: "every 6 months" # class: "ArchiveOldUnitsJob" diff --git a/db/migrate/20260729043436_create_notifications_and_notification_preferences.rb b/db/migrate/20260729043436_create_notifications_and_notification_preferences.rb new file mode 100644 index 0000000000..8f2b2ff780 --- /dev/null +++ b/db/migrate/20260729043436_create_notifications_and_notification_preferences.rb @@ -0,0 +1,54 @@ +class CreateNotificationsAndNotificationPreferences < ActiveRecord::Migration[8.0] + def change + create_table :notifications do |t| + t.references :recipient, null: false, foreign_key: { to_table: :users } + t.references :unit, null: false, foreign_key: true + t.references :project, null: true, foreign_key: true + t.references :task, null: true, foreign_key: true + t.references :actor, null: true, foreign_key: { to_table: :users } + + t.string :kind, null: false, limit: 64 + t.string :source_type, null: true, limit: 64 + t.bigint :source_id, null: true + t.string :deduplication_key, null: false, limit: 191 + t.json :metadata, null: false + + t.datetime :read_at + t.datetime :email_processed_at + t.datetime :email_sent_at + + t.timestamps + end + + add_index :notifications, [:source_type, :source_id] + add_index :notifications, [:recipient_id, :deduplication_key], + unique: true, + name: 'index_notifications_on_recipient_and_deduplication_key' + add_index :notifications, [:recipient_id, :read_at, :created_at], + name: 'index_notifications_on_recipient_read_created' + add_index :notifications, [:recipient_id, :unit_id, :email_processed_at], + name: 'index_notifications_for_email_delivery' + add_index :notifications, [:recipient_id, :task_id, :read_at], + name: 'index_notifications_on_recipient_task_read' + + create_table :notification_preferences do |t| + t.references :user, null: false, foreign_key: true + t.references :unit, null: false, foreign_key: true + + t.json :email_categories, null: false + t.string :email_frequency, null: false, default: 'weekly', limit: 16 + t.string :email_time, null: false, default: '09:00', limit: 5 + t.integer :email_weekday, null: false, default: 1 + t.string :timezone, null: false, default: 'UTC' + t.datetime :next_digest_at + t.datetime :last_digest_at + + t.timestamps + end + + add_index :notification_preferences, [:user_id, :unit_id], + unique: true, + name: 'index_notification_preferences_on_user_and_unit' + add_index :notification_preferences, :next_digest_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 90bbe62f82..0b53985eb5 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[8.0].define(version: 2026_07_24_015355) do +ActiveRecord::Schema[8.0].define(version: 2026_07_29_043436) do create_table "activity_types", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.string "name", null: false t.string "abbreviation", null: false @@ -342,6 +342,54 @@ t.index ["task_id"], name: "index_moderated_tasks_on_task_id" end + create_table "notification_preferences", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "user_id", null: false + t.bigint "unit_id", null: false + t.text "email_categories", size: :long, null: false, collation: "utf8mb4_bin" + t.string "email_frequency", limit: 16, default: "weekly", null: false + t.string "email_time", limit: 5, default: "09:00", null: false + t.integer "email_weekday", default: 1, null: false + t.string "timezone", default: "UTC", null: false + t.datetime "next_digest_at" + t.datetime "last_digest_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["next_digest_at"], name: "index_notification_preferences_on_next_digest_at" + t.index ["unit_id"], name: "index_notification_preferences_on_unit_id" + t.index ["user_id", "unit_id"], name: "index_notification_preferences_on_user_and_unit", unique: true + t.index ["user_id"], name: "index_notification_preferences_on_user_id" + t.check_constraint "json_valid(`email_categories`)", name: "email_categories" + end + + create_table "notifications", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "recipient_id", null: false + t.bigint "unit_id", null: false + t.bigint "project_id" + t.bigint "task_id" + t.bigint "actor_id" + t.string "kind", limit: 64, null: false + t.string "source_type", limit: 64 + t.bigint "source_id" + t.string "deduplication_key", limit: 191, null: false + t.text "metadata", size: :long, null: false, collation: "utf8mb4_bin" + t.datetime "read_at" + t.datetime "email_processed_at" + t.datetime "email_sent_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["actor_id"], name: "index_notifications_on_actor_id" + t.index ["project_id"], name: "index_notifications_on_project_id" + t.index ["recipient_id", "deduplication_key"], name: "index_notifications_on_recipient_and_deduplication_key", unique: true + t.index ["recipient_id", "read_at", "created_at"], name: "index_notifications_on_recipient_read_created" + t.index ["recipient_id", "task_id", "read_at"], name: "index_notifications_on_recipient_task_read" + t.index ["recipient_id", "unit_id", "email_processed_at"], name: "index_notifications_for_email_delivery" + t.index ["recipient_id"], name: "index_notifications_on_recipient_id" + t.index ["source_type", "source_id"], name: "index_notifications_on_source_type_and_source_id" + t.index ["task_id"], name: "index_notifications_on_task_id" + t.index ["unit_id"], name: "index_notifications_on_unit_id" + t.check_constraint "json_valid(`metadata`)", name: "metadata" + end + create_table "overflow_task_claim_logs", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.bigint "unit_id", null: false t.bigint "task_id", null: false @@ -1030,6 +1078,13 @@ add_foreign_key "feedback_chips", "learning_outcomes" add_foreign_key "learning_outcome_links", "learning_outcomes", column: "source_id" add_foreign_key "learning_outcome_links", "learning_outcomes", column: "target_id" + add_foreign_key "notification_preferences", "units" + add_foreign_key "notification_preferences", "users" + add_foreign_key "notifications", "projects" + add_foreign_key "notifications", "tasks" + add_foreign_key "notifications", "units" + add_foreign_key "notifications", "users", column: "actor_id" + add_foreign_key "notifications", "users", column: "recipient_id" add_foreign_key "user_oauth_states", "users" add_foreign_key "user_oauth_tokens", "users" end diff --git a/test/api/comments/status_test.rb b/test/api/comments/status_test.rb index 9c2e729b12..3d744af371 100644 --- a/test/api/comments/status_test.rb +++ b/test/api/comments/status_test.rb @@ -55,13 +55,13 @@ def test_status_comments rff_comment = task.comments.where(task_status_id: TaskStatus.ready_for_feedback.id).first te_comment = task.comments.where(task_status_id: TaskStatus.time_exceeded.id).first - # Task status generated by students is marked read by staff + # Task status generated by students is read by the author but remains unread for staff. assert rff_comment.read_by?(user), 'Error: RFF status comment should be read by the student' - assert rff_comment.read_by?(tutor), 'Error: TE status comment should be read by the tutor' + assert_not rff_comment.read_by?(tutor), 'Error: RFF status comment should remain unread for the tutor' - # Task status comments by staff is not marked read by students + # Task status comments by staff remain unread for students until comments are opened. assert te_comment.read_by?(tutor), 'Error: TE status comment should be read by the tutor' - assert te_comment.read_by?(user), 'Error: TE status comment should be read by the student' + assert_not te_comment.read_by?(user), 'Error: TE status comment should remain unread for the student' td.destroy! end diff --git a/test/api/notifications_api_test.rb b/test/api/notifications_api_test.rb new file mode 100644 index 0000000000..e2060058fd --- /dev/null +++ b/test/api/notifications_api_test.rb @@ -0,0 +1,103 @@ +require 'test_helper' + +class NotificationsApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def setup + super + @unit = FactoryBot.create(:unit, task_count: 1) + @project = @unit.active_projects.first + @task = @project.task_for_task_definition(@unit.task_definitions.first) + @student = @project.student + @tutor = @project.tutor_for(@task.task_definition) + @task.add_text_comment(@tutor, 'New feedback') + add_auth_header_for(user: @student) + end + + def test_get_returns_only_current_users_grouped_notifications + other_user = FactoryBot.create(:user) + FactoryBot.create(:notification, recipient: other_user, unit: @unit) + + get '/api/notifications' + + assert_equal 200, last_response.status + assert_equal 1, last_response_body['groups'].count + assert_equal @task.id, last_response_body.dig('groups', 0, 'task', 'id') + assert_equal 1, last_response_body['unread_count'] + end + + def test_unread_count_counts_a_task_group_instead_of_each_event + @task.add_text_comment(@tutor, 'More feedback') + @task.add_status_comment(@tutor, TaskStatus.fix_and_resubmit) + + get '/api/notifications/unread_count' + + assert_equal 200, last_response.status + assert_equal 1, last_response_body['count'] + end + + def test_get_filters_groups_by_category_and_search + get '/api/notifications', + state: 'unread', + kinds: ['feedback_left'], + query: @unit.code + + assert_equal 200, last_response.status + assert_equal 1, last_response_body['groups'].count + assert_equal({ 'feedback_left' => 1 }, last_response_body.dig('groups', 0, 'counts')) + end + + def test_mark_read_cannot_update_another_users_notification + own_notification = Notification.find_by!(recipient: @student) + other_notification = FactoryBot.create(:notification, unit: @unit) + + put_json '/api/notifications/read', + notification_ids: [own_notification.id, other_notification.id] + + assert_equal 200, last_response.status + assert_not_nil own_notification.reload.read_at + assert_nil other_notification.reload.read_at + end + + def test_mark_all_read_can_be_scoped_to_a_unit + own_notification = Notification.find_by!(recipient: @student) + other_unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + other_notification = FactoryBot.create(:notification, recipient: @student, unit: other_unit) + + put_json '/api/notifications/read_all', unit_id: @unit.id + + assert_equal 200, last_response.status + assert_not_nil own_notification.reload.read_at + assert_nil other_notification.reload.read_at + end + + def test_updating_preferences_requires_unit_access + inaccessible_unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + + put_json "/api/notification_preferences/#{inaccessible_unit.id}", + email_categories: ['feedback_left'], + email_frequency: 'daily', + email_time: '10:30', + email_weekday: 1, + timezone: 'UTC' + + assert_equal 403, last_response.status + end + + def test_updating_preferences_validates_timezone + put_json "/api/notification_preferences/#{@unit.id}", + email_categories: ['feedback_left'], + email_frequency: 'daily', + email_time: '10:30', + email_weekday: 1, + timezone: 'Not/A-Timezone' + + assert_equal 400, last_response.status + end +end diff --git a/test/factories/notifications.rb b/test/factories/notifications.rb new file mode 100644 index 0000000000..67174bb5db --- /dev/null +++ b/test/factories/notifications.rb @@ -0,0 +1,19 @@ +FactoryBot.define do + factory :notification do + recipient { create(:user) } + unit { create(:unit, with_students: false, task_count: 0) } + kind { 'feedback_left' } + sequence(:deduplication_key) { |number| "factory-notification-#{number}" } + metadata { {} } + end + + factory :notification_preference do + user + unit { create(:unit, with_students: false, task_count: 0) } + email_categories { Notification::KINDS } + email_frequency { 'weekly' } + email_time { '09:00' } + email_weekday { 1 } + timezone { 'UTC' } + end +end diff --git a/test/mailers/unit_mail_test.rb b/test/mailers/unit_mail_test.rb index 86285d28f0..f6107ce634 100644 --- a/test/mailers/unit_mail_test.rb +++ b/test/mailers/unit_mail_test.rb @@ -110,22 +110,22 @@ def test_discuss_timeout_notifications_send_emails task.update!(moved_to_discuss_at: 8.days.ago) assert_equal 1, unit.notify_discuss_timeouts! - assert_equal 1, SendDiscussTimeoutEmailJob.jobs.count + assert_equal 1, SendImmediateNotificationJob.jobs.count - approaching_job = SendDiscussTimeoutEmailJob.jobs.shift + approaching_job = SendImmediateNotificationJob.jobs.shift assert_emails 1 do - SendDiscussTimeoutEmailJob.new.perform(*approaching_job['args']) + SendImmediateNotificationJob.new.perform(*approaching_job['args']) end assert_includes ActionMailer::Base.deliveries.last.subject, 'Discussion deadline approaching' task.update!(moved_to_discuss_at: 15.days.ago) assert_equal 1, unit.notify_discuss_timeouts! - assert_equal 1, SendDiscussTimeoutEmailJob.jobs.count + assert_equal 1, SendImmediateNotificationJob.jobs.count - missed_job = SendDiscussTimeoutEmailJob.jobs.shift + missed_job = SendImmediateNotificationJob.jobs.shift assert_emails 1 do - SendDiscussTimeoutEmailJob.new.perform(*missed_job['args']) + SendImmediateNotificationJob.new.perform(*missed_job['args']) end assert_includes ActionMailer::Base.deliveries.last.subject, 'Discussion deadline missed' end diff --git a/test/models/notification_preference_test.rb b/test/models/notification_preference_test.rb new file mode 100644 index 0000000000..5458892626 --- /dev/null +++ b/test/models/notification_preference_test.rb @@ -0,0 +1,111 @@ +require 'test_helper' + +class NotificationPreferenceTest < ActiveSupport::TestCase + def test_defaults_to_monday_morning_weekly_delivery + user = FactoryBot.create(:user) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + + preference = NotificationPreference.for(user, unit) + + assert_equal 'weekly', preference.email_frequency + assert_equal '09:00', preference.email_time + assert_equal 1, preference.email_weekday + assert_includes preference.email_categories, 'tutor_note' + end + + def test_default_categories_preserve_legacy_opt_outs_only_when_first_created + user = FactoryBot.create( + :user, + receive_feedback_notifications: false, + receive_task_notifications: false + ) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + + preference = NotificationPreference.for(user, unit) + user.update!( + receive_feedback_notifications: true, + receive_task_notifications: true + ) + + assert_equal ['tutor_note'], preference.reload.email_categories + end + + def test_student_default_timezone_uses_their_project_campus + unit = FactoryBot.create(:unit, task_count: 0) + project = unit.active_projects.first + project.campus.update!(timezone: 'Australia/Perth') + + preference = NotificationPreference.for(project.student, unit) + + assert_equal 'Australia/Perth', preference.timezone + end + + def test_next_occurrence_uses_the_selected_timezone + preference = FactoryBot.build( + :notification_preference, + email_frequency: 'daily', + email_time: '09:00', + timezone: 'Australia/Melbourne' + ) + from = Time.utc(2026, 7, 29, 1, 0, 0) + + next_occurrence = preference.next_occurrence(from) + + assert_equal 9, next_occurrence.in_time_zone('Australia/Melbourne').hour + assert_operator next_occurrence, :>, from + end + + def test_daily_delivery_keeps_local_time_across_daylight_saving_transition + preference = FactoryBot.build( + :notification_preference, + email_frequency: 'daily', + email_time: '09:00', + timezone: 'Australia/Melbourne' + ) + from = Time.utc(2026, 10, 3, 13, 30, 0) + + next_occurrence = preference.next_occurrence(from).in_time_zone('Australia/Melbourne') + + assert_equal Date.new(2026, 10, 4), next_occurrence.to_date + assert_equal 9, next_occurrence.hour + assert_equal '+11:00', next_occurrence.strftime('%:z') + end + + def test_switching_delivery_off_processes_existing_pending_notifications + preference = FactoryBot.create(:notification_preference) + notification = FactoryBot.create( + :notification, + recipient: preference.user, + unit: preference.unit + ) + + preference.update!(email_frequency: 'off') + + assert_not_nil notification.reload.email_processed_at + assert_nil preference.reload.next_digest_at + end + + def test_invalid_categories_are_rejected + preference = FactoryBot.build(:notification_preference, email_categories: ['unknown']) + + assert_not preference.valid? + assert_includes preference.errors[:email_categories].join, 'unknown' + end + + def test_email_categories_must_be_an_array + preference = FactoryBot.build(:notification_preference, email_categories: 'feedback_left') + + assert_not preference.valid? + assert_includes preference.errors[:email_categories], 'must be an array' + end + + def test_email_categories_normalize_a_text_backed_json_value + preference = FactoryBot.build( + :notification_preference, + email_categories: %w[feedback_left tutor_note].to_json + ) + + assert preference.valid? + assert_equal %w[feedback_left tutor_note], preference.email_categories + end +end diff --git a/test/models/notification_test.rb b/test/models/notification_test.rb new file mode 100644 index 0000000000..14cd68f703 --- /dev/null +++ b/test/models/notification_test.rb @@ -0,0 +1,222 @@ +require 'test_helper' + +class NotificationTest < ActiveSupport::TestCase + def setup + super + @unit = FactoryBot.create(:unit, task_count: 1) + @project = @unit.active_projects.first + @task = @project.task_for_task_definition(@unit.task_definitions.first) + @student = @project.student + @tutor = @project.tutor_for(@task.task_definition) + end + + def test_tutor_feedback_creates_an_unread_student_notification + comment = @task.add_text_comment(@tutor, 'Please revise this section') + + notification = Notification.find_by(source: comment, recipient: @student) + + assert_not_nil notification + assert_equal 'feedback_left', notification.kind + assert_equal @task, notification.task + assert_nil notification.read_at + end + + def test_student_feedback_notifies_staff_but_student_status_does_not + comment = @task.add_text_comment(@student, 'Could you clarify this feedback?') + @task.add_status_comment(@student, TaskStatus.ready_for_feedback) + + assert Notification.exists?(source: comment, recipient: @tutor, kind: 'feedback_left') + assert_not Notification.exists?(recipient: @tutor, kind: 'task_status_changed') + end + + def test_staff_group_feedback_fans_out_to_each_students_corresponding_task + unit = FactoryBot.create( + :unit, + task_count: 1, + student_count: 2, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + group_sets: 1, + group_tasks: [{ idx: 0, gs: 0 }], + groups: [{ gs: 0, students: 2 }] + ) + group = unit.groups.first + tasks = group.projects.map { |project| project.task_for_task_definition(unit.task_definitions.first) } + comment = tasks.first.add_text_comment(tasks.first.project.tutor_for(tasks.first.task_definition), 'Group feedback') + group_member_status = tasks.first.add_status_comment( + tasks.second.project.student, + TaskStatus.ready_for_feedback + ) + + notifications = Notification.where(source: comment).order(:recipient_id) + + assert_equal group.projects.map(&:student).sort_by(&:id), notifications.map(&:recipient).sort_by(&:id) + assert_equal tasks.map(&:id).sort, notifications.map(&:task_id).sort + assert_not Notification.exists?(source: group_member_status) + end + + def test_only_latest_staff_status_notification_remains_unread + first_comment = @task.add_status_comment(@tutor, TaskStatus.fix_and_resubmit) + second_comment = @task.add_status_comment(@tutor, TaskStatus.complete) + + first_notification = Notification.find_by!(source: first_comment, recipient: @student) + second_notification = Notification.find_by!(source: second_comment, recipient: @student) + + assert_not_nil first_notification.read_at + assert_nil second_notification.read_at + assert_equal 'complete', second_notification.metadata['status'] + assert_equal 1, Notification.where(recipient: @student, task: @task, kind: 'task_status_changed').unread.count + end + + def test_group_builder_merges_mixed_task_activity + 3.times { |number| @task.add_text_comment(@tutor, "Feedback #{number}") } + @task.add_status_comment(@tutor, TaskStatus.fix_and_resubmit) + + groups = NotificationGroupBuilder.new(Notification.where(recipient: @student).unread).groups + + assert_equal 1, groups.count + assert_equal 3, groups.first[:counts]['feedback_left'] + assert_equal 'fix_and_resubmit', groups.first[:latest_status] + assert_includes groups.first[:summary], '3 new comments' + assert_includes groups.first[:summary], 'Fix and resubmit' + end + + def test_discuss_expiry_supersedes_warning_without_hiding_feedback + @task.add_text_comment(@tutor, 'Please review this before your discussion') + warning = @task.add_discuss_timeout_comment( + @tutor, + DiscussTimeoutComment.warning, + 'Your discussion deadline is approaching' + ) + expiry = @task.add_discuss_timeout_comment( + @tutor, + DiscussTimeoutComment.expired, + 'Your discussion deadline has passed' + ) + + warning_notification = Notification.find_by!(source: warning, recipient: @student) + expiry_notification = Notification.find_by!(source: expiry, recipient: @student) + group = NotificationGroupBuilder.new(Notification.where(recipient: @student).unread).groups.first + + assert_not_nil warning_notification.read_at + assert_nil expiry_notification.read_at + assert_equal 'critical', group[:severity] + assert_equal 1, group[:counts]['discuss_expired'] + assert_equal 1, group[:counts]['feedback_left'] + assert_includes group[:summary], 'discussion deadline missed' + end + + def test_task_tutor_note_and_student_feedback_share_a_group_with_two_actions + @task.add_text_comment(@student, 'Can you check this change?') + unit_role = @unit.unit_role_for(@tutor) + tutor_note = unit_role.add_tutor_note(@unit.main_convenor_user, 'Please follow up', @task.id) + Notification.create_for_tutor_note(tutor_note, @tutor) + + group = NotificationGroupBuilder.new(Notification.where(recipient: @tutor).unread).groups.first + + assert_equal 1, group[:counts]['feedback_left'] + assert_equal 1, group[:counts]['tutor_note'] + assert_equal [tutor_note.id], group[:tutor_note_ids] + assert_equal [Notification.find_by!(source: tutor_note).id], group[:tutor_note_notification_ids] + assert group.dig(:task, :staff_view) + assert_equal @student.name, group.dig(:task, :student_name) + end + + def test_duplicate_source_event_is_deduplicated_per_recipient + comment = @task.add_text_comment(@tutor, 'Only notify once') + attributes = { + recipient: @student, + unit: @unit, + project: @project, + task: @task, + actor: @tutor, + kind: 'feedback_left', + source: comment, + deduplication_key: 'same-event', + metadata: {} + } + + first = Notification.create_event(**attributes) + second = Notification.create_event(**attributes) + + assert_equal first, second + assert_equal 1, Notification.where(recipient: @student, deduplication_key: 'same-event').count + end + + def test_metadata_normalizes_a_text_backed_json_value + notification = FactoryBot.build(:notification, metadata: { source: 'comment' }.to_json) + + assert notification.valid? + assert_equal({ 'source' => 'comment' }, notification.metadata) + end + + def test_overseer_failure_is_created_immediately_but_email_is_held_for_the_grace_period + submission_history = FactoryBot.create(:submission_history, task: @task) + assessment = FactoryBot.create( + :overseer_assessment, + task: @task, + submission_history: submission_history, + status: :pre_queued + ) + assessment.add_assessment_comment('Automated tests failed') + + assessment.update!(status: :failed) + + grace_period = OverseerAssessment.student_notification_grace_period + notification = Notification.find_by!( + source: assessment, + recipient: @student, + kind: 'overseer_failed' + ) + assert notification.email_ready?(at: Time.current + grace_period + 1.minute) + assert_not notification.email_ready?(at: Time.current + grace_period - 1.minute) + end + + def test_destroying_a_source_removes_its_notification + comment = @task.add_text_comment(@tutor, 'Temporary feedback') + notification = Notification.find_by!(source: comment, recipient: @student) + + comment.destroy! + + assert_not Notification.exists?(notification.id) + end + + def test_unread_groups_sort_before_read_history_even_when_read_group_is_urgent + read_notification = FactoryBot.create( + :notification, + recipient: @student, + unit: @unit, + kind: 'discuss_expired', + read_at: Time.current + ) + unread_notification = FactoryBot.create( + :notification, + recipient: @student, + unit: @unit, + kind: 'feedback_left' + ) + + groups = NotificationGroupBuilder.new([read_notification, unread_notification]).groups + + assert_equal unread_notification.id, groups.first[:notification_ids].first + assert_not groups.first[:read] + end + + def test_marking_task_read_processes_email_and_marking_source_unread_reopens_in_app_only + comment = @task.add_text_comment(@tutor, 'Read me') + notification = Notification.find_by!(source: comment, recipient: @student) + + Notification.mark_task_read(@student, @task) + notification.reload + + assert_not_nil notification.read_at + assert_not_nil notification.email_processed_at + + Notification.reopen_for_source(comment, @student) + notification.reload + + assert_nil notification.read_at + assert_not_nil notification.email_processed_at + end +end diff --git a/test/sidekiq/notification_jobs_test.rb b/test/sidekiq/notification_jobs_test.rb new file mode 100644 index 0000000000..148ecfd915 --- /dev/null +++ b/test/sidekiq/notification_jobs_test.rb @@ -0,0 +1,98 @@ +require 'test_helper' + +class NotificationJobsTest < ActiveSupport::TestCase + def test_digest_sends_only_enabled_unread_events + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + user = FactoryBot.create(:user) + preference = FactoryBot.create( + :notification_preference, + user: user, + unit: unit, + email_categories: ['feedback_left'], + next_digest_at: 1.minute.ago + ) + enabled = FactoryBot.create(:notification, recipient: user, unit: unit, kind: 'feedback_left') + disabled = FactoryBot.create(:notification, recipient: user, unit: unit, kind: 'pdf_generation_failed') + + assert_emails 1 do + SendNotificationDigestJob.new.perform(preference.id) + end + + digest_html = ActionMailer::Base.deliveries.last.html_part.body.to_s + assert_equal 1, digest_html.scan('