FEATURE: Polymorphic bookmarks pt. 1 (CRUD) (#16308)

This commit introduces a new use_polymorphic_bookmarks site setting
that is default false and hidden, that will be used to help continuous
development of polymorphic bookmarks. This setting **should not** be
enabled anywhere in production yet, it is purely for local development.

This commit uses the setting to enable create/update/delete actions
for polymorphic bookmarks on the server and client side. The bookmark
interactions on topics/posts are all usable. Listing, searching,
sending bookmark reminders, and other edge cases will be handled
in subsequent PRs.

Comprehensive UI tests will be added in the final PR -- we already
have them for regular bookmarks, so it will just be a matter of
changing them to be for polymorphic bookmarks.

spec/fabricators/bookmark_fabricator.rb

@@ -6,6 +6,15 @@ Fabricator(:bookmark) do
   name "This looked interesting"
   reminder_at { 1.day.from_now.iso8601 }
   reminder_set_at { Time.zone.now }
+
+  # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
+  before_create do |bookmark|
+    if bookmark.bookmarkable_id.present? || bookmark.bookmarkable.present?
+      bookmark.post = nil
+      bookmark.post_id = nil
+      bookmark.for_topic = false
+    end
+  end
 end
 
 Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do

spec/lib/bookmark_manager_spec.rb

@@ -34,17 +34,19 @@ RSpec.describe BookmarkManager do
       expect(bookmark.for_topic).to eq(false)
     end
 
+    # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
+    # Topic bookmarks will be distinct, not attached to a post.
     it "errors when creating a for_topic bookmark for a post that is not the first one" do
       subject.create(post_id: Fabricate(:post, topic: post.topic).id, name: name, for_topic: true)
       expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.for_topic_must_use_first_post"))
     end
 
-    it "when topic is deleted it raises invalid access from guardian check" do
+    it "when topic is deleted it raises invalid access" do
       post.topic.trash!
       expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
     end
 
-    it "when post is deleted it raises invalid access from guardian check" do
+    it "when post is deleted it raises invalid access" do
       post.trash!
       expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
     end
@@ -147,17 +149,11 @@ RSpec.describe BookmarkManager do
   describe ".destroy" do
     let!(:bookmark) { Fabricate(:bookmark, user: user, post: post) }
     it "deletes the existing bookmark" do
-      result = subject.destroy(bookmark.id)
+      subject.destroy(bookmark.id)
       expect(Bookmark.exists?(id: bookmark.id)).to eq(false)
-      expect(result[:topic_bookmarked]).to eq(false)
-    end
-
-    it "returns a value indicating whether there are still other bookmarks in the topic for the user" do
-      Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic))
-      result = subject.destroy(bookmark.id)
-      expect(result[:topic_bookmarked]).to eq(true)
     end
 
+    # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
     context "if the bookmark is the last one bookmarked in the topic" do
       it "marks the topic user bookmarked column as false" do
         TopicUser.create(user: user, topic: bookmark.post.topic, bookmarked: true)
@@ -167,6 +163,19 @@ RSpec.describe BookmarkManager do
       end
     end
 
+    context "if the bookmark is the last one bookmarked in the topic (polymorphic)" do
+      before do
+        SiteSetting.use_polymorphic_bookmarks = true
+      end
+      it "marks the topic user bookmarked column as false" do
+        poly_bookmark = Fabricate(:bookmark, user: user, bookmarkable: post)
+        TopicUser.create(user: user, topic: post.topic, bookmarked: true)
+        subject.destroy(poly_bookmark.id)
+        tu = TopicUser.find_by(user: user)
+        expect(tu.bookmarked).to eq(false)
+      end
+    end
+
     context "if the bookmark is belonging to some other user" do
       let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) }
       it "raises an invalid access error" do
@@ -262,6 +271,35 @@ RSpec.describe BookmarkManager do
     end
   end
 
+  describe ".destroy_for_topic (polymorphic)" do
+    before do
+      SiteSetting.use_polymorphic_bookmarks = true
+    end
+
+    let!(:topic) { Fabricate(:topic) }
+    let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
+    let!(:bookmark2) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) }
+
+    it "destroys all bookmarks for the topic for the specified user" do
+      subject.destroy_for_topic(topic)
+      expect(Bookmark.for_user_in_topic(user.id, topic.id).length).to eq(0)
+    end
+
+    it "does not destroy any other user's topic bookmarks" do
+      user2 = Fabricate(:user)
+      Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user2)
+      subject.destroy_for_topic(topic)
+      expect(Bookmark.for_user_in_topic(user2.id, topic.id).length).to eq(1)
+    end
+
+    it "updates the topic user bookmarked column to false" do
+      TopicUser.create(user: user, topic: topic, bookmarked: true)
+      subject.destroy_for_topic(topic)
+      tu = TopicUser.find_by(user: user)
+      expect(tu.bookmarked).to eq(false)
+    end
+  end
+
   describe ".send_reminder_notification" do
     let(:bookmark) { Fabricate(:bookmark, user: user) }
     it "sets the reminder_last_sent_at" do
