diff --git a/.ci-setup/crontab b/.ci-setup/crontab index b1298d5e39..a150f953b5 100644 --- a/.ci-setup/crontab +++ b/.ci-setup/crontab @@ -6,5 +6,6 @@ PATH=/tmp/texlive/bin/x86_64-linux:/tmp/texlive/bin/aarch64-linux:/usr/local/bun 0,10,20,30,40,50 * * * * /doubtfire/lib/shell/send_overseer_notifications.sh 0 5 * * * /doubtfire/lib/shell/check_plagiarism.sh 0 8 * * * /doubtfire/lib/shell/portfolio_autogen_check.sh -0 7 * * 1 /doubtfire/lib/shell/send_weekly_emails.sh +# Deprecated: notification digests replace the weekly progress summary. +# 0 7 * * 1 /doubtfire/lib/shell/send_weekly_emails.sh 0 1 * * * /doubtfire/lib/shell/sync_enrolments.sh diff --git a/app/api/api_root.rb b/app/api/api_root.rb index 5f4a659237..88cc484e17 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -97,6 +97,7 @@ class ApiRoot < Grape::API mount UnitContentsApi mount UnitsApi mount TutorNotesApi + mount NotificationsApi mount D2lIntegrationApi::D2lApi mount D2lIntegrationApi::OauthPublicApi @@ -161,6 +162,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/entities/notification_setting_entity.rb b/app/api/entities/notification_setting_entity.rb new file mode 100644 index 0000000000..baa18d3551 --- /dev/null +++ b/app/api/entities/notification_setting_entity.rb @@ -0,0 +1,19 @@ +module Entities + class NotificationSettingEntity < Grape::Entity + expose :id + expose :channels + expose :digest_frequency + expose :digest_interval_hours + expose :digest_start_time + expose :digest_time + expose :digest_timezone do |settings, _options| + settings.resolved_digest_timezone + end + expose :digest_weekday + expose :next_digest_at + expose :last_digest_at + expose :units, using: NotificationUnitOverrideEntity do |settings| + settings.user.notification_unit_overrides.order(:unit_id) + end + end +end diff --git a/app/api/entities/notification_unit_override_entity.rb b/app/api/entities/notification_unit_override_entity.rb new file mode 100644 index 0000000000..9a19e559ad --- /dev/null +++ b/app/api/entities/notification_unit_override_entity.rb @@ -0,0 +1,7 @@ +module Entities + class NotificationUnitOverrideEntity < Grape::Entity + expose :unit_id + expose :muted + expose :channels + end +end diff --git a/app/api/entities/tutor_note_entity.rb b/app/api/entities/tutor_note_entity.rb index 1184b01d2a..074cd155b5 100644 --- a/app/api/entities/tutor_note_entity.rb +++ b/app/api/entities/tutor_note_entity.rb @@ -17,5 +17,8 @@ class TutorNoteEntity < Grape::Entity expose :read_by_unit_role + expose :requires_current_user_read do |tutor_note, options| + tutor_note.requires_read_by?(options[:user]) + end end end diff --git a/app/api/notifications_api.rb b/app/api/notifications_api.rb new file mode 100644 index 0000000000..23447feb76 --- /dev/null +++ b/app/api/notifications_api.rb @@ -0,0 +1,200 @@ +# frozen_string_literal: true + +require 'grape' + +class NotificationsApi < Grape::API + helpers AuthenticationHelpers + + before do + authenticated? + end + + helpers do + def notification_settings + @notification_settings ||= NotificationSetting.for(current_user) + end + + def notification_scope + current_user + .received_notifications + .includes(:recipient, :actor, :unit, { project: :campus }, task: [:task_definition, { project: :user }]) + end + + # Kinds the user has switched off in the app stay in the ledger for the + # digest, so they are dropped here rather than never recorded. + def shown_in_app(notifications) + notifications.select { |notification| notification_settings.shows_in_app?(notification.unit_id, notification.kind) } + end + + def unread_group_keys_by_unit + @unread_group_keys_by_unit ||= current_user.received_notifications.unread.pluck( + :id, :task_id, :project_id, :unit_id, :kind, :unit_role_id, :email_sent_at + ).filter_map do |id, task_id, project_id, unit_id, kind, unit_role_id, email_sent_at| + next unless notification_settings.shows_in_app?(unit_id, kind) + + key = + if Notification::MODERATION_KINDS.include?(kind) + "tutor-notes:#{unit_role_id}:#{task_id}" + elsif kind == 'feedback_warning' + batch = email_sent_at ? "emailed:#{email_sent_at.to_f}" : 'not-emailed' + "unit:#{unit_id}:feedback-warning:#{batch}" + elsif kind == 'weekly_summary' + "weekly-summary:#{id}" + elsif task_id.present? + "task:#{task_id}" + elsif Notification::COMMUNICATION_KINDS.include?(kind) + "communication-email:#{id}" + elsif Notification::PORTFOLIO_KINDS.include?(kind) + "portfolio:#{project_id}" + else + "unit:#{unit_id}:#{kind}" + end + + [unit_id, key] + end.uniq + end + + def unread_group_count + unread_group_keys_by_unit.count + end + + def unread_group_counts_by_unit + unread_group_keys_by_unit.each_with_object(Hash.new(0)) do |(unit_id, _key), counts| + counts[unit_id] += 1 + end + end + + # The units sent are the whole set that departs from the settings, so any unit + # missing from it has been reset and no longer needs an override. + def replace_unit_overrides(units) + accessible = accessible_unit_ids + wanted = Array(units).select { |unit| accessible.include?(unit[:unit_id]) } + + current_user.notification_unit_overrides.where.not(unit_id: wanted.map { |unit| unit[:unit_id] }).destroy_all + wanted.each do |unit| + override = current_user.notification_unit_overrides.find_or_initialize_by(unit_id: unit[:unit_id]) + override.update!(muted: unit[:muted], channels: unit[:channels]) + end + 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) + + Unit.where(id: project_units).or(Unit.where(id: role_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(shown_in_app(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), + group[:message_subject], + group[:message_body], + group[:weekly_summary]&.to_json + ].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, + unread_counts_by_unit: unread_group_counts_by_unit + } + end + + desc 'Get the grouped unread notification count for the current user' + get '/notifications/unread_count' do + { count: unread_group_count, unread_counts_by_unit: unread_group_counts_by_unit } + 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 = 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 = Notification.mark_read(scope) + { count: count } + end + + desc 'Get the notification settings for the current user' + get '/notification_settings' do + present NotificationSetting.for(current_user), with: Entities::NotificationSettingEntity + end + + desc 'Update the notification settings for the current user' + params do + optional :channels, type: Hash + optional :digest_frequency, type: String, values: NotificationSetting::FREQUENCIES + optional :digest_interval_hours, type: Integer, values: NotificationSetting::DIGEST_INTERVAL_HOURS + optional :digest_start_time, type: String + optional :digest_time, type: String + optional :digest_weekday, type: Integer + optional :units, type: Array do + requires :unit_id, type: Integer + requires :muted, type: Boolean + optional :channels, type: Hash + end + end + put '/notification_settings' do + settings = NotificationSetting.for(current_user) + changes = declared(params, include_missing: false) + + NotificationSetting.transaction do + settings.update!(changes.except(:units)) + replace_unit_overrides(changes[:units]) if changes.key?(:units) + end + + present settings, with: Entities::NotificationSettingEntity + end +end diff --git a/app/api/task_comments_api.rb b/app/api/task_comments_api.rb index cfe2500a87..a1cbda50f4 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_from_comment(task_comment, current_user) SessionTracker.record_assessment_activity( action: 'mark-comment-unread', diff --git a/app/api/tutor_notes_api.rb b/app/api/tutor_notes_api.rb index 83c99945ce..6fb7e54f3d 100644 --- a/app/api/tutor_notes_api.rb +++ b/app/api/tutor_notes_api.rb @@ -37,7 +37,7 @@ def can_access_tutor_notes?(unit, current_user, unit_role) error!({ error: 'You do not have permission to access this.' }, 403) end - result = unit_role.tutor_notes + result = unit_role.tutor_notes.includes(:notifications) present result, with: Entities::TutorNoteEntity, user: current_user end @@ -60,13 +60,16 @@ def can_access_tutor_notes?(unit, current_user, unit_role) tutor_note = unit_role.tutor_notes.find(params[:id]) - current_unit_role = unit.unit_role_for(current_user) + note_is_about_me = unit.unit_role_for(current_user) == unit_role - unless current_unit_role == unit_role && unit_role == tutor_note.unit_role + unless note_is_about_me || tutor_note.notification_for(current_user).present? error!({ error: 'You do not have permission to update this note.' }, 403) end - tutor_note.update!(read_by_unit_role: true) + TutorNote.transaction do + tutor_note.update!(read_by_unit_role: true) if note_is_about_me + Notification.mark_tutor_note_read(current_user, tutor_note) + end true end @@ -113,18 +116,21 @@ def can_access_tutor_notes?(unit, current_user, unit_role) reply_target = original_staff_note && unit.unit_role_for(original_staff_note.user) - notify_unit_role = + notify_unit_role, notification_kind = if reply_target && original_staff_note.user != current_user - reply_target # tutor is responding to a reply -> notify original user that tutor is replying to + # tutor is responding to a reply -> notify original user that tutor is replying to + [reply_target, 'moderation_note_reply'] elsif current_unit_role == unit_role - unit_role.mentor # tutor is writing on their own notes -> notify the mentor + # tutor is writing on their own notes -> notify the mentor + [unit_role.mentor, 'moderation_note_from_mentee'] else - unit_role # anyone else wrote about this tutor, whether its their mentor or another convenor -> notify tutor + # anyone else wrote about this tutor, whether its their mentor or another convenor -> notify tutor + [unit_role, 'moderation_note_added'] end if result.present? && notify_unit_role.present? && notify_unit_role.user_id != current_user.id begin - NotifyTutorNotesJob.perform_async(result.id, notify_unit_role.user.id) + NotifyTutorNotesJob.perform_async(result.id, notify_unit_role.user.id, notification_kind) rescue StandardError => e Rails.logger.error("Failed to send tutor note email for TutorNote #{result.id}: #{e.class} - #{e.message}") end diff --git a/app/mailers/notifications_mailer.rb b/app/mailers/notifications_mailer.rb index f4b0d255ad..f81c930af8 100644 --- a/app/mailers/notifications_mailer.rb +++ b/app/mailers/notifications_mailer.rb @@ -1,86 +1,65 @@ class NotificationsMailer < ApplicationMailer + TASK_DEADLINE_SECTIONS = [ + ['task_start_now', 'Tasks ready to start'], + ['task_due_soon', 'Tasks due within five days'], + ['task_overdue', 'Tasks past due'] + ].freeze + layout 'discussion_deadline_mailer', only: %i[discussion_deadline_approaching discussion_deadline_missed] def add_general @doubtfire_host = Doubtfire::Application.config.institution[:host] @doubtfire_product_name = Doubtfire::Application.config.institution[:product_name] - @unsubscribe_url = "#{@doubtfire_host}/edit_profile" + @unsubscribe_url = "#{@doubtfire_host}/notifications/settings" end - def weekly_staff_summary(unit_role, summary_stats) - return nil if unit_role.nil? + def notification_digest(recipient, notifications) + return nil if recipient.nil? || notifications.blank? add_general + @recipient = recipient + @notification_url = "#{@doubtfire_host}/notifications" + @units = notifications.group_by(&:unit).sort_by { |unit, _| unit.code }.map do |unit, for_unit| + groups = NotificationGroupBuilder.new(for_unit).groups + add_task_deadline_due_dates!(groups, for_unit) + deadline_groups = TASK_DEADLINE_SECTIONS.map do |kind, heading| + { heading: heading, kind: kind, groups: groups.select { |group| group[:counts][kind].positive? } } + end + deadline_groups.select! { |section| section[:groups].any? } + grouped_deadline_notifications = deadline_groups.flat_map { |section| section[:groups] } + other_groups = groups.reject { |group| grouped_deadline_notifications.include?(group) } + group_sections = deadline_groups.dup + if other_groups.any? + group_sections << { + heading: deadline_groups.any? ? 'Task updates' : nil, + divider: deadline_groups.any?, + kind: nil, + groups: other_groups + } + end + + { unit: unit, groups: groups, group_sections: group_sections } + end + # Grouped events, so the count matches the rows the reader can see below. + @notification_count = @units.sum { |section| section[:groups].count } - @staff = unit_role.user - @unit_role = unit_role - @unit = summary_stats[:unit] - - @received_comments = @unit.comments - .where("task_comments.recipient_id = :uid AND task_comments.created_at > :start", uid: @staff.id, start: 7.days.ago) - .where(content_type: [:text, :assessment, :audio, :image, :pdf, :discussion, :extension]) - .count - - @sent_comments = @unit.comments - .where("task_comments.user_id = :uid AND task_comments.created_at > :start", uid: @staff.id, start: 7.days.ago) - .where(content_type: [:text, :assessment, :audio, :image, :pdf, :discussion, :extension]) - .count - - @data = { - sent_comments: @sent_comments, # Sent by tutor - received_comments: @received_comments, # Received by tutor - tasks_awaiting_feedback_count: summary_stats[:staff][unit_role.user][:tasks_awaiting_feedback_count], # For the tutor - weekly_engagements_count: summary_stats[:staff][unit_role.user][:weekly_engagements_count], # Engagements from the student? - staff_engagements: summary_stats[:staff][unit_role.user][:staff_engagements], # Engagements by the tutor - oldest_task_days: summary_stats[:staff][unit_role.user][:oldest_task_days], - weekly_total_tasks_discussed: summary_stats[:staff][unit_role.user][:weekly_total_tasks_discussed] # Total for the tutor for the week for the tutor - } - - @convenor = @unit.main_convenor_user - @summary_stats = summary_stats - - email_with_name = %("#{@staff.name}" <#{@staff.email}>) - convenor_email = %("#{@convenor.name}" <#{@convenor.email}>) - subject = "#{@unit.name}: Weekly Summary" - - mail(to: email_with_name, from: convenor_email, subject: subject) + subject = "#{@notification_count} new #{'notification'.pluralize(@notification_count)}" + mail( + to: %("#{@recipient.name}" <#{@recipient.email}>), + from: %("#{@doubtfire_product_name}" ), + subject: subject + ) end - def weekly_student_summary(project, summary_stats, did_revert_to_pass) - return nil if project.nil? - - add_general - - @student = project.student - @project = project - @tutor = project.main_convenor_user - @summary_stats = summary_stats - @did_revert_to_pass = did_revert_to_pass - - @engagements = @project.task_engagements.where("task_engagements.engagement_time >= :start AND task_engagements.engagement_time < :end", start: summary_stats[:week_start], end: summary_stats[:week_end]) - - @engagements_count = @engagements.count - - @student_engagements = @engagements.select { |e| [TaskStatus.not_started.name, TaskStatus.need_help.name, TaskStatus.working_on_it.name, TaskStatus.ready_for_feedback.name].include? e.engagement }.count - - @staff_engagements = @engagements.select { |e| [TaskStatus.complete.name, TaskStatus.feedback_exceeded.name, TaskStatus.redo.name, TaskStatus.discuss.name, TaskStatus.rediscuss.name, TaskStatus.attention_required.name, TaskStatus.demonstrate.name, TaskStatus.fail.name].include? e.engagement }.count - - @task_states = project.tasks.joins(:task_status).select("count(tasks.id) as number, task_statuses.name as status").group("task_statuses.name") - - @received_comments = project.comments.where("recipient_id = :student_id AND task_comments.created_at > :start", student_id: @student.id, start: Time.zone.now - 7.days).count - @sent_comments = project.comments.where("user_id = :student_id AND task_comments.created_at > :start", student_id: @student.id, start: Time.zone.now - 7.days).count - - @top_tasks = project.top_tasks - @overdue_top = @top_tasks.select { |tt| tt[:reason] == :overdue } - @soon_top = @top_tasks.select { |tt| tt[:reason] == :soon } - @ahead_top = @top_tasks.select { |tt| tt[:reason] == :ahead } - - email_with_name = %("#{@student.name}" <#{@student.email}>) - tutor_email = %("#{@tutor.name}" <#{@tutor.email}>) - subject = "#{project.unit.name}: Weekly Summary" + def notification_timestamp(group) + time = group[:latest_at].in_time_zone(group[:timezone]) + "#{time.day.ordinalize} #{time.strftime('%B %Y at %H:%M')}" + end - mail(to: email_with_name, from: tutor_email, subject: subject) + def notification_due_date(group) + due_date = group.dig(:task, :due_date) + "Due #{due_date.day.ordinalize} #{due_date.strftime('%B')}" end def discussion_deadline_approaching(task, sender, expiry_date) @@ -104,40 +83,30 @@ def discussion_deadline_missed(task, sender) ) end - def top_task_desc(tt) - "#{tt[:task_definition].abbreviation} - #{tt[:task_definition].name} #{"- which you need to discuss with your tutor" if tt[:status] == :discuss}" - end + helper_method :notification_timestamp + helper_method :notification_due_date + helper_method :weekly_summary_period - def were_was(num) - if num == 1 - "was" - else - "were" - end - end + private - def are_is(num) - if num == 1 - "is" - else - "are" - end + def weekly_summary_period(summary) + from = Time.zone.parse(summary.fetch('week_start')) + to = Time.zone.parse(summary.fetch('week_end')) + "#{from.day.ordinalize} #{from.strftime('%B')} to #{to.day.ordinalize} #{to.strftime('%B %Y')}" end - def this_these(num) - if num == 1 - "this" - else - "these" - end - end + def add_task_deadline_due_dates!(groups, notifications) + due_dates = notifications.filter_map do |notification| + next unless Notification::TASK_DEADLINE_KINDS.include?(notification.kind) && notification.task.present? - helper_method :top_task_desc - helper_method :were_was - helper_method :are_is - helper_method :this_these + [notification.task_id, notification.task.local_due_date.to_date] + end.to_h - private + groups.each do |group| + task = group[:task] + task[:due_date] = due_dates[task[:id]] if task.present? && due_dates.key?(task[:id]) + end + end def add_discussion_deadline_details(task, sender) add_general diff --git a/app/mailers/portfolio_evidence_mailer.rb b/app/mailers/portfolio_evidence_mailer.rb index 743503c25b..3127bad53e 100644 --- a/app/mailers/portfolio_evidence_mailer.rb +++ b/app/mailers/portfolio_evidence_mailer.rb @@ -2,11 +2,12 @@ class PortfolioEvidenceMailer < ApplicationMailer def add_general @doubtfire_host = Doubtfire::Application.config.institution[:host] @doubtfire_product_name = Doubtfire::Application.config.institution[:product_name] - @unsubscribe_url = "#{@doubtfire_host}/edit_profile" + @unsubscribe_url = "#{@doubtfire_host}/notifications/settings" end def task_pdf_failed(project, tasks) return nil if project.nil? || tasks.nil? || tasks.empty? + return nil unless notification_email_enabled?(project, 'pdf_generation_failed') add_general @student = project.student @@ -37,25 +38,9 @@ def task_pdf_ready_message(project, tasks) mail(to: email_with_name, from: tutor_email, subject: subject) end - def task_feedback_ready(project, tasks) - return nil if project.nil? || tasks.nil? || tasks.empty? - - add_general - @student = project.student - @project = project - @tasks = tasks.sort_by { |t| t.task_definition.abbreviation } - @tutor = project.main_convenor_user - @has_comments = !@tasks.select { |t| t.is_last_comment_by?(@tutor) }.empty? - return nil if @tutor.nil? || @student.nil? - - email_with_name = %("#{@student.name}" <#{@student.email}>) - tutor_email = %("#{@tutor.name}" <#{@tutor.email}>) - subject = "#{project.unit.name}: Feedback ready to review" - mail(to: email_with_name, from: tutor_email, subject: subject) - end - def overseer_assessment_failed(project, tasks) return nil if project.nil? || tasks.nil? || tasks.empty? + return nil unless notification_email_enabled?(project, 'overseer_failed') add_general @student = project.student @@ -99,4 +84,10 @@ def portfolio_failed(project) subject = "#{project.unit.name}: Portfolio failed to compile" mail(to: email_with_name, from: convenor_email, subject: subject) end + + private + + def notification_email_enabled?(project, kind) + NotificationSetting.for(project.student).delivers?(project.unit, kind, :email) + end end diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index 555dbe10a8..cb4dd9cd8e 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -18,6 +18,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, dependent: :destroy has_many :comment_read_cursors, foreign_key: :last_read_comment_id, inverse_of: :last_read_comment, @@ -41,6 +42,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 :rewind_comment_read_cursors, prepend: true @@ -179,13 +183,24 @@ def new_for?(user) requires_attention_for?(user) && !read_by?(user) end - def read_by?(user) - return true if self.user == user || !requires_attention_for?(user) + # Has this user's read cursor for the task moved past this comment? + # + # Unlike #read_by? this ignores attention_audience, so comments that never sit + # unread in the comment inbox - status changes and other automated comments - + # still report whether the user has actually seen them. + def seen_by?(user) + return true if self.user == user cursor = CommentReadCursor.find_by(task_id: task_id, user_id: user.id) cursor.present? && cursor.last_read_comment_id >= id end + def read_by?(user) + return true unless requires_attention_for?(user) + + seen_by?(user) + end + def time_read_by(user) return nil unless requires_attention_for?(user) diff --git a/app/models/notification.rb b/app/models/notification.rb new file mode 100644 index 0000000000..8135263f9f --- /dev/null +++ b/app/models/notification.rb @@ -0,0 +1,570 @@ +# frozen_string_literal: true + +require 'set' + +class Notification < ApplicationRecord + KINDS = %w[ + new_task_comment + task_status_changed + task_start_now + task_due_soon + task_overdue + feedback_warning + weekly_summary + overseer_failed + pdf_generation_failed + discuss_warning + discuss_expired + moderation_note_added + moderation_note_reply + moderation_note_from_mentee + portfolio_ready + portfolio_failed + communication_email + ].freeze + + CHANNELS = %w[in_app email push].freeze + + MODERATION_KINDS = %w[moderation_note_added moderation_note_reply moderation_note_from_mentee].freeze + PORTFOLIO_KINDS = %w[portfolio_ready portfolio_failed].freeze + COMMUNICATION_KINDS = %w[communication_email].freeze + TASK_DEADLINE_KINDS = %w[task_start_now task_due_soon task_overdue].freeze + FEEDBACK_WARNING_KINDS = %w[feedback_warning].freeze + + 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 + + # What raised the notification. + belongs_to :task_comment, optional: true + belongs_to :overseer_assessment, optional: true + belongs_to :tutor_note, optional: true + + # Extras, set only by the kinds that have them. + belongs_to :task_status, optional: true + belongs_to :unit_role, optional: true + + validates :kind, inclusion: { in: KINDS } + validates :deduplication_key, presence: true, uniqueness: { scope: :recipient_id } + + scope :unread, -> { where(read_at: nil) } + scope :recently_read, -> { where(read_at: 30.days.ago..) } + scope :email_pending, -> { unread.where(email_processed_at: nil) } + scope :email_ready, ->(at = Time.current) { where(email_not_before: [nil, ..at]) } + + 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| + # Use the read cursor rather than read_by?, which reports "read" for any + # comment that does not require the user's attention. Status changes and + # other attention_audience :none comments still need to raise notifications. + next if comment.seen_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 + + create_event( + recipient: recipient, + unit: recipient_task.unit, + project: recipient_task.project, + task: recipient_task, + actor: comment.user, + kind: kind, + task_comment: comment, + deduplication_key: "task-comment:#{comment.id}:#{kind}", + task_status: comment.is_a?(TaskStatusComment) ? comment.task_status : nil, + discuss_deadline: discuss_deadline_for(comment, recipient_task) + ) + 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(overseer_assessment: assessment).unread + ) + next if assessment_comment.seen_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', + overseer_assessment: assessment, + deduplication_key: "overseer-assessment:#{assessment.id}:failed", + email_not_before: assessment.updated_at + OverseerAssessment.student_notification_grace_period + ) + 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', + deduplication_key: "pdf-generation:#{task.id}:#{version}:failed" + ) + end + end + + def self.create_for_tutor_note(tutor_note, recipient, kind) + create_event( + recipient: recipient, + unit: tutor_note.unit_role.unit, + project: tutor_note.task&.project, + task: tutor_note.task, + actor: tutor_note.user, + kind: kind, + tutor_note: tutor_note, + unit_role: tutor_note.unit_role, + deduplication_key: "tutor-note:#{tutor_note.id}" + ) + end + + def self.create_for_portfolio(project, success:) + recipient = project.student + kind = success ? 'portfolio_ready' : 'portfolio_failed' + notification = create_event( + recipient: recipient, + unit: project.unit, + project: project, + actor: project.main_convenor_user, + kind: kind, + deduplication_key: "portfolio:#{project.id}:#{project.updated_at.to_f}:#{kind}" + ) + return if notification.nil? + + mark_read( + where(recipient: recipient, project: project, kind: PORTFOLIO_KINDS) + .where.not(id: notification.id) + .unread + ) + notification + end + + # The communications system has already delivered this message as an email. + # Record an in-app copy without passing it through the notification email + # channel, which would send the recipient the same message twice. + def self.create_for_communication_email( + recipient:, + unit:, + project:, + actor:, + subject:, + body:, + deduplication_key: + ) + create_event( + recipient: recipient, + unit: unit, + project: project, + actor: actor, + kind: 'communication_email', + message_subject: subject, + message_body: body, + deduplication_key: deduplication_key, + channels: ['in_app'] + ) + end + + def self.create_weekly_summary(recipient:, unit:, data:, project: nil) + week_start = Time.zone.parse(data.fetch(:week_start).to_s).to_date.iso8601 + audience = data.fetch(:audience) + create_event( + recipient: recipient, + unit: unit, + project: project, + actor: unit.main_convenor_user, + kind: 'weekly_summary', + message_subject: "#{unit.code}: Weekly summary", + message_body: JSON.generate(data), + deduplication_key: "weekly-summary:#{unit.id}:#{audience}:#{week_start}" + ) + end + + def self.create_event(**attributes) + recipient = attributes.fetch(:recipient) + unit = attributes.fetch(:unit) + kind = attributes.fetch(:kind) + deduplication_key = attributes.fetch(:deduplication_key) + return if withdrawn_student?(attributes[:project], recipient) + + settings = NotificationSetting.for(recipient) + channels = attributes[:channels] || settings.channels_for_unit_id(unit.id, kind) + return if channels.empty? + + 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: kind, + task_comment: attributes[:task_comment], + overseer_assessment: attributes[:overseer_assessment], + tutor_note: attributes[:tutor_note], + task_status: attributes[:task_status], + unit_role: attributes[:unit_role], + discuss_deadline: attributes[:discuss_deadline], + email_not_before: attributes[:email_not_before], + message_subject: attributes[:message_subject], + message_body: attributes[:message_body] + ) + notification.save! + + unless channels.include?('email') + notification.update!(email_processed_at: Time.current) + end + + notification + rescue ActiveRecord::RecordNotUnique + find_by(recipient: recipient, deduplication_key: deduplication_key) + end + + def self.refresh_task_deadline_notifications!(now: Time.current, started_at: nil) + stale = where(kind: TASK_DEADLINE_KINDS).unread.includes(task: [:task_definition, { project: %i[unit campus user] }]) + stale.find_each do |notification| + mark_read(where(id: notification.id)) unless notification.current_task_deadline?(now: now) + end + + created = 0 + each_task_deadline_candidate(now: now) do |task| + kind = task_deadline_kind(task, now: now) + next if kind.nil? + # Already in this state when deadline notifications rolled out, so there is nothing new to announce. + next if started_at && task_deadline_kind(task, now: started_at) == kind + + notification = create_event( + recipient: task.student, + unit: task.unit, + project: task.project, + task: task, + kind: kind, + deduplication_key: task_deadline_key(task, kind) + ) + created += 1 if notification&.previously_new_record? + end + created + end + + def self.refresh_feedback_warning_notifications!(now: Time.current, started_at: nil) + stale = where(kind: FEEDBACK_WARNING_KINDS).unread.includes( + :recipient, + task: [ + :task_definition, + { + project: [ + :campus, + { tutorial_enrolments: { tutorial: { unit_role: :user } } }, + { unit: [{ teaching_period: :breaks }, { main_convenor: :user }] } + ] + } + ] + ) + stale.find_each do |notification| + mark_read(where(id: notification.id)) unless notification.current_feedback_warning?(now: now) + end + + candidates = feedback_warning_candidates(now: now) + existing = where(kind: FEEDBACK_WARNING_KINDS, task_id: candidates.select(:id)) + .pluck(:recipient_id, :deduplication_key) + .to_set + + created = 0 + candidates.find_each do |task| + next unless feedback_warning_eligible?(task, now: now, started_at: started_at) + + recipient = feedback_warning_recipient(task) + next if recipient.nil? + + deduplication_key = feedback_warning_key(task) + next if existing.include?([recipient.id, deduplication_key]) + + notification = create_event( + recipient: recipient, + unit: task.unit, + project: task.project, + task: task, + kind: 'feedback_warning', + deduplication_key: deduplication_key + ) + created += 1 if notification&.previously_new_record? + end + created + end + + def self.task_deadline_kind(task, now: Time.current) + return nil unless task_deadline_eligible?(task, now: now) + + today = now.in_time_zone(task.project.campus&.timezone.presence || Time.zone.name).to_date + start_date = task.local_start_date.to_date + due_date = task.local_due_date.to_date + + return 'task_overdue' if today > due_date + return 'task_due_soon' if today >= due_date - 5.days + return 'task_start_now' if today >= start_date + + nil + end + + def self.task_deadline_key(task, kind) + date = kind == 'task_start_now' ? task.local_start_date : task.local_due_date + "task-deadline:#{task.id}:#{kind}:#{date.to_date.iso8601}" + end + + def self.feedback_warning_key(task) + "feedback-warning:#{task.id}:#{task.submission_date.utc.iso8601(6)}" + end + + def self.feedback_warning_status_id + @feedback_warning_status_id ||= TaskStatus.ready_for_feedback.id + end + + # Moderation notifications stay unread until their recipient marks the tutor note itself as read. + def self.mark_read(relation, at: Time.current, include_moderation: false) + relation = relation.where.not(kind: MODERATION_KINDS) unless include_moderation + + # 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.mark_tutor_note_read(recipient, tutor_note) + mark_read( + where(recipient: recipient, tutor_note: tutor_note).unread, + include_moderation: true + ) + end + + # Marking a comment unread rewinds the task's read cursor to the comment before + # it, so every later comment on that task becomes unread too. Reopen all of + # their notifications so the notification list agrees with the comment view. + def self.reopen_from_comment(comment, recipient) + later_comment_ids = TaskComment + .where(task_id: comment.task_id) + .where(id: comment.id..) + .select(:id) + + # Keep email_processed_at unchanged so manually reopening a comment never resends email. + # rubocop:disable Rails/SkipsModelValidations + where(recipient: recipient, task_comment_id: later_comment_ids) + .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 + + # A student who is no longer enrolled must not be notified about their old project. + def self.withdrawn_student?(project, recipient) + project.present? && !project.enrolled && project.user_id == recipient.id + end + + def recipient_withdrawn? + self.class.withdrawn_student?(project, recipient) + end + + def weekly_summary_data + return unless kind == 'weekly_summary' && message_body.present? + + JSON.parse(message_body) + rescue JSON::ParserError + nil + end + + def email_ready?(at: Time.current) + email_not_before.blank? || email_not_before <= at + end + + def current_task_deadline?(now: Time.current) + return true unless TASK_DEADLINE_KINDS.include?(kind) + return false if task.nil? + + current_kind = self.class.task_deadline_kind(task, now: now) + current_kind == kind && deduplication_key == self.class.task_deadline_key(task, kind) + end + + def current_feedback_warning?(now: Time.current) + return true unless FEEDBACK_WARNING_KINDS.include?(kind) + return false if task.nil? + + self.class.feedback_warning_eligible?(task, now: now) && + self.class.feedback_warning_recipient(task) == recipient && + deduplication_key == self.class.feedback_warning_key(task) + end + + def current_for_delivery?(now: Time.current) + current_task_deadline?(now: now) && current_feedback_warning?(now: now) + 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 + # Automated bookkeeping comments - plan changes, check-ins, discussed-in-class, + # feedback review requests - are attention_audience :none and raise no + # notification. Status comments are :none as well, but they do notify and are + # handled by the branch above. + return nil if comment.attention_none? + + 'new_task_comment' + end + end + + def self.discuss_deadline_for(comment, recipient_task) + return nil unless comment.content_type == DiscussTimeoutComment.warning + + recipient_task.unit.discuss_timeout_expiry_date(recipient_task) + 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 self.each_task_deadline_candidate(now:) + Unit.set_active.current_for_date(now).where(send_notifications: true).find_each do |unit| + unit.active_projects.includes(:user, :campus).find_each do |project| + project.assigned_task_defs.find_each do |task_definition| + yield project.task_for_task_definition(task_definition) + end + end + end + end + + def self.feedback_warning_recipient(task) + tutorial_enrolment = task.project.tutorial_enrolments.find do |enrolment| + tutorial_stream_id = enrolment.tutorial.tutorial_stream_id + tutorial_stream_id.nil? || tutorial_stream_id == task.task_definition.tutorial_stream_id + end + + tutorial_enrolment&.tutorial&.unit_role&.user || task.unit.main_convenor_user + end + + def self.feedback_warning_candidates(now:) + Task + .joins(:task_definition, project: :unit) + .where(projects: { enrolled: true }) + .where(units: { active: true, send_notifications: true }) + .where('units.start_date <= :now AND units.end_date >= :now', now: now) + .where(task_status_id: feedback_warning_status_id) + .where.not(submission_date: nil) + .where( + 'TIMESTAMPDIFF(DAY, tasks.submission_date, :now) >= units.feedback_warning_threshold_days', + now: now + ) + .where('projects.target_grade >= task_definitions.target_grade') + .preload( + :task_definition, + project: [ + :campus, + { tutorial_enrolments: { tutorial: { unit_role: :user } } }, + { unit: [{ teaching_period: :breaks }, { main_convenor: :user }] } + ] + ) + end + + def self.task_deadline_eligible?(task, now:) + task.project.enrolled && + task.unit.active && + task.unit.send_notifications && + task.unit.start_date <= now && + task.unit.end_date >= now && + task.task_definition.target_grade <= task.project.target_grade && + task.submission_date.nil? && + !task.submitted_status? + end + + def self.feedback_warning_eligible?(task, now:, started_at: nil) + eligible = + task.project.enrolled && + task.unit.active && + task.unit.send_notifications && + task.unit.start_date <= now && + task.unit.end_date >= now && + task.task_definition.target_grade <= task.project.target_grade && + task.task_status_id == feedback_warning_status_id && + task.submission_date.present? && + task.days_awaiting_feedback(now) >= task.unit.feedback_warning_threshold_days + return false unless eligible + return true if started_at.nil? || task.submission_date > started_at + + task.days_awaiting_feedback(started_at) < task.unit.feedback_warning_threshold_days + end + + private_class_method :kind_for_comment, :discuss_deadline_for, :recipients_for_comment, :student_actor?, + :each_task_deadline_candidate, :task_deadline_eligible?, :feedback_warning_candidates +end diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb new file mode 100644 index 0000000000..21f080d497 --- /dev/null +++ b/app/models/notification_setting.rb @@ -0,0 +1,195 @@ +# frozen_string_literal: true + +# A user's notification settings: the one digest schedule they choose, and the +# channel defaults every unit follows until it is customised. +class NotificationSetting < ApplicationRecord + FREQUENCIES = %w[off hourly daily weekly].freeze + DIGEST_INTERVAL_HOURS = [1, 2, 3, 4, 6, 12].freeze + TIME_FORMAT = /\A(?:[01]\d|2[0-3]):[0-5]\d\z/ + + attribute :channels, :json + + belongs_to :user + + validates :channels, notification_channels: true + validates :digest_frequency, inclusion: { in: FREQUENCIES } + validates :digest_interval_hours, inclusion: { in: DIGEST_INTERVAL_HOURS } + validates :digest_start_time, format: { with: TIME_FORMAT } + validates :digest_time, format: { with: TIME_FORMAT } + validates :digest_weekday, inclusion: { in: 1..7 } + validate :digest_timezone_supported + + before_validation :apply_defaults + before_save :refresh_next_digest_at, if: :schedule_changed? + after_commit :process_pending_notifications, if: :saved_change_to_digest_off? + + scope :due, -> { where.not(digest_frequency: 'off').where(next_digest_at: ..Time.current) } + + def self.for(user) + find_or_create_by!(user: user) + rescue ActiveRecord::RecordNotUnique + find_by!(user: user) + end + + def self.default_channels + Notification::KINDS.index_with do |kind| + Notification::COMMUNICATION_KINDS.include?(kind) ? ['in_app'] : %w[in_app email] + end + end + + def self.default_digest_timezone + configured_timezone = ENV.fetch('TZ', nil).presence + return configured_timezone if configured_timezone && ActiveSupport::TimeZone[configured_timezone].present? + + Time.zone.tzinfo.name + end + + # Use the first unit the user enrolled in as their home campus for now. + def resolved_digest_timezone + first_project = user.projects.where(enrolled: true).order(:id).includes(:campus).first + first_project&.campus&.timezone.presence || self.class.default_digest_timezone + end + + # The channels a unit delivers a kind on, honouring its override when it has + # one. A muted unit delivers nothing. + def channels_for_unit_id(unit_id, kind) + override = override_for(unit_id) + return [] if override&.muted + return ['in_app'] if Notification::COMMUNICATION_KINDS.include?(kind.to_s) + + Array((override&.customised? ? override.channels : channels)[kind.to_s]) + end + + def delivers?(unit, kind, channel) + channels_for_unit_id(unit&.id, kind).include?(channel.to_s) + end + + def weekly_summary_for?(unit) + override = user.notification_unit_overrides.find_by(unit_id: unit&.id) + return false if override&.muted + + Array((override&.customised? ? override.channels : channels)['weekly_summary']).any? + end + + def weekly_summary_opted_in? + return true if Array(channels['weekly_summary']).any? + + user.notification_unit_overrides.any? do |override| + !override.muted && override.customised? && Array(override.channels['weekly_summary']).any? + end + end + + def weekly_summary_due?(at = Time.current) + local = at.in_time_zone(digest_timezone) + local.monday? && local.hour == 7 && local.min < 15 + end + + # Notifications are always recorded so the digest has something to send, so the + # in-app list has to filter them out rather than rely on them never existing. + def shows_in_app?(unit_id, kind) + channels_for_unit_id(unit_id, kind).include?('in_app') + end + + # Move the schedule on before a digest is delivered, so a delivery that raises + # cannot leave this setting due for the five minute poll to enqueue again on + # every cycle. Returns false when the slot has already been claimed, which is + # how a retry of the same digest avoids advancing the schedule a second time. + def claim_digest!(from: Time.current) + return false unless next_digest_at.nil? || next_digest_at <= from + + update!(next_digest_at: next_occurrence(from)) + true + end + + def record_digest_sent!(at = Time.current) + update!(last_digest_at: at) + end + + def next_occurrence(from = Time.current) + return nil if digest_frequency == 'off' + + local_from = from.in_time_zone(digest_time_zone) + if digest_frequency == 'hourly' + start_hour, start_minute = digest_start_time.split(':').map(&:to_i) + return next_hourly_occurrence(local_from, start_hour, start_minute) + end + + hour, minute = digest_time.split(':').map(&:to_i) + date = local_from.to_date + date += (digest_weekday - date.cwday) % 7 if digest_frequency == 'weekly' + + candidate = digest_time_zone.local(date.year, date.month, date.day, hour, minute) + candidate += digest_frequency == 'weekly' ? 1.week : 1.day if candidate <= local_from + candidate + end + + private + + def override_for(unit_id) + return nil if unit_id.nil? + + @overrides_by_unit ||= user.notification_unit_overrides.index_by(&:unit_id) + @overrides_by_unit[unit_id] + end + + def apply_defaults + self.channels = self.class.default_channels if channels.nil? + self.digest_timezone = resolved_digest_timezone + 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_digest_frequency? || + will_save_change_to_digest_interval_hours? || + will_save_change_to_digest_start_time? || + will_save_change_to_digest_time? || + will_save_change_to_digest_timezone? || + will_save_change_to_digest_weekday? + end + + def saved_change_to_digest_off? + digest_frequency == 'off' && saved_change_to_digest_frequency? + end + + # The start time anchors the wall-clock slots, which continue across midnight. + # Rebuilding them each day avoids job delays and daylight-saving changes + # shifting the user's schedule. + def next_hourly_occurrence(from, hour, minute) + date = from.to_date + candidate_hours = (24 / digest_interval_hours).times.map do |offset| + (hour + (offset * digest_interval_hours)) % 24 + end.sort + candidates = candidate_hours.map do |candidate_hour| + digest_time_zone.local(date.year, date.month, date.day, candidate_hour, minute) + end + + candidates.find { |candidate| candidate > from } || begin + next_date = date + 1.day + digest_time_zone.local(next_date.year, next_date.month, next_date.day, candidate_hours.first, minute) + end + end + + def digest_time_zone + ActiveSupport::TimeZone[resolved_digest_timezone] + end + + def digest_timezone_supported + return if digest_timezone.present? && digest_time_zone.present? + + errors.add(:digest_timezone, 'must be a valid timezone') + end + + # Nothing is left waiting for a digest that will never run. + def process_pending_notifications + now = Time.current + # rubocop:disable Rails/SkipsModelValidations + user.received_notifications + .email_pending + .update_all(email_processed_at: now, updated_at: now) + # rubocop:enable Rails/SkipsModelValidations + end +end diff --git a/app/models/notification_unit_override.rb b/app/models/notification_unit_override.rb new file mode 100644 index 0000000000..bacb3de5da --- /dev/null +++ b/app/models/notification_unit_override.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +# One unit's departure from a user's notification settings. A unit without an +# override follows those settings; an override with no channels has only been +# muted, and still follows them. +class NotificationUnitOverride < ApplicationRecord + attribute :channels, :json + + belongs_to :user + belongs_to :unit + + validates :channels, notification_channels: true + + def customised? + channels.present? + end +end diff --git a/app/models/overseer_assessment.rb b/app/models/overseer_assessment.rb index 6dc0b808a9..4b8a49213a 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, 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_cursor.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_cursor.last_read_comment_id IS NULL ' \ @@ -119,7 +123,7 @@ def latest_assessment_comment end def add_assessment_comment(text = 'Automated Assessment Started') - text.strip! + text = text.strip return nil if text.blank? tutor = project.tutor_for(task.task_definition) @@ -139,7 +143,7 @@ def add_assessment_comment(text = 'Automated Assessment Started') end def update_assessment_comment(text) - text.strip! + text = text.strip return nil if text.blank? assessment_comment = assessment_comments.last diff --git a/app/models/portfolio_evidence.rb b/app/models/portfolio_evidence.rb index 65d1d481b6..8f2b1bdde3 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 @@ -71,12 +72,13 @@ def self.process_new_to_pdf(my_source) end errors.each do |project, tasks| - logger.debug "checking email for project #{project.id}" - next unless project.student.receive_task_notifications + tasks.each { |task| Notification.create_pdf_failure(task) } + + next unless NotificationSetting.for(project.student).delivers?(project.unit, 'pdf_generation_failed', :email) logger.info "emailing task notification to #{project.student.name}" begin - PortfolioEvidenceMailer.task_pdf_failed(project, tasks).deliver + PortfolioEvidenceMailer.task_pdf_failed(project, tasks).deliver_now rescue StandardError => e logger.error "Failed to send task pdf failed email for project #{project.id}!\n#{e.message}" end diff --git a/app/models/project.rb b/app/models/project.rb index 2644e7fe43..72f92a2d87 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 @@ -677,7 +678,7 @@ def group_membership_for_groupset(gs) group_memberships.joins(:group).where('groups.group_set_id = :id', id: gs).first end - def send_weekly_status_email(summary_stats, middle_of_unit) + def create_weekly_summary_notification(summary_stats, middle_of_unit) did_revert_to_pass = false # TODO: refactor automatic target grade reset # if middle_of_unit && should_revert_to_pass && !portfolio_exists? @@ -689,14 +690,15 @@ def send_weekly_status_email(summary_stats, middle_of_unit) # summary_stats[:revert][main_convenor_user] << self # end - return unless student.receive_feedback_notifications + return unless NotificationSetting.for(student).weekly_summary_for?(unit) return if portfolio_exists? && !middle_of_unit - begin - NotificationsMailer.weekly_student_summary(self, summary_stats, did_revert_to_pass).deliver_now - rescue StandardError => e - logger.error "Failed to send weekly status email for project #{id}!\n#{e.message}" - end + data = WeeklySummaryNotificationBuilder.for_student( + self, + summary_stats, + did_revert_to_pass: did_revert_to_pass + ) + Notification.create_weekly_summary(recipient: student, unit: unit, project: self, data: data) end def archive_submissions(out) diff --git a/app/models/task.rb b/app/models/task.rb index 3df0c2b17e..63d1fe11a5 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -142,6 +142,7 @@ 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 delegate :unit, to: :project delegate :student, to: :project diff --git a/app/models/tutor_note.rb b/app/models/tutor_note.rb index dbc42645d7..39399ae007 100644 --- a/app/models/tutor_note.rb +++ b/app/models/tutor_note.rb @@ -3,6 +3,21 @@ class TutorNote < ApplicationRecord belongs_to :user belongs_to :task, optional: true belongs_to :reply_to, class_name: "TutorNote", optional: true + has_many :notifications, dependent: :destroy + + def notification_for(recipient) + notifications.detect { |notification| notification.recipient_id == recipient.id } + end + + # Falls back to read_by_unit_role for notes with no notification, either older + # ones or a recipient who has moderation notifications switched off. + def requires_read_by?(recipient) + notification = notification_for(recipient) + return notification.read_at.nil? unless notification.nil? + return false if user_id == recipient.id + + unit_role.user_id == recipient.id && !read_by_unit_role + end def task_definition_id task&.task_definition&.id diff --git a/app/models/unit.rb b/app/models/unit.rb index 140c28216c..bdb7d21cea 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -188,6 +188,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_unit_overrides, dependent: :destroy has_many :comments, through: :projects has_many :tasks, through: :projects @@ -387,7 +389,9 @@ def formatted_discuss_timeout_date(date) def queue_discuss_timeout_email(task, actor, type, expiry_date = nil) return unless send_notifications return unless task.project.enrolled - return unless task.project.student.receive_feedback_notifications + + kind = type == :approaching ? 'discuss_warning' : 'discuss_expired' + return unless NotificationSetting.for(task.project.student).delivers?(self, kind, :email) SendDiscussTimeoutEmailJob.perform_async(task.id, actor.id, type.to_s, expiry_date&.iso8601) end @@ -3330,20 +3334,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}" - next unless project.enrolled - next unless project.student.receive_feedback_notifications - - logger.info "Emailing feedback notification to #{project.student.name}" - PortfolioEvidenceMailer.task_feedback_ready(project, tasks).deliver - end - rescue => e - logger.error "Failed to send emails from feedback submission. Rescued with error: #{e.message}" - end - true end @@ -3817,9 +3807,14 @@ def upload_batch_feedback_zip(user, task_definition, file, progress_callback: ni repacked_zip.close! if defined?(repacked_zip) && repacked_zip.present? end - def send_weekly_status_emails(summary_stats) + def create_weekly_summary_notifications(summary_stats, recipient_ids: nil) return unless send_notifications + summary_recipients = recipient_ids.presence + student_projects = summary_recipients ? active_projects.where(user_id: summary_recipients) : active_projects + staff_recipients = summary_recipients ? staff.where(user_id: summary_recipients) : staff + build_staff_summaries = staff_recipients.exists? + summary_stats[:unit] = self summary_stats[:tutorials] = {} summary_stats[:tutorial_streams] = {} @@ -3836,25 +3831,30 @@ def send_weekly_status_emails(summary_stats) summary_stats[:unit_week_engagements] = task_engagements.where("task_engagements.engagement_time > :start AND task_engagements.engagement_time < :end", start: summary_stats[:week_start], end: summary_stats[:week_end]).count - days_to_end_of_unit = (end_date.to_date - DateTime.now).to_i - days_from_start_of_unit = (DateTime.now - start_date.to_date).to_i + summary_date = summary_stats[:week_end].to_date + days_to_end_of_unit = (end_date.to_date - summary_date).to_i + days_from_start_of_unit = (summary_date - start_date.to_date).to_i return if days_from_start_of_unit < 4 || days_to_end_of_unit < 0 - staff.each do |ur| - summary_stats[:staff][ur.user] ||= {} - summary_stats[:staff][ur.user][:staff_engagements] ||= 0 - summary_stats[:staff][ur.user][:tasks_awaiting_feedback_count] ||= 0 - summary_stats[:staff][ur.user][:weekly_engagements_count] ||= 0 - summary_stats[:staff][ur.user][:weekly_total_tasks_discussed] ||= 0 - summary_stats[:staff][ur.user][:oldest_task_days] ||= 0 - summary_stats[:revert][ur.user] = [] + if build_staff_summaries + staff.each do |ur| + summary_stats[:staff][ur.user] ||= {} + summary_stats[:staff][ur.user][:staff_engagements] ||= 0 + summary_stats[:staff][ur.user][:tasks_awaiting_feedback_count] ||= 0 + summary_stats[:staff][ur.user][:weekly_engagements_count] ||= 0 + summary_stats[:staff][ur.user][:weekly_total_tasks_discussed] ||= 0 + summary_stats[:staff][ur.user][:oldest_task_days] ||= 0 + summary_stats[:revert][ur.user] = [] + end end - active_projects.each do |project| - project.send_weekly_status_email(summary_stats, days_from_start_of_unit > 28 && days_to_end_of_unit > 14) + student_projects.each do |project| + project.create_weekly_summary_notification(summary_stats, days_from_start_of_unit > 28 && days_to_end_of_unit > 14) end + return unless build_staff_summaries + tutorial_streams.each do |tutorial_stream| summary_stats[:tutorial_streams][tutorial_stream] ||= {} @@ -3891,8 +3891,8 @@ def send_weekly_status_emails(summary_stats) # Group tutorials by tutor group_tutorials_by_tutor(summary_stats) - staff.each do |ur| - ur.send_weekly_status_email(summary_stats) + staff_recipients.each do |ur| + ur.create_weekly_summary_notification(summary_stats) end summary_stats[:staff] = {} diff --git a/app/models/unit_role.rb b/app/models/unit_role.rb index 68fc0f5e09..bcd98ca7f4 100644 --- a/app/models/unit_role.rb +++ b/app/models/unit_role.rb @@ -174,17 +174,23 @@ def populate_summary_stats(summary_stats, tutorial_stream, tutorial, row) .where(content_type: :discussed_in_class) data[:weekly_tasks_discussed] = data[:total_tasks_discussed] - .where("task_comments.created_at > :start", start: Time.zone.now - 7.days) + .where( + "task_comments.created_at >= :start AND task_comments.created_at < :end", + start: summary_stats[:week_start], + end: summary_stats[:week_end] + ) data[:received_comments] = total_comments - .where("recipient_id = :staff_id AND task_comments.created_at > :start", + .where("recipient_id = :staff_id AND task_comments.created_at >= :start AND task_comments.created_at < :end", staff_id: data[:staff].id, - start: Time.zone.now - 7.days) + start: summary_stats[:week_start], + end: summary_stats[:week_end]) data[:sent_comments] = total_comments - .where("task_comments.user_id = :staff_id AND task_comments.created_at > :start", + .where("task_comments.user_id = :staff_id AND task_comments.created_at >= :start AND task_comments.created_at < :end", staff_id: data[:staff].id, - start: Time.zone.now - 7.days) + start: summary_stats[:week_start], + end: summary_stats[:week_end]) data[:total_comments] = total_comments @@ -200,14 +206,11 @@ def populate_summary_stats(summary_stats, tutorial_stream, tutorial, row) row.replace(data) end - def send_weekly_status_email(summary_stats) - return unless user.receive_feedback_notifications + def create_weekly_summary_notification(summary_stats) + return unless NotificationSetting.for(user).weekly_summary_for?(unit) - begin - NotificationsMailer.weekly_staff_summary(self, summary_stats).deliver_now - rescue StandardError => e - Rails.logger.error "Failed to send weekly staff summary email to #{user.email} - #{e.message}" - end + data = WeeklySummaryNotificationBuilder.for_staff(self, summary_stats) + Notification.create_weekly_summary(recipient: user, unit: unit, data: data) end def ensure_valid_user_for_role diff --git a/app/models/user.rb b/app/models/user.rb index 6066b148f8..cf5eff380a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -183,6 +183,18 @@ 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_unit_overrides, dependent: :destroy + has_one :notification_setting, dependent: :destroy # Model validations/constraints validates :first_name, presence: true, allowed_characters: { type: :first_name } diff --git a/app/services/notification_group_builder.rb b/app/services/notification_group_builder.rb new file mode 100644 index 0000000000..d38eff581c --- /dev/null +++ b/app/services/notification_group_builder.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +class NotificationGroupBuilder + EVENT_DETAILS = { + 'discuss_expired' => 'discussion deadline missed', + 'discuss_warning' => 'discussion deadline approaching', + 'pdf_generation_failed' => 'submission PDF generation failed', + 'overseer_failed' => 'overseer assessment failed', + 'portfolio_failed' => 'portfolio compilation failed', + 'portfolio_ready' => 'portfolio ready to review', + 'task_start_now' => 'ready to start now', + 'task_due_soon' => 'due within 5 days', + 'task_overdue' => 'past its due date' + }.freeze + + def initialize(notifications) + @notifications = notifications.to_a + end + + def groups + @notifications + .group_by { |notification| grouping_key(notification) } + .values + .map { |items| build_group(items) } + .sort_by { |group| -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}:communication-email:#{notification.id}" if notification.kind == 'communication_email' + return "#{state_key}:weekly-summary:#{notification.id}" if notification.kind == 'weekly_summary' + + # A moderation note belongs to one staff member's thread, so it groups by that + # rather than joining the task's other notifications. + return "#{state_key}:tutor-notes:#{notification.unit_role_id}:#{notification.task_id}" if Notification::MODERATION_KINDS.include?(notification.kind) + if notification.kind == 'feedback_warning' + return "#{state_key}:unit:#{notification.unit_id}:feedback-warning:#{feedback_warning_batch(notification)}" + end + + return "#{state_key}:task:#{notification.task_id}" if notification.task_id.present? + return "#{state_key}:portfolio:#{notification.project_id}" if Notification::PORTFOLIO_KINDS.include?(notification.kind) + + "#{state_key}:unit:#{notification.unit_id}:#{notification.kind}" + end + + def build_group(items) + latest = items.max_by(&:created_at) + counts = items.each_with_object(Hash.new(0)) { |notification, result| result[notification.kind] += 1 } + feedback_warning = counts['feedback_warning'].positive? + task = feedback_warning ? nil : latest.task + latest_status = items + .select { |notification| notification.kind == 'task_status_changed' } + .max_by(&:created_at) + &.task_status + &.status_key + tutor_notes = items + .select { |notification| Notification::MODERATION_KINDS.include?(notification.kind) } + .sort_by { |notification| [notification.created_at, notification.id] } + detail = detail_for(items, counts, latest_status) + project = feedback_warning ? nil : (latest.project || task&.project) + weekly_summary = latest.weekly_summary_data + + { + 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 + }, + project_id: project&.id, + task: task_details(task, latest.recipient), + destination: feedback_warning ? { type: 'unit_inbox', unit_id: latest.unit_id } : nil, + 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, + timezone: project&.campus&.timezone || Time.zone.name, + tutor_note_ids: tutor_notes.filter_map(&:tutor_note_id), + tutor_note_unit_role_id: tutor_notes.first&.unit_role_id, + tutor_note_on_task_tutor: tutor_note_on_task_tutor?(tutor_notes.first, task), + overseer_assessment_id: overseer_assessment_id(items), + message_subject: latest.message_subject, + message_body: weekly_summary ? nil : latest.message_body, + weekly_summary: weekly_summary, + detail: detail, + summary: "#{subject_for(task, latest.recipient, counts, latest)} - #{detail}" + } + end + + def feedback_warning_batch(notification) + notification.email_sent_at ? "emailed:#{notification.email_sent_at.to_f}" : 'not-emailed' + end + + # The task's Mod Notes tab shows the notes on its own tutor, so a note about + # anyone else - a convenor's thread that happens to name this task - has to be + # opened as a thread instead. + def tutor_note_on_task_tutor?(notification, task) + return false if notification.nil? || task.nil? + + tutor = task.project.tutor_for(task.task_definition) + tutor.present? && tutor.id == notification.unit_role&.user_id + 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 portfolio_failed task_overdue]) + return 'warning' if kinds.intersect?(%w[discuss_warning overseer_failed task_due_soon feedback_warning] + Notification::MODERATION_KINDS) + + 'normal' + end + + # The newest failed run in the group, so opening the notification can jump + # straight to that report. + def overseer_assessment_id(items) + items.select { |notification| notification.kind == 'overseer_failed' }.max_by(&:created_at)&.overseer_assessment_id + end + + def subject_for(task, recipient, counts, latest) + return latest.message_subject.presence || 'Email message' if counts['communication_email'].positive? + return latest.message_subject.presence || 'Weekly summary' if counts['weekly_summary'].positive? + return 'Portfolio' if Notification::PORTFOLIO_KINDS.any? { |kind| counts[kind].positive? } + return 'Feedback required' if counts['feedback_warning'].positive? + return 'Unit notification' if task.nil? + + return task.task_definition.abbreviation if task.project.student == recipient + + "#{task.task_definition.abbreviation} for #{task.project.student.name}" + end + + # What happened, without the task it happened to, so callers can show the two + # separately rather than splitting the summary back apart. + def detail_for(items, counts, latest_status) + details = [ + communication_detail(items, counts), + weekly_summary_detail(items, counts), + feedback_warning_detail(counts), + *event_details(counts), + moderation_detail(items), + comment_detail(counts), + status_detail(latest_status) + ].compact + + # Sentence case, so a single detail reads as a heading and several still join + # into one readable sentence. + details.to_sentence.upcase_first + end + + def weekly_summary_detail(items, counts) + return unless counts['weekly_summary'].positive? + + data = items.max_by(&:created_at).weekly_summary_data || {} + if data['audience'] == 'staff' + assessed = data.fetch('assessed_tasks', 0) + awaiting = data.fetch('awaiting_feedback', 0) + "You assessed #{pluralize(assessed, 'task')} and have #{pluralize(awaiting, 'task')} awaiting feedback" + else + received = data.fetch('received_comments', 0) + activity = data.fetch('task_activity', 0) + "You received #{pluralize(received, 'comment')} and had activity on #{pluralize(activity, 'task')} this week" + end + end + + def feedback_warning_detail(counts) + count = counts['feedback_warning'] + return unless count.positive? + + "You have #{pluralize(count, 'task')} that #{count == 1 ? 'requires' : 'require'} feedback as soon as possible" + end + + def communication_detail(items, counts) + return unless counts['communication_email'].positive? + + sender = items.max_by(&:created_at).actor&.name + sender.present? ? "Message from #{sender}" : 'Message' + end + + def event_details(counts) + EVENT_DETAILS.filter_map { |kind, detail| detail if counts[kind].positive? } + end + + def moderation_detail(items) + moderation_notifications = items.select { |notification| Notification::MODERATION_KINDS.include?(notification.kind) } + return if moderation_notifications.empty? + + staff_names = moderation_notifications.filter_map { |notification| notification.actor&.name }.uniq + detail = pluralize(moderation_notifications.count, 'moderation note') + staff_names.any? ? "#{detail} from #{staff_names.to_sentence}" : detail + end + + def comment_detail(counts) + count = counts['new_task_comment'] + pluralize(count, 'new comment') if count.positive? + end + + def status_detail(latest_status) + "task status changed to #{status_name(latest_status)}" if latest_status.present? + end + + def status_name(status_key) + TaskStatus.status_for_name(status_key.to_s)&.name || status_key.to_s.humanize + end + + def pluralize(count, noun) + "#{count} #{count == 1 ? noun : noun.pluralize}" + end +end diff --git a/app/services/weekly_summary_notification_builder.rb b/app/services/weekly_summary_notification_builder.rb new file mode 100644 index 0000000000..32bac4ea40 --- /dev/null +++ b/app/services/weekly_summary_notification_builder.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +class WeeklySummaryNotificationBuilder + COMMENT_TYPES = %i[text assessment audio image pdf discussion extension].freeze + + def self.for_student(project, summary_stats, did_revert_to_pass: false) + week_start = summary_stats.fetch(:week_start) + week_end = summary_stats.fetch(:week_end) + engagements = project.task_engagements.where(engagement_time: week_start...week_end) + student_statuses = [ + TaskStatus.not_started.name, + TaskStatus.need_help.name, + TaskStatus.working_on_it.name, + TaskStatus.ready_for_feedback.name + ] + + { + audience: 'student', + week_start: week_start.iso8601, + week_end: week_end.iso8601, + unit_comments: summary_stats.fetch(:unit_week_comments), + unit_task_activity: summary_stats.fetch(:unit_week_engagements), + sent_comments: project.comments.where(user_id: project.user_id, created_at: week_start...week_end).count, + received_comments: project.comments.where(recipient_id: project.user_id, created_at: week_start...week_end).count, + task_activity: engagements.count, + student_task_activity: engagements.where(engagement: student_statuses).count, + tutor_allocated: project.tutorial_enrolments.exists?, + did_revert_to_pass: did_revert_to_pass, + portfolio_exists: project.portfolio_exists?, + top_tasks: student_top_tasks(project) + } + end + + def self.for_staff(unit_role, summary_stats) + unit = summary_stats.fetch(:unit) + staff = unit_role.user + week_start = summary_stats.fetch(:week_start) + week_end = summary_stats.fetch(:week_end) + staff_stats = summary_stats.fetch(:staff).fetch(staff) + + data = { + audience: 'staff', + week_start: week_start.iso8601, + week_end: week_end.iso8601, + has_students: unit_role.has_students?, + is_convenor: unit_role.is_convenor?, + unit_comments: summary_stats.fetch(:unit_week_comments), + unit_task_activity: summary_stats.fetch(:unit_week_engagements), + received_comments: unit.comments.where( + recipient_id: staff.id, + created_at: week_start...week_end, + content_type: COMMENT_TYPES + ).count, + sent_comments: unit.comments.where( + user_id: staff.id, + created_at: week_start...week_end, + content_type: COMMENT_TYPES + ).count, + student_task_activity: staff_stats.fetch(:weekly_engagements_count), + assessed_tasks: staff_stats.fetch(:staff_engagements), + discussed_tasks: staff_stats.fetch(:weekly_total_tasks_discussed), + awaiting_feedback: staff_stats.fetch(:tasks_awaiting_feedback_count), + oldest_task_days: staff_stats.fetch(:oldest_task_days), + reverted_students: Array(summary_stats.dig(:revert, staff)).map { |project| project.student.name } + } + + if unit_role.is_convenor? + data[:reverted_student_count] = summary_stats.fetch(:revert_count) + data[:tutorial_streams] = convenor_tutorial_streams(summary_stats) + end + + data + end + + def self.student_top_tasks(project) + project.top_tasks.map do |top_task| + definition = top_task.fetch(:task_definition) + { + abbreviation: definition.abbreviation, + name: definition.name, + reason: top_task.fetch(:reason).to_s, + reason_label: { overdue: 'Overdue', soon: 'Due soon', ahead: 'Get ahead' }.fetch( + top_task.fetch(:reason), + top_task.fetch(:reason).to_s.humanize + ), + status: top_task[:status]&.to_s + } + end + end + private_class_method :student_top_tasks + + def self.convenor_tutorial_streams(summary_stats) + summary_stats.fetch(:tutorial_streams).filter_map do |stream, stream_data| + next unless stream_data[:stream_linked_to_task_definition] + + rows = Array(stream_data[:unit_roles]).filter_map do |unit_role, data| + next unless data[:number_of_students].to_i.positive? + + { + tutor_name: unit_role.user.name, + students: data[:number_of_students], + total_assessments: data[:total_staff_engagements], + weekly_assessments: data[:staff_engagements], + total_comments: data[:total_comments].uniq.count, + weekly_comments: data[:sent_comments].uniq.count, + awaiting_feedback: data[:tasks_awaiting_feedback_count], + oldest_task_days: data[:oldest_task_days], + total_discussions: data[:total_tasks_discussed], + weekly_discussions: data[:weekly_tasks_discussed] + } + end + rows.sort_by! { |row| [-row[:oldest_task_days].to_i, -row[:awaiting_feedback].to_i, row[:tutor_name]] } + + { + name: stream.name, + unallocated_students: stream_data[:num_students_without_tutors].to_i, + tutors: rows + } + end + end + private_class_method :convenor_tutorial_streams +end diff --git a/app/sidekiq/accept_overseer_job.rb b/app/sidekiq/accept_overseer_job.rb index 5227e287d0..d8b12636c7 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 1d163d8afb..d5cb8afb59 100644 --- a/app/sidekiq/accept_submission_job.rb +++ b/app/sidekiq/accept_submission_job.rb @@ -30,10 +30,10 @@ 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 + Notification.create_pdf_failure(task) + if NotificationSetting.for(task.project.student).delivers?(task.unit, 'pdf_generation_failed', :email) begin - PortfolioEvidenceMailer.task_pdf_failed(task.project, [task]).deliver + PortfolioEvidenceMailer.task_pdf_failed(task.project, [task]).deliver_now rescue StandardError => mail_error logger.error "Failed to send task pdf failed email for project #{task.project.id}!\n#{mail_error.message}" end @@ -52,7 +52,7 @@ def perform(task_id, user_id, accepted_tii_eula, test_submission) ) end mail = ErrorLogMailer.error_message('Accept Submission', "Failed to convert submission to PDF for task #{task.log_details}", e) - mail.deliver if mail.present? + mail.presence&.deliver rescue StandardError => e logger.error "Failed to send error log to admin" end @@ -60,6 +60,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_weekly_summary_notifications_job.rb b/app/sidekiq/create_weekly_summary_notifications_job.rb new file mode 100644 index 0000000000..e103dbc026 --- /dev/null +++ b/app/sidekiq/create_weekly_summary_notifications_job.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'set' + +class CreateWeeklySummaryNotificationsJob + include Sidekiq::Job + + PROCESSED_KEY_PREFIX = 'notifications:weekly_summary:created' + + sidekiq_options lock: :until_executed, + lock_args_method: ->(_args) { ['create-weekly-summary-notifications'] }, + on_conflict: :reject, + retry: 2 + + def perform(at = nil) + create_summaries(at: at, force: false) + end + + def perform_now + create_summaries(at: nil, force: true) + end + + private + + def create_summaries(at:, force:) + week_end = at.present? ? Time.zone.parse(at) : Time.current + due_timezones = due_settings_by_timezone(week_end, force: force).reject do |timezone, _settings| + timezone_processed?(timezone, week_end) + end + recipient_ids = due_timezones.values.flatten.to_set(&:user_id) + return if recipient_ids.empty? + + summary_stats = { + week_end: week_end, + week_start: week_end - 7.days, + weeks_comments: TaskComment.where(created_at: (week_end - 7.days)...week_end).count, + weeks_engagements: TaskEngagement.where(engagement_time: (week_end - 7.days)...week_end).count + } + + project_units = Project.where(user_id: recipient_ids, enrolled: true).select(:unit_id) + role_units = UnitRole.where(user_id: recipient_ids).select(:unit_id) + units = Unit.where(id: project_units).or(Unit.where(id: role_units)) + + units.where(active: true, send_notifications: true).find_each do |unit| + next unless week_end > unit.start_date && summary_stats[:week_start] < unit.end_date + + unit.create_weekly_summary_notifications(summary_stats, recipient_ids: recipient_ids) + end + + due_timezones.each_key { |timezone| mark_timezone_processed(timezone, week_end) } + end + + def due_settings_by_timezone(at, force:) + NotificationSetting.includes(user: :notification_unit_overrides).find_each.select do |settings| + (force || settings.weekly_summary_due?(at)) && settings.weekly_summary_opted_in? + end.group_by(&:digest_timezone) + end + + def timezone_processed?(timezone, at) + Sidekiq.redis { |redis| redis.get(processed_key(timezone, at)).present? } + end + + def mark_timezone_processed(timezone, at) + Sidekiq.redis { |redis| redis.set(processed_key(timezone, at), '1', ex: 8.days.to_i) } + end + + def processed_key(timezone, at) + local_date = at.in_time_zone(timezone).to_date.iso8601 + "#{PROCESSED_KEY_PREFIX}:#{timezone}:#{local_date}" + end +end diff --git a/app/sidekiq/execute_communication_set_job.rb b/app/sidekiq/execute_communication_set_job.rb index e645cb698f..580b3ee7fb 100644 --- a/app/sidekiq/execute_communication_set_job.rb +++ b/app/sidekiq/execute_communication_set_job.rb @@ -1,4 +1,5 @@ require 'csv' +require 'securerandom' class ExecuteCommunicationSetJob include Sidekiq::Job @@ -170,6 +171,8 @@ def execute_email_student_action(action, projects, unit, rule) rule: rule ).deliver_now + create_email_notification(action, project, recipient, unit, subject, body) + { action_id: action.id, action_type: action.type, @@ -211,6 +214,8 @@ def execute_email_staff_action(action, projects, unit, rule) rule: rule ).deliver_now + create_email_notification(action, project, recipient, unit, subject, body) + { action_id: action.id, action_type: action.type, @@ -364,6 +369,31 @@ def staff_recipients_for(project, unit, action) recipients.select { |recipient| recipient&.email.present? }.uniq(&:id) end + def create_email_notification(action, project, recipient, unit, subject, body) + Notification.create_for_communication_email( + recipient: recipient, + unit: unit, + project: project, + actor: sender_user_for(unit), + subject: subject, + body: body, + deduplication_key: [ + 'communication-email', + execution_identifier, + action.id, + project.id, + recipient.id + ].join(':') + ) + end + + # Sidekiq keeps a jid across retries, so a retry cannot create a second + # notification for the same delivered action. Direct test/job invocation has + # no jid and receives a per-run identifier instead. + def execution_identifier + @execution_identifier ||= (respond_to?(:jid) ? jid : nil).presence || SecureRandom.uuid + end + def render_template(template, project, unit, rule, affected_students_count, target_grade_override = nil, action_results = []) return '' if template.blank? diff --git a/app/sidekiq/notify_feedback_warnings_job.rb b/app/sidekiq/notify_feedback_warnings_job.rb new file mode 100644 index 0000000000..dfdbac0eb7 --- /dev/null +++ b/app/sidekiq/notify_feedback_warnings_job.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +class NotifyFeedbackWarningsJob + include Sidekiq::Job + + ROLLOUT_KEY = 'notifications:feedback_warning:rollout_at' + + sidekiq_options lock: :until_executed, + lock_args_method: ->(_args) { ['notify-feedback-warnings'] }, + on_conflict: :reject, + retry: 1 + + def perform + now = Time.current + # Avoid backfilling notifications for tasks that were already overdue when this ships + # The first run stores a one-off rollout time so we only pick up tasks that cross the threshold after started_at + started_at = rollout_started_at + if started_at.nil? + store_rollout_started_at(now) + return + end + + Notification.refresh_feedback_warning_notifications!(now: now, started_at: started_at) + end + + private + + def rollout_started_at + value = Sidekiq.redis { |redis| redis.get(ROLLOUT_KEY) } + Time.zone.parse(value) if value.present? + end + + def store_rollout_started_at(time) + Sidekiq.redis { |redis| redis.set(ROLLOUT_KEY, time.utc.iso8601(6), nx: true) } + end +end diff --git a/app/sidekiq/notify_task_deadlines_job.rb b/app/sidekiq/notify_task_deadlines_job.rb new file mode 100644 index 0000000000..5c751b7bad --- /dev/null +++ b/app/sidekiq/notify_task_deadlines_job.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class NotifyTaskDeadlinesJob + include Sidekiq::Job + + ROLLOUT_KEY = 'notifications:task_deadline:rollout_at' + + sidekiq_options lock: :until_executed, + lock_args_method: ->(_args) { ['notify-task-deadlines'] }, + on_conflict: :reject, + retry: 1 + + def perform + now = Time.current + # The first run only records the rollout time, so tasks already started, due soon or overdue are not backfilled + started_at = rollout_started_at + if started_at.nil? + store_rollout_started_at(now) + return + end + + Notification.refresh_task_deadline_notifications!(now: now, started_at: started_at) + end + + private + + def rollout_started_at + value = Sidekiq.redis { |redis| redis.get(ROLLOUT_KEY) } + Time.zone.parse(value) if value.present? + end + + def store_rollout_started_at(time) + Sidekiq.redis { |redis| redis.set(ROLLOUT_KEY, time.utc.iso8601(6), nx: true) } + end +end diff --git a/app/sidekiq/notify_tutor_notes_job.rb b/app/sidekiq/notify_tutor_notes_job.rb index 194c6b4f35..aabb8dcc71 100644 --- a/app/sidekiq/notify_tutor_notes_job.rb +++ b/app/sidekiq/notify_tutor_notes_job.rb @@ -1,11 +1,11 @@ class NotifyTutorNotesJob include Sidekiq::Job - def perform(tutor_note_id, recipient_user_id) + def perform(tutor_note_id, recipient_user_id, kind) 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, kind) 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..9d197df064 --- /dev/null +++ b/app/sidekiq/poll_notification_digests_job.rb @@ -0,0 +1,19 @@ +# 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 + # Create the 7am weekly summaries first so a digest due at the same time can include them. + CreateWeeklySummaryNotificationsJob.new.perform + + NotificationSetting.due.find_each do |setting| + SendNotificationDigestJob.perform_async(setting.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_discuss_timeout_email_job.rb b/app/sidekiq/send_discuss_timeout_email_job.rb index 76349e7835..e47d0a5327 100644 --- a/app/sidekiq/send_discuss_timeout_email_job.rb +++ b/app/sidekiq/send_discuss_timeout_email_job.rb @@ -8,7 +8,9 @@ def perform(task_id, sender_id, notification_type, expiry_date = nil) sender = User.find_by(id: sender_id) return if task.blank? || sender.blank? return unless task.unit.send_notifications - return unless task.project.student.receive_feedback_notifications + + kind = notification_type == 'approaching' ? 'discuss_warning' : 'discuss_expired' + return unless NotificationSetting.for(task.project.student).delivers?(task.unit, kind, :email) mail = case notification_type when 'approaching' diff --git a/app/sidekiq/send_notification_digest_job.rb b/app/sidekiq/send_notification_digest_job.rb new file mode 100644 index 0000000000..e04fc21452 --- /dev/null +++ b/app/sidekiq/send_notification_digest_job.rb @@ -0,0 +1,57 @@ +# 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(setting_id) + setting = NotificationSetting.find(setting_id) + return if setting.digest_frequency == 'off' + + now = Time.current + # Claim the slot first. Anything that raises after this point leaves the + # notifications unprocessed, so they roll into the next digest rather than + # this one being enqueued and re-sent every time the poll runs. + setting.claim_digest!(from: now) + + ready = setting.user + .received_notifications + .email_pending + .email_ready(now) + .includes(:recipient, :actor, :unit, { project: :campus }, task: [:task_definition, { project: :user }]) + .to_a + deliverable, skipped = ready.partition { |notification| deliverable?(setting, notification, now) } + + mark_processed(skipped, now) + return if deliverable.empty? + + NotificationsMailer.notification_digest(setting.user, deliverable).deliver_now + mark_processed(deliverable, now, sent: true) + setting.record_digest_sent!(now) + end + + private + + def deliverable?(setting, notification, now) + notification.unit.send_notifications && + !notification.recipient_withdrawn? && + notification.current_for_delivery?(now: now) && + setting.delivers?(notification.unit, notification.kind, :email) + end + + # The delivery ledger is immutable once written, so these intentionally bypass callbacks. + def mark_processed(notifications, at, sent: false) + return if notifications.empty? + + attributes = { email_processed_at: at, updated_at: at } + attributes[:email_sent_at] = at if sent + + # rubocop:disable Rails/SkipsModelValidations + Notification.where(id: notifications.map(&:id)).update_all(attributes) + # rubocop:enable Rails/SkipsModelValidations + end +end diff --git a/app/validators/notification_channels_validator.rb b/app/validators/notification_channels_validator.rb new file mode 100644 index 0000000000..25863d311e --- /dev/null +++ b/app/validators/notification_channels_validator.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# Checks the `channels` column, which maps a notification kind to the channels it +# is delivered on: { 'new_task_comment' => ['in_app', 'email'] }. +class NotificationChannelsValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return if value.nil? + + unless value.is_a?(Hash) + record.errors.add(attribute, 'must be an object') + return + end + + unknown_kinds = value.keys - Notification::KINDS + record.errors.add(attribute, "includes unknown kinds: #{unknown_kinds.join(', ')}") if unknown_kinds.any? + + unknown_channels = value.values.flatten.uniq - Notification::CHANNELS + record.errors.add(attribute, "includes unknown channels: #{unknown_channels.join(', ')}") if unknown_channels.any? + end +end diff --git a/app/views/communications_mailer/communication_email.text.erb b/app/views/communications_mailer/communication_email.text.erb index 5ad89c15fb..772977b565 100644 --- a/app/views/communications_mailer/communication_email.text.erb +++ b/app/views/communications_mailer/communication_email.text.erb @@ -1,12 +1,7 @@ -<%# Hi <%= @recipient.nickname.presence || @recipient.first_name %> %> - <% @body_paragraphs.each do |paragraph| %> <%= paragraph %> <% end %> -<%# Cheers, -The <%= @doubtfire_product_name %> Team on behalf of <%= @sender.name %> %> - --- Generated with <%= @doubtfire_product_name %> 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..e965c3afc0 --- /dev/null +++ b/app/views/notifications_mailer/notification_digest.html.erb @@ -0,0 +1,232 @@ + + + + + + + <%= @doubtfire_product_name %> notifications + + + +
+
+
+
+
+ Notification Summary +
+ <%= "#{Time.zone.now.day.ordinalize} #{Time.zone.now.strftime('%B %Y')}" %> +
+
+ <%= @doubtfire_product_name %> +
+ +

