FEATURE: Add search to user bookmark list (#10230)

FEATURE: Add search to user bookmark list (#10230)

User bookmarks can now be searched by name or post raw content. The q querystring param is hooked up from the Ember router as well.

diff --git a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js b/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
index 48cd32a..9a87faf 100644
--- a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
+++ b/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
@@ -14,6 +14,10 @@ export default Controller.extend({
   content: null,
   loading: false,
   noResultsHelp: null,
+  searchTerm: null,
+  q: null,
+
+  queryParams: ["q"],
 
   loadItems() {
     this.setProperties({
@@ -22,8 +26,12 @@ export default Controller.extend({
       noResultsHelp: null
     });
 
+    if (this.q && !this.searchTerm) {
+      this.set("searchTerm", this.q);
+    }
+
     return this.model
-      .loadItems()
+      .loadItems({ q: this.searchTerm })
       .then(response => this._processLoadResponse(response))
       .catch(() => this._bookmarksListDenied())
       .finally(() =>
@@ -44,6 +52,12 @@ export default Controller.extend({
   },
 
   @action
+  search() {
+    this.set("q", this.searchTerm);
+    this.loadItems();
+  },
+
+  @action
   removeBookmark(bookmark) {
     const deleteBookmark = () => {
       return bookmark
@@ -86,7 +100,7 @@ export default Controller.extend({
     this.set("loadingMore", true);
 
     return this.model
-      .loadMore()
+      .loadMore({ q: this.searchTerm })
       .then(response => this._processLoadResponse(response))
       .catch(() => this._bookmarksListDenied())
       .finally(() => this.set("loadingMore", false));
diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js
index c7a3249..ba5a347 100644
--- a/app/assets/javascripts/discourse/app/models/bookmark.js
+++ b/app/assets/javascripts/discourse/app/models/bookmark.js
@@ -120,11 +120,17 @@ const Bookmark = RestModel.extend({
     ).capitalize();
   },
 
-  loadItems() {
-    return ajax(`/u/${this.user.username}/bookmarks.json`, { cache: "false" });
+  loadItems(params) {
+    let url = `/u/${this.user.username}/bookmarks.json`;
+
+    if (params) {
+      url += "?" + $.param(params);
+    }
+
+    return ajax(url, { cache: "false" });
   },
 
-  loadMore() {
+  loadMore(additionalParams) {
     if (!this.more_bookmarks_url) {
       return Promise.resolve();
     }
@@ -136,7 +142,15 @@ const Bookmark = RestModel.extend({
       if (params) {
         moreUrl += "?" + params;
       }
+      if (additionalParams) {
+        if (moreUrl.includes("?")) {
+          moreUrl += "&" + $.param(additionalParams);
+        } else {
+          moreUrl += "?" + $.param(additionalParams);
+        }
+      }
     }
+
     return ajax({ url: moreUrl });
   },
 
diff --git a/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks-with-reminders.js b/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks-with-reminders.js
index 7c5cf81..a4883b7 100644
--- a/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks-with-reminders.js
+++ b/app/assets/javascripts/discourse/app/routes/user-activity-bookmarks-with-reminders.js
@@ -1,6 +1,10 @@
 import DiscourseRoute from "discourse/routes/discourse";
 
 export default DiscourseRoute.extend({
+  queryParams: {
+    q: { replace: true }
+  },
+
   redirect() {
     this.transitionTo("userActivity.bookmarks");
   }
diff --git a/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs b/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs
index 33a43e6..b6a169b 100644
--- a/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs
+++ b/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs
@@ -1,3 +1,15 @@
+<div class="form-horizontal" style="margin-bottom: 1em">
+  {{input type="text"
+          value=searchTerm
+          placeholder=(i18n "bookmarks.search_placeholder")
+          enter=(action "search")
+          id="bookmark-search" autocomplete="discourse"}}
+  {{d-button
+      class="btn-primary"
+      action=(action "search")
+      type="button"
+      label="bookmarks.search"}}
+</div>
 {{#if noContent}}
   <div class="alert alert-info">{{noResultsHelp}}</div>
 {{else}}
diff --git a/app/serializers/user_bookmark_list_serializer.rb b/app/serializers/user_bookmark_list_serializer.rb
index 4e82e44..a7a4ed0 100644
--- a/app/serializers/user_bookmark_list_serializer.rb
+++ b/app/serializers/user_bookmark_list_serializer.rb
@@ -6,6 +6,6 @@ class UserBookmarkListSerializer < ApplicationSerializer
   has_many :bookmarks, serializer: UserBookmarkSerializer, embed: :objects
 
   def include_more_bookmarks_url?
-    object.bookmarks.size == object.per_page
+    @include_more_bookmarks_url ||= object.bookmarks.size == object.per_page
   end
 end
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index da71b0f..e20a701 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -320,6 +320,8 @@ en:
       invalid_custom_datetime: "The date and time you provided is invalid, please try again."
       list_permission_denied: "You do not have permission to view this user's bookmarks."
       delete_when_reminder_sent: "Delete this bookmark when the reminder notification is sent."
+      search_placeholder: "Search bookmarks by name or post content"
+      search: "Search"
       reminders:
         at_desktop: "Next time I'm at my desktop"
         later_today: "Later today"
diff --git a/db/migrate/20200713071305_add_bookmark_name_index.rb b/db/migrate/20200713071305_add_bookmark_name_index.rb
new file mode 100644
index 0000000..1f7107b
--- /dev/null
+++ b/db/migrate/20200713071305_add_bookmark_name_index.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddBookmarkNameIndex < ActiveRecord::Migration[6.0]
+  def change
+    add_index :bookmarks, :name
+  end
+end
diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb
index b5351f1..2f9daf5 100644
--- a/lib/bookmark_query.rb
+++ b/lib/bookmark_query.rb
@@ -35,6 +35,10 @@ class BookmarkQuery
 
     results = results.merge(Post.secured(@guardian))
 
+    if @params[:q].present?
+      results = results.where("bookmarks.name ILIKE :q OR posts.raw ILIKE :q", q: "%#{@params[:q]}%")
+    end
+
     if @page.positive?
       results = results.offset(@page * @params[:per_page])
     end
diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb
index de71757..c1f932b 100644
--- a/spec/lib/bookmark_query_spec.rb
+++ b/spec/lib/bookmark_query_spec.rb
@@ -11,8 +11,9 @@ RSpec.describe BookmarkQuery do
   end
 
   describe "#list_all" do
-    fab!(:bookmark1) { Fabricate(:bookmark, user: user) }
-    fab!(:bookmark2) { Fabricate(:bookmark, user: user) }
+    fab!(:post) { Fabricate(:post, raw: "Some post content here") }
+    fab!(:bookmark1) { Fabricate(:bookmark, user: user, name: "Check up later") }
+    fab!(:bookmark2) { Fabricate(:bookmark, user: user, post: post, topic: post.topic) }
 
     it "returns all the bookmarks for a user" do
       expect(bookmark_query.list_all.count).to eq(2)
@@ -37,6 +38,18 @@ RSpec.describe BookmarkQuery do
       expect(preloaded_bookmarks.any?).to eq(true)
     end
 
+    context "when q param is provided" do
+      it "can search by post content" do
+        bookmarks = bookmark_query(params: { q: 'content' }).list_all
+        expect(bookmarks.map(&:id)).to eq([bookmark2.id])
+      end
+
+      it "can search by bookmark name" do
+        bookmarks = bookmark_query(params: { q: 'check' }).list_all
+        expect(bookmarks.map(&:id)).to eq([bookmark1.id])
+      end
+    end
+
     context "for a whispered post" do
       before do
         bookmark1.post.update(post_type: Post.types[:whisper])

GitHub sha: bcc80e0e

This commit appears in #10230 which was merged by martin.

Can you run a EXPLAIN ANALYZE on the query to see if this index is being used because I don’t think a btree index will help here. It seems like we’ll have to use a GIN index? https://dba.stackexchange.com/a/21648

1 Like

Removed index in FIX: Bookmark search fixes (#10239) · discourse/discourse@716ccf7 · GitHub