From 1e05175364e2de9e73f723bdf883e734711404dd Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Thu, 9 Sep 2021 08:50:26 +0800
Subject: [PATCH] PERF: Avoid running query unnecessarily when updating
 bookmark. (#14276)

* Avoid loading an entire ActiveRecord object when saving and updating.
* Avoid running a DB query when `post_id` or `user_id` is not changed.
---
 app/models/bookmark.rb       | 11 +++++++----
 spec/models/bookmark_spec.rb | 21 +++++++++++++++++----
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index ac76bb19d69..6bc1a016cdf 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -31,7 +31,10 @@ class Bookmark < ActiveRecord::Base
     if: -> { reminder_type.present? }
   }
 
-  validate :unique_per_post_for_user
+  validate :unique_per_post_for_user,
+    on: [:create, :update],
+    if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_user_id? }
+
   validate :ensure_sane_reminder_at_time
   validate :bookmark_limit_not_reached
   validates :name, length: { maximum: 100 }
@@ -47,9 +50,9 @@ class Bookmark < ActiveRecord::Base
   end
 
   def unique_per_post_for_user
-    existing_bookmark = Bookmark.find_by(user_id: user_id, post_id: post_id)
-    return if existing_bookmark.blank? || existing_bookmark.id == id
-    self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post"))
+    if Bookmark.exists?(user_id: user_id, post_id: post_id)
+      self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post"))
+    end
   end
 
   def ensure_sane_reminder_at_time
diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb
index 500f4bf475d..401712c2ed1 100644
--- a/spec/models/bookmark_spec.rb
+++ b/spec/models/bookmark_spec.rb
@@ -3,9 +3,25 @@
 require 'rails_helper'
 
 describe Bookmark do
+  fab!(:post) { Fabricate(:post) }
+
+  context 'validations' do
+    it 'does not allow user to bookmark a post twice' do
+      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      user = bookmark.user
+
+      bookmark_2 = Fabricate.build(:bookmark,
+        post: post,
+        topic: post.topic,
+        user: user
+      )
+
+      expect(bookmark_2.valid?).to eq(false)
+    end
+  end
+
   describe "#cleanup!" do
     it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do
-      post = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
       post.trash!
@@ -16,7 +32,6 @@ describe Bookmark do
     end
 
     it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do
-      post = Fabricate(:post)
       post2 = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
@@ -36,7 +51,6 @@ describe Bookmark do
     end
 
     it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do
-      post = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark, topic: post.topic, post: Fabricate(:post, topic: post.topic))
       bookmark3 = Fabricate(:bookmark)
@@ -49,7 +63,6 @@ describe Bookmark do
     end
 
     it "does not delete bookmarks attached to posts that are not deleted or that have not met the 3 day grace period" do
-      post = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
       bookmark2 = Fabricate(:bookmark)
       Bookmark.cleanup!