Hi <%= @recipient.first_name %>,

+ +

+ You have <%= @notification_count %> new + <%= 'notification'.pluralize(@notification_count) %> in <%= @doubtfire_product_name %>. +

+ + <% @units.each do |section| %> +
+
+ <%= section[:unit].code %> +
+
+ <%= section[:unit].name %> +
+ + <% section[:group_sections].each do |group_section| %> + <% if group_section[:divider] %> +
+ <% end %> + <% if group_section[:heading] %> +
+ <%= group_section[:heading] %> +
+ <% end %> + <% group_section[:groups].each do |group| %> +
+
+
+ <% if group[:task] %> + <%= group[:task][:abbreviation] %> - <%= group[:task][:name] %> + <% elsif group.dig(:destination, :type) == 'unit_inbox' %> + Feedback required + <% elsif group[:message_subject].present? %> + <%= group[:message_subject] %> + <% elsif group[:project_id] %> + Portfolio + <% else %> + Unit notification + <% end %> +
+ <% if group_section[:kind] && group.dig(:task, :due_date) %> +
+ <%= notification_due_date(group) %> +
+ <% end %> +
+ <% if group[:task] && group[:task][:student_name].present? %> +
<%= group[:task][:student_name] %>
+ <% end %> + <% unless group_section[:kind] && group[:event_count] == group[:counts][group_section[:kind]] %> +
<%= group[:detail] %>
+ <% end %> + <% if group[:weekly_summary] %> + <% summary = group[:weekly_summary] %> +
<%= weekly_summary_period(summary) %>
+ + + <% if summary['audience'] == 'staff' %> + <% [['Assessed', summary['assessed_tasks']], ['Awaiting feedback', summary['awaiting_feedback']], ['Comments sent', summary['sent_comments']], ['Tasks discussed', summary['discussed_tasks']]].each do |label, value| %> + + <% end %> + <% else %> + <% [['Comments received', summary['received_comments']], ['Comments posted', summary['sent_comments']], ['Your task activity', summary['task_activity']], ['Unit task activity', summary['unit_task_activity']]].each do |label, value| %> + + <% end %> + <% end %> + +
+
<%= value %>
+
<%= label %>
+ 		+
<%= value %>
+
<%= label %>
+
+ + <% if summary['audience'] == 'staff' %> +
Across the unit there were <%= pluralize(summary['unit_comments'], 'comment') %> and <%= pluralize(summary['unit_task_activity'], 'task status change') %>. Your students' tasks changed state <%= pluralize(summary['student_task_activity'], 'time') %>, and you received <%= pluralize(summary['received_comments'], 'comment') %>.
+ <% else %> +
Across the unit there were <%= pluralize(summary['unit_comments'], 'comment') %>. Of your task activity, <%= pluralize(summary['student_task_activity'], 'change') %> came from your own progress.
+ <% end %> + + <% if summary['audience'] == 'staff' && summary['awaiting_feedback'].to_i.positive? %> +
The oldest task awaiting feedback has waited <%= pluralize(summary['oldest_task_days'], 'day') %>.
+ <% elsif summary['audience'] == 'student' %> + <% unless summary['tutor_allocated'] %> +
You are not currently allocated to a tutor. Please check your tutorial allocation.
+ <% end %> + <% if summary['top_tasks'].any? %> +
What to focus on next
+ + <% else %> +
<%= summary['portfolio_exists'] ? 'You have completed your tasks and prepared your portfolio.' : 'You have completed your tasks—make sure you log in and prepare your portfolio.' %>
+ <% end %> + <% end %> + + <% if summary['did_revert_to_pass'] %> +
Your target grade was reset to Pass so you can focus on catching up with your Pass tasks.
+ <% end %> + + <% if summary['reverted_students'].present? %> +
The following students were advised to focus on their Pass tasks: <%= summary['reverted_students'].to_sentence %>.
+ <% end %> + + <% if summary['is_convenor'] && summary['tutorial_streams'].present? %> +
Tutor progress
+ <% summary['tutorial_streams'].each do |stream| %> +
<%= stream['name'] %>
+ <% if stream['unallocated_students'].to_i.positive? %> +
<%= pluralize(stream['unallocated_students'], 'student') %> not allocated to a tutorial in this stream.
+ <% end %> + <% if stream['tutors'].any? %> +
+ + + + + + + + + + + + + <% stream['tutors'].each do |tutor| %> + + + + + + + + + <% end %> + +
TutorStudentsAssessments
week / total		Comments
week / total		Awaiting
count / oldest		Discussed
week / total
<%= tutor['tutor_name'] %><%= tutor['students'] %><%= tutor['weekly_assessments'] %> / <%= tutor['total_assessments'] %><%= tutor['weekly_comments'] %> / <%= tutor['total_comments'] %><%= tutor['awaiting_feedback'] %> / <%= tutor['oldest_task_days'] %>d<%= tutor['weekly_discussions'] %> / <%= tutor['total_discussions'] %>
+
+ <% end %> + <% end %> + <% end %> + <% end %> + <% unless group_section[:kind] %> +
<%= notification_timestamp(group) %>
+ <% end %> + <% if group[:task] %> + + <% elsif group.dig(:destination, :type) == 'unit_inbox' %> + + <% elsif group[:weekly_summary] && group[:project_id] %> + + <% elsif group[:project_id] %> + + <% end %> +
+ <% end %> + <% end %> +
+ <% end %> + +
+ + View all notifications + +
+
+ + +
+
+ + 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..c5ef35bd8c --- /dev/null +++ b/app/views/notifications_mailer/notification_digest.text.erb @@ -0,0 +1,87 @@ +Hi <%= @recipient.first_name %>, + +You have <%= @notification_count %> new <%= 'notification'.pluralize(@notification_count) %> in <%= @doubtfire_product_name %>. +<% @units.each do |section| %> +<%= section[:unit].code %> - <%= section[:unit].name %> +<% section[:group_sections].each do |group_section| %> +<% if group_section[:divider] %> +------------------- +<% end %> +<% if group_section[:heading] %> +<%= group_section[:heading] %> +<% end %> +<% group_section[:groups].each do |group| %> +- <% if group[:task] %><%= group[:task][:abbreviation] %> - <%= group[:task][:name] %><% elsif group.dig(:destination, :type) == 'unit_inbox' %>Feedback required<% elsif group[:message_subject].present? %><%= group[:message_subject] %><% elsif group[:project_id] %>Portfolio<% else %>Unit notification<% end %><% if group_section[:kind] && group.dig(:task, :due_date) %> — <%= notification_due_date(group) %><% end %> +<% unless group_section[:kind] %> + <%= notification_timestamp(group) %> +<% end %> +<% unless group_section[:kind] && group[:event_count] == group[:counts][group_section[:kind]] %> + <%= group[:detail] %> +<% end %> +<% if group[:weekly_summary] %> +<% summary = group[:weekly_summary] %> + <%= weekly_summary_period(summary) %> +<% if summary['audience'] == 'staff' %> + Assessed: <%= summary['assessed_tasks'] %> + Awaiting feedback: <%= summary['awaiting_feedback'] %><% if summary['awaiting_feedback'].to_i.positive? %> (oldest: <%= pluralize(summary['oldest_task_days'], 'day') %>)<% end %> + Comments sent: <%= summary['sent_comments'] %> + Comments received: <%= summary['received_comments'] %> + Tasks discussed: <%= summary['discussed_tasks'] %> + Across the unit: <%= pluralize(summary['unit_comments'], 'comment') %> and <%= pluralize(summary['unit_task_activity'], 'task status change') %>. + Your students' tasks changed state <%= pluralize(summary['student_task_activity'], 'time') %> and you received <%= pluralize(summary['received_comments'], 'comment') %>. +<% else %> + Comments received: <%= summary['received_comments'] %> + Comments posted: <%= summary['sent_comments'] %> + Your task activity: <%= summary['task_activity'] %> + Unit task activity: <%= summary['unit_task_activity'] %> + Your own progress accounted for <%= pluralize(summary['student_task_activity'], 'change') %>; across the unit there were <%= pluralize(summary['unit_comments'], 'comment') %>. +<% unless summary['tutor_allocated'] %> + Warning: You are not currently allocated to a tutor. Please check your tutorial allocation. +<% end %> +<% if summary['top_tasks'].any? %> + What to focus on next: +<% summary['top_tasks'].each do |task| %> + - <%= task['abbreviation'] %> — <%= task['name'] %> (<%= task['reason_label'] %>) + <%= @doubtfire_host %>/projects/<%= group[:project_id] %>/dashboard/<%= task['abbreviation'] %> +<% end %> +<% else %> + <%= summary['portfolio_exists'] ? 'You have completed your tasks and prepared your portfolio.' : 'You have completed your tasks—make sure you log in and prepare your portfolio.' %> +<% end %> +<% if summary['did_revert_to_pass'] %> + Your target grade was reset to Pass so you can focus on catching up with your Pass tasks. +<% end %> +<% end %> +<% if summary['reverted_students'].present? %> + Students advised to focus on their Pass tasks: <%= summary['reverted_students'].to_sentence %>. +<% end %> +<% if summary['is_convenor'] && summary['tutorial_streams'].present? %> + Tutor progress: +<% summary['tutorial_streams'].each do |stream| %> + <%= stream['name'] %> +<% if stream['unallocated_students'].to_i.positive? %> + Warning: <%= pluralize(stream['unallocated_students'], 'student') %> not allocated to a tutorial in this stream. +<% end %> +<% stream['tutors'].each do |tutor| %> + - <%= tutor['tutor_name'] %>: <%= tutor['students'] %> students; assessments <%= tutor['weekly_assessments'] %> this week / <%= tutor['total_assessments'] %> total; comments <%= tutor['weekly_comments'] %> / <%= tutor['total_comments'] %>; awaiting <%= tutor['awaiting_feedback'] %> (oldest <%= tutor['oldest_task_days'] %>d); discussed <%= tutor['weekly_discussions'] %> / <%= tutor['total_discussions'] %> +<% end %> +<% end %> +<% end %> +<% end %> +<% if group[:task] %> + <%= @doubtfire_host %>/projects/<%= group[:task][:project_id] %>/dashboard/<%= group[:task][:abbreviation] %><%= '?tutor=true' if group[:task][:staff_view] %> +<% elsif group.dig(:destination, :type) == 'unit_inbox' %> + <%= @doubtfire_host %>/units/<%= group.dig(:destination, :unit_id) %>/tasks/inbox +<% elsif group[:weekly_summary] && group[:project_id] %> + <%= @doubtfire_host %>/projects/<%= group[:project_id] %>/dashboard +<% elsif group[:project_id] %> + <%= @doubtfire_host %>/projects/<%= group[:project_id] %>/portfolio +<% end %> +<% end %> +<% end %> +<% end %> + +View all notifications: +<%= @notification_url %> + +Unsubscribe or change your settings: +<%= @unsubscribe_url %> diff --git a/app/views/notifications_mailer/weekly_staff_summary.html.erb b/app/views/notifications_mailer/weekly_staff_summary.html.erb deleted file mode 100644 index c8360dfde8..0000000000 --- a/app/views/notifications_mailer/weekly_staff_summary.html.erb +++ /dev/null @@ -1,160 +0,0 @@ - - - - - - -
-

