Merge pull request #1040 from Retrospring/fix/n+1-notification-type-counters

Prevent 𝑛+1 for notification type counters

app/controllers/notifications_controller.rb

@@ -16,8 +16,8 @@ class NotificationsController < ApplicationController
   def index
     @type = TYPE_MAPPINGS[params[:type]] || params[:type]
     @notifications = cursored_notifications_for(type: @type, last_id: params[:last_id])
-    @notifications_last_id = @notifications.map(&:id).min
-    @more_data_available = !cursored_notifications_for(type: @type, last_id: @notifications_last_id, size: 1).count.zero?
+    paginate_notifications
+    @counters = count_unread_by_type
 
     respond_to do |format|
       format.html
@@ -27,6 +27,17 @@ class NotificationsController < ApplicationController
 
   private
 
+  def paginate_notifications
+    @notifications_last_id = @notifications.map(&:id).min
+    @more_data_available = !cursored_notifications_for(type: @type, last_id: @notifications_last_id, size: 1).count.zero?
+  end
+
+  def count_unread_by_type
+    Notification.where(recipient: current_user, new: true)
+                .group(:target_type)
+                .count(:target_type)
+  end
+
   def mark_notifications_as_read
     # using .dup to not modify @notifications -- useful in tests
     @notifications&.dup&.update_all(new: false) # rubocop:disable Rails/SkipsModelValidations

app/models/notification.rb

@@ -11,11 +11,11 @@ class Notification < ApplicationRecord
     define_cursor_paginator :cursored_for_type, :for_type
 
     def for(recipient, **kwargs)
-      where(kwargs.merge!(recipient:)).order(:created_at).reverse_order
+      where(kwargs.merge!(recipient:)).includes(:target).order(:created_at).reverse_order
     end
 
     def for_type(recipient, type, **kwargs)
-      where(kwargs.merge!(recipient:)).where(type:).order(:created_at).reverse_order
+      where(kwargs.merge!(recipient:)).includes(:target).where(type:).order(:created_at).reverse_order
     end
 
     def notify(recipient, target)

app/views/tabs/_notifications.html.haml

@@ -9,18 +9,18 @@
   .list-group
     = list_group_item t('.answer'),
       notifications_path('answer'),
-      badge: Notification.for(current_user).where(target_type: 'Answer', new: true).count
+      badge: @counters['Answer']
     = list_group_item t('.smile'),
       notifications_path('smile'),
-      badge: Notification.for(current_user).where(target_type: 'Smile', new: true).count
+      badge: @counters['Smile']
     = list_group_item t('.comment'),
       notifications_path('comment'),
-      badge: Notification.for(current_user).where(target_type: 'Comment', new: true).count
+      badge: @counters['Comment']
     = list_group_item t('.commentsmile'),
       notifications_path('commentsmile'),
-      badge: Notification.for(current_user).where(target_type: 'CommentSmile', new: true).count
+      badge: @counters['CommentSmile']
     = list_group_item t('.relationship'),
       notifications_path('relationship'),
-      badge: Notification.for(current_user).where(target_type: 'Relationship', new: true).count
+      badge: @counters['Relationship']
 
 .d-none.d-sm-block= render 'shared/links'