From 184ce647ea7e9159b43b3552f334e6508e6aef51 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Fri, 17 Mar 2023 17:20:24 +0100 Subject: [PATCH] FIX: correctly infer polymorphic class from bookmarkable type (#20719) Prior to this change `registered_bookmarkable` would return `nil` as `type` in `Bookmark.registered_bookmarkable_from_type(type)` would be `ChatMessage` and we registered a `Chat::Message` class. This commit will now properly rely on each model `polymorphic_class_for(name)` to help us infer the proper type from a a `bookmarkable_type`. Tests have also been added to ensure that creating/destroying chat message bookmarks is working correctly. --- Longer explanation Currently when you save a bookmark in the database, it's associated to another object through a polymorphic relationship, which will is represented by two columns: `bookmarkable_id` and `bookmarkable_type`. The `bookmarkable_id` contains the id of the relationship (a post ID for example) and the `bookmarkable_type` contains the type of the object as a string by default, (`"Post"` for example). Chat plugin just started namespacing objects, as a result a model named `ChatMessage` is now named `Chat::Message`, to avoid complex and risky migrations we rely on methods provided by rails to alter the `bookmarkable_type` when we save it: we want to still save it as `"ChatMessage"` and not `"Chat::Message"`. And, to retrieve the correct model when we load the bookmark from the database: we want `"ChatMessage"` to load the `Chat::Message` model and not the `ChatMessage`model which doesn't exist anymore. On top of this the bookmark codepath is allowing plugins to register types and will check against these types, so we alter this code path to be able to do a similar ChatMessage <-> Chat::Message dance and allow to check the type is valid. In the specific case of this commit, we were retrieving a `"ChatMessage"` bookmarkable_type from the DB and looking for it in the registered bookmarkable types which contain `Chat::Message` and not `ChatMessage`. --- app/models/bookmark.rb | 3 +- .../core_ext/bookmarks_controller_spec.rb | 36 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 plugins/chat/spec/requests/core_ext/bookmarks_controller_spec.rb diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 845ba5c76a..be664d346e 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -46,7 +46,8 @@ class Bookmark < ActiveRecord::Base validates :name, length: { maximum: 100 } def registered_bookmarkable - Bookmark.registered_bookmarkable_from_type(self.bookmarkable_type) + type = Bookmark.polymorphic_class_for(self.bookmarkable_type).name + Bookmark.registered_bookmarkable_from_type(type) end def polymorphic_columns_present diff --git a/plugins/chat/spec/requests/core_ext/bookmarks_controller_spec.rb b/plugins/chat/spec/requests/core_ext/bookmarks_controller_spec.rb new file mode 100644 index 0000000000..b82e38171c --- /dev/null +++ b/plugins/chat/spec/requests/core_ext/bookmarks_controller_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +RSpec.describe BookmarksController do + let(:current_user) { Fabricate(:user) } + let(:bookmark_message) { Fabricate(:chat_message) } + let(:bookmark_user) { current_user } + + before { sign_in(current_user) } + + context "when bookmarking a chat message" do + describe "#create" do + it "creates the bookmark" do + post "/bookmarks.json", + params: { + bookmarkable_id: bookmark_message.id, + bookmarkable_type: "Chat::Message", + reminder_at: (Time.zone.now + 1.day).iso8601, + } + + expect(response.status).to eq(200) + expect(Bookmark.find_by(bookmarkable: bookmark_message).user_id).to eq(current_user.id) + end + end + + describe "#destroy" do + let!(:bookmark) { Fabricate(:bookmark, bookmarkable: bookmark_message, user: bookmark_user) } + + it "destroys the bookmark" do + delete "/bookmarks/#{bookmark.id}.json" + + expect(response.status).to eq(200) + expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) + end + end + end +end