<%= @summary_stats[:unit].name %> - Weekly Summary

-

<%= "#{@summary_stats[:week_start].day.ordinalize} #{@summary_stats[:week_start].strftime("%B %Y")}" %> to <%= "#{@summary_stats[:week_end].day.ordinalize} #{@summary_stats[:week_end].strftime("%B %Y")}" %>

- -

Hi <%= @staff.first_name %>,

-

- Hope you had a good week! Here's a summary of what has happened in this unit over the last week. -

-<% if @unit_role.has_students? %> -

-

-

-<% if @summary_stats[:revert][@staff].count > 0 %> -

- The following students in your tutorials have been advised to revert their grade to Pass as they are falling behind with their pass tasks: -

-

-<% end %> -<%# Else if unit role has no students %> -<% else %> -

-

-

-<% end %> - -<% if @unit_role.is_convenor? && !@summary_stats[:tutorial_streams].nil? && !@summary_stats[:tutorial_streams].empty?%> - <%# Create a table for each tutorial stream %> - <% @summary_stats[:tutorial_streams].each do |tutorial_stream, tutorial_stream_data | %> - <% next unless tutorial_stream_data[:stream_linked_to_task_definition] %> -

- Tutorial Stream: <%= tutorial_stream.name %> -

- <%# Skip table if no tutorials%> - <% unless tutorial_stream_data.nil? || tutorial_stream_data[:tutorials].nil?%> - - - - - - - - - - - - - - - - - - - - - - <% sorted_data = tutorial_stream_data[:unit_roles].sort_by do |unit_role, data| - [-data[:oldest_task_days].to_i, -data[:tasks_awaiting_feedback_count].to_i] - end %> - <% sorted_data.each do | unit_role, data | %> - <% next unless data[:number_of_students] > 0%> - - - - - - - - - - - - - <% end %> - -
NameStudentsAssessmentsCommentsAwaiting FeedbackOldest TaskDiscussions
TotalWeekTotalWeekTotalWeek
<%= '%-12s' % unit_role.user.name.truncate(14) %><%= '%8i' % data[:number_of_students] %><%= '%7i' % data[:total_staff_engagements] %><%= '%6i' % data[:staff_engagements] %><%= '%5i' % data[:total_comments].uniq.count %><%= '%5i' % data[:sent_comments].uniq.count %><%= '%8i' % data[:tasks_awaiting_feedback_count] %>><%= '%6i' % data[:oldest_task_days] %> days<%= '%5i' % data[:total_tasks_discussed] %><%= '%5i' % data[:weekly_tasks_discussed] %>
- <% end %> - <% if tutorial_stream_data[:num_students_without_tutors] > 0 %> -

