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/api/tasks_api.rb

@@ -132,17 +132,16 @@ class TasksApi < Grape::API
       task.extensions = params[:extensions]
       task.save!
 
-      comment = TaskComment.create(
+      TaskComment.create(
         task: task,
         user: current_user,
         comment: "Planned date adjusted to #{task.due_date.strftime('%d %b')}.",
         content_type: :plan,
+        attention_audience: :none,
         recipient: project.student,
         extension_weeks: params[:extensions]
       )
 
-      comment.mark_as_read(project.tutor_for(task_definition))
-
       present task, with: Entities::TaskEntity, include_other_projects: true, update_only: true
     else
       error!({ error: "You are not permitted to adjust the plan." }, 403)
@@ -398,16 +397,15 @@ class TasksApi < Grape::API
                        "Planned date reset: #{task_definition.start_date.strftime('%d %b')} - #{task_definition.target_date.strftime('%d %b')}."
                      end
 
-      comment = TaskComment.create(
+      TaskComment.create(
         task: task,
         user: current_user,
         comment: comment_text,
         content_type: :plan,
+        attention_audience: :none,
         recipient: project.student
       )
 
-      comment.mark_as_read(project.tutor_for(task_definition))
-
       present task, with: Entities::TaskEntity, include_other_projects: true, update_only: true
     else
       error!({ error: "You are not permitted to adjust the plan." }, 403)
@@ -437,15 +435,14 @@ class TasksApi < Grape::API
         )
 
         comment_text = "Planned date reset: #{task.task_definition.start_date.strftime('%d %b')} - #{task.task_definition.target_date.strftime('%d %b')}."
-        comment = TaskComment.create(
+        TaskComment.create(
           task: task,
           user: current_user,
           comment: comment_text,
           content_type: :plan,
+          attention_audience: :none,
           recipient: project.student
         )
-
-        comment.mark_as_read(project.tutor_for(task.task_definition))
       end
 
       present project, with: Entities::ProjectEntity, user: current_user, for_student: true, in_project: true

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/discussion_comment.rb

@@ -1,6 +1,10 @@
 class DiscussionComment < TaskComment
   include FileHelper
 
+  before_create do
+    self.attention_audience = :student
+  end
+
   def status
     return "not started" if not started and not completed
     return "opened" if started and not completed

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,10 +1,7 @@
 class TaskCheckedInComment < TaskComment
   before_create do
     self.content_type = :checked_in
-  end
-
-  after_create do
-    mark_as_read(self.recipient)
+    self.attention_audience = :none
   end
 
   def serialize(user)

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,62 @@
   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?
+    return if task.group_submission.present? && task.student_participant?(user)
+
+    self.attention_audience = user == task.project.student ? :staff : :student
   end
 end

app/models/comments/task_discussed_comment.rb

@@ -1,10 +1,7 @@
 class TaskDiscussedComment < TaskComment
   before_create do
     self.content_type = :discussed_in_class
-  end
-
-  after_create do
-    mark_as_read(self.recipient)
+    self.attention_audience = :none
   end
 
   def serialize(user)

app/models/comments/task_feedback_review_request_comment.rb

@@ -1,10 +1,7 @@
 class TaskFeedbackReviewRequestComment < TaskComment
   before_create do
     self.content_type = :feedback_review_request
-  end
-
-  after_create do
-    mark_as_read(self.recipient)
+    self.attention_audience = :none
   end
 
   def serialize(user)

app/models/comments/task_status_comment.rb

@@ -3,10 +3,7 @@ class TaskStatusComment < TaskComment
 
   before_create do
     self.content_type = :status
-  end
-
-  after_create do
-    mark_as_read(self.recipient)
+    self.attention_audience = :none
   end
 
   def serialize(user)

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