@@ -334,4 +372,127 @@ RSpec.describe BookmarkManager do
       end
     end
   end
+
+  describe "#create_for (use_polymorphic_bookmarks)" do
+    before do
+      SiteSetting.use_polymorphic_bookmarks = true
+    end
+
+    it "allows creating a bookmark for the topic and for the first post" do
+      subject.create_for(bookmarkable_id: post.topic_id, bookmarkable_type: "Topic", name: name)
+      bookmark = Bookmark.find_by(user: user, bookmarkable: post.topic)
+      expect(bookmark.present?).to eq(true)
+
+      subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name)
+      bookmark = Bookmark.find_by(user: user, bookmarkable: post)
+      expect(bookmark).not_to eq(nil)
+    end
+
+    it "when topic is deleted it raises invalid access from guardian check" do
+      post.topic.trash!
+      expect {
+        subject.create_for(bookmarkable_id: post.topic_id, bookmarkable_type: "Topic", name: name)
+      }.to raise_error(Discourse::InvalidAccess)
+    end
+
+    it "when post is deleted it raises invalid access from guardian check" do
+      post.trash!
+      expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess)
+    end
+
+    it "updates the topic user bookmarked column to true if any post is bookmarked" do
+      subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at)
+      tu = TopicUser.find_by(user: user)
+      expect(tu.bookmarked).to eq(true)
+      tu.update(bookmarked: false)
+      new_post = Fabricate(:post, topic: post.topic)
+      subject.create_for(bookmarkable_id: new_post.id, bookmarkable_type: "Post")
+      tu.reload
+      expect(tu.bookmarked).to eq(true)
+    end
+
+    context "when a reminder time is provided" do
+      it "saves the values correctly" do
+        subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at)
+        bookmark = Bookmark.find_by(user: user, bookmarkable: post)
+
+        expect(bookmark.reminder_at).to eq_time(reminder_at)
+        expect(bookmark.reminder_set_at).not_to eq(nil)
+      end
+    end
+
+    context "when options are provided" do
+      let(:options) { { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] } }
+
+      it "saves any additional options successfully" do
+        subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at, options: options)
+        bookmark = Bookmark.find_by(user: user, bookmarkable: post)
+
+        expect(bookmark.auto_delete_preference).to eq(1)
+      end
+    end
+
+    context "when the bookmark already exists for the user & post" do
+      before do
+        Bookmark.create(bookmarkable: post, user: user)
+      end
+
+      it "adds an error to the manager" do
+        subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post")
+        expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked", type: "Post"))
+      end
+    end
+
+    context "when the bookmark name is too long" do
+      it "adds an error to the manager" do
+        subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: "test" * 100)
+        expect(subject.errors.full_messages).to include("Name is too long (maximum is 100 characters)")
+      end
+    end
+
+    context "when the reminder time is in the past" do
+      let(:reminder_at) { 10.days.ago }
+
+      it "adds an error to the manager" do
+        subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at)
+        expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_past_reminder"))
+      end
+    end
+
+    context "when the reminder time is far-flung (> 10 years from now)" do
+      let(:reminder_at) { 11.years.from_now }
+
+      it "adds an error to the manager" do
+        subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at)
+        expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future"))
+      end
+    end
+
+    context "when the post is inaccessible for the user" do
+      before do
+        post.trash!
+      end
+      it "raises an invalid access error" do
+        expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess)
+      end
+    end
+
+    context "when the topic is inaccessible for the user" do
+      before do
+        post.topic.update(category: Fabricate(:private_category, group: Fabricate(:group)))
+      end
+      it "raises an invalid access error" do
+        expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess)
+      end
+    end
+
+    it "saves user's preference" do
+      subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:when_reminder_sent])
+
+      bookmark = Bookmark.find_by(user: user)
+      subject.update(bookmark_id: bookmark.id, name: "test", reminder_at: 1.day.from_now, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] })
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
+    end
+  end
 end

spec/lib/topic_view_spec.rb

@@ -394,17 +394,21 @@ RSpec.describe TopicView do
       end
     end
 
-    context "#user_post_bookmarks" do
+    context "#bookmarks" do
       let!(:user) { Fabricate(:user) }
       let!(:bookmark1) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) }
       let!(:bookmark2) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) }
       let!(:bookmark3) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic)) }
 
       it "returns all the bookmarks in the topic for a user" do