- Please note that <%=tutorial_stream_data[:num_students_without_tutors]%> student<%= "s" unless tutorial_stream_data[:num_students_without_tutors] == 1 %> <%= are_is(tutorial_stream_data[:num_students_without_tutors]) %> not allocated to a tutorial in this stream. Work submitted by <%= this_these(tutorial_stream_data[:num_students_without_tutors]) %> student<%= "s" unless tutorial_stream_data[:num_students_without_tutors] == 1 %> will not appear in any of the tutor inboxes! -

- <% end %> - <% end %> -<% end %> -

- Cheers,
- The <%= @doubtfire_product_name %> Team on behalf of <%= @convenor.name %> -

-
- - - diff --git a/app/views/notifications_mailer/weekly_staff_summary.text.erb b/app/views/notifications_mailer/weekly_staff_summary.text.erb deleted file mode 100644 index 1bdde187a8..0000000000 --- a/app/views/notifications_mailer/weekly_staff_summary.text.erb +++ /dev/null @@ -1,70 +0,0 @@ -Hi <%= @staff.first_name %>, - -<%= @summary_stats[:unit].name %> - Weekly Summary -<%= "#{@summary_stats[:week_start].day.ordinalize} #{@summary_stats[:week_start].strftime("%B %Y")}" %> to <%= "#{@summary_stats[:week_end].day.ordinalize} #{@summary_stats[:week_end].strftime("%B %Y")}" %> - -Hope you had a good week! Here's a summary of what has happened in this unit over the last week. - -<% if @unit_role.has_students? %> -* In total, there <%= were_was(@summary_stats[:unit_week_comments]) %> <%= @summary_stats[:unit_week_comments] %> comment<%= "s" if @summary_stats[:unit_week_comments] != 1 %> made this unit. -* You received <%= @data[:received_comments] %> comment<%= "s" if @data[:received_comments] != 1 %> and posted a total of <%= @data[:sent_comments] %> comment<%= "s" if @data[:sent_comments] != 1 %>. -* Tasks changed state <%= @summary_stats[:unit_week_engagements] %> time<%= "s" unless @summary_stats[:unit_week_engagements] == 1 %> in this unit. -* Your student's tasks changed state <%= @data[:weekly_engagements_count] %> time<%= "s" unless @data[:weekly_engagements_count] == 1 %>. -* You assigned a new status to <%= @data[:staff_engagements] %> task<%= "s" unless @data[:staff_engagements] == 1 %>. -* You have discussed <%= @data[:weekly_total_tasks_discussed] %> task<%= "s" unless @data[:weekly_total_tasks_discussed] == 1 %>. -* You have <%= @data[:tasks_awaiting_feedback_count] %> task<%= "s" unless @data[:tasks_awaiting_feedback_count] == 1 %> waiting for feedback. -<% if @data[:tasks_awaiting_feedback_count] > 0 %> -* The oldest of these tasks was submitted <%= @data[:oldest_task_days] %> day<%= "s" unless @data[:oldest_task_days] == 1 %> ago. -<% end %> -<% if @summary_stats[:revert][@staff].count > 0 %> - -The following students in your tutorials have been advised to revert their grade to Pass as they are falling behind with their pass tasks: -<% @summary_stats[:revert][@staff].each do |p|%> -* <%= p.student.name %> -<% end %> -<% end %> -<% else %> -* In total, there <%= were_was(@summary_stats[:unit_week_comments]) %> <%= @summary_stats[:unit_week_comments] %> comment<%= "s" if @summary_stats[:unit_week_comments] != 1 %> made in this unit. -* Tasks changed state <%= @summary_stats[:unit_week_engagements] %> time<%= "s" unless @summary_stats[:unit_week_engagements] == 1 %> in this unit. -<% end %> - -<% if @unit_role.is_convenor? && !@summary_stats[:tutorial_streams].nil? && !@summary_stats[:tutorial_streams].empty?%> -Here is a quick summary of how the tutors are going in this unit: - -<% @summary_stats[:tutorial_streams].each do |tutorial_stream, tutorial_stream_data | %> -<% next unless tutorial_stream_data[:stream_linked_to_task_definition] %> -Tutorial Stream: <%= tutorial_stream.name %> -<% unless tutorial_stream_data.nil? || tutorial_stream_data[:tutorials].nil?%> -------------------------------------------------------------------------------------------------- -Name | Number | Task Assessments | Comments Made | Awaiting | Oldest | Tasks Discussed - | Students | Total | Week | Total | Week | Feedback | Task | Total | Week -------------------------------------------------------------------------------------------------- -<% sorted_data = tutorial_stream_data[:unit_roles].sort_by do |unit_role, data| - [-data[:oldest_task_days].to_i, -data[:tasks_awaiting_feedback_count].to_i] - end %> -<% sorted_data.each do | unit_role, data | %> -<% next unless data[:number_of_students] > 0%> -<%='%-14s' % unit_role.user.name.truncate(14) %> | <%= '%8i' % data[:number_of_students] %> | <%= '%7i' % data[:total_staff_engagements] %> | <%= '%6i' % data[:staff_engagements] %> | <%= '%5i' % data[:total_comments].uniq.count %> | <%= '%5i' % data[:sent_comments].uniq.count %> | <%= '%8i' % data[:tasks_awaiting_feedback_count] %> | <%= '%5i' % data[:oldest_task_days] %>d | <%= '%6i' % data[:total_tasks_discussed] %> | <%= '%5i' % data[:weekly_tasks_discussed] %> | -<% end %> ------------------------------------------------------------------------------------------------------------------ -<% end %> -<% if tutorial_stream_data[:num_students_without_tutors].to_i > 0 %> -Please note that <%=tutorial_stream_data[:num_students_without_tutors].to_i%> student<%= "s" unless tutorial_stream_data[:num_students_without_tutors].to_i == 1 %> <%= are_is(tutorial_stream_data[:num_students_without_tutors].to_i) %> not allocated to a tutorial in this stream. Work submitted by <%= this_these(tutorial_stream_data[:num_students_without_tutors].to_i) %> student<%= "s" unless tutorial_stream_data[:num_students_without_tutors].to_i == 1 %> will not appear in any of the tutor inboxes! -<% end %> - -<% end %> - - -<% if @summary_stats[:revert_count] > 0 %> -<%= @summary_stats[:revert_count] %> student<%= "s" unless @summary_stats[:revert_count] == 1%> <%= were_was(@summary_stats[:revert_count])%> automatically reverted to Pass target grade due to progress on their pass tasks. -<% end %> -<% end %> - -Cheers, -The <%= @doubtfire_product_name %> Team on behalf of <%= @convenor.name %> - ---- - -Visit <%= @unsubscribe_url%> to unsubscribe from these notifications. - -Generated with <%= @doubtfire_product_name %> diff --git a/app/views/notifications_mailer/weekly_student_summary.html.erb b/app/views/notifications_mailer/weekly_student_summary.html.erb deleted file mode 100644 index 3a0593d36e..0000000000 --- a/app/views/notifications_mailer/weekly_student_summary.html.erb +++ /dev/null @@ -1,118 +0,0 @@ - - - - - - -
-

