Refactor notification handling in controllers and add NotificationReadable concern

Author: rsmithlal

app/controllers/better_together/conversations_controller.rb

@@ -3,6 +3,8 @@
 module BetterTogether
   # Handles managing conversations
   class ConversationsController < ApplicationController # rubocop:todo Metrics/ClassLength
+    include BetterTogether::NotificationReadable
+
     before_action :authenticate_user!
     before_action :disallow_robots
     before_action :set_conversations, only: %i[index new show]
@@ -103,11 +105,8 @@ def show # rubocop:todo Metrics/MethodLength, Metrics/AbcSize
       @message = @conversation.messages.build
 
       if @messages.any?
-        # Move this to separate action/bg process only activated when the messages are actually read.
-        events = BetterTogether::NewMessageNotifier.where(record_id: @messages.pluck(:id)).select(:id)
-
-        notifications = helpers.current_person.notifications.unread.where(event_id: events.pluck(:id))
-        notifications.update_all(read_at: Time.current)
+        mark_notifications_read_for_event_records(BetterTogether::NewMessageNotifier, @messages.pluck(:id),
+                                                  recipient: helpers.current_person)
       end
 
       respond_to do |format|

app/controllers/better_together/joatu/joatu_controller.rb

@@ -4,6 +4,8 @@ module BetterTogether
   module Joatu
     # Base controller for Joatu resources, adds notification mark-as-read helpers
     class JoatuController < BetterTogether::FriendlyResourceController
+      include BetterTogether::NotificationReadable
+
       # Normalize translated params so base keys are populated for current locale.
       # This helps presence validations (esp. for ActionText) during create/update
       # when forms submit locale-suffixed fields like `description_en`.
@@ -23,38 +25,6 @@ def resource_params # rubocop:todo Metrics/CyclomaticComplexity, Metrics/MethodL
 
         rp
       end
-
-      protected
-
-      # Mark Noticed notifications as read for a specific record-based event
-      def mark_notifications_read_for_record(record)
-        return unless helpers.current_person && record.respond_to?(:id)
-
-        helpers.current_person.notifications.unread
-               .includes(:event)
-               .references(:event)
-               .where(event: { record_id: record.id })
-               .update_all(read_at: Time.current)
-      end
-
-      # Mark Joatu match notifications as read when viewing an offer or request
-      def mark_match_notifications_read_for(record) # rubocop:todo Metrics/CyclomaticComplexity, Metrics/AbcSize, Metrics/MethodLength, Metrics/PerceivedComplexity
-        return unless helpers.current_person && record.respond_to?(:id)
-
-        helpers.current_person.notifications.unread.includes(:event).find_each do |notification|
-          event = notification.event
-          next unless event.is_a?(BetterTogether::Joatu::MatchNotifier)
-
-          begin
-            ids = []
-            ids << event.offer&.id if event.respond_to?(:offer)
-            ids << event.request&.id if event.respond_to?(:request)
-            notification.update(read_at: Time.current) if ids.compact.include?(record.id)
-          rescue StandardError
-            next
-          end
-        end
-      end
     end
   end
 end

app/controllers/better_together/notifications_controller.rb

@@ -3,6 +3,8 @@
 module BetterTogether
   # handles rendering and marking notifications as read
   class NotificationsController < ApplicationController
+    include BetterTogether::NotificationReadable
+
     before_action :authenticate_user!
     before_action :disallow_robots
 
@@ -43,10 +45,7 @@ def mark_notification_as_read(id)
     end
 
     def mark_record_notification_as_read(id)
-      @notifications = helpers.current_person.notifications.unread.includes(
-        :event
-      ).references(:event).where(event: { record_id: id })
-      @notifications.update_all(read_at: Time.current)
+      mark_notifications_read_for_record(Struct.new(id: id), recipient: helpers.current_person)
     end
   end
 end

app/controllers/concerns/better_together/notification_readable.rb

