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.
2024-11-25 09:42:07 +08:00 · 2020-07-17 15:55:07 +10:00 · 2020-07-17 15:55:07 +10:00 · 716ccf7fe4
commit 716ccf7fe4
parent ff7678e210
7 changed files with 131 additions and 54 deletions
--- 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
--- a/app/assets/stylesheets/common/components/bookmark-list-item.scss
+++ b/app/assets/stylesheets/common/components/bookmark-list-item.scss
@ -1,29 +0,0 @@
-.bookmark-list {
-  th.post-metadata {
-    text-align: center;
-  }
-}
-
-.bookmark-list-item {
-  td.post-metadata {
-    text-align: center;
-  }
-  .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;
-    }
-  }
-}
--- 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;
+      }
+    }
+  }
+}
--- 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"
--- 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
--- a/lib/bookmark_query.rb
+++ b/lib/bookmark_query.rb
@ -36,7 +36,14 @@ 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]}%")
+      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?
--- 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 @@ RSpec.describe BookmarkQuery do
  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 @@ RSpec.describe BookmarkQuery do
    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