<%= @summary_stats[:unit].name %> - Weekly Summary

-

<%= "#{@summary_stats[:week_start].day.ordinalize} #{@summary_stats[:week_start].strftime("%B %Y")}" %> to <%= "#{@summary_stats[:week_end].day.ordinalize} #{@summary_stats[:week_end].strftime("%B %Y")}" %>

- -

Hi <%= @student.first_name %>,

-<% if @did_revert_to_pass %> -

Hope you had a good week!

-

Before we get into the summary, it seems that you are falling behind in your Pass Tasks. It is really important that you catch up with these tasks, as you must have all Pass tasks marked as Complete to Pass the unit. I've reset your Target Grade to Pass for the moment, to help you focus on these tasks. I would like to encourage you to work through the Pass tasks in order to catch up as quickly as you can. Once you have caught up, you can upgrade your Target Grade again, and go back and complete any higher grade tasks you skipped.

-

With that out of the way, here's a summary of what has happened in this unit over the last week and some notes on what you should do next.

-<% else %> -

Hope you had a good week! Here's a summary of what has happened in this unit over the last week and some notes on what you should do next.

-<% end %> - -<% if @project.tutorial_enrolments.blank? %> -

Firstly... it looks like you are not assigned a tutor! Please login and make sure your tutorial is correctly set. You should be able to do that on the tutorials page.

-<% end %> -

Here is what you should focus on right now.

- -<% if @top_tasks && @top_tasks.count > 0 %> -<% if @overdue_top && @overdue_top.count > 0 %> -

Catch up by completing the following overdue task<%="s" if @overdue_top.count > 1%>!

- -<% end %> -<% if @soon_top && @soon_top.count > 0 %> -

Work to get the following task<%="s" if @soon_top.count > 1%> done, as these are due soon!

- -<% end %> -<% if @ahead_top && @ahead_top.count > 0 %> -

Get ahead by working on the following task<%="s" if @ahead_top.count > 1%> next!

- -<% end %> -<% elsif @project.portfolio_exists? %> -

Its time to Party! You have completed all of the tasks and prepared your portfolio.

-<% else %> -

Its almost party time... You have completed all of the tasks, now make sure you login and prepare your portfolio.

-<% end %> -

- What has happened in <%= @doubtfire_product_name %> this week: -

- We hope your studies are going well, and look forward to your submissions over the next week! -

- -

- Cheers,
- The <%= @doubtfire_product_name %> Team on behalf of <%= @tutor.name %> -

-
- - - diff --git a/app/views/notifications_mailer/weekly_student_summary.text.erb b/app/views/notifications_mailer/weekly_student_summary.text.erb deleted file mode 100644 index 808fc7c3bd..0000000000 --- a/app/views/notifications_mailer/weekly_student_summary.text.erb +++ /dev/null @@ -1,67 +0,0 @@ -Hi <%= @student.first_name %>, - -<%= @summary_stats[:unit].name %> - Weekly Summary -<%= "#{@summary_stats[:week_start].day.ordinalize} #{@summary_stats[:week_start].strftime("%B %Y")}" %> to <%= "#{@summary_stats[:week_end].day.ordinalize} #{@summary_stats[:week_end].strftime("%B %Y")}" %> - -<% if @did_revert_to_pass %> -Hope you had a good week! - -Before we get into the summary, it seems that you are falling behind in your Pass Tasks. It is really important that you catch up with these tasks, as you must have all Pass tasks marked as Complete to Pass the unit. I've reset your Target Grade to Pass for the moment, to help you focus on these tasks. I would like to encourage you to work through the Pass tasks in order to catch up as quickly as you can. Once you have caught up, you can upgrade your Target Grade again, and go back and complete any higher grade tasks you skipped. - -With that out of the way, here's a summary of what has happened in this unit over the last week and some notes on what you should do next. -<% else %> -Hope you had a good week! Here's a summary of what has happened in this unit over the last week and some notes on what you should do next. -<% end %> - -<% if @project.tutorial_enrolments.blank? %> - -Firstly... it looks like you are not assigned a tutor! Please login and make sure your tutorial is correctly set. You should be able to do that here: <%= @doubtfire_host %>/#/projects/<%= @project.id %>/tutorials - -Here is what you should focus on right now. -<% else %> - -Firstly, here is what you should focus on right now. -<% end %> - -<% if @top_tasks && @top_tasks.count > 0 %> -<% if @overdue_top && @overdue_top.count > 0 %> -Catch up by completing the following *overdue* task<%="s" if @overdue_top.count > 1%>! -<% @overdue_top.each do |ot| %> -* <%= top_task_desc(ot) %> -<% end %> -<% end %> -<% if @soon_top && @soon_top.count > 0 %> -Work to get the following task<%="s" if @soon_top.count > 1%> done, as these are *due* soon! -<% @soon_top.each do |st| %> -* <%= top_task_desc(st) %> -<% end %> -<% end %> -<% if @ahead_top && @ahead_top.count > 0 %> -Get ahead by working on the following task<%="s" if @ahead_top.count > 1%> next! -<% @ahead_top.each do |at| %> -* <%= top_task_desc(at) %> -<% end %> -<% end %> -<% elsif @project.portfolio_exists? %> -Its time to Party! You have completed all of the tasks and prepared your portfolio. -<% else %> -Its almost party time... You have completed all of the tasks, now make sure you login and prepare your portfolio. -<% end %> - -What has happened in <%= @doubtfire_product_name %> this week: - -* In total, there <%= were_was(@summary_stats[:unit_week_comments]) %> <%= @summary_stats[:unit_week_comments] %> comment<%= "s" if @summary_stats[:unit_week_comments] != 1 %> made in this unit. -* You posted a total of <%= @sent_comments %> comment<%= "s" if @sent_comments != 1 %>, and received back <%= @received_comments %> comment<%= "s" if @received_comments != 1 %> <%= "- try posting some comments this week" if @sent_comments == 0 %> -* Tasks changed state <%= @summary_stats[:unit_week_engagements] %> time<%= "s" unless @summary_stats[:unit_week_engagements] == 1 %> in this unit. -* Your tasks changed state <%= @engagements_count %> time<%= "s" unless @engagements_count == 1 %> <%= "- looks like you need to be more active" if @student_engagements == 0%> - -We hope your studies are going well, and look forward to your submissions over the next week! - -Cheers, -The <%= @doubtfire_product_name %> Team on behalf of <%= @tutor.name %> - ---- - -Visit <%= @unsubscribe_url%> to unsubscribe from these notifications. - -Generated with <%= @doubtfire_product_name %> diff --git a/app/views/portfolio_evidence_mailer/task_feedback_ready.html.erb b/app/views/portfolio_evidence_mailer/task_feedback_ready.html.erb deleted file mode 100644 index 362e6f86b6..0000000000 --- a/app/views/portfolio_evidence_mailer/task_feedback_ready.html.erb +++ /dev/null @@ -1,70 +0,0 @@ - - - - - - -
-

