FIX: Bookmark search fixes (#10239)

* Remove unneeded bookmark name index. * Change bookmark search query to use post_search_data. This allows searching on topic title and post content * Tweak the style/layout of the bookmark list so the search looks better and the whole page fits better on mobile.
discourse · Jul 17, 2020 · 716ccf7 · 716ccf7
1 parent ff7678e
commit 716ccf7
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 54 deletions.
diff --git a/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs b/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs
@@ -1,4 +1,4 @@
-<div class="form-horizontal" style="margin-bottom: 1em">
+<div class="form-horizontal bookmark-search-form">
   {{input type="text"
           value=searchTerm
           placeholder=(i18n "bookmarks.search_placeholder")
@@ -8,7 +8,7 @@
       class="btn-primary"
       action=(action "search")
       type="button"
-      label="bookmarks.search"}}
+      icon="search"}}
 </div>
 {{#if noContent}}
   <div class="alert alert-info">{{noResultsHelp}}</div>
@@ -17,15 +17,30 @@
     {{#load-more selector=".bookmark-list tr" action=(action "loadMore")}}
       <table class="topic-list bookmark-list">
         <thead>
-          <th>{{i18n "topic.title"}}</th>
-          <th>&nbsp;</th>
-          <th class="post-metadata">{{i18n "post.bookmarks.updated"}}</th>
-          <th class="post-metadata">{{i18n "activity"}}</th>
-          <th>&nbsp;</th>
+          {{#if site.mobileView}}
+            <th>&nbsp;</th>
+            <th>{{i18n "topic.title"}}</th>
+            <th>&nbsp;</th>
+          {{else}}
+            <th>{{i18n "topic.title"}}</th>
+            <th>&nbsp;</th>
+            <th class="post-metadata">{{i18n "post.bookmarks.updated"}}</th>
+            <th class="post-metadata">{{i18n "activity"}}</th>
+            <th>&nbsp;</th>
+          {{/if}}
         </thead>
         <tbody>
           {{#each content as |bookmark|}}
             <tr class="topic-list-item bookmark-list-item">
+              {{#if site.mobileView}}
+                <td>
+                  {{#if bookmark.post_user_avatar_template}}
+                  <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}}>
+                    {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
+                  </a>
+                  {{/if}}
+                </td>
+              {{/if}}
               <td class="main-link">
                 <span class="link-top-line">
                   <div class="bookmark-metadata">
@@ -52,15 +67,17 @@
                   {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}}
                 </div>
               </td>
-              <td>
-                {{#if bookmark.post_user_avatar_template}}
-                  <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}}>
-                    {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
-                  </a>
-                {{/if}}
-              </td>
-              <td class="post-metadata">{{format-date bookmark.updated_at format="tiny"}}</td>
-              {{raw "list/activity-column" topic=bookmark class="num post-metadata" tagName="td"}}
+              {{#unless site.mobileView}}
+                <td>
+                  {{#if bookmark.post_user_avatar_template}}
+                    <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}}>
+                      {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
+                    </a>
+                  {{/if}}
+                </td>
+                <td class="post-metadata">{{format-date bookmark.updated_at format="tiny"}}</td>
+                {{raw "list/activity-column" topic=bookmark class="num post-metadata" tagName="td"}}
+              {{/unless}}
               <td>
                 {{bookmark-actions-dropdown
                   bookmark=bookmark

diff --git a/app/assets/stylesheets/common/components/bookmark-list-item.scss b/app/assets/stylesheets/common/components/bookmark-list-item.scss
diff --git a/app/assets/stylesheets/common/components/bookmark-list.scss b/app/assets/stylesheets/common/components/bookmark-list.scss
@@ -0,0 +1,61 @@
+$mobile-breakpoint: 700px;
+
+.bookmark-list {
+  th.post-metadata {
+    text-align: center;
+  }
+}
+
+.bookmark-search-form {
+  margin-bottom: 1em;
+  display: flex;
+
+  input[type="text"] {
+    flex: 1;
+    margin-right: 1em;
+  }
+
+  @media (max-width: $mobile-breakpoint) {
+    input[type="text"] {
+      margin-bottom: 0;
+    }
+  }
+}
+
+.bookmark-list-item {
+  td.post-metadata {
+    text-align: center;
+  }
+  @media (max-width: $mobile-breakpoint) {
+    .main-link {
+      padding-left: 0.5em;
+      padding-right: 0.5em;
+    }
+  }
+  .bookmark-metadata {
+    font-size: $font-down-2;
+    display: flex;
+    margin-bottom: 0.2em;
+
+    &-item {
+      margin-right: 0.5em;
+      display: flex;
+      align-items: center;
+    }
+
+    .d-icon {
+      // not aligning center because of multi-line notes
+      align-self: flex-start;
+      margin-right: 0.2em;
+      padding-top: 0.12em;
+    }
+
+    @media (max-width: $mobile-breakpoint) {
+      flex-direction: column;
+      &-item {
+        display: block;
+        margin-bottom: 0.2em;
+      }
+    }
+  }
+}
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
@@ -320,7 +320,7 @@ 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_placeholder: "Search bookmarks by name, topic title, or post content"
       search: "Search"
       reminders:
         at_desktop: "Next time I'm at my desktop"

diff --git a/db/migrate/20200715030908_remove_unneccessary_bookmark_name_index.rb b/db/migrate/20200715030908_remove_unneccessary_bookmark_name_index.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class RemoveUnneccessaryBookmarkNameIndex < ActiveRecord::Migration[6.0]
+  def change
+    remove_index :bookmarks, :name
+  end
+end
diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb
@@ -36,7 +36,14 @@ def list_all
     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]}%")
+      term = @params[:q]
+      bookmark_ts_query = Search.ts_query(term: term)
+      results = results
+        .joins("LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.post_id")
+        .where(
+          "bookmarks.name ILIKE :q OR #{bookmark_ts_query} @@ post_search_data.search_data",
+          q: "%#{term}%"
+        )
     end
 
     if @page.positive?

diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb
@@ -3,6 +3,10 @@
 require 'rails_helper'
 
 RSpec.describe BookmarkQuery do
+  before do
+    SearchIndexer.enable
+  end
+
   fab!(:user) { Fabricate(:user) }
   let(:params) { {} }
 
@@ -11,9 +15,8 @@ def bookmark_query(user: nil, params: nil)
   end
 
   describe "#list_all" do
-    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) }
+    fab!(:bookmark1) { Fabricate(:bookmark, user: user) }
+    fab!(:bookmark2) { Fabricate(:bookmark, user: user) }
 
     it "returns all the bookmarks for a user" do
       expect(bookmark_query.list_all.count).to eq(2)
@@ -39,14 +42,25 @@ def bookmark_query(user: nil, params: nil)
     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])
+      before do
+        @post = Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs"))
+        @bookmark3 = Fabricate(:bookmark, user: user, name: "Check up later")
+        @bookmark4 = Fabricate(:bookmark, user: user, post: @post, topic: @post.topic)
       end
 
       it "can search by bookmark name" do
         bookmarks = bookmark_query(params: { q: 'check' }).list_all
-        expect(bookmarks.map(&:id)).to eq([bookmark1.id])
+        expect(bookmarks.map(&:id)).to eq([@bookmark3.id])
+      end
+
+      it "can search by post content" do
+        bookmarks = bookmark_query(params: { q: 'content' }).list_all
+        expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
+      end
+
+      it "can search by topic title" do
+        bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
+        expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
       end
     end