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

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

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;

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}"

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")

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