<%= @doubtfire_product_name %> Notification

-

Hi <%= @student.first_name %>,

-

- I have checked your tasks and provided some feedback. Login and check the status <%= @hasComments ? 'and comments' : ''%> of the following tasks: -

-

-

- Cheers,
- The <%= @doubtfire_product_name %> Team on behalf of <%= @tutor.name %> -

-
- - - diff --git a/app/views/portfolio_evidence_mailer/task_feedback_ready.text.erb b/app/views/portfolio_evidence_mailer/task_feedback_ready.text.erb deleted file mode 100644 index d40a8c7ce5..0000000000 --- a/app/views/portfolio_evidence_mailer/task_feedback_ready.text.erb +++ /dev/null @@ -1,16 +0,0 @@ -Hi <%= @student.first_name %>, - -I have checked your tasks and provided some feedback. Login and check the status<%= @has_comments ? ' and comments' : ''%> of the following tasks: - -<% @tasks.each do |task| %> - * <%= task.task_definition.abbreviation %> - <%=task.task_definition.name%> <%= task.is_last_comment_by?(@tutor) ? ' - with comments' : ''%> -<% end %> - -Cheers, -The <%= @doubtfire_product_name %> Team on behalf of <%= @tutor.name %> - ---- - -Visit <%= @unsubscribe_url%> to unsubscribe from these notifications. - -Generated with <%= @doubtfire_product_name %> diff --git a/config/schedule.yml b/config/schedule.yml index 00a51683b6..3b637fa52c 100644 --- a/config/schedule.yml +++ b/config/schedule.yml @@ -28,6 +28,22 @@ notify_discuss_timeout: cron: "every day at 8am" class: "NotifyDiscussTimeoutJob" +notify_task_deadlines: + cron: "every hour" + class: "NotifyTaskDeadlinesJob" + +notify_feedback_warnings: + cron: "every hour" + class: "NotifyFeedbackWarningsJob" + +poll_notification_digests: + cron: "every 5 minutes" + class: "PollNotificationDigestsJob" + +prune_notifications: + cron: "every day at 2am" + class: "PruneNotificationsJob" + sync_lms_integrations: cron: "every day at 3am" class: "SyncLmsIntegrationsJob" diff --git a/db/migrate/20260924110628_create_notifications_and_preferences.rb b/db/migrate/20260924110628_create_notifications_and_preferences.rb new file mode 100644 index 0000000000..65ae1e0965 --- /dev/null +++ b/db/migrate/20260924110628_create_notifications_and_preferences.rb @@ -0,0 +1,88 @@ +class CreateNotificationsAndPreferences < ActiveRecord::Migration[8.0] + def change + create_table :notifications do |t| + t.references :recipient, null: false, index: false + t.references :unit, null: false + t.references :project, null: true + t.references :task, null: true + t.references :actor, null: true + + t.string :kind, null: false, limit: 64 + # Identifies the event, so cron re-runs and job retries cannot raise it twice. + t.string :deduplication_key, null: false, limit: 191 + + # Rendered content for messages sent by the communications system. + t.string :message_subject + t.text :message_body + + # What raised the notification + t.references :task_comment, null: true + t.references :overseer_assessment, null: true, index: false + t.references :tutor_note, null: true, index: false + + # Set only for the kinds that need them + t.references :task_status, null: true, index: false # what a status change moved to + t.references :unit_role, null: true, index: false # whose moderation notes were written on + t.date :discuss_deadline, null: true # when the task has to be discussed by + + t.datetime :read_at + t.datetime :email_processed_at + t.datetime :email_sent_at + t.datetime :email_not_before # holds a failed run back until the student has had a chance to read it + + t.timestamps + end + + 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' + + # One row per user. `channels` maps each notification to the channels it is + # delivered on, and applies to every unit the user is in: + # + # { "new_task_comment" => ["in_app", "email"], "overseer_failed" => [] } + # + create_table :notification_settings do |t| + t.references :user, null: false, index: { unique: true } + + t.json :channels, null: false + + t.string :digest_frequency, null: false, default: 'weekly', limit: 16 + t.integer :digest_interval_hours, null: false, default: 4 + # The wall-clock anchor for intervals that continue through the night. + t.string :digest_start_time, null: false, default: '07:00', limit: 5 + t.string :digest_time, null: false, default: '07:00', limit: 5 + t.string :digest_timezone, null: false, limit: 64 + t.integer :digest_weekday, null: false, default: 1 + t.datetime :next_digest_at + t.datetime :last_digest_at + + t.timestamps + end + + add_index :notification_settings, :next_digest_at + + # One row per unit the user has changed, holding only what differs from their + # notification_settings. A muted unit sends nothing, and null channels mean + # the unit still uses the channels from notification_settings. + create_table :notification_unit_overrides do |t| + t.references :user, null: false, index: false + t.references :unit, null: false + + t.boolean :muted, null: false, default: false + t.json :channels, null: true + + t.timestamps + end + + add_index :notification_unit_overrides, [:user_id, :unit_id], + unique: true, + name: 'index_notification_unit_overrides_on_user_and_unit' + end +end diff --git a/db/schema.rb b/db/schema.rb index 7bbf1f038c..53baa9b92d 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_09_16_003706) do +ActiveRecord::Schema[8.0].define(version: 2026_09_24_110628) 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 @@ -416,6 +416,69 @@ t.index ["task_id"], name: "index_moderated_tasks_on_task_id" end + create_table "notification_settings", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "user_id", null: false + t.text "channels", size: :long, null: false, collation: "utf8mb4_bin" + t.string "digest_frequency", limit: 16, default: "weekly", null: false + t.integer "digest_interval_hours", default: 4, null: false + t.string "digest_start_time", limit: 5, default: "07:00", null: false + t.string "digest_time", limit: 5, default: "07:00", null: false + t.string "digest_timezone", limit: 64, null: false + t.integer "digest_weekday", default: 1, 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_settings_on_next_digest_at" + t.index ["user_id"], name: "index_notification_settings_on_user_id", unique: true + t.check_constraint "json_valid(`channels`)", name: "channels" + end + + create_table "notification_unit_overrides", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "user_id", null: false + t.bigint "unit_id", null: false + t.boolean "muted", default: false, null: false + t.text "channels", size: :long, collation: "utf8mb4_bin" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["unit_id"], name: "index_notification_unit_overrides_on_unit_id" + t.index ["user_id", "unit_id"], name: "index_notification_unit_overrides_on_user_and_unit", unique: true + t.check_constraint "json_valid(`channels`)", name: "channels" + 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 "deduplication_key", limit: 191, null: false + t.string "message_subject" + t.text "message_body" + t.bigint "task_comment_id" + t.bigint "overseer_assessment_id" + t.bigint "tutor_note_id" + t.bigint "task_status_id" + t.bigint "unit_role_id" + t.date "discuss_deadline" + t.datetime "read_at" + t.datetime "email_processed_at" + t.datetime "email_sent_at" + t.datetime "email_not_before" + 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 ["task_comment_id"], name: "index_notifications_on_task_comment_id" + t.index ["task_id"], name: "index_notifications_on_task_id" + t.index ["unit_id"], name: "index_notifications_on_unit_id" + 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 diff --git a/lib/tasks/generate_pdfs.rake b/lib/tasks/generate_pdfs.rake index 5db26d07e7..df4cdcfe54 100644 --- a/lib/tasks/generate_pdfs.rake +++ b/lib/tasks/generate_pdfs.rake @@ -148,16 +148,17 @@ namespace :submission do end end - next unless project.student.receive_portfolio_notifications + notification = Notification.create_for_portfolio(project, success: success) + next if notification.nil? + + settings = NotificationSetting.for(project.student) + next unless settings.delivers?(project.unit, notification.kind, :email) logger.info "emailing portfolio notification to #{project.student.name}" begin - if success - PortfolioEvidenceMailer.portfolio_ready(project).deliver_now - else - PortfolioEvidenceMailer.portfolio_failed(project).deliver_now - end + mail = success ? PortfolioEvidenceMailer.portfolio_ready(project) : PortfolioEvidenceMailer.portfolio_failed(project) + mail.deliver_now if mail.present? rescue StandardError => e logger.error "Failed to send portfolio email for project #{project.id}!\n#{e.message}" end diff --git a/lib/tasks/maintenance.rake b/lib/tasks/maintenance.rake index df7056c42a..3b8ced02ff 100644 --- a/lib/tasks/maintenance.rake +++ b/lib/tasks/maintenance.rake @@ -45,7 +45,9 @@ namespace :maintenance do end def notify_failed_submission(task, message) - if task.project.student.receive_task_notifications + Notification.create_pdf_failure(task) + + if NotificationSetting.for(task.project.student).delivers?(task.unit, 'pdf_generation_failed', :email) begin PortfolioEvidenceMailer.task_pdf_failed(task.project, [task]).deliver_now rescue StandardError => e diff --git a/lib/tasks/overseer_notifications.rake b/lib/tasks/overseer_notifications.rake index 2b69ee3f9e..35f01ba50a 100644 --- a/lib/tasks/overseer_notifications.rake +++ b/lib/tasks/overseer_notifications.rake @@ -6,15 +6,21 @@ namespace :overseer_notifications do assessments.group_by(&:project).each do |project, project_assessments| tasks = project_assessments.map(&:task).uniq + mark_notified = -> { project_assessments.each { |assessment| assessment.update!(student_notified_at: Time.current) } } + + # The student may have switched this email off, or muted the unit. Mark them + # notified anyway, so they are not reconsidered every ten minutes. + unless NotificationSetting.for(project.student).delivers?(project.unit, 'overseer_failed', :email) + mark_notified.call + next + end begin mail = PortfolioEvidenceMailer.overseer_assessment_failed(project, tasks) next if mail.blank? mail.deliver_now - project_assessments.each do |assessment| - assessment.update!(student_notified_at: Time.current) - end + mark_notified.call rescue StandardError => e Rails.logger.error "Failed to send overseer assessment email for project #{project.id}!\n#{e.message}" end diff --git a/lib/tasks/send_status_emails.rake b/lib/tasks/send_status_emails.rake index 98ce225b57..1127bd8ba3 100644 --- a/lib/tasks/send_status_emails.rake +++ b/lib/tasks/send_status_emails.rake @@ -1,16 +1,6 @@ namespace :mailer do + desc 'Create this week\'s opted-in summary notifications without sending legacy emails' task send_status_emails: :environment do - summary_stats = {} - - summary_stats[:week_end] = Time.zone.now - summary_stats[:week_start] = summary_stats[:week_end] - 7.days - summary_stats[:weeks_comments] = TaskComment.where("created_at >= :start AND created_at < :end", start: summary_stats[:week_start], end: summary_stats[:week_end]).count - summary_stats[:weeks_engagements] = TaskEngagement.where("engagement_time >= :start AND engagement_time < :end", start: summary_stats[:week_start], end: summary_stats[:week_end]).count - - Unit.where(active: true).find_each do |unit| - next unless summary_stats[:week_end] > unit.start_date && summary_stats[:week_start] < unit.end_date - - unit.send_weekly_status_emails(summary_stats) - end + CreateWeeklySummaryNotificationsJob.new.perform_now end end diff --git a/test/api/comments/status_test.rb b/test/api/comments/status_test.rb index 9c2e729b12..49d2164736 100644 --- a/test/api/comments/status_test.rb +++ b/test/api/comments/status_test.rb @@ -10,9 +10,18 @@ def app end def test_status_comments - project = Project.first + unit = FactoryBot.create( + :unit, + task_count: 0, + student_count: 1, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + allow_flexible_dates: false, + mark_late_submissions_as_assess_in_portfolio: false + ) + project = unit.active_projects.first user = project.student - unit = project.unit td = TaskDefinition.new({ unit_id: unit.id, diff --git a/test/api/notifications_api_test.rb b/test/api/notifications_api_test.rb new file mode 100644 index 0000000000..fa6d81919c --- /dev/null +++ b/test/api/notifications_api_test.rb @@ -0,0 +1,338 @@ +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'] + assert_equal({ @unit.id.to_s => 1 }, last_response_body['unread_counts_by_unit']) + 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'] + assert_equal({ @unit.id.to_s => 1 }, last_response_body['unread_counts_by_unit']) + end + + def test_unread_count_returns_grouped_counts_for_each_unit + other_unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + FactoryBot.create(:notification, recipient: @student, unit: other_unit) + + get '/api/notifications/unread_count' + + assert_equal 200, last_response.status + assert_equal 2, last_response_body['count'] + assert_equal({ @unit.id.to_s => 1, other_unit.id.to_s => 1 }, last_response_body['unread_counts_by_unit']) + end + + def test_feedback_warnings_group_by_unit_and_email_delivery_batch + @student.received_notifications.destroy_all + delivered_at = Time.current.change(usec: 0) + 2.times do + FactoryBot.create( + :notification, + recipient: @student, + unit: @unit, + kind: 'feedback_warning', + email_processed_at: delivered_at, + email_sent_at: delivered_at + ) + end + FactoryBot.create( + :notification, + recipient: @student, + unit: @unit, + kind: 'feedback_warning' + ) + + get '/api/notifications', state: 'unread' + + assert_equal 2, last_response_body['unread_count'] + assert_equal [1, 2], last_response_body['groups'].map { |group| group.dig('counts', 'feedback_warning') }.sort + assert_equal [{ 'type' => 'unit_inbox', 'unit_id' => @unit.id }], + last_response_body['groups'].pluck('destination').uniq + end + + def test_portfolio_notifications_are_grouped_by_project + @student.received_notifications.destroy_all + Notification.create_for_portfolio(@project, success: true) + + get '/api/notifications' + + assert_equal 200, last_response.status + assert_equal 1, last_response_body['unread_count'] + assert_equal @project.id, last_response_body.dig('groups', 0, 'project_id') + assert_equal({ 'portfolio_ready' => 1 }, last_response_body.dig('groups', 0, 'counts')) + end + + def test_communication_emails_are_returned_individually_with_their_full_content + @student.received_notifications.destroy_all + 2.times do |index| + Notification.create_for_communication_email( + recipient: @student, + unit: @unit, + project: @project, + actor: @tutor, + subject: "Update #{index}", + body: "Full email body #{index}", + deduplication_key: "communication-email:api-test:#{index}" + ) + end + + get '/api/notifications', state: 'unread' + + assert_equal 200, last_response.status + assert_equal 2, last_response_body['unread_count'] + assert_equal 2, last_response_body['groups'].count + assert_equal ['Update 0', 'Update 1'], last_response_body['groups'].pluck('message_subject').sort + assert_equal ['Full email body 0', 'Full email body 1'], last_response_body['groups'].pluck('message_body').sort + end + + def test_weekly_summaries_are_returned_individually_with_structured_statistics + @student.received_notifications.destroy_all + settings = NotificationSetting.for(@student) + settings.update!(channels: settings.channels.merge('weekly_summary' => %w[in_app email])) + data = { + audience: 'student', + week_start: 1.week.ago.iso8601, + week_end: Time.current.iso8601, + unit_comments: 8, + unit_task_activity: 12, + sent_comments: 2, + received_comments: 3, + task_activity: 4, + student_task_activity: 4, + tutor_allocated: true, + top_tasks: [] + } + Notification.create_weekly_summary(recipient: @student, unit: @unit, project: @project, data: data) + + get '/api/notifications', state: 'unread' + + assert_equal 200, last_response.status + assert_equal 1, last_response_body['unread_count'] + assert_equal({ 'weekly_summary' => 1 }, last_response_body.dig('groups', 0, 'counts')) + assert_equal 3, last_response_body.dig('groups', 0, 'weekly_summary', 'received_comments') + assert_nil last_response_body.dig('groups', 0, 'message_body') + end + + def test_get_filters_groups_by_category_and_search + get '/api/notifications', + state: 'unread', + kinds: ['new_task_comment'], + query: @unit.code + + assert_equal 200, last_response.status + assert_equal 1, last_response_body['groups'].count + assert_equal({ 'new_task_comment' => 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_read_cannot_clear_a_tutor_note_notification + notification, = create_tutor_note_notification(recipient: @student) + + put_json '/api/notifications/read', notification_ids: [notification.id] + + assert_equal 200, last_response.status + assert_equal 0, last_response_body['count'] + assert_nil 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_mark_all_read_leaves_tutor_note_notifications_unread + notification, = create_tutor_note_notification(recipient: @student) + + put_json '/api/notifications/read_all', {} + + assert_equal 200, last_response.status + assert_nil notification.reload.read_at + end + + def test_marking_a_tutor_note_read_clears_its_recipient_notification + recipient = FactoryBot.create(:user, :convenor) + @unit.employ_staff(recipient, Role.convenor) + notification, tutor_note = create_tutor_note_notification(recipient: recipient) + add_auth_header_for(user: recipient) + + put_json "/api/unit_roles/#{tutor_note.unit_role_id}/tutor_notes/#{tutor_note.id}/mark_as_read", {} + + assert_equal 200, last_response.status + assert_not_nil notification.reload.read_at + assert_not tutor_note.reload.read_by_unit_role + end + + def test_the_tutor_a_note_is_about_still_marks_it_read_for_their_role + notification, tutor_note = create_tutor_note_notification(recipient: @tutor) + add_auth_header_for(user: @tutor) + + put_json "/api/unit_roles/#{tutor_note.unit_role_id}/tutor_notes/#{tutor_note.id}/mark_as_read", {} + + assert_equal 200, last_response.status + assert_not_nil notification.reload.read_at + assert tutor_note.reload.read_by_unit_role + end + + def test_a_kind_switched_off_in_app_is_hidden_but_still_emailed + settings = NotificationSetting.for(@student) + settings.update!(channels: settings.channels.merge('new_task_comment' => ['email'])) + + get '/api/notifications' + + assert_equal 200, last_response.status + assert_empty last_response_body['groups'] + assert_equal 0, last_response_body['unread_count'] + + # The event is still on the ledger, waiting for the digest to pick it up. + assert_equal 1, @student.received_notifications.email_pending.where(kind: 'new_task_comment').count + end + + def test_settings_start_from_the_defaults + get '/api/notification_settings' + + assert_equal 200, last_response.status + assert_equal 'weekly', last_response_body['digest_frequency'] + assert_equal 4, last_response_body['digest_interval_hours'] + assert_equal '07:00', last_response_body['digest_start_time'] + assert_equal @project.campus.timezone, last_response_body['digest_timezone'] + assert_equal %w[in_app email], last_response_body.dig('channels', 'new_task_comment') + assert_equal %w[in_app email], last_response_body.dig('channels', 'feedback_warning') + assert_equal %w[in_app email], last_response_body.dig('channels', 'weekly_summary') + assert_empty last_response_body['units'] + end + + def test_updating_settings_stores_the_schedule_and_the_units_that_differ + put_json '/api/notification_settings', + channels: { new_task_comment: ['in_app'] }, + digest_frequency: 'daily', + digest_interval_hours: 6, + digest_start_time: '09:00', + digest_time: '10:30', + digest_weekday: 1, + units: [{ unit_id: @unit.id, muted: true }] + + assert_equal 200, last_response.status + assert_equal 'daily', last_response_body['digest_frequency'] + assert_equal 6, last_response_body['digest_interval_hours'] + assert_equal '09:00', last_response_body['digest_start_time'] + assert_equal @project.campus.timezone, last_response_body['digest_timezone'] + assert_equal ['in_app'], last_response_body.dig('channels', 'new_task_comment') + assert_equal [{ 'unit_id' => @unit.id, 'muted' => true, 'channels' => nil }], last_response_body['units'] + end + + def test_updating_settings_drops_units_that_no_longer_differ + NotificationUnitOverride.create!(user: @student, unit: @unit, muted: true) + + put_json '/api/notification_settings', + channels: { new_task_comment: ['in_app'] }, + digest_frequency: 'daily', + digest_time: '10:30', + digest_weekday: 1, + units: [] + + assert_equal 200, last_response.status + assert_empty @student.notification_unit_overrides.reload + end + + def test_updating_settings_ignores_units_the_user_cannot_access + inaccessible = FactoryBot.create(:unit, with_students: false, task_count: 0) + + put_json '/api/notification_settings', + channels: { new_task_comment: ['in_app'] }, + digest_frequency: 'daily', + digest_time: '10:30', + digest_weekday: 1, + units: [{ unit_id: inaccessible.id, muted: true }] + + assert_equal 200, last_response.status + assert_empty last_response_body['units'] + end + + def test_updating_settings_rejects_an_unknown_frequency + put_json '/api/notification_settings', digest_frequency: 'fortnightly' + + assert_equal 400, last_response.status + end + + def test_updating_settings_rejects_an_unknown_digest_interval + put_json '/api/notification_settings', digest_interval_hours: 5 + + assert_equal 400, last_response.status + end + + def test_updating_settings_leaves_out_what_was_not_sent + put_json '/api/notification_settings', digest_frequency: 'daily' + NotificationUnitOverride.create!(user: @student, unit: @unit, muted: true) + + # The client only sends what changed, so an absent key must not clear anything. + put_json '/api/notification_settings', digest_time: '06:00' + + assert_equal 200, last_response.status + assert_equal 'daily', last_response_body['digest_frequency'] + assert_equal '06:00', last_response_body['digest_time'] + assert_equal 1, @student.notification_unit_overrides.reload.count + end + + private + + def create_tutor_note_notification(recipient:) + unit_role = @unit.unit_role_for(@tutor) + author = FactoryBot.create(:user, :convenor) + @unit.employ_staff(author, Role.convenor) + tutor_note = unit_role.add_tutor_note(author, 'Please review this moderation note', @task.id) + notification = Notification.create_for_tutor_note(tutor_note, recipient, 'moderation_note_added') + + [notification, tutor_note] + end +end diff --git a/test/api/tutor_notes_api_test.rb b/test/api/tutor_notes_api_test.rb new file mode 100644 index 0000000000..8659955199 --- /dev/null +++ b/test/api/tutor_notes_api_test.rb @@ -0,0 +1,45 @@ +require 'test_helper' + +class TutorNotesApiTest < 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) + @convenor = @unit.main_convenor_user + + @tutor = FactoryBot.create(:user, :tutor) + @unit.employ_staff(@tutor, Role.tutor) + @tutor_role = @unit.unit_role_for(@tutor) + end + + def test_a_note_written_about_you_asks_you_to_mark_it_read + about_me = @tutor_role.add_tutor_note(@convenor, 'Please review this feedback', @task.id) + add_auth_header_for(user: @tutor) + + get "/api/unit_roles/#{@tutor_role.id}/tutor_notes" + + assert_equal 200, last_response.status + note = last_response_body.find { |result| result['id'] == about_me.id } + assert note['requires_current_user_read'] + end + + def test_your_own_note_never_asks_you_to_mark_it_read + mine = @tutor_role.add_tutor_note(@tutor, 'A note on my own moderation notes', @task.id) + add_auth_header_for(user: @tutor) + + get "/api/unit_roles/#{@tutor_role.id}/tutor_notes" + + assert_equal 200, last_response.status + note = last_response_body.find { |result| result['id'] == mine.id } + assert_not note['requires_current_user_read'] + end +end diff --git a/test/factories/notifications.rb b/test/factories/notifications.rb new file mode 100644 index 0000000000..9326499a34 --- /dev/null +++ b/test/factories/notifications.rb @@ -0,0 +1,24 @@ +FactoryBot.define do + factory :notification do + recipient { create(:user) } + unit { create(:unit, with_students: false, task_count: 0) } + kind { 'new_task_comment' } + sequence(:deduplication_key) { |number| "factory-notification-#{number}" } + end + + factory :notification_setting do + user + digest_frequency { 'weekly' } + digest_interval_hours { 4 } + digest_start_time { '08:00' } + digest_time { '07:00' } + digest_timezone { 'UTC' } + digest_weekday { 1 } + end + + factory :notification_unit_override do + user + unit { create(:unit, with_students: false, task_count: 0) } + muted { false } + end +end diff --git a/test/mailers/unit_mail_test.rb b/test/mailers/unit_mail_test.rb index eb8d85ca90..4cc242df6f 100644 --- a/test/mailers/unit_mail_test.rb +++ b/test/mailers/unit_mail_test.rb @@ -2,7 +2,7 @@ require 'grade_helper' class UnitMailTest < ActionMailer::TestCase - def test_send_summary_email + def test_create_weekly_summary_notifications_without_sending_the_legacy_emails unit = FactoryBot.create :unit summary_stats = {} @@ -11,10 +11,24 @@ def test_send_summary_email summary_stats[:week_start] = summary_stats[:week_end] - 7.days summary_stats[:weeks_comments] = TaskComment.where("created_at >= :start AND created_at < :end", start: summary_stats[:week_start], end: summary_stats[:week_end]).count summary_stats[:weeks_engagements] = TaskEngagement.where("engagement_time >= :start AND engagement_time < :end", start: summary_stats[:week_start], end: summary_stats[:week_end]).count + (unit.active_projects.map(&:student) + unit.staff.map(&:user)).uniq.each do |recipient| + settings = NotificationSetting.for(recipient) + settings.update!(channels: settings.channels.merge('weekly_summary' => %w[in_app email])) + end + + assert_no_emails { unit.create_weekly_summary_notifications(summary_stats) } - unit.send_weekly_status_emails(summary_stats) + summaries = Notification.where(unit: unit, kind: 'weekly_summary') + assert_equal unit.active_projects.count + unit.staff.count, summaries.count + student_summary_count = summaries.filter_map(&:weekly_summary_data).count { |data| data['audience'] == 'student' } + staff_summary_count = summaries.filter_map(&:weekly_summary_data).count { |data| data['audience'] == 'staff' } + assert_equal unit.active_projects.count, student_summary_count + assert_equal unit.staff.count, staff_summary_count - assert_equal unit.active_projects.count + unit.staff.count, ActionMailer::Base.deliveries.count + # Retrying the same week's work does not create another copy. + assert_no_difference -> { summaries.reload.count } do + unit.create_weekly_summary_notifications(summary_stats) + end unit.destroy! end @@ -30,8 +44,8 @@ def test_send_portfolio_ready_from_main_convenor mail = PortfolioEvidenceMailer.portfolio_ready(project) - assert_equal 1, mail.from().count - assert_equal convenor.email, mail.from().first + assert_equal 1, mail.from.count + assert_equal convenor.email, mail.from.first assert mail.html_part.body.include? "projects/#{project.id}/portfolio" unit.destroy! end @@ -48,8 +62,8 @@ def test_send_portfolio_fail_from_main_convenor mail = PortfolioEvidenceMailer.portfolio_failed(project) - assert_equal 1, mail.from().count - assert_equal convenor.email, mail.from().first + assert_equal 1, mail.from.count + assert_equal convenor.email, mail.from.first assert mail.html_part.body.include? "projects/#{project.id}/portfolio" unit.destroy! end @@ -73,6 +87,24 @@ def test_send_overseer_assessment_failed_email assert mail.html_part.body.include? "projects/#{project.id}/dashboard/#{task.task_definition.abbreviation}" end + def test_failed_submission_emails_honour_the_students_email_channels + unit = FactoryBot.create(:unit) + project = unit.active_projects.first + task = project.task_for_task_definition(unit.task_definitions.first) + settings = NotificationSetting.for(project.student) + settings.update!( + channels: settings.channels.merge( + 'overseer_failed' => ['in_app'], + 'pdf_generation_failed' => ['in_app'] + ) + ) + + assert_no_emails do + PortfolioEvidenceMailer.overseer_assessment_failed(project, [task]).deliver_now + PortfolioEvidenceMailer.task_pdf_failed(project, [task]).deliver_now + end + end + def test_send_discussion_deadline_emails unit = FactoryBot.create(:unit) project = unit.active_projects.first @@ -131,13 +163,110 @@ def test_discuss_timeout_notifications_send_emails end def test_discuss_timeout_notifications_are_not_queued_for_unenrolled_students - unit = FactoryBot.create(:unit) + unit = FactoryBot.create( + :unit, + discuss_timeout_enabled: true, + discuss_timeout_warning_days: 7, + discuss_timeout_expire_days: 14 + ) project = unit.active_projects.first task = project.task_for_task_definition(unit.task_definitions.first) + task.update!(task_status: TaskStatus.discuss) + task.update!(moved_to_discuss_at: 8.days.ago) project.update!(enrolled: false) - unit.queue_discuss_timeout_email(task, unit.main_convenor_user, :approaching, 7.days.from_now) + # The timeout comment is still recorded, the withdrawn student just is not notified. + assert_equal 1, unit.notify_discuss_timeouts! + + assert_empty Notification.where(recipient: project.student, task: task) + assert_empty SendDiscussTimeoutEmailJob.jobs + end + + def test_discuss_timeout_notification_is_created_when_immediate_email_is_off + unit = FactoryBot.create( + :unit, + discuss_timeout_enabled: true, + discuss_timeout_warning_days: 7, + discuss_timeout_expire_days: 14 + ) + project = unit.active_projects.first + task = project.task_for_task_definition(unit.task_definitions.first) + task.update!(task_status: TaskStatus.discuss) + task.update!(moved_to_discuss_at: 8.days.ago) + settings = NotificationSetting.for(project.student) + settings.update!(channels: settings.channels.merge('discuss_warning' => ['in_app'])) + + assert_equal 1, unit.notify_discuss_timeouts! + + notification = Notification.find_by!(recipient: project.student, task: task, kind: 'discuss_warning') + assert_nil notification.read_at + assert_not_nil notification.email_processed_at + assert_empty SendDiscussTimeoutEmailJob.jobs + end + + def test_discuss_expiry_notification_is_created_when_immediate_email_is_off + unit = FactoryBot.create( + :unit, + discuss_timeout_enabled: true, + discuss_timeout_warning_days: 7, + discuss_timeout_expire_days: 14 + ) + project = unit.active_projects.first + task = project.task_for_task_definition(unit.task_definitions.first) + task.update!(task_status: TaskStatus.discuss) + task.update!(moved_to_discuss_at: 15.days.ago) + settings = NotificationSetting.for(project.student) + settings.update!(channels: settings.channels.merge('discuss_expired' => ['in_app'])) + + assert_equal 1, unit.notify_discuss_timeouts! + + notification = Notification.find_by!(recipient: project.student, task: task, kind: 'discuss_expired') + assert_nil notification.read_at + assert_not_nil notification.email_processed_at + assert_empty SendDiscussTimeoutEmailJob.jobs + end + + # The job is queued a step ahead of its delivery, so it has to re-read the + # setting rather than trust the one that was true when it was queued. + def test_discuss_timeout_email_job_rechecks_the_setting_before_sending + unit = FactoryBot.create( + :unit, + discuss_timeout_enabled: true, + discuss_timeout_warning_days: 7, + discuss_timeout_expire_days: 14 + ) + project = unit.active_projects.first + task = project.task_for_task_definition(unit.task_definitions.first) + task.update!(task_status: TaskStatus.discuss) + task.update!(moved_to_discuss_at: 8.days.ago) + + assert_equal 1, unit.notify_discuss_timeouts! + queued = SendDiscussTimeoutEmailJob.jobs.shift + + settings = NotificationSetting.for(project.student) + settings.update!(channels: settings.channels.merge('discuss_warning' => ['in_app'])) + + assert_no_emails do + SendDiscussTimeoutEmailJob.new.perform(*queued['args']) + end + end + + def test_discuss_timeout_emails_stop_for_a_muted_unit + unit = FactoryBot.create( + :unit, + discuss_timeout_enabled: true, + discuss_timeout_warning_days: 7, + discuss_timeout_expire_days: 14 + ) + project = unit.active_projects.first + task = project.task_for_task_definition(unit.task_definitions.first) + task.update!(task_status: TaskStatus.discuss) + task.update!(moved_to_discuss_at: 8.days.ago) + NotificationUnitOverride.create!(user: project.student, unit: unit, muted: true) + + assert_equal 1, unit.notify_discuss_timeouts! + assert_empty Notification.where(recipient: project.student, task: task, kind: 'discuss_warning') assert_empty SendDiscussTimeoutEmailJob.jobs end @@ -170,6 +299,7 @@ def test_batch_feedback_updates_unenrolled_students_without_emailing_them assert_empty errors assert_equal TaskStatus.complete, task.reload.task_status assert_equal 'Imported feedback', task.comments.last.comment + assert_empty Notification.where(recipient: project.student, task: task) end end diff --git a/test/models/notification_setting_test.rb b/test/models/notification_setting_test.rb new file mode 100644 index 0000000000..236087498c --- /dev/null +++ b/test/models/notification_setting_test.rb @@ -0,0 +1,211 @@ +require 'test_helper' + +class NotificationSettingTest < ActiveSupport::TestCase + def test_default_digest_timezone_falls_back_when_tz_is_unset + original_timezone = ENV.delete('TZ') + + assert_equal Time.zone.tzinfo.name, NotificationSetting.default_digest_timezone + ensure + ENV['TZ'] = original_timezone if original_timezone + end + + def test_defaults_to_a_monday_morning_digest_on_every_channel_but_push + settings = NotificationSetting.for(FactoryBot.create(:user)) + + assert_equal 'weekly', settings.digest_frequency + assert_equal 4, settings.digest_interval_hours + assert_equal '07:00', settings.digest_start_time + assert_equal '07:00', settings.digest_time + assert_equal NotificationSetting.default_digest_timezone, settings.digest_timezone + assert_equal 1, settings.digest_weekday + assert_equal %w[in_app email], settings.channels['new_task_comment'] + assert_equal %w[in_app email], settings.channels['weekly_summary'] + assert_equal ['in_app'], settings.channels['communication_email'] + assert_equal Notification::KINDS.sort, settings.channels.keys.sort + end + + def test_a_unit_without_a_preference_follows_the_defaults + settings = NotificationSetting.for(FactoryBot.create(:user)) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + + assert settings.delivers?(unit, 'new_task_comment', :email) + assert_not settings.delivers?(unit, 'new_task_comment', :push) + end + + def test_a_customised_unit_uses_its_own_channels + settings = NotificationSetting.for(FactoryBot.create(:user)) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + FactoryBot.create( + :notification_unit_override, + user: settings.user, + unit: unit, + channels: { 'new_task_comment' => ['push'] } + ) + + assert_not settings.delivers?(unit, 'new_task_comment', :email) + assert settings.delivers?(unit, 'new_task_comment', :push) + end + + def test_a_muted_unit_delivers_nothing + settings = NotificationSetting.for(FactoryBot.create(:user)) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + FactoryBot.create(:notification_unit_override, user: settings.user, unit: unit, muted: true) + + assert_empty settings.channels_for_unit_id(unit.id, 'new_task_comment') + end + + def test_weekly_summary_honours_the_unit_mute + settings = NotificationSetting.for(FactoryBot.create(:user)) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + settings.update!(channels: settings.channels.merge('weekly_summary' => %w[in_app email])) + + assert settings.weekly_summary_for?(unit) + + FactoryBot.create(:notification_unit_override, user: settings.user, unit: unit, muted: true) + assert_not settings.weekly_summary_for?(unit) + end + + def test_weekly_summary_can_be_opted_out_of_without_changing_other_notifications + settings = NotificationSetting.for(FactoryBot.create(:user)) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + + assert settings.weekly_summary_for?(unit) + settings.update!(channels: settings.channels.merge('weekly_summary' => [])) + + assert_not settings.weekly_summary_for?(unit) + assert settings.delivers?(unit, 'new_task_comment', :email) + end + + def test_weekly_summary_is_due_in_the_users_timezone + user = FactoryBot.create(:user) + campus = FactoryBot.create(:campus, timezone: 'Pacific/Auckland') + FactoryBot.create(:project, user: user, campus: campus) + settings = FactoryBot.create(:notification_setting, user: user) + + assert settings.weekly_summary_due?(Time.utc(2026, 7, 26, 19, 5)) + assert_not settings.weekly_summary_due?(Time.utc(2026, 7, 26, 18, 55)) + assert_not settings.weekly_summary_due?(Time.utc(2026, 7, 26, 19, 15)) + end + + def test_a_muted_unit_keeps_following_the_defaults_underneath + settings = NotificationSetting.for(FactoryBot.create(:user)) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + preference = FactoryBot.create( + :notification_unit_override, + user: settings.user, + unit: unit, + muted: true + ) + + assert_not preference.customised? + + preference.update!(muted: false) + assert settings.delivers?(unit, 'new_task_comment', :email) + end + + def test_daily_delivery_keeps_its_time_across_a_daylight_saving_transition + user = FactoryBot.create(:user) + campus = FactoryBot.create(:campus, timezone: 'Australia/Melbourne') + FactoryBot.create(:project, user: user, campus: campus) + settings = FactoryBot.build( + :notification_setting, + user: user, + digest_frequency: 'daily', + digest_time: '09:00' + ) + from = Time.zone.local(2026, 10, 3, 13, 30, 0) + + next_occurrence = settings.next_occurrence(from) + + assert_equal Date.new(2026, 10, 4), next_occurrence.to_date + assert_equal 9, next_occurrence.hour + end + + def test_delivery_uses_the_first_enrolled_projects_campus_timezone + user = FactoryBot.create(:user) + first_campus = FactoryBot.create(:campus, timezone: 'Australia/Melbourne') + second_campus = FactoryBot.create(:campus, timezone: 'Pacific/Auckland') + FactoryBot.create(:project, user: user, campus: first_campus, enrolled: true) + FactoryBot.create(:project, user: user, campus: second_campus, enrolled: true) + settings = FactoryBot.build( + :notification_setting, + user: user, + digest_frequency: 'daily', + digest_time: '08:00' + ) + + next_occurrence = settings.next_occurrence(Time.utc(2026, 7, 28, 21, 30)) + + assert_equal Time.utc(2026, 7, 28, 22), next_occurrence + assert_equal 'Australia/Melbourne', next_occurrence.time_zone.name + end + + def test_hourly_delivery_continues_across_midnight_without_drifting + settings = FactoryBot.build( + :notification_setting, + digest_frequency: 'hourly', + digest_interval_hours: 4, + digest_start_time: '08:00' + ) + digest_zone = ActiveSupport::TimeZone[settings.resolved_digest_timezone] + + assert_equal digest_zone.local(2026, 7, 29, 12), + settings.next_occurrence(digest_zone.local(2026, 7, 29, 10, 30)) + assert_equal digest_zone.local(2026, 7, 30, 0), + settings.next_occurrence(digest_zone.local(2026, 7, 29, 20, 30)) + end + + def test_three_hourly_delivery_continues_from_its_anchor + settings = FactoryBot.build( + :notification_setting, + digest_frequency: 'hourly', + digest_interval_hours: 3, + digest_start_time: '08:00' + ) + digest_zone = ActiveSupport::TimeZone[settings.resolved_digest_timezone] + + assert_equal digest_zone.local(2026, 7, 30, 2), + settings.next_occurrence(digest_zone.local(2026, 7, 29, 23, 30)) + end + + def test_weekly_delivery_lands_on_the_selected_weekday + settings = FactoryBot.build( + :notification_setting, + digest_frequency: 'weekly', + digest_weekday: 3, + digest_time: '07:00' + ) + + next_occurrence = settings.next_occurrence(Time.zone.local(2026, 7, 29, 10, 0, 0)) + + assert_equal 3, next_occurrence.to_date.cwday + assert_equal 7, next_occurrence.hour + end + + def test_switching_the_digest_off_processes_notifications_waiting_for_it + settings = FactoryBot.create(:notification_setting) + digested = FactoryBot.create(:notification, recipient: settings.user) + alert = FactoryBot.create(:notification, recipient: settings.user, kind: 'discuss_warning') + + settings.update!(digest_frequency: 'off') + + assert_not_nil digested.reload.email_processed_at + assert_not_nil alert.reload.email_processed_at + assert_nil settings.reload.next_digest_at + end + + def test_unknown_kinds_and_channels_are_rejected + settings = FactoryBot.build(:notification_setting, channels: { 'unknown' => ['smoke'] }) + + assert_not settings.valid? + assert_includes settings.errors[:channels].join, 'unknown' + assert_includes settings.errors[:channels].join, 'smoke' + end + + def test_channels_must_be_an_object + settings = FactoryBot.build(:notification_setting, channels: ['new_task_comment']) + + assert_not settings.valid? + assert_includes settings.errors[:channels], 'must be an object' + end +end diff --git a/test/models/notification_test.rb b/test/models/notification_test.rb new file mode 100644 index 0000000000..37d6624d3a --- /dev/null +++ b/test/models/notification_test.rb @@ -0,0 +1,332 @@ +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(task_comment: comment, recipient: @student) + + assert_not_nil notification + assert_equal 'new_task_comment', 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?(task_comment: comment, recipient: @tutor, kind: 'new_task_comment') + 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(task_comment: 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?(task_comment: 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!(task_comment: first_comment, recipient: @student) + second_notification = Notification.find_by!(task_comment: second_comment, recipient: @student) + + assert_not_nil first_notification.read_at + assert_nil second_notification.read_at + assert_equal TaskStatus.complete, second_notification.task_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) + @task.add_status_comment(@tutor, TaskStatus.complete) + + groups = NotificationGroupBuilder.new(Notification.where(recipient: @student).unread).groups + + assert_equal 1, groups.count + assert_equal 3, groups.first[:counts]['new_task_comment'] + assert_equal 1, groups.first[:counts]['task_status_changed'] + assert_equal :complete, groups.first[:latest_status] + assert_includes groups.first[:summary], '3 new comments' + assert_includes groups.first[:summary], 'task status changed to Complete' + 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!(task_comment: warning, recipient: @student) + expiry_notification = Notification.find_by!(task_comment: 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]['new_task_comment'] + assert_includes group[:summary], 'Discussion deadline missed' + end + + def test_a_tutor_note_groups_apart_from_the_tasks_own_notifications + @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, 'moderation_note_added') + + groups = NotificationGroupBuilder.new(Notification.where(recipient: @tutor).unread).groups + moderation = groups.find { |group| group[:counts]['moderation_note_added'].positive? } + comments = groups.find { |group| group[:counts]['new_task_comment'].positive? } + + assert_equal 2, groups.count + assert_equal 0, moderation[:counts]['new_task_comment'] + assert_equal [tutor_note.id], moderation[:tutor_note_ids] + assert_equal [Notification.find_by!(tutor_note: tutor_note).id], moderation[:tutor_note_notification_ids] + assert_equal unit_role.id, moderation[:tutor_note_unit_role_id] + assert moderation[:tutor_note_on_task_tutor] + assert_includes moderation[:detail], "1 moderation note from #{@unit.main_convenor_user.name}" + assert comments.dig(:task, :staff_view) + assert_equal @student.name, comments.dig(:task, :student_name) + end + + # A group opens one staff member's thread, so notes about two of them on the same + # task cannot share a row - the count would promise more than the thread shows. + def test_notes_about_different_staff_on_one_task_group_apart + other_staff = FactoryBot.create(:user, :tutor) + @unit.employ_staff(other_staff, Role.tutor) + author = @unit.main_convenor_user + about_tutor = @unit.unit_role_for(@tutor).add_tutor_note(author, 'About the tutor', @task.id) + about_other = @unit.unit_role_for(other_staff).add_tutor_note(author, 'About the other tutor', @task.id) + Notification.create_for_tutor_note(about_tutor, @tutor, 'moderation_note_added') + Notification.create_for_tutor_note(about_other, @tutor, 'moderation_note_reply') + + groups = NotificationGroupBuilder.new(Notification.where(recipient: @tutor).unread).groups + + assert_equal 2, groups.count + assert_equal [[about_tutor.id], [about_other.id]].sort, groups.map { |group| group[:tutor_note_ids] }.sort + # Only the task's own tutor's thread can be opened from the task's Mod Notes tab. + assert_equal [false, true], groups.map { |group| group[:tutor_note_on_task_tutor] }.sort_by(&:to_s) + end + + def test_mark_task_read_does_not_clear_a_tutor_note_notification + unit_role = @unit.unit_role_for(@tutor) + tutor_note = unit_role.add_tutor_note(@unit.main_convenor_user, 'Please follow up', @task.id) + notification = Notification.create_for_tutor_note(tutor_note, @tutor, 'moderation_note_added') + + Notification.mark_task_read(@tutor, @task) + + assert_nil notification.reload.read_at + end + + def test_mark_tutor_note_read_clears_only_the_named_recipients_notification + unit_role = @unit.unit_role_for(@tutor) + tutor_note = unit_role.add_tutor_note(@unit.main_convenor_user, 'Please follow up', @task.id) + tutor_notification = Notification.create_for_tutor_note(tutor_note, @tutor, 'moderation_note_added') + other_recipient = FactoryBot.create(:user) + other_notification = Notification.create_for_tutor_note(tutor_note, other_recipient, 'moderation_note_reply') + + Notification.mark_tutor_note_read(@tutor, tutor_note) + + assert_not_nil tutor_notification.reload.read_at + assert_nil other_notification.reload.read_at + 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: 'new_task_comment', + task_comment: comment, + deduplication_key: 'same-event' + } + + 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_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!( + overseer_assessment: 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_portfolio_result_uses_the_notification_ledger + notification = Notification.create_for_portfolio(@project, success: true) + group = NotificationGroupBuilder.new([notification]).groups.first + + assert_equal 'portfolio_ready', notification.kind + assert_equal @project, notification.project + assert_equal @student, notification.recipient + assert_nil notification.task + assert_nil notification.read_at + assert_equal @project.id, group[:project_id] + assert_equal 'Portfolio - Portfolio ready to review', group[:summary] + end + + def test_communication_email_keeps_its_rendered_content_as_an_individual_in_app_notification + notification = Notification.create_for_communication_email( + recipient: @student, + unit: @unit, + project: @project, + actor: @tutor, + subject: 'A personalised update', + body: "Hello #{@student.first_name},\n\nHere is the full message.", + deduplication_key: 'communication-email:test-run:1' + ) + group = NotificationGroupBuilder.new([notification]).groups.first + + assert_equal 'communication_email', notification.kind + assert_equal 'A personalised update', group[:message_subject] + assert_equal "Hello #{@student.first_name},\n\nHere is the full message.", group[:message_body] + assert_equal "A personalised update - Message from #{@tutor.name}", group[:summary] + assert_not_nil notification.email_processed_at + end + + def test_communication_emails_are_not_grouped_together + notifications = 2.times.map do |index| + Notification.create_for_communication_email( + recipient: @student, + unit: @unit, + project: @project, + actor: @tutor, + subject: "Message #{index}", + body: "Body #{index}", + deduplication_key: "communication-email:test-run:#{index}" + ) + end + + assert_equal 2, NotificationGroupBuilder.new(notifications).groups.count + end + + def test_a_new_portfolio_result_resolves_the_previous_one + ready = Notification.create_for_portfolio(@project, success: true) + @project.update!(updated_at: 1.second.from_now) + + failed = Notification.create_for_portfolio(@project, success: false) + + assert_not_nil ready.reload.read_at + assert_nil failed.read_at + assert_equal 'portfolio_failed', failed.kind + end + + def test_retrying_the_same_portfolio_result_does_not_mark_it_read + first = Notification.create_for_portfolio(@project, success: true) + duplicate = Notification.create_for_portfolio(@project, success: true) + + assert_equal first, duplicate + assert_nil first.reload.read_at + end + + def test_destroying_a_source_removes_its_notification + comment = @task.add_text_comment(@tutor, 'Temporary feedback') + notification = Notification.find_by!(task_comment: comment, recipient: @student) + + comment.destroy! + + assert_not Notification.exists?(notification.id) + end + + def test_groups_are_sorted_by_newest_activity + older_unread_notification = FactoryBot.create( + :notification, + recipient: @student, + unit: @unit, + kind: 'discuss_expired', + created_at: 2.hours.ago + ) + newer_read_notification = FactoryBot.create( + :notification, + recipient: @student, + unit: @unit, + kind: 'new_task_comment', + created_at: 1.hour.ago, + read_at: Time.current + ) + + groups = NotificationGroupBuilder.new([older_unread_notification, newer_read_notification]).groups + + assert_equal newer_read_notification.id, groups.first[:notification_ids].first + assert 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!(task_comment: 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_from_comment(comment, @student) + notification.reload + + assert_nil notification.read_at + assert_not_nil notification.email_processed_at + end +end diff --git a/test/sidekiq/execute_communication_set_job_test.rb b/test/sidekiq/execute_communication_set_job_test.rb index 8ec1afb8b3..656073c8a0 100644 --- a/test/sidekiq/execute_communication_set_job_test.rb +++ b/test/sidekiq/execute_communication_set_job_test.rb @@ -3,6 +3,66 @@ require 'test_helper' class ExecuteCommunicationSetJobTest < ActiveSupport::TestCase + def test_email_student_action_creates_a_notification_with_the_rendered_email + unit = FactoryBot.create( + :unit, + student_count: 1, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 0 + ) + project = unit.active_projects.first + student = project.student + communication_set = unit.communication_sets.create!(name: 'Student email set', active: true) + rule = communication_set.communication_rules.create!(name: 'Welcome', operator: 'and', position: 0) + rule.communication_actions.create!( + type: 'EmailStudentAction', + subject: 'Hello {{student.first_name}}', + body: "Welcome to {{unit.code}}.\nThis is your full message." + ) + + assert_difference -> { ActionMailer::Base.deliveries.count }, 1 do + ExecuteCommunicationSetJob.new.perform(communication_set.id) + end + + notification = Notification.find_by!(recipient: student, project: project, kind: 'communication_email') + assert_equal "Hello #{student.first_name}", notification.message_subject + assert_equal "Welcome to #{unit.code}.\nThis is your full message.", notification.message_body + assert_nil notification.read_at + assert_not_nil notification.email_processed_at + end + + def test_email_staff_action_creates_a_notification_for_the_staff_recipient + unit = FactoryBot.create( + :unit, + student_count: 1, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 1 + ) + project = unit.active_projects.first + tutor = project.tutor_for(unit.task_definitions.first) + communication_set = unit.communication_sets.create!(name: 'Staff email set', active: true) + rule = communication_set.communication_rules.create!(name: 'Follow up', operator: 'and', position: 0) + rule.communication_actions.create!( + type: 'EmailStaffAction', + subject: 'Follow up with {{student.full_name}}', + body: 'Please contact {{student.username}}.', + email_tutors: true, + email_convenors: false + ) + + assert_difference -> { ActionMailer::Base.deliveries.count }, 1 do + ExecuteCommunicationSetJob.new.perform(communication_set.id) + end + + notification = Notification.find_by!(recipient: tutor, project: project, kind: 'communication_email') + assert_equal "Follow up with #{project.student.name}", notification.message_subject + assert_equal "Please contact #{project.student.username}.", notification.message_body + end + def test_task_comment_action_adds_a_comment_to_each_selected_students_task unit = FactoryBot.create( :unit, @@ -51,7 +111,7 @@ def test_task_comment_action_adds_a_comment_to_each_selected_students_task assert_equal comment_author, comment_one.user assert_equal comment_author, comment_two.user - assert_equal 'Please review Ada for ' + unit.code, comment_one.comment - assert_equal 'Please review Grace for ' + unit.code, comment_two.comment + assert_equal "Please review Ada for #{unit.code}", comment_one.comment + assert_equal "Please review Grace for #{unit.code}", comment_two.comment end end diff --git a/test/sidekiq/feedback_warning_notification_job_test.rb b/test/sidekiq/feedback_warning_notification_job_test.rb new file mode 100644 index 0000000000..cf07a07548 --- /dev/null +++ b/test/sidekiq/feedback_warning_notification_job_test.rb @@ -0,0 +1,245 @@ +require 'test_helper' + +class FeedbackWarningNotificationJobTest < ActiveSupport::TestCase + include ActionMailer::TestHelper + + class FakeRedis + attr_reader :store + + def initialize + @store = {} + end + + def get(key) + store[key] + end + + def set(key, value, **options) + return if options[:nx] && store.key?(key) + + store[key] = value + 'OK' + end + end + + def setup + @now = Time.zone.local(2026, 9, 7, 9) + teaching_period = FactoryBot.create( + :teaching_period, + start_date: @now - 2.weeks, + end_date: @now + 2.months, + active_until: @now + 3.months + ) + @unit = FactoryBot.create( + :unit, + teaching_period: teaching_period, + task_count: 1, + feedback_warning_threshold_days: 5 + ) + definition = @unit.task_definitions.first + # The factory grades the definition and its projects at random, so pin the + # definition below every project rather than hoping one was generated above + # it. Only the fully enrolled students hold a tutorial. + definition.update!(target_grade: 0) + @project = @unit.active_projects.find { |project| project.tutorial_for(definition).present? } + @task = @project.task_for_task_definition(definition) + @tutor = FactoryBot.create(:user, :tutor) + @tutor_role = @unit.employ_staff(@tutor, Role.tutor) + @project.tutorial_for(@task.task_definition).update!(unit_role: @tutor_role) + @task.update!( + task_status: TaskStatus.ready_for_feedback, + submission_date: @now - 5.days + ) + end + + def test_creates_one_warning_for_the_assigned_tutor_at_the_threshold + 2.times { Notification.refresh_feedback_warning_notifications!(now: @now) } + + notifications = Notification.where(task: @task, kind: 'feedback_warning') + assert_equal 1, notifications.count + assert_equal @tutor, notifications.first.recipient + end + + def test_does_not_warn_before_the_threshold + @task.update!(submission_date: @now - 5.days + 1.minute) + + Notification.refresh_feedback_warning_notifications!(now: @now) + + assert_not Notification.exists?(task: @task, kind: 'feedback_warning') + end + + def test_teaching_breaks_pause_the_warning_clock + @unit.teaching_period.add_break(@now - 4.days, 7) + + Notification.refresh_feedback_warning_notifications!(now: @now) + + assert_not Notification.exists?(task: @task, kind: 'feedback_warning') + end + + def test_falls_back_to_the_main_convenor_when_no_tutor_is_assigned + @project.tutorial_for(@task.task_definition).update!(unit_role: nil) + + Notification.refresh_feedback_warning_notifications!(now: @now) + + assert_equal @unit.main_convenor_user, + Notification.find_by!(task: @task, kind: 'feedback_warning').recipient + end + + def test_resolves_a_warning_when_the_task_no_longer_needs_feedback + Notification.refresh_feedback_warning_notifications!(now: @now) + notification = Notification.find_by!(task: @task, kind: 'feedback_warning') + @task.update!(task_status: TaskStatus.complete) + + Notification.refresh_feedback_warning_notifications!(now: @now + 1.hour) + + assert_not_nil notification.reload.read_at + assert_not_nil notification.email_processed_at + end + + def test_reassignment_resolves_the_old_warning_and_notifies_the_new_tutor + Notification.refresh_feedback_warning_notifications!(now: @now) + original = Notification.find_by!(task: @task, kind: 'feedback_warning') + replacement = FactoryBot.create(:user, :tutor) + replacement_role = @unit.employ_staff(replacement, Role.tutor) + @project.tutorial_for(@task.task_definition).update!(unit_role: replacement_role) + + Notification.refresh_feedback_warning_notifications!(now: @now + 1.hour) + + assert_not_nil original.reload.read_at + assert Notification.exists?(task: @task, kind: 'feedback_warning', recipient: replacement, read_at: nil) + end + + def test_withdrawal_threshold_and_unit_state_changes_resolve_warnings + assert_resolves_warning { @project.update!(enrolled: false) } + reset_warning(submission_date: @now - 6.days) + assert_resolves_warning { @unit.update!(feedback_warning_threshold_days: 7) } + @unit.update!(feedback_warning_threshold_days: 5) + reset_warning(submission_date: @now - 8.days) + assert_resolves_warning { @unit.update!(active: false) } + end + + def test_a_resubmission_can_raise_another_warning + Notification.refresh_feedback_warning_notifications!(now: @now) + first = Notification.find_by!(task: @task, kind: 'feedback_warning') + @task.update!(task_status: TaskStatus.complete) + Notification.refresh_feedback_warning_notifications!(now: @now + 1.hour) + @task.update!( + task_status: TaskStatus.ready_for_feedback, + submission_date: @now + 2.days + ) + + Notification.refresh_feedback_warning_notifications!(now: @now + 7.days) + + warnings = Notification.where(task: @task, kind: 'feedback_warning').order(:id) + assert_equal 2, warnings.count + assert_equal first, warnings.first + assert_nil warnings.last.read_at + end + + def test_groups_pending_tasks_then_starts_a_new_group_after_email_delivery + create_warning_task(submitted_at: @now - 6.days) + Notification.refresh_feedback_warning_notifications!(now: @now) + settings = NotificationSetting.for(@tutor) + + assert_emails 1 do + SendNotificationDigestJob.new.perform(settings.id) + end + + delivered = Notification.where(recipient: @tutor, kind: 'feedback_warning') + assert_equal 2, delivered.count + assert(delivered.all? { |notification| notification.email_sent_at.present? }) + assert_includes ActionMailer::Base.deliveries.last.text_part.body.to_s, '2 tasks that require feedback' + assert_includes ActionMailer::Base.deliveries.last.text_part.body.to_s, + "/units/#{@unit.id}/tasks/inbox" + + create_warning_task(submitted_at: @now - 7.days) + Notification.refresh_feedback_warning_notifications!(now: @now + 1.hour) + groups = NotificationGroupBuilder.new( + Notification.where(recipient: @tutor, kind: 'feedback_warning').unread + ).groups + + assert_equal [1, 2], groups.map { |group| group[:counts]['feedback_warning'] }.sort + pending = groups.find { |group| group[:counts]['feedback_warning'] == 1 } + assert_equal({ type: 'unit_inbox', unit_id: @unit.id }, pending[:destination]) + assert_equal 'warning', pending[:severity] + end + + def test_disabled_channels_prevent_creation + settings = NotificationSetting.for(@tutor) + settings.update!(channels: settings.channels.merge('feedback_warning' => [])) + + Notification.refresh_feedback_warning_notifications!(now: @now) + + assert_not Notification.exists?(task: @task, kind: 'feedback_warning') + end + + def test_first_job_run_only_sets_the_rollout_boundary + Notification.refresh_feedback_warning_notifications!(now: @now) + existing = Notification.find_by!(task: @task, kind: 'feedback_warning') + redis = FakeRedis.new + + with_sidekiq_redis(redis) do + travel_to(@now) { NotifyFeedbackWarningsJob.new.perform } + travel_to(@now + 1.hour) { NotifyFeedbackWarningsJob.new.perform } + end + + assert_nil existing.reload.read_at + assert_nil existing.email_processed_at + assert_equal 1, Notification.where(task: @task, kind: 'feedback_warning').count + assert redis.store.key?(NotifyFeedbackWarningsJob::ROLLOUT_KEY) + end + + def test_job_notifies_an_existing_submission_only_when_it_crosses_after_rollout + @task.update!(submission_date: @now - 4.days) + redis = FakeRedis.new + + with_sidekiq_redis(redis) do + travel_to(@now) { NotifyFeedbackWarningsJob.new.perform } + assert_not Notification.exists?(task: @task, kind: 'feedback_warning') + + travel_to(@now + 1.day) { NotifyFeedbackWarningsJob.new.perform } + end + + assert Notification.exists?(task: @task, kind: 'feedback_warning', recipient: @tutor) + end + + private + + def create_warning_task(submitted_at:) + definition = FactoryBot.create( + :task_definition, + unit: @unit, + tutorial_stream: @task.task_definition.tutorial_stream, + target_grade: @task.task_definition.target_grade + ) + FactoryBot.create( + :task, + project: @project, + task_definition: definition, + task_status: TaskStatus.ready_for_feedback, + submission_date: submitted_at + ) + end + + def reset_warning(submission_date:) + @project.update!(enrolled: true) + @task.update!(task_status: TaskStatus.ready_for_feedback, submission_date: submission_date) + Notification.refresh_feedback_warning_notifications!(now: @now) + end + + def assert_resolves_warning + Notification.refresh_feedback_warning_notifications!(now: @now) + notification = Notification.where(task: @task, kind: 'feedback_warning').unread.first! + yield + Notification.refresh_feedback_warning_notifications!(now: @now + 1.hour) + assert_not_nil notification.reload.read_at + end + + def with_sidekiq_redis(redis) + original = Sidekiq.method(:redis) + Sidekiq.define_singleton_method(:redis) { |&redis_block| redis_block.call(redis) } + yield + ensure + Sidekiq.define_singleton_method(:redis, original) + end +end diff --git a/test/sidekiq/notification_jobs_test.rb b/test/sidekiq/notification_jobs_test.rb new file mode 100644 index 0000000000..73ffd2d54d --- /dev/null +++ b/test/sidekiq/notification_jobs_test.rb @@ -0,0 +1,320 @@ +require 'test_helper' +require 'minitest/mock' + +class NotificationJobsTest < ActiveSupport::TestCase + include ActionMailer::TestHelper + + # refresh_next_digest_at rewrites next_digest_at whenever the schedule is + # assigned, so the due time has to be forced back past the callback. + def create_settings(due_at: 1.minute.ago, **attributes) + settings = FactoryBot.create(:notification_setting, **attributes) + # rubocop:disable Rails/SkipsModelValidations + settings.update_column(:next_digest_at, due_at) + # rubocop:enable Rails/SkipsModelValidations + settings.reload + end + + def test_digest_sends_only_the_events_enabled_on_the_email_channel + settings = create_settings(channels: { 'new_task_comment' => ['in_app'] }.merge( + NotificationSetting.default_channels.except('new_task_comment') + )) + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + NotificationUnitOverride.create!( + user: settings.user, + unit: unit, + channels: { 'new_task_comment' => %w[in_app email] } + ) + + enabled = FactoryBot.create(:notification, recipient: settings.user, unit: unit, kind: 'new_task_comment') + disabled = FactoryBot.create(:notification, recipient: settings.user, kind: 'new_task_comment') + + assert_emails 1 do + SendNotificationDigestJob.new.perform(settings.id) + end + + digest_html = ActionMailer::Base.deliveries.last.html_part.body.to_s + assert_equal 1, digest_html.scan('