From 649d8c5b5617f4487298a7d745bc2242b9a3df0e Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Mon, 3 Jun 2024 00:11:45 +0200
Subject: [PATCH] Mark notifications as read in a single SQL query

There is no need to load all comments only to count them. Lets just let
the database do all the work. If there are no comments found, nothing
will happen anyway.

Also already filter the comments to only search for notifications for
own comments.

And add some tests :)
---
 app/services/post_service.rb       |  7 ++---
 spec/services/post_service_spec.rb | 44 ++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/app/services/post_service.rb b/app/services/post_service.rb
index bc05b84bf..02a6a7930 100644
--- a/app/services/post_service.rb
+++ b/app/services/post_service.rb
@@ -104,9 +104,8 @@ class PostService
   end
 
   def mark_like_on_comment_notifications_read(post_id)
-    comment_ids = Comment.where(commentable_id: post_id)
-
-    Notification.where(recipient_id: user.id, target_type: "Comment", target_id: comment_ids, unread: true)
-                .update_all(unread: false) if comment_ids.any?
+    Notification.where(recipient_id: user.id, target_type: "Comment",
+                       target_id: Comment.where(commentable_id: post_id, author_id: user.person.id),
+                       unread: true).update_all(unread: false) # rubocop:disable Rails/SkipsModelValidations
   end
 end
diff --git a/spec/services/post_service_spec.rb b/spec/services/post_service_spec.rb
index 491c9a7e4..b35d45947 100644
--- a/spec/services/post_service_spec.rb
+++ b/spec/services/post_service_spec.rb
@@ -168,6 +168,50 @@ describe PostService do
       expect_any_instance_of(PostService).not_to receive(:mark_mention_notifications_read).with(post.id)
       PostService.new.mark_user_notifications(post.id)
     end
+
+    context "for comments" do
+      let(:comment) { post.comments.create(author: alice.person, text: "comment") }
+
+      it "marks a corresponding notifications as read" do
+        FactoryBot.create(:notification, recipient: alice, target: comment, unread: true)
+        FactoryBot.create(:notification, recipient: alice, target: comment, unread: true)
+
+        expect {
+          PostService.new(alice).mark_user_notifications(post.id)
+        }.to change(Notification.where(unread: true), :count).by(-2)
+      end
+
+      it "does not change the update_at date/time for comment notifications" do
+        notification = Timecop.travel(1.minute.ago) do
+          FactoryBot.create(:notification, recipient: alice, target: comment, unread: true)
+        end
+
+        expect {
+          PostService.new(alice).mark_user_notifications(post.id)
+        }.not_to(change { Notification.where(id: notification.id).pluck(:updated_at) })
+      end
+
+      it "does not change other users notifications" do
+        alice_notification = FactoryBot.create(:notification, recipient: alice, target: comment, unread: true)
+        bob_notification = FactoryBot.create(:notification, recipient: bob, target: comment, unread: true)
+
+        PostService.new(alice).mark_user_notifications(post.id)
+
+        expect(Notification.find(alice_notification.id).unread).to be_falsey
+        expect(Notification.find(bob_notification.id).unread).to be_truthy
+      end
+
+      it "marks notifications for all comments on a post as read" do
+        comment2 = post.comments.create(author: alice.person, text: "comment2")
+
+        FactoryBot.create(:notification, recipient: alice, target: comment, unread: true)
+        FactoryBot.create(:notification, recipient: alice, target: comment2, unread: true)
+
+        expect {
+          PostService.new(alice).mark_user_notifications(post.id)
+        }.to change(Notification.where(unread: true), :count).by(-2)
+      end
+    end
   end
 
   describe "#destroy" do