From 716ccf7fe4b156670c3610fc75ccf7a08098ff91 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 17 Jul 2020 15:55:07 +1000 Subject: [PATCH] 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. --- .../app/templates/user/bookmarks.hbs | 49 ++++++++++----- .../common/components/bookmark-list-item.scss | 29 --------- .../common/components/bookmark-list.scss | 61 +++++++++++++++++++ config/locales/client.en.yml | 2 +- ...remove_unneccessary_bookmark_name_index.rb | 7 +++ lib/bookmark_query.rb | 9 ++- spec/lib/bookmark_query_spec.rb | 28 ++++++--- 7 files changed, 131 insertions(+), 54 deletions(-) delete mode 100644 app/assets/stylesheets/common/components/bookmark-list-item.scss create mode 100644 app/assets/stylesheets/common/components/bookmark-list.scss create mode 100644 db/migrate/20200715030908_remove_unneccessary_bookmark_name_index.rb diff --git a/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs b/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs index b6a169bd387..c4a0425e8b0 100644 --- a/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs +++ b/app/assets/javascripts/discourse/app/templates/user/bookmarks.hbs @@ -1,4 +1,4 @@ -
+
{{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"}}
{{#if noContent}}
{{noResultsHelp}}
@@ -17,15 +17,30 @@ {{#load-more selector=".bookmark-list tr" action=(action "loadMore")}} - - - - - + {{#if site.mobileView}} + + + + {{else}} + + + + + + {{/if}} {{#each content as |bookmark|}} + {{#if site.mobileView}} + + {{/if}} - - - {{raw "list/activity-column" topic=bookmark class="num post-metadata" tagName="td"}} + {{#unless site.mobileView}} + + + {{raw "list/activity-column" topic=bookmark class="num post-metadata" tagName="td"}} + {{/unless}}
{{i18n "topic.title"}} {{i18n "post.bookmarks.updated"}}{{i18n "activity"}}  {{i18n "topic.title"}} {{i18n "topic.title"}} {{i18n "post.bookmarks.updated"}}{{i18n "activity"}} 
+ {{#if bookmark.post_user_avatar_template}} + + {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} + + {{/if}} +
@@ -52,15 +67,17 @@ {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}}
- {{#if bookmark.post_user_avatar_template}} - - {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} - - {{/if}} - {{format-date bookmark.updated_at format="tiny"}} + {{#if bookmark.post_user_avatar_template}} + + {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} + + {{/if}} + {{format-date bookmark.updated_at format="tiny"}} {{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 deleted file mode 100644 index 4710f9b25f7..00000000000 --- a/app/assets/stylesheets/common/components/bookmark-list-item.scss +++ /dev/null @@ -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; - } - } -} diff --git a/app/assets/stylesheets/common/components/bookmark-list.scss b/app/assets/stylesheets/common/components/bookmark-list.scss new file mode 100644 index 00000000000..5d7d6c51c81 --- /dev/null +++ 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 index 66bb5492f45..d4eee0c8c74 100644 --- 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 new file mode 100644 index 00000000000..460b4483774 --- /dev/null +++ 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 index 2f9daf52952..f2122f7c6c7 100644 --- 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? diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index c1f932b804d..4f11254ad8d 100644 --- 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