@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+module BetterTogether
+  # Controller concern to mark Noticed notifications as read for a record
+  module NotificationReadable
+    extend ActiveSupport::Concern
+
+    # Marks notifications (for the current person) as read for events bound to a record
+    # via Noticed::Event#record_id (generic helper used across features).
+    def mark_notifications_read_for_record(record, recipient: helpers.current_person) # rubocop:todo Metrics/AbcSize
+      return unless recipient && record.respond_to?(:id)
+
+      nn = Noticed::Notification.arel_table
+      ne = Noticed::Event.arel_table
+
+      join = nn.join(ne).on(ne[:id].eq(nn[:event_id])).join_sources
+
+      relation = Noticed::Notification
+                 .where(recipient:)
+                 .where(nn[:read_at].eq(nil))
+                 .joins(join)
+                 .where(ne[:record_id].eq(record.id))
+
+      relation.update_all(read_at: Time.current)
+    end
+
+    # Marks notifications as read for a set of records associated to a given Noticed event class
+    # using the event's record_id field.
+    def mark_notifications_read_for_event_records(event_class, record_ids, recipient: helpers.current_person) # rubocop:disable Metrics/MethodLength,Metrics/AbcSize
+      return unless recipient && record_ids.present?
+
+      nn = Noticed::Notification.arel_table
+      ne = Noticed::Event.arel_table
+
+      join = nn.join(ne).on(ne[:id].eq(nn[:event_id])).join_sources
+
+      relation = Noticed::Notification
+                 .where(recipient:)
+                 .where(nn[:read_at].eq(nil))
+                 .joins(join)
+                 .where(ne[:type].eq(event_class.to_s))
+                 .where(ne[:record_id].in(Array(record_ids)))
+
+      relation.update_all(read_at: Time.current)
+    end
+
+    # Marks Joatu match notifications as read for an Offer or Request record by matching
+    # the record's GlobalID against the Noticed::Event params (supports both direct string
+    # serialization and ActiveJob-style nested _aj_globalid key).
+    def mark_match_notifications_read_for(record, recipient: helpers.current_person) # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
+      return unless recipient && record.respond_to?(:to_global_id)
+
+      gid = record.to_global_id.to_s
+      return if gid.blank?
+
+      nn = Noticed::Notification.arel_table
+      ne = Noticed::Event.arel_table
+
+      join = nn.join(ne).on(ne[:id].eq(nn[:event_id])).join_sources
+
+      relation = Noticed::Notification
+                 .where(recipient:)
+                 .where(nn[:read_at].eq(nil))
+                 .joins(join)
+                 .where(ne[:type].eq('BetterTogether::Joatu::MatchNotifier'))
+
+      # JSONB params filter (offer/request match on direct string or AJ global id)
+      json_filter_sql = <<~SQL.squish
+        (noticed_events.params ->> 'offer' = :gid OR
+         noticed_events.params -> 'offer' ->> '_aj_globalid' = :gid OR
+         noticed_events.params ->> 'request' = :gid OR
+         noticed_events.params -> 'request' ->> '_aj_globalid' = :gid)
+      SQL
+      relation = relation.where(
+        ActiveRecord::Base.send(
+          :sanitize_sql_array, [json_filter_sql, { gid: }]
+        )
+      )
+
+      relation.update_all(read_at: Time.current)
+    end
+  end
+end

spec/controllers/concerns/notification_readable_spec.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe BetterTogether::NotificationReadable do # rubocop:todo RSpec/SpecFilePathFormat
+  let(:recipient) { create(:better_together_person) }
+
+  let(:concern_host) do
+    Class.new do
+      include BetterTogether::NotificationReadable
+    end.new
+  end
+
+  describe '#mark_match_notifications_read_for' do
+    # rubocop:todo RSpec/MultipleExpectations
+    it 'marks unread match notifications for the given record as read' do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations
+      # rubocop:enable RSpec/MultipleExpectations
+      offer = create(:better_together_joatu_offer)
+      request = create(:better_together_joatu_request)
+
+      BetterTogether::Joatu::MatchNotifier.with(offer:, request:).deliver(recipient)
+
+      unread_before = recipient.notifications.unread.joins(:event)
+                               .where(noticed_events: { type: 'BetterTogether::Joatu::MatchNotifier' }).count
+      expect(unread_before).to be >= 1
+
+      concern_host.mark_match_notifications_read_for(offer, recipient:)
+
+      unread_after = recipient.notifications.unread.joins(:event)
+                              .where(noticed_events: { type: 'BetterTogether::Joatu::MatchNotifier' }).count
+      expect(unread_after).to eq(0)
+    end
+  end
+
+  describe '#mark_notifications_read_for_record' do
+    # rubocop:todo RSpec/MultipleExpectations
+    it 'marks unread notifications tied to the event record as read' do # rubocop:todo RSpec/ExampleLength, RSpec/MultipleExpectations
+      # rubocop:enable RSpec/MultipleExpectations
+      conversation = create(:better_together_conversation)
+      create(:better_together_user, person: recipient)
+      # Ensure recipient is a participant in the conversation to be notified
+      conversation.participants << recipient unless conversation.participants.include?(recipient)
+
+      message = conversation.messages.create!(sender: recipient, content: 'Hi there')
+      BetterTogether::NewMessageNotifier.with(record: message, conversation_id: conversation.id).deliver(recipient)
+
+      unread_before = recipient.notifications.unread.joins(:event)
+                               .where(noticed_events: { type: 'BetterTogether::NewMessageNotifier' }).count
+      expect(unread_before).to be >= 1
+
+      concern_host.mark_notifications_read_for_record(message, recipient:)
+
+      unread_after = recipient.notifications.unread.joins(:event)
+                              .where(noticed_events: { type: 'BetterTogether::NewMessageNotifier' }).count
+      expect(unread_after).to eq(0)
+    end
+  end
+end

spec/factories/better_together/platforms.rb

@@ -9,7 +9,7 @@
     id { SecureRandom.uuid }
     name { Faker::Company.name }
     description { Faker::Lorem.paragraph }
-    identifier { name.parameterize }
+    identifier { Faker::Internet.unique.username(specifier: 10..20) }
     url { Faker::Internet.url }
     host { false }
     time_zone { Faker::Address.time_zone }