refactor: replace comment read receipts with per-task cursors

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?
 

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)

app/models/comments/comment_read_cursor.rb

@@ -0,0 +1,34 @@
+# 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
+  validate :last_read_comment_belongs_to_task
+
+  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
+
+  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

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

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
 

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)

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
 

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

app/models/comments/task_comment.rb

@@ -8,6 +8,8 @@
   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
@@ -16,6 +18,10 @@
   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
@@ -29,12 +35,15 @@
   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)
   end
 
   # Delete action - before dependent association
+  before_destroy :rewind_comment_read_cursors, prepend: true
   before_destroy :delete_associated_files
 
   def valid_reply_to?
@@ -76,8 +85,8 @@
     }
   end
 
-  def create_comment_read_receipt_entry(user)
-    comment_read_receipt = CommentsReadReceipts.find_or_create_by(user: user, task_comment: self)
+  def advance_read_cursor(user)
+    CommentReadCursor.advance(task: task, user: user, comment: self)
   end
 
   def comment
@@ -135,18 +144,30 @@
   end
 
   def remove_comment_read_entry(user)
-    CommentsReadReceipts.delete_all(user: user, task_comment: self)
-  end
+    cursor = CommentReadCursor.find_by(task_id: task_id, user_id: user.id)
+    return if cursor.nil? || cursor.last_read_comment_id < id
 
-  def mark_as_read(user, unit = self.unit)
-    return if read_by?(user) # avoid propagating if not needed
+    previous_comment_id = TaskComment
+                          .where(task_id: task_id)
+                          .where('id < ?', id)
+                          .maximum(:id)
 
-    if user == project.tutor_for(task.task_definition)
-      unit.staff.each do |staff_member|
-        create_comment_read_receipt_entry(staff_member.user)
-      end
+    if previous_comment_id.nil?
+      cursor.destroy!
     else
-      create_comment_read_receipt_entry(user)
+      cursor.update!(
+        last_read_comment_id: previous_comment_id,
+        read_at: Time.current
+      )
+    end
+  end
+
+  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
 
@@ -155,15 +176,61 @@
   end
 
   def new_for?(user)
-    !read_by? user
+    requires_attention_for?(user) && !read_by?(user)
   end
 
   def read_by?(user)
-    CommentsReadReceipts.find_by(user: user, task_comment: self).present?
+    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)
-    read_reciept = CommentsReadReceipts.find_by(user: user, task_comment: self)
-    read_reciept&.created_at
+    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 task.student_participant?(user)
+
+    attention_staff?
+  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
+
+  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?
+
+    self.attention_audience = user == task.project.student ? :staff : :student
   end
 end

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

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

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

app/models/project.rb

@@ -281,14 +281,18 @@ 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 comments_read_receipts crr ON crr.task_comment_id = task_comments.id AND crr.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 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',
+        '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',
         'SUM(case when task_similarities.flagged then 1 else 0 end) as similar_to_count'