-        expect(TopicView.new(topic.id, user).user_post_bookmarks.pluck(:id)).to match_array(
+        expect(TopicView.new(topic.id, user).bookmarks.pluck(:id)).to match_array(
           [bookmark1.id, bookmark2.id]
         )
       end
+
+      it "returns [] for anon users" do
+        expect(TopicView.new(topic.id, nil).bookmarks.pluck(:id)).to eq([])
+      end
     end
 
     context "#bookmarks" do

spec/models/bookmark_spec.rb

@@ -37,6 +37,29 @@ describe Bookmark do
 
       expect(bookmark_3.valid?).to eq(false)
     end
+
+    describe "polymorphic bookmarks" do
+      before do
+        SiteSetting.use_polymorphic_bookmarks = true
+      end
+
+      it "does not allow a user to create a bookmark with only one polymorphic column" do
+        user = Fabricate(:user)
+        bm = Bookmark.create(bookmarkable_id: post.id, user: user)
+        expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
+        bm = Bookmark.create(bookmarkable_type: "Post", user: user)
+        expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required"))
+        bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
+        expect(bm.errors.full_messages).to be_empty
+      end
+
+      it "does not allow a user to create a bookmark for the same record more than once" do
+        user = Fabricate(:user)
+        Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
+        bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
+        expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked", type: "Post"))
+      end
+    end
   end
 
   describe "#find_for_topic_by_user" do

spec/requests/bookmarks_controller_spec.rb

@@ -3,6 +3,7 @@
 describe BookmarksController do
   let(:current_user) { Fabricate(:user) }
   let(:bookmark_post) { Fabricate(:post) }
+  let(:bookmark_topic) { Fabricate(:topic) }
   let(:bookmark_user) { current_user }
 
   before do
@@ -28,6 +29,7 @@ describe BookmarksController do
       expect(response.status).to eq(429)
     end
 
+    # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
     it "creates a for_topic bookmark" do
       post "/bookmarks.json", params: {
         post_id: bookmark_post.id,
@@ -40,6 +42,7 @@ describe BookmarksController do
       expect(bookmark.for_topic).to eq(true)
     end
 
+    # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
     it "errors when trying to create a for_topic bookmark for post_number > 1" do
       post "/bookmarks.json", params: {
         post_id: Fabricate(:post, topic: bookmark_post.topic).id,
@@ -92,6 +95,38 @@ describe BookmarksController do
         )
       end
     end
+
+    context "if the user already has bookmarked the record (polymorphic)" do
+      before do
+        SiteSetting.use_polymorphic_bookmarks = true
+        Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user)
+        Fabricate(:bookmark, bookmarkable: bookmark_topic, user: bookmark_user)
+      end
+
+      it "returns failed JSON with a 400 error" do
+        post "/bookmarks.json", params: {
+          bookmarkable_id: bookmark_post.id,
+          bookmarkable_type: "Post",
+          reminder_at: (Time.zone.now + 1.day).iso8601
+        }
+
+        expect(response.status).to eq(400)
+        expect(response.parsed_body['errors']).to include(
+          I18n.t("bookmarks.errors.already_bookmarked", type: "Post")
+        )
+
+        post "/bookmarks.json", params: {
+          bookmarkable_id: bookmark_topic.id,
+          bookmarkable_type: "Topic",
+          reminder_at: (Time.zone.now + 1.day).iso8601
+        }
+
+        expect(response.status).to eq(400)
+        expect(response.parsed_body['errors']).to include(
+          I18n.t("bookmarks.errors.already_bookmarked", type: "Topic")
+        )
+      end
+    end
   end
 
   describe "#destroy" do
@@ -102,6 +137,39 @@ describe BookmarksController do
       expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
     end
 
+    it "returns an indication of whether there are still bookmarks in the topic" do
+      delete "/bookmarks/#{bookmark.id}.json"
+      expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
+      expect(response.parsed_body["topic_bookmarked"]).to eq(false)
+      bm2 = Fabricate(:bookmark, user: bookmark_user, post: Fabricate(:post, topic: bookmark_post.topic))
+      Fabricate(:bookmark, user: bookmark_user, post: Fabricate(:post, topic: bookmark_post.topic))
+      delete "/bookmarks/#{bm2.id}.json"
+      expect(Bookmark.find_by(id: bm2.id)).to eq(nil)
+      expect(response.parsed_body["topic_bookmarked"]).to eq(true)
+    end
+
+    context "for polymorphic bookmarks" do
+      let!(:bookmark) { Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user) }
+
+      before do
+        SiteSetting.use_polymorphic_bookmarks = true
+      end
+
+      it "returns an indication of whether there are still bookmarks in the topic" do
+        delete "/bookmarks/#{bookmark.id}.json"
+        expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
+        expect(response.parsed_body["topic_bookmarked"]).to eq(false)
+        bm2 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: Fabricate(:post, topic: bookmark_post.topic))
+        bm3 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: bookmark_post.topic)
+        delete "/bookmarks/#{bm2.id}.json"
+        expect(Bookmark.find_by(id: bm2.id)).to eq(nil)
+        expect(response.parsed_body["topic_bookmarked"]).to eq(true)
+        delete "/bookmarks/#{bm3.id}.json"
+        expect(Bookmark.find_by(id: bm3.id)).to eq(nil)
+        expect(response.parsed_body["topic_bookmarked"]).to eq(false)
+      end
+    end
+
     context "if the bookmark has already been destroyed" do
       it "returns failed JSON with a 403 error" do
         bookmark.destroy!