From 8e9164fb60873e65b96e836bc3639f38b384e0b1 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Thu, 12 May 2022 10:29:01 +1000
Subject: [PATCH] DEV: Minor bookmark tweaks for polymorphism (#16728)

* Make the modal for bookmarks display more consistently
* Make sure bookmark query can handle empty results for certain
  bookmarkable queries
---
 .../discourse/app/controllers/bookmark.js          |  7 +------
 .../common/components/bookmark-modal.scss          |  4 ++++
 config/locales/client.en.yml                       |  4 ++--
 lib/bookmark_query.rb                              | 12 +++++++++++-
 spec/lib/bookmark_query_spec.rb                    | 14 ++++++++++++++
 5 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/controllers/bookmark.js b/app/assets/javascripts/discourse/app/controllers/bookmark.js
index 0d906bbf907..fc1737a8df8 100644
--- a/app/assets/javascripts/discourse/app/controllers/bookmark.js
+++ b/app/assets/javascripts/discourse/app/controllers/bookmark.js
@@ -19,12 +19,7 @@ export function openBookmarkModal(
   return new Promise((resolve) => {
     const modalTitle = () => {
       if (options.use_polymorphic_bookmarks) {
-        return I18n.t(
-          bookmark.id ? "bookmarks.edit_for" : "bookmarks.create_for",
-          {
-            type: bookmark.bookmarkable_type,
-          }
-        );
+        return I18n.t(bookmark.id ? "bookmarks.edit" : "bookmarks.create");
       } else if (bookmark.for_topic) {
         return I18n.t(
           bookmark.id
diff --git a/app/assets/stylesheets/common/components/bookmark-modal.scss b/app/assets/stylesheets/common/components/bookmark-modal.scss
index bb638c5b1a7..5c71250caa8 100644
--- a/app/assets/stylesheets/common/components/bookmark-modal.scss
+++ b/app/assets/stylesheets/common/components/bookmark-modal.scss
@@ -31,6 +31,10 @@
     }
   }
 
+  .title {
+    max-width: 300px;
+  }
+
   .existing-reminder-at-alert {
     display: flex;
     flex-direction: row;
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 2b11317cfd6..b9785d11b38 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -335,8 +335,8 @@ en:
 
     bookmarks:
       created: "You've bookmarked this post. %{name}"
-      create_for: "Create bookmark for %{type}"
-      edit_for: "Edit bookmark for %{type}"
+      create: "Create bookmark"
+      edit: "Edit bookmark"
       not_bookmarked: "bookmark this post"
       remove_reminder_keep_bookmark: "Remove reminder and keep bookmark"
       created_with_reminder: "You've bookmarked this post with a reminder %{date}. %{name}"
diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb
index fea26a77e85..16f5536d1ee 100644
--- a/lib/bookmark_query.rb
+++ b/lib/bookmark_query.rb
@@ -72,6 +72,11 @@ class BookmarkQuery
     queries = Bookmark.registered_bookmarkables.map do |bookmarkable|
       interim_results = bookmarkable.perform_list_query(@user, @guardian)
 
+      # this could occur if there is some security reason that the user cannot
+      # access the bookmarkables that they have bookmarked, e.g. if they had 1 bookmark
+      # on a topic and that topic was moved into a private category
+      next if interim_results.blank?
+
       if search_term.present?
         interim_results = bookmarkable.perform_search_query(
           interim_results, search_term_wildcard, ts_query
@@ -81,7 +86,12 @@ class BookmarkQuery
       # this is purely to make the query easy to read and debug, otherwise it's
       # all mashed up into a massive ball in MiniProfiler :)
       "---- #{bookmarkable.model.to_s} bookmarkable ---\n\n #{interim_results.to_sql}"
-    end
+    end.compact
+
+    # same for interim results being blank, the user might have been locked out
+    # from all their various bookmarks, in which case they will see nothing and
+    # no further pagination/ordering/etc is required
+    return [] if queries.empty?
 
     union_sql = queries.join("\n\nUNION\n\n")
     results = Bookmark.select("bookmarks.*").from("(\n\n#{union_sql}\n\n) as bookmarks")
diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb
index 636fdf8ed21..58cdc1d7933 100644
--- a/spec/lib/bookmark_query_spec.rb
+++ b/spec/lib/bookmark_query_spec.rb
@@ -73,6 +73,20 @@ RSpec.describe BookmarkQuery do
         bookmarks = bookmark_query.list_all
         expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
       end
+
+      it "handles the user not having permission for all of the bookmarks of a certain bookmarkable" do
+        UserTestBookmarkable.expects(:list_query).returns(nil)
+        bookmarks = bookmark_query.list_all
+        expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id])
+      end
+
+      it "handles the user not having permission to see any of their bookmarks" do
+        topic_bookmark.bookmarkable.update(category: Fabricate(:private_category, group: Fabricate(:group)))
+        post_bookmark.bookmarkable.topic.update(category: topic_bookmark.bookmarkable.category)
+        UserTestBookmarkable.expects(:list_query).returns(nil)
+        bookmarks = bookmark_query.list_all
+        expect(bookmarks.map(&:id)).to eq([])
+      end
     end
 
     context "when q param is provided" do