From 8d1aa65d5f30e7f6f44a16084613e099c9426b4d Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 24 May 2022 11:22:40 +1000 Subject: [PATCH] FEATURE: Promote polymorphic bookmarks (#339) This removes the use_polymorphic_bookmarks site setting guard and promotes that code to the only code used. --- plugin.rb | 78 ++++++------------- .../user_bookmark_base_serializer_spec.rb | 9 +-- 2 files changed, 28 insertions(+), 59 deletions(-) diff --git a/plugin.rb b/plugin.rb index ad6d1f0..cfd01a0 100644 --- a/plugin.rb +++ b/plugin.rb @@ -188,24 +188,14 @@ after_initialize do BookmarkQuery.on_preload do |bookmarks, bookmark_query| if SiteSetting.assign_enabled? - if SiteSetting.use_polymorphic_bookmarks - topics = Bookmark.select_type(bookmarks, "Topic").map(&:bookmarkable).concat( - Bookmark.select_type(bookmarks, "Post").map { |bm| bm.bookmarkable.topic } - ).uniq - assignments = Assignment.strict_loading.where(topic_id: topics).includes(:assigned_to).index_by(&:topic_id) + topics = Bookmark.select_type(bookmarks, "Topic").map(&:bookmarkable).concat( + Bookmark.select_type(bookmarks, "Post").map { |bm| bm.bookmarkable.topic } + ).uniq + assignments = Assignment.strict_loading.where(topic_id: topics).includes(:assigned_to).index_by(&:topic_id) - topics.each do |topic| - assigned_to = assignments[topic.id]&.assigned_to - topic.preload_assigned_to(assigned_to) - end - else - topics = bookmarks.map(&:topic) - assignments = Assignment.strict_loading.where(topic_id: topics).includes(:assigned_to).index_by(&:topic_id) - - topics.each do |topic| - assigned_to = assignments[topic.id]&.assigned_to - topic.preload_assigned_to(assigned_to) - end + topics.each do |topic| + assigned_to = assignments[topic.id]&.assigned_to + topic.preload_assigned_to(assigned_to) end end end @@ -601,52 +591,32 @@ after_initialize do register_permitted_bulk_action_parameter :username - # UserBookmarkSerializer - add_to_serializer(:user_bookmark, :assigned_to_user, false) do - BasicUserSerializer.new(topic.assigned_to, scope: scope, root: false).as_json + add_to_class(:user_bookmark_base_serializer, :assigned_to) do + @assigned_to ||= bookmarkable_type == "Topic" ? bookmarkable.assigned_to : bookmarkable.topic.assigned_to end - add_to_serializer(:user_bookmark, 'include_assigned_to_user?') do - (SiteSetting.assigns_public || scope.can_assign?) && topic.assigned_to&.is_a?(User) + add_to_class(:user_bookmark_base_serializer, :can_have_assignment?) do + ["Post", "Topic"].include?(bookmarkable_type) end - add_to_serializer(:user_bookmark, :assigned_to_group, false) do - BasicGroupSerializer.new(topic.assigned_to, scope: scope, root: false).as_json + add_to_serializer(:user_bookmark_base, :assigned_to_user, false) do + return if !can_have_assignment? + BasicUserSerializer.new(assigned_to, scope: scope, root: false).as_json end - add_to_serializer(:user_bookmark, 'include_assigned_to_group?') do - (SiteSetting.assigns_public || scope.can_assign?) && topic.assigned_to&.is_a?(Group) + add_to_serializer(:user_bookmark_base, 'include_assigned_to_user?') do + return false if !can_have_assignment? + (SiteSetting.assigns_public || scope.can_assign?) && assigned_to&.is_a?(User) end - if SiteSetting.use_polymorphic_bookmarks - # UserBookmarkBaseSerializer - add_to_class(:user_bookmark_base_serializer, :assigned_to) do - @assigned_to ||= bookmarkable_type == "Topic" ? bookmarkable.assigned_to : bookmarkable.topic.assigned_to - end + add_to_serializer(:user_bookmark_base, :assigned_to_group, false) do + return if !can_have_assignment? + BasicGroupSerializer.new(assigned_to, scope: scope, root: false).as_json + end - add_to_class(:user_bookmark_base_serializer, :can_have_assignment?) do - ["Post", "Topic"].include?(bookmarkable_type) - end - - add_to_serializer(:user_bookmark_base, :assigned_to_user, false) do - return if !can_have_assignment? - BasicUserSerializer.new(assigned_to, scope: scope, root: false).as_json - end - - add_to_serializer(:user_bookmark_base, 'include_assigned_to_user?') do - return false if !can_have_assignment? - (SiteSetting.assigns_public || scope.can_assign?) && assigned_to&.is_a?(User) - end - - add_to_serializer(:user_bookmark_base, :assigned_to_group, false) do - return if !can_have_assignment? - BasicGroupSerializer.new(assigned_to, scope: scope, root: false).as_json - end - - add_to_serializer(:user_bookmark_base, 'include_assigned_to_group?') do - return false if !can_have_assignment? - (SiteSetting.assigns_public || scope.can_assign?) && assigned_to&.is_a?(Group) - end + add_to_serializer(:user_bookmark_base, 'include_assigned_to_group?') do + return false if !can_have_assignment? + (SiteSetting.assigns_public || scope.can_assign?) && assigned_to&.is_a?(Group) end add_to_serializer(:basic_user, :assign_icon) do diff --git a/spec/serializers/user_bookmark_base_serializer_spec.rb b/spec/serializers/user_bookmark_base_serializer_spec.rb index e223377..a28a23c 100644 --- a/spec/serializers/user_bookmark_base_serializer_spec.rb +++ b/spec/serializers/user_bookmark_base_serializer_spec.rb @@ -7,7 +7,6 @@ describe UserBookmarkBaseSerializer do include_context 'A group that is allowed to assign' before do - SiteSetting.use_polymorphic_bookmarks = true SiteSetting.assign_enabled = true add_to_assign_allowed_group(user) end @@ -19,7 +18,7 @@ describe UserBookmarkBaseSerializer do context "for Topic bookmarkable" do let!(:bookmark) { Fabricate(:bookmark, user: user, bookmarkable: post.topic) } - xit "includes assigned user in serializer" do + it "includes assigned user in serializer" do Assigner.new(topic, user).assign(user) serializer = UserTopicBookmarkSerializer.new(bookmark, scope: guardian) bookmark = serializer.as_json[:user_topic_bookmark] @@ -29,7 +28,7 @@ describe UserBookmarkBaseSerializer do expect(bookmark[:assigned_to_group]).to be(nil) end - xit "includes assigned group in serializer" do + it "includes assigned group in serializer" do Assigner.new(topic, user).assign(assign_allowed_group) serializer = UserTopicBookmarkSerializer.new(bookmark, scope: guardian) bookmark = serializer.as_json[:user_topic_bookmark] @@ -42,7 +41,7 @@ describe UserBookmarkBaseSerializer do context "for Post bookmarkable" do let!(:bookmark) { Fabricate(:bookmark, user: user, bookmarkable: post) } - xit "includes assigned user in serializer" do + it "includes assigned user in serializer" do Assigner.new(topic, user).assign(user) serializer = UserPostBookmarkSerializer.new(bookmark, scope: guardian) bookmark = serializer.as_json[:user_post_bookmark] @@ -52,7 +51,7 @@ describe UserBookmarkBaseSerializer do expect(bookmark[:assigned_to_group]).to be(nil) end - xit "includes assigned group in serializer" do + it "includes assigned group in serializer" do Assigner.new(topic, user).assign(assign_allowed_group) serializer = UserPostBookmarkSerializer.new(bookmark, scope: guardian) bookmark = serializer.as_json[:user_post_bookmark]