From b7bafe935042d12793cf71f3cbfd25a453ae50c7 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:41:26 +1000 Subject: [PATCH 01/12] refactor: replace comment read receipts with per-task cursors --- app/models/comments/comment_read_cursor.rb | 57 ++++++++ app/models/comments/task_comment.rb | 64 +++++++-- app/models/overseer_assessment.rb | 11 +- app/models/project.rb | 45 +++++-- app/models/task.rb | 25 +++- app/models/unit.rb | 125 ++++++++++++------ app/models/user.rb | 1 + ...60728051502_create_comment_read_cursors.rb | 34 +++++ db/schema.rb | 18 ++- test/models/comment_read_cursor_test.rb | 29 ++++ 10 files changed, 339 insertions(+), 70 deletions(-) create mode 100644 app/models/comments/comment_read_cursor.rb create mode 100644 db/migrate/20260728051502_create_comment_read_cursors.rb create mode 100644 test/models/comment_read_cursor_test.rb diff --git a/app/models/comments/comment_read_cursor.rb b/app/models/comments/comment_read_cursor.rb new file mode 100644 index 0000000000..d148ab7c34 --- /dev/null +++ b/app/models/comments/comment_read_cursor.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +class CommentReadCursor < ApplicationRecord + belongs_to :task + belongs_to :user + belongs_to :last_read_comment, class_name: 'TaskComment' + + validates :task, :user, :last_read_comment, :read_at, presence: true + validates :task_id, uniqueness: { scope: :user_id } + validate :last_read_comment_belongs_to_task + + def self.advance(task_id:, user_ids:, comment_id:, read_at: Time.current) + user_ids = Array(user_ids).compact.map(&:to_i).uniq + return if user_ids.empty? + + now = Time.current + values = user_ids.map do |user_id| + [ + task_id, + user_id, + comment_id, + read_at, + now, + now + ].map { |value| connection.quote(value) }.join(', ') + end.join('), (') + + connection.execute(<<~SQL.squish) + INSERT INTO comment_read_cursors + (task_id, user_id, last_read_comment_id, read_at, created_at, updated_at) + VALUES (#{values}) + ON DUPLICATE KEY UPDATE + read_at = IF( + last_read_comment_id < VALUES(last_read_comment_id), + VALUES(read_at), + read_at + ), + updated_at = IF( + last_read_comment_id < VALUES(last_read_comment_id), + VALUES(updated_at), + updated_at + ), + last_read_comment_id = GREATEST( + last_read_comment_id, + VALUES(last_read_comment_id) + ) + SQL + end + + private + + def last_read_comment_belongs_to_task + return if last_read_comment.nil? || last_read_comment.task_id == task_id + + errors.add(:last_read_comment, 'must belong to the same task') + end +end diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index c74883d014..d4d32822e9 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -16,6 +16,10 @@ 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 :comment_read_cursors, + foreign_key: :last_read_comment_id, + inverse_of: :last_read_comment, + dependent: :restrict_with_exception # Can optionally be a reply to a comment belongs_to :task_comment, optional: true @@ -35,6 +39,7 @@ class TaskComment < ApplicationRecord end # Delete action - before dependent association + before_destroy :rewind_comment_read_cursors, prepend: true before_destroy :delete_associated_files def valid_reply_to? @@ -77,7 +82,11 @@ def serialize(user) end def create_comment_read_receipt_entry(user) - comment_read_receipt = CommentsReadReceipts.find_or_create_by(user: user, task_comment: self) + CommentReadCursor.advance( + task_id: task_id, + user_ids: user.id, + comment_id: id + ) end def comment @@ -135,16 +144,33 @@ def attachment_mime_type end def remove_comment_read_entry(user) - CommentsReadReceipts.delete_all(user: user, task_comment: self) + cursor = CommentReadCursor.find_by(task_id: task_id, user_id: user.id) + return if cursor.nil? || cursor.last_read_comment_id < id + + previous_comment_id = TaskComment + .where(task_id: task_id) + .where('id < ?', id) + .maximum(:id) + + if previous_comment_id.nil? + cursor.destroy! + else + cursor.update!( + last_read_comment_id: previous_comment_id, + read_at: Time.current + ) + end end def mark_as_read(user, unit = self.unit) return if read_by?(user) # avoid propagating if not needed if user == project.tutor_for(task.task_definition) - unit.staff.each do |staff_member| - create_comment_read_receipt_entry(staff_member.user) - end + CommentReadCursor.advance( + task_id: task_id, + user_ids: unit.staff.pluck(:user_id), + comment_id: id + ) else create_comment_read_receipt_entry(user) end @@ -159,11 +185,33 @@ def new_for?(user) end def read_by?(user) - CommentsReadReceipts.find_by(user: user, task_comment: self).present? + cursor = CommentReadCursor.find_by(task_id: task_id, user_id: user.id) + cursor.present? && cursor.last_read_comment_id >= id end def time_read_by(user) - read_reciept = CommentsReadReceipts.find_by(user: user, task_comment: self) - read_reciept&.created_at + cursor = CommentReadCursor.find_by(task_id: task_id, user_id: user.id) + cursor&.read_at if cursor&.last_read_comment_id.to_i >= id + end + + def rewind_comment_read_cursors + previous_comment_id = TaskComment + .where(task_id: task_id) + .where('id < ?', id) + .maximum(:id) + + cursors = CommentReadCursor.where(last_read_comment_id: id) + if previous_comment_id.nil? + cursors.delete_all + else + # A single comment can be the cursor for every teaching staff member. + # Keep destruction bounded to one SQL update. + # rubocop:disable Rails/SkipsModelValidations + cursors.update_all( + last_read_comment_id: previous_comment_id, + updated_at: Time.current + ) + # rubocop:enable Rails/SkipsModelValidations + end end end diff --git a/app/models/overseer_assessment.rb b/app/models/overseer_assessment.rb index d01a3210ed..6dc0b808a9 100644 --- a/app/models/overseer_assessment.rb +++ b/app/models/overseer_assessment.rb @@ -38,14 +38,17 @@ def self.student_notification_grace_period AND assessment_comments.type = 'AssessmentComment' SQL .joins(<<~SQL.squish) - LEFT JOIN comments_read_receipts student_read_receipts - ON student_read_receipts.task_comment_id = assessment_comments.id - AND student_read_receipts.user_id = projects.user_id + LEFT JOIN comment_read_cursors student_read_cursor + ON student_read_cursor.task_id = assessment_comments.task_id + 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_receipts.id IS NULL') + .where( + 'student_read_cursor.last_read_comment_id IS NULL ' \ + 'OR student_read_cursor.last_read_comment_id < assessment_comments.id' + ) .where(<<~SQL.squish) assessment_comments.id = ( SELECT latest_comment.id diff --git a/app/models/project.rb b/app/models/project.rb index b74cc84add..0426022d6a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -279,22 +279,47 @@ def reference_date def task_details_for_shallow_serializer(user) teaching_breaks = unit.teaching_period&.breaks.to_a + comment_summary = TaskComment + .joins(:task) + .joins( + "LEFT JOIN comment_read_cursors project_cursor " \ + "ON project_cursor.task_id = task_comments.task_id " \ + "AND project_cursor.user_id = #{user.id.to_i}" + ) + .where(tasks: { project_id: id }) + .where("task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment'") + .select( + 'task_comments.task_id AS task_id', + 'SUM(CASE WHEN project_cursor.last_read_comment_id IS NULL ' \ + 'OR task_comments.id > project_cursor.last_read_comment_id ' \ + 'THEN 1 ELSE 0 END) AS number_unread' + ) + .group('task_comments.task_id') + .to_sql + + similarity_summary = TaskSimilarity + .joins(:task) + .where(tasks: { project_id: id }) + .where(flagged: true) + .select('task_similarities.task_id AS task_id', 'COUNT(*) AS similar_to_count') + .group('task_similarities.task_id') + .to_sql tasks .joins(:task_status) - .joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment')") - .joins("LEFT JOIN comments_read_receipts crr ON crr.task_comment_id = task_comments.id AND crr.user_id = #{user.id}") - .joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id') + .joins( + "LEFT OUTER JOIN (#{comment_summary}) AS project_comment_summary " \ + 'ON project_comment_summary.task_id = tasks.id' + ) + .joins( + "LEFT OUTER JOIN (#{similarity_summary}) AS project_similarity_summary " \ + 'ON project_similarity_summary.task_id = tasks.id' + ) .select( - 'SUM(case when crr.user_id is null AND NOT task_comments.id is null then 1 else 0 end) as number_unread', 'project_id', 'tasks.id as id', + 'COALESCE(project_comment_summary.number_unread, 0) AS number_unread', 'project_id', 'tasks.id as id', 'task_definition_id', 'task_statuses.id as status_id', 'completion_date', 'times_assessed', 'submission_date', 'tasks.grade as grade', 'quality_pts', 'include_in_portfolio', 'grade', - 'SUM(case when task_similarities.flagged then 1 else 0 end) as similar_to_count' - ) - .group( - 'task_statuses.id', 'tasks.project_id', 'tasks.id', 'task_definition_id', 'status_id', - 'completion_date', 'times_assessed', 'submission_date', 'grade', 'quality_pts', - 'include_in_portfolio', 'grade' + 'COALESCE(project_similarity_summary.similar_to_count, 0) AS similar_to_count' ) .map do |r| t = Task.find(r.id) diff --git a/app/models/task.rb b/app/models/task.rb index 9a535b5a13..845ac74132 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -131,6 +131,7 @@ def specific_permission_hash(role, perm_hash, _other) has_one :overflow_task_claim, dependent: :destroy has_many :comments, class_name: 'TaskComment', dependent: :destroy, inverse_of: :task + has_many :comment_read_cursors, dependent: :destroy, inverse_of: :task has_many :task_similarities, class_name: 'TaskSimilarity', dependent: :destroy, inverse_of: :task has_many :reverse_jplag_similarities, class_name: 'JplagTaskSimilarity', dependent: :destroy, inverse_of: :other_task, foreign_key: 'other_task_id' has_many :reverse_moss_similarities, class_name: 'MossTaskSimilarity', dependent: :destroy, inverse_of: :other_task, foreign_key: 'other_task_id' @@ -226,7 +227,14 @@ def all_comments end def mark_comments_as_read(user, comments) + latest_comment_by_task = {} + comments.each do |comment| + current = latest_comment_by_task[comment.task_id] + latest_comment_by_task[comment.task_id] = comment if current.nil? || current.id < comment.id + end + + latest_comment_by_task.each_value do |comment| comment.mark_as_read(user, unit) end end @@ -241,16 +249,25 @@ def comments_for_user(user) TaskComment .joins('JOIN users AS authors ON authors.id = task_comments.user_id') .joins('JOIN users AS recipients ON recipients.id = task_comments.recipient_id') - .joins("LEFT JOIN comments_read_receipts u_crr ON u_crr.task_comment_id = task_comments.id AND u_crr.user_id = #{user.id}") - .joins("LEFT JOIN comments_read_receipts r_crr ON r_crr.task_comment_id = task_comments.id AND r_crr.user_id = recipients.id") + .joins( + "LEFT JOIN comment_read_cursors user_cursor " \ + "ON user_cursor.task_id = task_comments.task_id AND user_cursor.user_id = #{user.id.to_i}" + ) + .joins( + 'LEFT JOIN comment_read_cursors recipient_cursor ' \ + 'ON recipient_cursor.task_id = task_comments.task_id ' \ + 'AND recipient_cursor.user_id = recipients.id' + ) .where('task_comments.task_id = :task_id', task_id: self.id) .order('created_at ASC') .select( 'task_comments.id AS id', 'task_comments.comment AS comment', 'task_comments.content_type AS content_type', - "case when u_crr.created_at IS NULL then 1 else 0 end AS is_new", - 'r_crr.created_at AS recipient_read_time', + 'CASE WHEN user_cursor.last_read_comment_id IS NULL ' \ + 'OR task_comments.id > user_cursor.last_read_comment_id THEN 1 ELSE 0 END AS is_new', + 'CASE WHEN task_comments.id <= recipient_cursor.last_read_comment_id ' \ + 'THEN recipient_cursor.read_at ELSE NULL END AS recipient_read_time', 'task_comments.created_at AS created_at', 'authors.id AS author_id', 'authors.first_name AS author_first_name', diff --git a/app/models/unit.rb b/app/models/unit.rb index a3757c62e3..d7fbd7747b 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -2452,51 +2452,86 @@ def tutorial_enrolment_subquery .select('tutorials.tutorial_stream_id as tutorial_stream_id', 'tutorials.id as tutorial_id', 'project_id', 'tutorials.unit_role_id as unit_role_id').to_sql end + def task_comment_summary_subquery(user) + TaskComment + .joins(task: :project) + .joins( + "LEFT JOIN comment_read_cursors inbox_cursor " \ + "ON inbox_cursor.task_id = task_comments.task_id " \ + "AND inbox_cursor.user_id = #{user.id.to_i}" + ) + .where(projects: { unit_id: id }) + .where("task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment'") + .where( + "task_comments.content_type IS NULL OR " \ + "task_comments.content_type NOT IN ('plan', 'discussed_in_class')" + ) + .select( + 'task_comments.task_id AS task_id', + 'SUM(CASE WHEN inbox_cursor.last_read_comment_id IS NULL ' \ + 'OR task_comments.id > inbox_cursor.last_read_comment_id THEN 1 ELSE 0 END) AS number_unread', + 'MAX(task_comments.created_at) AS latest_comment_at', + "MAX(CASE WHEN task_comments.type = 'ExtensionComment' " \ + 'AND task_comments.date_extension_assessed IS NULL THEN 1 ELSE 0 END) AS has_extensions' + ) + .group('task_comments.task_id') + .to_sql + end + + def task_similarity_summary_subquery + TaskSimilarity + .joins(task: :project) + .where(projects: { unit_id: id }) + .where(flagged: true) + .select('task_similarities.task_id AS task_id', 'COUNT(*) AS similar_to_count') + .group('task_similarities.task_id') + .to_sql + end + # # Return all tasks from the database for this unit and given user # def get_all_tasks_for(user, my_tutorials_only = false) - result = student_tasks. - joins(:task_status). - joins("LEFT OUTER JOIN (#{tutorial_enrolment_subquery}) as sq ON sq.project_id = projects.id AND (sq.tutorial_stream_id = task_definitions.tutorial_stream_id OR sq.tutorial_stream_id IS NULL)"). - joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment') AND (task_comments.content_type IS NULL OR (task_comments.content_type <> 'plan' AND task_comments.content_type <> 'discussed_in_class'))"). - joins("LEFT JOIN comments_read_receipts crr ON crr.task_comment_id = task_comments.id AND crr.user_id = #{user.id}"). - joins("LEFT JOIN task_pins ON task_pins.task_id = tasks.id AND task_pins.user_id = #{user.id}"). - joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id'). - select( - 'sq.tutorial_id AS tutorial_id', - 'sq.tutorial_stream_id AS tutorial_stream_id', - 'tasks.id', - "SUM(case when crr.user_id is null AND NOT task_comments.id is null then 1 else 0 end) as number_unread", - 'COUNT(distinct task_pins.task_id) != 0 as pinned', - "SUM(case when task_comments.date_extension_assessed IS NULL AND task_comments.type = 'ExtensionComment' AND NOT task_comments.id IS NULL THEN 1 ELSE 0 END) > 0 as has_extensions", - 'project_id', - 'tasks.id as task_id', - 'task_definition_id', - 'task_definitions.start_date as start_date', - 'task_statuses.id as status_id', - 'completion_date', - 'times_assessed', - 'submission_date', - 'tasks.grade as grade', - 'quality_pts', - 'SUM(case when task_similarities.flagged then 1 else 0 end) as similar_to_count' - ). - group( - 'sq.tutorial_id', - 'sq.tutorial_stream_id', - 'task_statuses.id', - 'project_id', - 'tasks.id', - 'task_definition_id', - 'task_definitions.start_date', - 'status_id', - 'completion_date', - 'times_assessed', - 'submission_date', - 'grade', - 'quality_pts' - ) + result = student_tasks + .joins(:task_status) + .joins( + "LEFT OUTER JOIN (#{tutorial_enrolment_subquery}) AS sq " \ + 'ON sq.project_id = projects.id ' \ + 'AND (sq.tutorial_stream_id = task_definitions.tutorial_stream_id ' \ + 'OR sq.tutorial_stream_id IS NULL)' + ) + .joins( + "LEFT OUTER JOIN (#{task_comment_summary_subquery(user)}) AS comment_summary " \ + 'ON comment_summary.task_id = tasks.id' + ) + .joins( + "LEFT JOIN task_pins AS inbox_pins " \ + "ON inbox_pins.task_id = tasks.id AND inbox_pins.user_id = #{user.id.to_i}" + ) + .joins( + "LEFT OUTER JOIN (#{task_similarity_summary_subquery}) AS similarity_summary " \ + 'ON similarity_summary.task_id = tasks.id' + ) + .select( + 'sq.tutorial_id AS tutorial_id', + 'sq.tutorial_stream_id AS tutorial_stream_id', + 'tasks.id', + 'COALESCE(comment_summary.number_unread, 0) AS number_unread', + 'inbox_pins.task_id IS NOT NULL AS pinned', + 'COALESCE(comment_summary.has_extensions, 0) > 0 AS has_extensions', + 'tasks.project_id', + 'tasks.id AS task_id', + 'tasks.task_definition_id', + 'task_definitions.start_date AS start_date', + 'task_statuses.id AS status_id', + 'tasks.completion_date', + 'tasks.times_assessed', + 'tasks.submission_date', + 'tasks.grade AS grade', + 'tasks.quality_pts', + 'COALESCE(similarity_summary.similar_to_count, 0) AS similar_to_count', + 'comment_summary.latest_comment_at' + ) if my_tutorials_only unit_role = unit_role_for(user) unless unit_role.nil? @@ -2529,8 +2564,12 @@ def tasks_awaiting_feedback(user) # def tasks_for_task_inbox(user, my_students_only = false) get_all_tasks_for(user, my_students_only) - .having('task_statuses.id IN (:ids) OR COUNT(task_pins.task_id) > 0 OR SUM(case when crr.user_id is null AND NOT task_comments.id is null then 1 else 0 end) > 0', ids: [TaskStatus.ready_for_feedback, TaskStatus.need_help]) - .order('pinned DESC, submission_date ASC, MAX(task_comments.created_at) ASC, task_definition_id ASC') + .where( + 'task_statuses.id IN (:ids) OR inbox_pins.task_id IS NOT NULL OR ' \ + 'COALESCE(comment_summary.number_unread, 0) > 0', + ids: [TaskStatus.ready_for_feedback, TaskStatus.need_help] + ) + .order('pinned DESC, submission_date ASC, latest_comment_at ASC, task_definition_id ASC') end # diff --git a/app/models/user.rb b/app/models/user.rb index 67a50d1877..9349c33243 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -176,6 +176,7 @@ def token_for_text?(a_token, token_type) has_many :engagements, dependent: :restrict_with_exception, inverse_of: :user has_many :engagement_comments, dependent: :restrict_with_exception, inverse_of: :user has_many :auth_tokens, dependent: :destroy, inverse_of: :user + has_many :comment_read_cursors, dependent: :destroy, inverse_of: :user has_many :user_oauth_tokens, dependent: :destroy, inverse_of: :user has_many :user_oauth_states, dependent: :destroy, inverse_of: :user has_one :webcal, dependent: :destroy, inverse_of: :user diff --git a/db/migrate/20260728051502_create_comment_read_cursors.rb b/db/migrate/20260728051502_create_comment_read_cursors.rb new file mode 100644 index 0000000000..75cefa6087 --- /dev/null +++ b/db/migrate/20260728051502_create_comment_read_cursors.rb @@ -0,0 +1,34 @@ +class CreateCommentReadCursors < ActiveRecord::Migration[8.0] + def up + create_table :comment_read_cursors do |t| + t.references :task, null: false, foreign_key: true + t.references :user, null: false, foreign_key: true + t.references :last_read_comment, null: false, foreign_key: { to_table: :task_comments } + t.datetime :read_at, null: false + + t.timestamps + end + + add_index :comment_read_cursors, [:task_id, :user_id], unique: true + + execute <<~SQL.squish + INSERT INTO comment_read_cursors + (task_id, user_id, last_read_comment_id, read_at, created_at, updated_at) + SELECT + task_comments.task_id, + comments_read_receipts.user_id, + MAX(comments_read_receipts.task_comment_id), + MAX(comments_read_receipts.updated_at), + MIN(comments_read_receipts.created_at), + MAX(comments_read_receipts.updated_at) + FROM comments_read_receipts + INNER JOIN task_comments + ON task_comments.id = comments_read_receipts.task_comment_id + GROUP BY task_comments.task_id, comments_read_receipts.user_id + SQL + end + + def down + drop_table :comment_read_cursors + end +end diff --git a/db/schema.rb b/db/schema.rb index 90bbe62f82..9a8602b67c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_24_015355) do +ActiveRecord::Schema[8.0].define(version: 2026_07_28_051502) 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 @@ -63,6 +63,19 @@ t.index ["tutor_id"], name: "index_chip_usages_on_tutor_id" end + create_table "comment_read_cursors", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| + t.bigint "task_id", null: false + t.bigint "user_id", null: false + t.bigint "last_read_comment_id", null: false + t.datetime "read_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["last_read_comment_id"], name: "index_comment_read_cursors_on_last_read_comment_id" + t.index ["task_id", "user_id"], name: "index_comment_read_cursors_on_task_id_and_user_id", unique: true + t.index ["task_id"], name: "index_comment_read_cursors_on_task_id" + t.index ["user_id"], name: "index_comment_read_cursors_on_user_id" + end + create_table "comments_read_receipts", charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.bigint "task_comment_id", null: false t.bigint "user_id", null: false @@ -1026,6 +1039,9 @@ add_foreign_key "chip_usages", "feedback_chips" add_foreign_key "chip_usages", "users", column: "tutor_id" + add_foreign_key "comment_read_cursors", "task_comments", column: "last_read_comment_id" + add_foreign_key "comment_read_cursors", "tasks" + add_foreign_key "comment_read_cursors", "users" add_foreign_key "feedback_chips", "feedback_chips", column: "parent_chip_id" add_foreign_key "feedback_chips", "learning_outcomes" add_foreign_key "learning_outcome_links", "learning_outcomes", column: "source_id" diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb new file mode 100644 index 0000000000..d44b0d8f5b --- /dev/null +++ b/test/models/comment_read_cursor_test.rb @@ -0,0 +1,29 @@ +require 'test_helper' + +class CommentReadCursorTest < ActiveSupport::TestCase + def test_comment_reads_use_one_cursor_per_task_and_user + project = FactoryBot.create(:project) + task = project.task_for_task_definition(project.unit.task_definitions.first) + reader = project.student + author = project.unit.main_convenor_user + + comments = [ + task.add_text_comment(author, 'First'), + task.add_text_comment(author, 'Second'), + task.add_text_comment(author, 'Third') + ] + + task.mark_comments_as_read(reader, comments) + + cursor = CommentReadCursor.find_by!(task: task, user: reader) + assert_equal comments.last.id, cursor.last_read_comment_id + assert_equal 1, CommentReadCursor.where(task: task, user: reader).count + assert(comments.none? { |comment| comment.new_for?(reader) }) + + comments.second.mark_as_unread(reader) + + assert_not comments.first.new_for?(reader) + assert comments.second.new_for?(reader) + assert comments.last.new_for?(reader) + end +end From 4c0db4d69fe834f2a27103ff23257a0375e385b0 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:39:38 +1000 Subject: [PATCH 02/12] chore: fix test --- test/models/task_definition_test.rb | 4 ++-- test/models/task_test.rb | 2 +- test/models/unit_model_test.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/models/task_definition_test.rb b/test/models/task_definition_test.rb index 16b17bf9d3..1651c994ba 100644 --- a/test/models/task_definition_test.rb +++ b/test/models/task_definition_test.rb @@ -661,8 +661,8 @@ def test_overdue_tasks_update_to_assess_in_portfolio task1 = student.task_for_task_definition(td1) task2 = student.task_for_task_definition(td2) - task1.comments.delete_all - task2.comments.delete_all + task1.comments.destroy_all + task2.comments.destroy_all task1.update(task_status_id: TaskStatus.time_exceeded.id) diff --git a/test/models/task_test.rb b/test/models/task_test.rb index a75010c669..6ddeacb479 100644 --- a/test/models/task_test.rb +++ b/test/models/task_test.rb @@ -1851,7 +1851,7 @@ def test_prerequisite_tasks_change_to_fix_and_resubmit task3.trigger_transition(trigger: 'ready_for_feedback', by_user: unit.main_convenor_user) task4.trigger_transition(trigger: 'ready_for_feedback', by_user: unit.main_convenor_user) - task2.comments.delete_all + task2.comments.destroy_all task1.reload task2.reload diff --git a/test/models/unit_model_test.rb b/test/models/unit_model_test.rb index b53659ea02..3f8fe0abe4 100644 --- a/test/models/unit_model_test.rb +++ b/test/models/unit_model_test.rb @@ -1262,8 +1262,8 @@ def test_overdue_tasks_update_to_assess_in_portfolio task1 = student.task_for_task_definition(td1) task2 = student.task_for_task_definition(td2) - task1.comments.delete_all - task2.comments.delete_all + task1.comments.destroy_all + task2.comments.destroy_all task1.update(task_status_id: TaskStatus.time_exceeded.id) From 553d13c87dc58f4e76d368dd8a0926edf9d2bc28 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:07:26 +1000 Subject: [PATCH 03/12] chore: improve readibility --- app/models/comments/comment_read_cursor.rb | 62 ++++++++++--------- ...60728051502_create_comment_read_cursors.rb | 50 +++++++++------ test/models/comment_read_cursor_test.rb | 7 +++ 3 files changed, 69 insertions(+), 50 deletions(-) diff --git a/app/models/comments/comment_read_cursor.rb b/app/models/comments/comment_read_cursor.rb index d148ab7c34..1e289591ef 100644 --- a/app/models/comments/comment_read_cursor.rb +++ b/app/models/comments/comment_read_cursor.rb @@ -14,37 +14,39 @@ def self.advance(task_id:, user_ids:, comment_id:, read_at: Time.current) return if user_ids.empty? now = Time.current - values = user_ids.map do |user_id| - [ - task_id, - user_id, - comment_id, - read_at, - now, - now - ].map { |value| connection.quote(value) }.join(', ') - end.join('), (') - - connection.execute(<<~SQL.squish) - INSERT INTO comment_read_cursors - (task_id, user_id, last_read_comment_id, read_at, created_at, updated_at) - VALUES (#{values}) - ON DUPLICATE KEY UPDATE - read_at = IF( - last_read_comment_id < VALUES(last_read_comment_id), - VALUES(read_at), - read_at - ), - updated_at = IF( - last_read_comment_id < VALUES(last_read_comment_id), - VALUES(updated_at), - updated_at - ), - last_read_comment_id = GREATEST( - last_read_comment_id, - VALUES(last_read_comment_id) + + transaction do + cursors = where(task_id: task_id, user_id: user_ids) + + # rubocop:disable Rails/SkipsModelValidations + missing_user_ids = user_ids - cursors.pluck(:user_id) + + # Create first-time cursors in one query. + if missing_user_ids.any? + insert_all( + missing_user_ids.map do |user_id| + { + task_id: task_id, + user_id: user_id, + last_read_comment_id: comment_id, + read_at: read_at, + created_at: now, + updated_at: now + } + end + ) + end + + # Existing cursors only move forward. + cursors + .where('last_read_comment_id < ?', comment_id) + .update_all( + last_read_comment_id: comment_id, + read_at: read_at, + updated_at: now ) - SQL + # rubocop:enable Rails/SkipsModelValidations + end end private diff --git a/db/migrate/20260728051502_create_comment_read_cursors.rb b/db/migrate/20260728051502_create_comment_read_cursors.rb index 75cefa6087..7f68dadad1 100644 --- a/db/migrate/20260728051502_create_comment_read_cursors.rb +++ b/db/migrate/20260728051502_create_comment_read_cursors.rb @@ -1,5 +1,16 @@ class CreateCommentReadCursors < ActiveRecord::Migration[8.0] def up + create_cursor_table + backfill_cursors + end + + def down + drop_table :comment_read_cursors + end + + private + + def create_cursor_table create_table :comment_read_cursors do |t| t.references :task, null: false, foreign_key: true t.references :user, null: false, foreign_key: true @@ -7,28 +18,27 @@ def up t.datetime :read_at, null: false t.timestamps + t.index [:task_id, :user_id], unique: true end - - add_index :comment_read_cursors, [:task_id, :user_id], unique: true - - execute <<~SQL.squish - INSERT INTO comment_read_cursors - (task_id, user_id, last_read_comment_id, read_at, created_at, updated_at) - SELECT - task_comments.task_id, - comments_read_receipts.user_id, - MAX(comments_read_receipts.task_comment_id), - MAX(comments_read_receipts.updated_at), - MIN(comments_read_receipts.created_at), - MAX(comments_read_receipts.updated_at) - FROM comments_read_receipts - INNER JOIN task_comments - ON task_comments.id = comments_read_receipts.task_comment_id - GROUP BY task_comments.task_id, comments_read_receipts.user_id - SQL end - def down - drop_table :comment_read_cursors + def backfill_cursors + say_with_time 'Backfilling comment read cursors' do + execute <<~SQL + INSERT INTO comment_read_cursors + (task_id, user_id, last_read_comment_id, read_at, created_at, updated_at) + SELECT + task_comments.task_id, + comments_read_receipts.user_id, + MAX(comments_read_receipts.task_comment_id), + MAX(comments_read_receipts.updated_at), + MIN(comments_read_receipts.created_at), + MAX(comments_read_receipts.updated_at) + FROM comments_read_receipts + INNER JOIN task_comments + ON task_comments.id = comments_read_receipts.task_comment_id + GROUP BY task_comments.task_id, comments_read_receipts.user_id + SQL + end end end diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb index d44b0d8f5b..aac62a5e48 100644 --- a/test/models/comment_read_cursor_test.rb +++ b/test/models/comment_read_cursor_test.rb @@ -20,6 +20,13 @@ def test_comment_reads_use_one_cursor_per_task_and_user assert_equal 1, CommentReadCursor.where(task: task, user: reader).count assert(comments.none? { |comment| comment.new_for?(reader) }) + CommentReadCursor.advance( + task_id: task.id, + user_ids: reader.id, + comment_id: comments.first.id + ) + assert_equal comments.last.id, cursor.reload.last_read_comment_id + comments.second.mark_as_unread(reader) assert_not comments.first.new_for?(reader) From 2ab9a98432a7e56ef22b6aa2755574ceb5ac22df Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:41:09 +1000 Subject: [PATCH 04/12] refactor: track comment read cursors per user --- app/api/discussion_comment_api.rb | 2 +- app/models/comments/comment_read_cursor.rb | 50 +++++-------------- app/models/comments/extension_comment.rb | 7 +-- .../comments/scorm_extension_comment.rb | 7 +-- app/models/comments/task_comment.rb | 22 ++------ app/models/task.rb | 4 +- test/models/comment_read_cursor_test.rb | 21 ++++++-- 7 files changed, 43 insertions(+), 70 deletions(-) diff --git a/app/api/discussion_comment_api.rb b/app/api/discussion_comment_api.rb index ffd70117fc..72658f15b1 100644 --- a/app/api/discussion_comment_api.rb +++ b/app/api/discussion_comment_api.rb @@ -149,7 +149,7 @@ class DiscussionCommentApi < Grape::API discussion_comment = task.all_comments.find(params[:task_comment_id]) # discussion_comment.mark_discussion_completed # mark comment read for student - discussion_comment.mark_as_read(current_user, project.unit) + discussion_comment.mark_as_read(current_user) error!({ error: 'No discussion comment found for the given task' }, 403) if discussion_comment.nil? diff --git a/app/models/comments/comment_read_cursor.rb b/app/models/comments/comment_read_cursor.rb index 1e289591ef..f0315f37fb 100644 --- a/app/models/comments/comment_read_cursor.rb +++ b/app/models/comments/comment_read_cursor.rb @@ -9,44 +9,20 @@ class CommentReadCursor < ApplicationRecord validates :task_id, uniqueness: { scope: :user_id } validate :last_read_comment_belongs_to_task - def self.advance(task_id:, user_ids:, comment_id:, read_at: Time.current) - user_ids = Array(user_ids).compact.map(&:to_i).uniq - return if user_ids.empty? - - now = Time.current - - transaction do - cursors = where(task_id: task_id, user_id: user_ids) - - # rubocop:disable Rails/SkipsModelValidations - missing_user_ids = user_ids - cursors.pluck(:user_id) - - # Create first-time cursors in one query. - if missing_user_ids.any? - insert_all( - missing_user_ids.map do |user_id| - { - task_id: task_id, - user_id: user_id, - last_read_comment_id: comment_id, - read_at: read_at, - created_at: now, - updated_at: now - } - end - ) - end - - # Existing cursors only move forward. - cursors - .where('last_read_comment_id < ?', comment_id) - .update_all( - last_read_comment_id: comment_id, - read_at: read_at, - updated_at: now - ) - # rubocop:enable Rails/SkipsModelValidations + def self.advance(task:, user:, comment:, read_at: Time.current) + cursor = create_or_find_by!(task: task, user: user) do |new_cursor| + new_cursor.last_read_comment = comment + new_cursor.read_at = read_at end + + # A cursor is a high-water mark, so an older comment cannot move it backwards. + return cursor if cursor.last_read_comment_id >= comment.id + + cursor.with_lock do + cursor.update!(last_read_comment: comment, read_at: read_at) if cursor.last_read_comment_id < comment.id + end + + cursor end private diff --git a/app/models/comments/extension_comment.rb b/app/models/comments/extension_comment.rb index abd9d1030c..1a3be16f39 100644 --- a/app/models/comments/extension_comment.rb +++ b/app/models/comments/extension_comment.rb @@ -19,11 +19,8 @@ def assessed? # Make sure we can access super's version of mark_as_read for assess extension alias :super_mark_as_read :mark_as_read - # Allow individual staff and the student to read this... but stop - # the main tutor reading without assessing. As only the main tutor - # propagates reads, this will work as required - other staff cant - # make it read for the main tutor. - def mark_as_read(user, unit = self.unit) + # Do not let the recipient tutor mark the request as read before assessing it. + def mark_as_read(user) super if assessed? || user == project.student || user != recipient end diff --git a/app/models/comments/scorm_extension_comment.rb b/app/models/comments/scorm_extension_comment.rb index 74bc9d0c8c..ad0280e594 100644 --- a/app/models/comments/scorm_extension_comment.rb +++ b/app/models/comments/scorm_extension_comment.rb @@ -16,11 +16,8 @@ def assessed? # Make sure we can access super's version of mark_as_read for assess extension alias super_mark_as_read mark_as_read - # Allow individual staff and the student to read this... but stop - # the main tutor reading without assessing. As only the main tutor - # propagates reads, this will work as required - other staff cant - # make it read for the main tutor. - def mark_as_read(user, unit = self.unit) + # Do not let the recipient tutor mark the request as read before assessing it. + def mark_as_read(user) super if assessed? || user == project.student || user != recipient end diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index d4d32822e9..da45536405 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -81,12 +81,8 @@ def serialize(user) } end - def create_comment_read_receipt_entry(user) - CommentReadCursor.advance( - task_id: task_id, - user_ids: user.id, - comment_id: id - ) + def advance_read_cursor(user) + CommentReadCursor.advance(task: task, user: user, comment: self) end def comment @@ -162,18 +158,10 @@ def remove_comment_read_entry(user) end end - def mark_as_read(user, unit = self.unit) - return if read_by?(user) # avoid propagating if not needed + def mark_as_read(user) + return if read_by?(user) - if user == project.tutor_for(task.task_definition) - CommentReadCursor.advance( - task_id: task_id, - user_ids: unit.staff.pluck(:user_id), - comment_id: id - ) - else - create_comment_read_receipt_entry(user) - end + advance_read_cursor(user) end def mark_as_unread(user) diff --git a/app/models/task.rb b/app/models/task.rb index 845ac74132..7687d27124 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -235,7 +235,7 @@ def mark_comments_as_read(user, comments) end latest_comment_by_task.each_value do |comment| - comment.mark_as_read(user, unit) + comment.mark_as_read(user) end end @@ -1092,7 +1092,7 @@ def add_discussion_comment(user, prompts) raise "Error attaching uploaded file." unless discussion.add_prompt(prompt, index) end - discussion.mark_as_read(user, unit) + discussion.mark_as_read(user) logger.info(discussion) return discussion diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb index aac62a5e48..87ad9ec67a 100644 --- a/test/models/comment_read_cursor_test.rb +++ b/test/models/comment_read_cursor_test.rb @@ -21,9 +21,9 @@ def test_comment_reads_use_one_cursor_per_task_and_user assert(comments.none? { |comment| comment.new_for?(reader) }) CommentReadCursor.advance( - task_id: task.id, - user_ids: reader.id, - comment_id: comments.first.id + task: task, + user: reader, + comment: comments.first ) assert_equal comments.last.id, cursor.reload.last_read_comment_id @@ -33,4 +33,19 @@ def test_comment_reads_use_one_cursor_per_task_and_user assert comments.second.new_for?(reader) assert comments.last.new_for?(reader) end + + def test_reading_a_comment_only_advances_the_readers_cursor + project = FactoryBot.create(:project) + task = project.task_for_task_definition(project.unit.task_definitions.first) + assigned_tutor = project.tutor_for(task.task_definition) + other_staff = FactoryBot.create(:user, :tutor) + project.unit.employ_staff(other_staff, Role.tutor) + comment = task.add_text_comment(project.student, 'A question') + + comment.mark_as_read(assigned_tutor) + + assert comment.read_by?(assigned_tutor) + assert_not comment.read_by?(other_staff) + assert_nil CommentReadCursor.find_by(task: task, user: other_staff) + end end From cbe8bb53d70c630493fa4a435148840ed8a0d62f Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:55:51 +1000 Subject: [PATCH 05/12] chore: add test to ensure comments can be deleted and cursors are updated --- app/models/comments/comment_read_cursor.rb | 1 - test/models/comment_read_cursor_test.rb | 30 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/app/models/comments/comment_read_cursor.rb b/app/models/comments/comment_read_cursor.rb index f0315f37fb..70c725af68 100644 --- a/app/models/comments/comment_read_cursor.rb +++ b/app/models/comments/comment_read_cursor.rb @@ -6,7 +6,6 @@ class CommentReadCursor < ApplicationRecord belongs_to :last_read_comment, class_name: 'TaskComment' validates :task, :user, :last_read_comment, :read_at, presence: true - validates :task_id, uniqueness: { scope: :user_id } validate :last_read_comment_belongs_to_task def self.advance(task:, user:, comment:, read_at: Time.current) diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb index 87ad9ec67a..7914eb39ba 100644 --- a/test/models/comment_read_cursor_test.rb +++ b/test/models/comment_read_cursor_test.rb @@ -48,4 +48,34 @@ def test_reading_a_comment_only_advances_the_readers_cursor assert_not comment.read_by?(other_staff) assert_nil CommentReadCursor.find_by(task: task, user: other_staff) end + + def test_destroying_the_cursor_comment_rewinds_each_users_cursor + project = FactoryBot.create(:project) + task = project.task_for_task_definition(project.unit.task_definitions.first) + author = project.unit.main_convenor_user + readers = [project.student, FactoryBot.create(:user, :tutor)] + previous_comment = task.add_text_comment(author, 'First') + cursor_comment = task.add_text_comment(author, 'Second') + + readers.each { |reader| cursor_comment.mark_as_read(reader) } + + cursor_comment.destroy! + + readers.each do |reader| + cursor = CommentReadCursor.find_by!(task: task, user: reader) + assert_equal previous_comment.id, cursor.last_read_comment_id + end + end + + def test_destroying_the_only_comment_removes_its_cursors + project = FactoryBot.create(:project) + task = project.task_for_task_definition(project.unit.task_definitions.first) + reader = project.student + comment = task.add_text_comment(project.unit.main_convenor_user, 'Only comment') + + comment.mark_as_read(reader) + comment.destroy! + + assert_nil CommentReadCursor.find_by(task: task, user: reader) + end end From b5c33adbd57438be8fd70835be0f99564d3129da Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:56:35 +1000 Subject: [PATCH 06/12] chore: revert inbox query refactor --- app/models/unit.rb | 125 ++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 82 deletions(-) diff --git a/app/models/unit.rb b/app/models/unit.rb index d7fbd7747b..c7a47f2700 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -2452,86 +2452,51 @@ def tutorial_enrolment_subquery .select('tutorials.tutorial_stream_id as tutorial_stream_id', 'tutorials.id as tutorial_id', 'project_id', 'tutorials.unit_role_id as unit_role_id').to_sql end - def task_comment_summary_subquery(user) - TaskComment - .joins(task: :project) - .joins( - "LEFT JOIN comment_read_cursors inbox_cursor " \ - "ON inbox_cursor.task_id = task_comments.task_id " \ - "AND inbox_cursor.user_id = #{user.id.to_i}" - ) - .where(projects: { unit_id: id }) - .where("task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment'") - .where( - "task_comments.content_type IS NULL OR " \ - "task_comments.content_type NOT IN ('plan', 'discussed_in_class')" - ) - .select( - 'task_comments.task_id AS task_id', - 'SUM(CASE WHEN inbox_cursor.last_read_comment_id IS NULL ' \ - 'OR task_comments.id > inbox_cursor.last_read_comment_id THEN 1 ELSE 0 END) AS number_unread', - 'MAX(task_comments.created_at) AS latest_comment_at', - "MAX(CASE WHEN task_comments.type = 'ExtensionComment' " \ - 'AND task_comments.date_extension_assessed IS NULL THEN 1 ELSE 0 END) AS has_extensions' - ) - .group('task_comments.task_id') - .to_sql - end - - def task_similarity_summary_subquery - TaskSimilarity - .joins(task: :project) - .where(projects: { unit_id: id }) - .where(flagged: true) - .select('task_similarities.task_id AS task_id', 'COUNT(*) AS similar_to_count') - .group('task_similarities.task_id') - .to_sql - end - # # Return all tasks from the database for this unit and given user # def get_all_tasks_for(user, my_tutorials_only = false) - result = student_tasks - .joins(:task_status) - .joins( - "LEFT OUTER JOIN (#{tutorial_enrolment_subquery}) AS sq " \ - 'ON sq.project_id = projects.id ' \ - 'AND (sq.tutorial_stream_id = task_definitions.tutorial_stream_id ' \ - 'OR sq.tutorial_stream_id IS NULL)' - ) - .joins( - "LEFT OUTER JOIN (#{task_comment_summary_subquery(user)}) AS comment_summary " \ - 'ON comment_summary.task_id = tasks.id' - ) - .joins( - "LEFT JOIN task_pins AS inbox_pins " \ - "ON inbox_pins.task_id = tasks.id AND inbox_pins.user_id = #{user.id.to_i}" - ) - .joins( - "LEFT OUTER JOIN (#{task_similarity_summary_subquery}) AS similarity_summary " \ - 'ON similarity_summary.task_id = tasks.id' - ) - .select( - 'sq.tutorial_id AS tutorial_id', - 'sq.tutorial_stream_id AS tutorial_stream_id', - 'tasks.id', - 'COALESCE(comment_summary.number_unread, 0) AS number_unread', - 'inbox_pins.task_id IS NOT NULL AS pinned', - 'COALESCE(comment_summary.has_extensions, 0) > 0 AS has_extensions', - 'tasks.project_id', - 'tasks.id AS task_id', - 'tasks.task_definition_id', - 'task_definitions.start_date AS start_date', - 'task_statuses.id AS status_id', - 'tasks.completion_date', - 'tasks.times_assessed', - 'tasks.submission_date', - 'tasks.grade AS grade', - 'tasks.quality_pts', - 'COALESCE(similarity_summary.similar_to_count, 0) AS similar_to_count', - 'comment_summary.latest_comment_at' - ) + result = student_tasks. + joins(:task_status). + joins("LEFT OUTER JOIN (#{tutorial_enrolment_subquery}) as sq ON sq.project_id = projects.id AND (sq.tutorial_stream_id = task_definitions.tutorial_stream_id OR sq.tutorial_stream_id IS NULL)"). + joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment') AND (task_comments.content_type IS NULL OR (task_comments.content_type <> 'plan' AND task_comments.content_type <> 'discussed_in_class'))"). + joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = tasks.id AND crc.user_id = #{user.id}"). + joins("LEFT JOIN task_pins ON task_pins.task_id = tasks.id AND task_pins.user_id = #{user.id}"). + joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id'). + select( + 'sq.tutorial_id AS tutorial_id', + 'sq.tutorial_stream_id AS tutorial_stream_id', + 'tasks.id', + "SUM(case when (crc.last_read_comment_id IS NULL OR task_comments.id > crc.last_read_comment_id) AND NOT task_comments.id is null then 1 else 0 end) as number_unread", + 'COUNT(distinct task_pins.task_id) != 0 as pinned', + "SUM(case when task_comments.date_extension_assessed IS NULL AND task_comments.type = 'ExtensionComment' AND NOT task_comments.id IS NULL THEN 1 ELSE 0 END) > 0 as has_extensions", + 'project_id', + 'tasks.id as task_id', + 'task_definition_id', + 'task_definitions.start_date as start_date', + 'task_statuses.id as status_id', + 'completion_date', + 'times_assessed', + 'submission_date', + 'tasks.grade as grade', + 'quality_pts', + 'SUM(case when task_similarities.flagged then 1 else 0 end) as similar_to_count' + ). + group( + 'sq.tutorial_id', + 'sq.tutorial_stream_id', + 'task_statuses.id', + 'project_id', + 'tasks.id', + 'task_definition_id', + 'task_definitions.start_date', + 'status_id', + 'completion_date', + 'times_assessed', + 'submission_date', + 'grade', + 'quality_pts' + ) if my_tutorials_only unit_role = unit_role_for(user) unless unit_role.nil? @@ -2564,12 +2529,8 @@ def tasks_awaiting_feedback(user) # def tasks_for_task_inbox(user, my_students_only = false) get_all_tasks_for(user, my_students_only) - .where( - 'task_statuses.id IN (:ids) OR inbox_pins.task_id IS NOT NULL OR ' \ - 'COALESCE(comment_summary.number_unread, 0) > 0', - ids: [TaskStatus.ready_for_feedback, TaskStatus.need_help] - ) - .order('pinned DESC, submission_date ASC, latest_comment_at ASC, task_definition_id ASC') + .having('task_statuses.id IN (:ids) OR COUNT(task_pins.task_id) > 0 OR SUM(case when (crc.last_read_comment_id IS NULL OR task_comments.id > crc.last_read_comment_id) AND NOT task_comments.id is null then 1 else 0 end) > 0', ids: [TaskStatus.ready_for_feedback, TaskStatus.need_help]) + .order('pinned DESC, submission_date ASC, MAX(task_comments.created_at) ASC, task_definition_id ASC') end # From 1894a94175744a6f5cf25f80ec17646c143d07d1 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:12:25 +1000 Subject: [PATCH 07/12] chore: revert query refactor --- app/models/project.rb | 45 ++++++++++--------------------------------- 1 file changed, 10 insertions(+), 35 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 0426022d6a..3e613a3147 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -279,47 +279,22 @@ def reference_date def task_details_for_shallow_serializer(user) teaching_breaks = unit.teaching_period&.breaks.to_a - comment_summary = TaskComment - .joins(:task) - .joins( - "LEFT JOIN comment_read_cursors project_cursor " \ - "ON project_cursor.task_id = task_comments.task_id " \ - "AND project_cursor.user_id = #{user.id.to_i}" - ) - .where(tasks: { project_id: id }) - .where("task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment'") - .select( - 'task_comments.task_id AS task_id', - 'SUM(CASE WHEN project_cursor.last_read_comment_id IS NULL ' \ - 'OR task_comments.id > project_cursor.last_read_comment_id ' \ - 'THEN 1 ELSE 0 END) AS number_unread' - ) - .group('task_comments.task_id') - .to_sql - - similarity_summary = TaskSimilarity - .joins(:task) - .where(tasks: { project_id: id }) - .where(flagged: true) - .select('task_similarities.task_id AS task_id', 'COUNT(*) AS similar_to_count') - .group('task_similarities.task_id') - .to_sql tasks .joins(:task_status) - .joins( - "LEFT OUTER JOIN (#{comment_summary}) AS project_comment_summary " \ - 'ON project_comment_summary.task_id = tasks.id' - ) - .joins( - "LEFT OUTER JOIN (#{similarity_summary}) AS project_similarity_summary " \ - 'ON project_similarity_summary.task_id = tasks.id' - ) + .joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment')") + .joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = tasks.id AND crc.user_id = #{user.id}") + .joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id') .select( - 'COALESCE(project_comment_summary.number_unread, 0) AS number_unread', 'project_id', 'tasks.id as id', + 'SUM(case when (crc.last_read_comment_id IS NULL OR task_comments.id > crc.last_read_comment_id) AND NOT task_comments.id is null then 1 else 0 end) as number_unread', 'project_id', 'tasks.id as id', 'task_definition_id', 'task_statuses.id as status_id', 'completion_date', 'times_assessed', 'submission_date', 'tasks.grade as grade', 'quality_pts', 'include_in_portfolio', 'grade', - 'COALESCE(project_similarity_summary.similar_to_count, 0) AS similar_to_count' + 'SUM(case when task_similarities.flagged then 1 else 0 end) as similar_to_count' + ) + .group( + 'task_statuses.id', 'tasks.project_id', 'tasks.id', 'task_definition_id', 'status_id', + 'completion_date', 'times_assessed', 'submission_date', 'grade', 'quality_pts', + 'include_in_portfolio', 'grade' ) .map do |r| t = Task.find(r.id) From 7b2099bf7df29352d98ca1a209e558f36dd019ba Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:18:35 +1000 Subject: [PATCH 08/12] chore: remove foreign keys --- db/migrate/20260728051502_create_comment_read_cursors.rb | 6 +++--- db/schema.rb | 3 --- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/db/migrate/20260728051502_create_comment_read_cursors.rb b/db/migrate/20260728051502_create_comment_read_cursors.rb index 7f68dadad1..82cdf101a5 100644 --- a/db/migrate/20260728051502_create_comment_read_cursors.rb +++ b/db/migrate/20260728051502_create_comment_read_cursors.rb @@ -12,9 +12,9 @@ def down def create_cursor_table create_table :comment_read_cursors do |t| - t.references :task, null: false, foreign_key: true - t.references :user, null: false, foreign_key: true - t.references :last_read_comment, null: false, foreign_key: { to_table: :task_comments } + t.references :task, null: false + t.references :user, null: false + t.references :last_read_comment, null: false t.datetime :read_at, null: false t.timestamps diff --git a/db/schema.rb b/db/schema.rb index 9a8602b67c..f2fb848f48 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1039,9 +1039,6 @@ add_foreign_key "chip_usages", "feedback_chips" add_foreign_key "chip_usages", "users", column: "tutor_id" - add_foreign_key "comment_read_cursors", "task_comments", column: "last_read_comment_id" - add_foreign_key "comment_read_cursors", "tasks" - add_foreign_key "comment_read_cursors", "users" add_foreign_key "feedback_chips", "feedback_chips", column: "parent_chip_id" add_foreign_key "feedback_chips", "learning_outcomes" add_foreign_key "learning_outcome_links", "learning_outcomes", column: "source_id" From 212d8b73fccd16dd79b26c6887ec5e5208c7bdd9 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:49:46 +1000 Subject: [PATCH 09/12] feat: track audience for task comments - this allows for a mix of student comments and automated comments - tasks with student comments show up in inbox, and ignores automated comments --- app/models/comments/assessment_comment.rb | 1 + .../comments/discuss_timeout_comment.rb | 4 +++ app/models/comments/scorm_comment.rb | 1 + .../comments/task_checked_in_comment.rb | 1 + app/models/comments/task_comment.rb | 25 +++++++++++++- app/models/comments/task_discussed_comment.rb | 1 + app/models/comments/task_status_comment.rb | 1 + app/models/project.rb | 3 +- app/models/task.rb | 17 ++++++++-- app/models/unit.rb | 9 ++++- app/sidekiq/accept_overseer_job.rb | 6 +++- app/sidekiq/execute_communication_set_job.rb | 2 +- ...add_attention_audience_to_task_comments.rb | 5 +++ db/schema.rb | 3 +- lib/tasks/maintenance.rake | 12 +++++-- test/models/comment_read_cursor_test.rb | 33 +++++++++++++++++++ 16 files changed, 113 insertions(+), 11 deletions(-) create mode 100644 db/migrate/20260729020310_add_attention_audience_to_task_comments.rb diff --git a/app/models/comments/assessment_comment.rb b/app/models/comments/assessment_comment.rb index 1b614b41f5..60dd4d26b4 100644 --- a/app/models/comments/assessment_comment.rb +++ b/app/models/comments/assessment_comment.rb @@ -1,6 +1,7 @@ class AssessmentComment < TaskComment before_create do self.content_type = :assessment + self.attention_audience = :student end def serialize(user) diff --git a/app/models/comments/discuss_timeout_comment.rb b/app/models/comments/discuss_timeout_comment.rb index a45b8aadd6..e6a2e58810 100644 --- a/app/models/comments/discuss_timeout_comment.rb +++ b/app/models/comments/discuss_timeout_comment.rb @@ -2,6 +2,10 @@ class DiscussTimeoutComment < TaskComment WARNING_CONTENT_TYPE = 'discuss_timeout_warning'.freeze EXPIRED_CONTENT_TYPE = 'discuss_timeout_expired'.freeze + before_create do + self.attention_audience = :student + end + def self.warning WARNING_CONTENT_TYPE end diff --git a/app/models/comments/scorm_comment.rb b/app/models/comments/scorm_comment.rb index df7bcc9f56..0603b62a5a 100644 --- a/app/models/comments/scorm_comment.rb +++ b/app/models/comments/scorm_comment.rb @@ -1,6 +1,7 @@ class ScormComment < TaskComment before_create do self.content_type = :scorm + self.attention_audience = :student end def serialize(user) diff --git a/app/models/comments/task_checked_in_comment.rb b/app/models/comments/task_checked_in_comment.rb index 36dfd4b135..cc17093c69 100644 --- a/app/models/comments/task_checked_in_comment.rb +++ b/app/models/comments/task_checked_in_comment.rb @@ -1,6 +1,7 @@ class TaskCheckedInComment < TaskComment before_create do self.content_type = :checked_in + self.attention_audience = :none end after_create do diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index da45536405..c84f679fd5 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -8,6 +8,8 @@ class TaskComment < ApplicationRecord include FileHelper include AuthorisationHelpers + enum :attention_audience, { none: 0, student: 1, staff: 2 }, prefix: :attention + belongs_to :task, optional: false # Foreign key belongs_to :user, optional: false has_one :unit, through: :task @@ -33,6 +35,8 @@ class TaskComment < ApplicationRecord validates :comment, length: { minimum: 0, maximum: 4095, allow_blank: true } validate :valid_reply_to?, on: :create + before_validation :set_default_attention_audience, on: :create + # After create, mark as read by user creating after_create do mark_as_read(self.user) @@ -169,19 +173,30 @@ def mark_as_unread(user) end def new_for?(user) - !read_by? user + requires_attention_for?(user) && !read_by?(user) end def read_by?(user) + return true if self.user == user || !requires_attention_for?(user) + cursor = CommentReadCursor.find_by(task_id: task_id, user_id: user.id) cursor.present? && cursor.last_read_comment_id >= id end def time_read_by(user) + return nil unless requires_attention_for?(user) + cursor = CommentReadCursor.find_by(task_id: task_id, user_id: user.id) cursor&.read_at if cursor&.last_read_comment_id.to_i >= id end + def requires_attention_for?(user) + return true if attention_audience.nil? + return attention_student? if user == project.student + + attention_staff? + end + def rewind_comment_read_cursors previous_comment_id = TaskComment .where(task_id: task_id) @@ -202,4 +217,12 @@ def rewind_comment_read_cursors # rubocop:enable Rails/SkipsModelValidations end end + + private + + def set_default_attention_audience + return if attention_audience.present? || user.nil? || task.nil? + + self.attention_audience = user == task.project.student ? :staff : :student + end end diff --git a/app/models/comments/task_discussed_comment.rb b/app/models/comments/task_discussed_comment.rb index 5c4ee3a71a..f756e54f10 100644 --- a/app/models/comments/task_discussed_comment.rb +++ b/app/models/comments/task_discussed_comment.rb @@ -1,6 +1,7 @@ class TaskDiscussedComment < TaskComment before_create do self.content_type = :discussed_in_class + self.attention_audience = :none end after_create do diff --git a/app/models/comments/task_status_comment.rb b/app/models/comments/task_status_comment.rb index a224279db8..16bf2d90bc 100644 --- a/app/models/comments/task_status_comment.rb +++ b/app/models/comments/task_status_comment.rb @@ -3,6 +3,7 @@ class TaskStatusComment < TaskComment before_create do self.content_type = :status + self.attention_audience = :none end after_create do diff --git a/app/models/project.rb b/app/models/project.rb index 3e613a3147..16634c8385 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -279,10 +279,11 @@ def reference_date def task_details_for_shallow_serializer(user) teaching_breaks = unit.teaching_period&.breaks.to_a + attention_audience = TaskComment.attention_audiences.fetch(user == student ? 'student' : 'staff') tasks .joins(:task_status) - .joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment')") + .joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.attention_audience IS NULL OR task_comments.attention_audience = #{attention_audience}) AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment')") .joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = tasks.id AND crc.user_id = #{user.id}") .joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id') .select( diff --git a/app/models/task.rb b/app/models/task.rb index 7687d27124..d2526f7775 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -230,6 +230,8 @@ def mark_comments_as_read(user, comments) latest_comment_by_task = {} comments.each do |comment| + next unless comment.requires_attention_for?(user) + current = latest_comment_by_task[comment.task_id] latest_comment_by_task[comment.task_id] = comment if current.nil? || current.id < comment.id end @@ -891,7 +893,11 @@ def assess(task_status, assessor, assess_date = Time.zone.now, recursive_fix = f # Since we are calling this assess method again, we recursively check for more dependent tasks that need to be updated task.assess(TaskStatus.fix_and_resubmit, assessor, assess_date, recursive_fix) task.add_status_comment(assessor, TaskStatus.fix_and_resubmit) - task.add_text_comment(assessor, "**Automated comment**: A prerequisite task was updated to Fix and Resubmit, so this task was updated as well. You may need to review and update the prerequisite before resubmitting.") + task.add_text_comment( + assessor, + "**Automated comment**: A prerequisite task was updated to Fix and Resubmit, so this task was updated as well. You may need to review and update the prerequisite before resubmitting.", + attention_audience: :student + ) end end @@ -991,7 +997,7 @@ def weight task_definition.weighting.to_f end - def add_text_comment(user, text, reply_to_id = nil) + def add_text_comment(user, text, reply_to_id = nil, attention_audience: nil) text = text.strip return nil if user.nil? || text.nil? || text.empty? @@ -1009,6 +1015,7 @@ def add_text_comment(user, text, reply_to_id = nil) comment.content_type = :text comment.recipient = user == project.student ? project.tutor_for(task_definition) : project.student comment.reply_to_id = reply_to_id + comment.attention_audience = attention_audience if attention_audience.present? comment.save! comment @@ -1631,7 +1638,11 @@ def convert_submission_to_pdf(source_folder: FileHelper.student_work_dir(:new), rescue => e SubmissionHistory.clear_document_previews(self) trigger_transition trigger: 'fix', by_user: project.tutor_for(task_definition) - add_text_comment project.tutor_for(task_definition), "**Automated Comment**: Something went wrong with your submission. Check the files and resubmit this task. #{e.message}" + add_text_comment( + project.tutor_for(task_definition), + "**Automated Comment**: Something went wrong with your submission. Check the files and resubmit this task. #{e.message}", + attention_audience: :student + ) raise e ensure # Ensure latex aux file is removed - if broken will cause issues for next submission in sidekiq diff --git a/app/models/unit.rb b/app/models/unit.rb index c7a47f2700..039e800c5c 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -2456,10 +2456,17 @@ def tutorial_enrolment_subquery # Return all tasks from the database for this unit and given user # def get_all_tasks_for(user, my_tutorials_only = false) + staff_attention = TaskComment.attention_audiences.fetch('staff') result = student_tasks. joins(:task_status). joins("LEFT OUTER JOIN (#{tutorial_enrolment_subquery}) as sq ON sq.project_id = projects.id AND (sq.tutorial_stream_id = task_definitions.tutorial_stream_id OR sq.tutorial_stream_id IS NULL)"). - joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment') AND (task_comments.content_type IS NULL OR (task_comments.content_type <> 'plan' AND task_comments.content_type <> 'discussed_in_class'))"). + joins( + "LEFT JOIN task_comments ON task_comments.task_id = tasks.id " \ + "AND (task_comments.attention_audience IS NULL OR task_comments.attention_audience = #{staff_attention}) " \ + "AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment') " \ + "AND (task_comments.content_type IS NULL OR (task_comments.content_type <> 'plan' " \ + "AND task_comments.content_type <> 'discussed_in_class'))" + ). joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = tasks.id AND crc.user_id = #{user.id}"). joins("LEFT JOIN task_pins ON task_pins.task_id = tasks.id AND task_pins.user_id = #{user.id}"). joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id'). diff --git a/app/sidekiq/accept_overseer_job.rb b/app/sidekiq/accept_overseer_job.rb index 59ddd753fd..5227e287d0 100644 --- a/app/sidekiq/accept_overseer_job.rb +++ b/app/sidekiq/accept_overseer_job.rb @@ -108,7 +108,11 @@ def perform(task_id, _output_path, docker_image_name_tag, submission, assessment task.add_status_comment(task.project.tutor_for(task.task_definition), failure_status) oa.update!(result_task_status: failure_status.status_key.to_s) end - task.add_text_comment(task.project.tutor_for(task.task_definition), "**Automated comment**: Some tests did not pass for this submission. Please review the Overseer report, verify your output, and resubmit.") + task.add_text_comment( + task.project.tutor_for(task.task_definition), + "**Automated comment**: Some tests did not pass for this submission. Please review the Overseer report, verify your output, and resubmit.", + attention_audience: :student + ) end FileUtils.rm_rf(work_dir) diff --git a/app/sidekiq/execute_communication_set_job.rb b/app/sidekiq/execute_communication_set_job.rb index 5f99bd3fe6..0edf567b8c 100644 --- a/app/sidekiq/execute_communication_set_job.rb +++ b/app/sidekiq/execute_communication_set_job.rb @@ -256,7 +256,7 @@ def execute_task_comment_action(action, projects, unit, rule) } end - comment = task.add_text_comment(comment_author, rendered_comment) + comment = task.add_text_comment(comment_author, rendered_comment, attention_audience: :student) if comment.nil? next { diff --git a/db/migrate/20260729020310_add_attention_audience_to_task_comments.rb b/db/migrate/20260729020310_add_attention_audience_to_task_comments.rb new file mode 100644 index 0000000000..469a93c0eb --- /dev/null +++ b/db/migrate/20260729020310_add_attention_audience_to_task_comments.rb @@ -0,0 +1,5 @@ +class AddAttentionAudienceToTaskComments < ActiveRecord::Migration[8.0] + def change + add_column :task_comments, :attention_audience, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index f2fb848f48..937893a976 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2026_07_28_051502) do +ActiveRecord::Schema[8.0].define(version: 2026_07_29_020310) 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 @@ -558,6 +558,7 @@ t.bigint "reply_to_id" t.bigint "commentable_id" t.string "commentable_type" + t.integer "attention_audience" t.index ["assessor_id"], name: "index_task_comments_on_assessor_id" t.index ["commentable_type", "commentable_id"], name: "index_task_comments_on_commentable_type_and_commentable_id" t.index ["discussion_comment_id"], name: "index_task_comments_on_discussion_comment_id" diff --git a/lib/tasks/maintenance.rake b/lib/tasks/maintenance.rake index d7053b6e2b..8986d838cf 100644 --- a/lib/tasks/maintenance.rake +++ b/lib/tasks/maintenance.rake @@ -79,7 +79,11 @@ namespace :maintenance do tutor = task.project.tutor_for(task.task_definition) task.trigger_transition(trigger: 'fix', by_user: tutor) - task.add_text_comment(tutor, "**Automated Comment**: Something went wrong with compiling your submission. Please resubmit the task.") + task.add_text_comment( + tutor, + "**Automated Comment**: Something went wrong with compiling your submission. Please resubmit the task.", + attention_audience: :student + ) rescue StandardError => e Rails.logger.error "Failed to move task #{task.id} to fix and add automated comment!\n#{e.message}" end @@ -88,7 +92,11 @@ namespace :maintenance do tutor = task.project.tutor_for(task.task_definition) task.trigger_transition(trigger: 'fix', by_user: tutor) - task.add_text_comment(tutor, "**Automated Comment**: Something went wrong while running the automated tests for this submission. Please resubmit the task.") + task.add_text_comment( + tutor, + "**Automated Comment**: Something went wrong while running the automated tests for this submission. Please resubmit the task.", + attention_audience: :student + ) rescue StandardError => e Rails.logger.error "Failed to move task #{task.id} to fix and add Overseer automated comment!\n#{e.message}" end diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb index 7914eb39ba..dfb6ede098 100644 --- a/test/models/comment_read_cursor_test.rb +++ b/test/models/comment_read_cursor_test.rb @@ -78,4 +78,37 @@ def test_destroying_the_only_comment_removes_its_cursors assert_nil CommentReadCursor.find_by(task: task, user: reader) end + + def test_only_staff_attention_comments_count_in_the_tutor_inbox + project = FactoryBot.create(:project) + task = project.task_for_task_definition(project.unit.task_definitions.first) + tutor = project.tutor_for(task.task_definition) + + student_comment = task.add_text_comment(project.student, 'Please review this') + status_comment = task.add_status_comment(project.student, TaskStatus.ready_for_feedback) + assessment_comment = AssessmentComment.create!( + task: task, + user: tutor, + recipient: project.student, + comment: 'Automated assessment complete' + ) + + assert student_comment.attention_staff? + assert status_comment.attention_none? + assert assessment_comment.attention_student? + assert student_comment.new_for?(tutor) + assert_not status_comment.new_for?(tutor) + assert_not assessment_comment.new_for?(tutor) + assert_nil CommentReadCursor.find_by(task: task, user: tutor) + + inbox_task = project.unit.tasks_for_task_inbox(tutor).find { |item| item.task_id == task.id } + assert_not_nil inbox_task + assert_equal 1, inbox_task.number_unread.to_i + + task.mark_comments_as_read(tutor, task.comments) + + cursor = CommentReadCursor.find_by!(task: task, user: tutor) + assert_equal student_comment.id, cursor.last_read_comment_id + assert_not(project.unit.tasks_for_task_inbox(tutor).any? { |item| item.task_id == task.id }) + end end From 3c848d2b24902dcb25cfbb577bf208b517852b73 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:31:07 +1000 Subject: [PATCH 10/12] chore: fix test --- test/models/comment_read_cursor_test.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb index dfb6ede098..393792a446 100644 --- a/test/models/comment_read_cursor_test.rb +++ b/test/models/comment_read_cursor_test.rb @@ -52,8 +52,10 @@ def test_reading_a_comment_only_advances_the_readers_cursor def test_destroying_the_cursor_comment_rewinds_each_users_cursor project = FactoryBot.create(:project) task = project.task_for_task_definition(project.unit.task_definitions.first) - author = project.unit.main_convenor_user - readers = [project.student, FactoryBot.create(:user, :tutor)] + other_staff = FactoryBot.create(:user, :tutor) + project.unit.employ_staff(other_staff, Role.tutor) + author = project.student + readers = [project.tutor_for(task.task_definition), other_staff] previous_comment = task.add_text_comment(author, 'First') cursor_comment = task.add_text_comment(author, 'Second') From 14c09beac7f2f89a2617b59c9193c9b2b0963512 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:08:26 +1000 Subject: [PATCH 11/12] feat: ensure comment from tutor is marked as read by all other staff --- app/models/comments/task_comment.rb | 12 ++++++-- test/models/comment_read_cursor_test.rb | 41 +++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index c84f679fd5..8e456fa72d 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -162,10 +162,18 @@ def remove_comment_read_entry(user) end end - def mark_as_read(user) + def mark_as_read(user, unit = self.unit) return if read_by?(user) - advance_read_cursor(user) + if user == project.tutor_for(task.task_definition) + CommentReadCursor.transaction do + unit.staff.each do |staff_member| + advance_read_cursor(staff_member.user) + end + end + else + advance_read_cursor(user) + end end def mark_as_unread(user) diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb index 393792a446..f5e144bc51 100644 --- a/test/models/comment_read_cursor_test.rb +++ b/test/models/comment_read_cursor_test.rb @@ -34,7 +34,7 @@ def test_comment_reads_use_one_cursor_per_task_and_user assert comments.last.new_for?(reader) end - def test_reading_a_comment_only_advances_the_readers_cursor + def test_assigned_tutor_reading_a_comment_advances_all_staff_cursors project = FactoryBot.create(:project) task = project.task_for_task_definition(project.unit.task_definitions.first) assigned_tutor = project.tutor_for(task.task_definition) @@ -45,8 +45,43 @@ def test_reading_a_comment_only_advances_the_readers_cursor comment.mark_as_read(assigned_tutor) assert comment.read_by?(assigned_tutor) - assert_not comment.read_by?(other_staff) - assert_nil CommentReadCursor.find_by(task: task, user: other_staff) + assert comment.read_by?(other_staff) + assert_equal comment.id, CommentReadCursor.find_by!(task: task, user: other_staff).last_read_comment_id + end + + def test_changing_tutorial_does_not_show_comments_already_read_by_the_teaching_team + project = FactoryBot.create(:project) + unit = project.unit + task_definition = unit.task_definitions.first + original_tutor = unit.main_convenor_user + new_tutor = FactoryBot.create(:user, :tutor) + new_tutor_role = unit.employ_staff(new_tutor, Role.tutor) + original_tutorial = FactoryBot.create( + :tutorial, + unit: unit, + campus: project.campus, + unit_role: unit.unit_role_for(original_tutor) + ) + new_tutorial = FactoryBot.create( + :tutorial, + unit: unit, + campus: project.campus, + unit_role: new_tutor_role + ) + project.enrol_in(original_tutorial) + task = project.task_for_task_definition(task_definition) + comment = task.add_text_comment(project.student, 'Please review this') + + original_inbox = unit.tasks_for_task_inbox(original_tutor, true).map(&:task_id) + assert_includes original_inbox, task.id + + comment.mark_as_read(original_tutor) + assert_equal comment.id, CommentReadCursor.find_by!(task: task, user: new_tutor).last_read_comment_id + + project.enrol_in(new_tutorial) + + new_tutor_inbox = unit.tasks_for_task_inbox(new_tutor, true).map(&:task_id) + assert_not_includes new_tutor_inbox, task.id end def test_destroying_the_cursor_comment_rewinds_each_users_cursor From 9c45b4f95be12006f3960103e7f7f9757d2d6c12 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Thu, 6 Aug 2026 15:20:20 +1000 Subject: [PATCH 12/12] wip --- app/models/comments/task_comment.rb | 24 ++++---- app/models/project.rb | 7 ++- app/models/task.rb | 10 +++ app/models/unit.rb | 20 ++++-- test/models/comment_read_cursor_test.rb | 82 ++++++++++++++++++------- 5 files changed, 102 insertions(+), 41 deletions(-) diff --git a/app/models/comments/task_comment.rb b/app/models/comments/task_comment.rb index 8e456fa72d..3dd8968dc4 100644 --- a/app/models/comments/task_comment.rb +++ b/app/models/comments/task_comment.rb @@ -162,17 +162,12 @@ def remove_comment_read_entry(user) end end - def mark_as_read(user, unit = self.unit) - return if read_by?(user) - - if user == project.tutor_for(task.task_definition) - CommentReadCursor.transaction do - unit.staff.each do |staff_member| - advance_read_cursor(staff_member.user) - end - end - else - advance_read_cursor(user) + def mark_as_read(user) + assigned_tutor = project.tutor_for(task.task_definition) + + CommentReadCursor.transaction do + advance_read_cursor(user) unless read_by?(user) + remove_unneeded_staff_cursors(assigned_tutor) if user == assigned_tutor end end @@ -200,7 +195,7 @@ def time_read_by(user) def requires_attention_for?(user) return true if attention_audience.nil? - return attention_student? if user == project.student + return attention_student? if task.student_participant?(user) attention_staff? end @@ -228,6 +223,11 @@ def rewind_comment_read_cursors private + def remove_unneeded_staff_cursors(assigned_tutor) + retained_user_ids = task.student_participant_ids << assigned_tutor.id + CommentReadCursor.where(task_id: task_id).where.not(user_id: retained_user_ids).delete_all + end + def set_default_attention_audience return if attention_audience.present? || user.nil? || task.nil? diff --git a/app/models/project.rb b/app/models/project.rb index b5f9692fef..8f18df990b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -285,8 +285,11 @@ def task_details_for_shallow_serializer(user) tasks .joins(:task_status) - .joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.attention_audience IS NULL OR task_comments.attention_audience = #{attention_audience}) AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment')") - .joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = tasks.id AND crc.user_id = #{user.id}") + .joins('LEFT JOIN tasks comment_tasks ON comment_tasks.id = tasks.id ' \ + 'OR (tasks.group_submission_id IS NOT NULL ' \ + 'AND comment_tasks.group_submission_id = tasks.group_submission_id)') + .joins("LEFT JOIN task_comments ON task_comments.task_id = comment_tasks.id AND (task_comments.attention_audience IS NULL OR task_comments.attention_audience = #{attention_audience}) AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment')") + .joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = task_comments.task_id AND crc.user_id = #{user.id.to_i}") .joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id') .select( 'SUM(case when (crc.last_read_comment_id IS NULL OR task_comments.id > crc.last_read_comment_id) AND NOT task_comments.id is null then 1 else 0 end) as number_unread', 'project_id', 'tasks.id as id', diff --git a/app/models/task.rb b/app/models/task.rb index d2526f7775..96813ddb39 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -588,6 +588,16 @@ def group_task? !group_submission.nil? || !task_definition.group_set.nil? end + def student_participant_ids + return [project.user_id] if group_submission.nil? + + group_submission.projects.distinct.pluck(:user_id) + end + + def student_participant?(user) + user.present? && student_participant_ids.include?(user.id) + end + def active_overflow_task_claim claim = overflow_task_claim return nil unless claim diff --git a/app/models/unit.rb b/app/models/unit.rb index 1bf92199b5..75c2cb269f 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -2550,24 +2550,34 @@ def tutorial_enrolment_subquery # def get_all_tasks_for(user, my_tutorials_only = false) staff_attention = TaskComment.attention_audiences.fetch('staff') + unread_comment = '(COALESCE(crc.last_read_comment_id, 0) < task_comments.id ' \ + 'AND COALESCE(tutor_crc.last_read_comment_id, 0) < task_comments.id)' result = student_tasks. joins(:task_status). joins("LEFT OUTER JOIN (#{tutorial_enrolment_subquery}) as sq ON sq.project_id = projects.id AND (sq.tutorial_stream_id = task_definitions.tutorial_stream_id OR sq.tutorial_stream_id IS NULL)"). + joins("LEFT JOIN unit_roles task_tutor_roles ON task_tutor_roles.id = COALESCE(sq.unit_role_id, #{main_convenor_id.to_i})"). joins( - "LEFT JOIN task_comments ON task_comments.task_id = tasks.id " \ + 'LEFT JOIN tasks comment_tasks ON comment_tasks.id = tasks.id ' \ + 'OR (tasks.group_submission_id IS NOT NULL ' \ + 'AND comment_tasks.group_submission_id = tasks.group_submission_id)' + ). + joins( + "LEFT JOIN task_comments ON task_comments.task_id = comment_tasks.id " \ "AND (task_comments.attention_audience IS NULL OR task_comments.attention_audience = #{staff_attention}) " \ "AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment') " \ "AND (task_comments.content_type IS NULL OR (task_comments.content_type <> 'plan' " \ "AND task_comments.content_type <> 'discussed_in_class'))" ). - joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = tasks.id AND crc.user_id = #{user.id}"). + joins("LEFT JOIN comment_read_cursors crc ON crc.task_id = task_comments.task_id AND crc.user_id = #{user.id.to_i}"). + joins('LEFT JOIN comment_read_cursors tutor_crc ON tutor_crc.task_id = task_comments.task_id ' \ + 'AND tutor_crc.user_id = task_tutor_roles.user_id'). joins("LEFT JOIN task_pins ON task_pins.task_id = tasks.id AND task_pins.user_id = #{user.id}"). joins('LEFT OUTER JOIN task_similarities ON tasks.id = task_similarities.task_id'). select( 'sq.tutorial_id AS tutorial_id', 'sq.tutorial_stream_id AS tutorial_stream_id', 'tasks.id', - "SUM(case when (crc.last_read_comment_id IS NULL OR task_comments.id > crc.last_read_comment_id) AND NOT task_comments.id is null then 1 else 0 end) as number_unread", + "SUM(case when #{unread_comment} AND task_comments.id IS NOT NULL then 1 else 0 end) as number_unread", 'COUNT(distinct task_pins.task_id) != 0 as pinned', "SUM(case when task_comments.date_extension_assessed IS NULL AND task_comments.type = 'ExtensionComment' AND NOT task_comments.id IS NULL THEN 1 ELSE 0 END) > 0 as has_extensions", 'project_id', @@ -2628,8 +2638,10 @@ def tasks_awaiting_feedback(user) # student comment -- whichever is newer. # def tasks_for_task_inbox(user, my_students_only = false) + unread_comment = '(COALESCE(crc.last_read_comment_id, 0) < task_comments.id ' \ + 'AND COALESCE(tutor_crc.last_read_comment_id, 0) < task_comments.id)' get_all_tasks_for(user, my_students_only) - .having('task_statuses.id IN (:ids) OR COUNT(task_pins.task_id) > 0 OR SUM(case when (crc.last_read_comment_id IS NULL OR task_comments.id > crc.last_read_comment_id) AND NOT task_comments.id is null then 1 else 0 end) > 0', ids: [TaskStatus.ready_for_feedback, TaskStatus.need_help]) + .having("task_statuses.id IN (:ids) OR COUNT(task_pins.task_id) > 0 OR SUM(case when #{unread_comment} AND task_comments.id IS NOT NULL then 1 else 0 end) > 0", ids: [TaskStatus.ready_for_feedback, TaskStatus.need_help]) .order('pinned DESC, submission_date ASC, MAX(task_comments.created_at) ASC, task_definition_id ASC') end diff --git a/test/models/comment_read_cursor_test.rb b/test/models/comment_read_cursor_test.rb index f5e144bc51..adee6236f6 100644 --- a/test/models/comment_read_cursor_test.rb +++ b/test/models/comment_read_cursor_test.rb @@ -34,54 +34,83 @@ def test_comment_reads_use_one_cursor_per_task_and_user assert comments.last.new_for?(reader) end - def test_assigned_tutor_reading_a_comment_advances_all_staff_cursors + def test_assigned_tutor_reading_a_comment_removes_other_staff_cursors project = FactoryBot.create(:project) task = project.task_for_task_definition(project.unit.task_definitions.first) assigned_tutor = project.tutor_for(task.task_definition) other_staff = FactoryBot.create(:user, :tutor) project.unit.employ_staff(other_staff, Role.tutor) + tutor_comment = task.add_text_comment(assigned_tutor, 'Some feedback') + tutor_comment.mark_as_read(project.student) comment = task.add_text_comment(project.student, 'A question') + comment.mark_as_read(other_staff) + assert_equal comment.id, CommentReadCursor.find_by!(task: task, user: other_staff).last_read_comment_id + comment.mark_as_read(assigned_tutor) assert comment.read_by?(assigned_tutor) - assert comment.read_by?(other_staff) - assert_equal comment.id, CommentReadCursor.find_by!(task: task, user: other_staff).last_read_comment_id + assert_nil CommentReadCursor.find_by(task: task, user: other_staff) + assert_equal tutor_comment.id, CommentReadCursor.find_by!(task: task, user: project.student).last_read_comment_id end - def test_changing_tutorial_does_not_show_comments_already_read_by_the_teaching_team + def test_assigned_tutor_cursor_hides_a_task_from_other_staff_inboxes project = FactoryBot.create(:project) unit = project.unit task_definition = unit.task_definitions.first - original_tutor = unit.main_convenor_user - new_tutor = FactoryBot.create(:user, :tutor) - new_tutor_role = unit.employ_staff(new_tutor, Role.tutor) - original_tutorial = FactoryBot.create( + assigned_tutor = FactoryBot.create(:user, :tutor) + assigned_tutor_role = unit.employ_staff(assigned_tutor, Role.tutor) + tutorial = FactoryBot.create( :tutorial, unit: unit, campus: project.campus, - unit_role: unit.unit_role_for(original_tutor) + tutorial_stream: task_definition.tutorial_stream, + unit_role: assigned_tutor_role ) - new_tutorial = FactoryBot.create( - :tutorial, - unit: unit, - campus: project.campus, - unit_role: new_tutor_role - ) - project.enrol_in(original_tutorial) + project.enrol_in(tutorial) + other_staff = FactoryBot.create(:user, :tutor) + unit.employ_staff(other_staff, Role.tutor) task = project.task_for_task_definition(task_definition) comment = task.add_text_comment(project.student, 'Please review this') - original_inbox = unit.tasks_for_task_inbox(original_tutor, true).map(&:task_id) - assert_includes original_inbox, task.id + assert_includes unit.tasks_for_task_inbox(other_staff).map(&:task_id), task.id - comment.mark_as_read(original_tutor) - assert_equal comment.id, CommentReadCursor.find_by!(task: task, user: new_tutor).last_read_comment_id + comment.mark_as_read(other_staff) + assert_equal comment.id, CommentReadCursor.find_by!(task: task, user: other_staff).last_read_comment_id - project.enrol_in(new_tutorial) + comment.mark_as_read(assigned_tutor) - new_tutor_inbox = unit.tasks_for_task_inbox(new_tutor, true).map(&:task_id) - assert_not_includes new_tutor_inbox, task.id + assert_nil CommentReadCursor.find_by(task: task, user: other_staff) + assert_not_includes unit.tasks_for_task_inbox(other_staff).map(&:task_id), task.id + end + + def test_group_students_have_independent_unread_comment_cursors + unit = FactoryBot.create( + :unit, + group_sets: 1, + groups: [{ gs: 0, students: 2 }], + group_tasks: [{ idx: 0, gs: 0 }] + ) + task_definition = unit.task_definitions.first + projects = unit.groups.first.projects.to_a + first_task = projects.first.task_for_task_definition(task_definition) + second_task = projects.second.task_for_task_definition(task_definition) + tutor = projects.first.tutor_for(task_definition) + + comment = first_task.add_text_comment(tutor, 'Feedback for the group') + second_task.reload + + assert comment.new_for?(projects.first.student) + assert comment.new_for?(projects.second.student) + assert_equal 1, unread_count(projects.first, task_definition) + assert_equal 1, unread_count(projects.second, task_definition) + + first_task.mark_comments_as_read(projects.first.student, first_task.all_comments) + + assert_not comment.new_for?(projects.first.student) + assert comment.new_for?(projects.second.student) + assert_equal 0, unread_count(projects.first, task_definition) + assert_equal 1, unread_count(projects.second, task_definition) end def test_destroying_the_cursor_comment_rewinds_each_users_cursor @@ -148,4 +177,11 @@ def test_only_staff_attention_comments_count_in_the_tutor_inbox assert_equal student_comment.id, cursor.last_read_comment_id assert_not(project.unit.tasks_for_task_inbox(tutor).any? { |item| item.task_id == task.id }) end + + private + + def unread_count(project, task_definition) + project.task_details_for_shallow_serializer(project.student) + .find { |task| task[:task_definition_id] == task_definition.id }[:num_new_comments].to_i + end end