Merge pull request #994 from Retrospring/improve-notifications-update-query

notifications: only update all new notifications

app/controllers/notifications_controller.rb

@@ -3,6 +3,8 @@
 class NotificationsController < ApplicationController
   before_action :authenticate_user!
 
+  after_action :mark_notifications_as_read, only: %i[index]
+
   TYPE_MAPPINGS = {
     "answer"       => Notification::QuestionAnswered.name,
     "comment"      => Notification::Commented.name,
@@ -25,6 +27,11 @@ class NotificationsController < ApplicationController
 
   private
 
+  def mark_notifications_as_read
+    # using .dup to not modify @notifications -- useful in tests
+    @notifications&.dup&.update_all(new: false) # rubocop:disable Rails/SkipsModelValidations
+  end
+
   def cursored_notifications_for(type:, last_id:, size: nil)
     cursor_params = { last_id: last_id, size: size }.compact
 

app/views/layouts/notifications.html.haml

@@ -8,5 +8,4 @@
 .d-block.d-sm-none= render "shared/links"
 
 :ruby
-  Notification.for(current_user).update_all(new: false)
   parent_layout 'base'

spec/controllers/notifications_controller_spec.rb

@@ -33,5 +33,9 @@ describe NotificationsController do
       expect(response).to render_template(:index)
       expect(controller.instance_variable_get(:@notifications)).to have_attributes(size: 2)
     end
+
+    it "marks notifications as read" do
+      expect { subject }.to change { Notification.for(user).where(new: true).count }.from(2).to(0)
+    end
   end
 end