From ce39733b1aedb5348ae5f325481c01f09fe9a9f8 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 14 Jul 2020 11:05:57 +0800 Subject: [PATCH] FIX: Incorrect search blurb when advanced search filters are used take2 Also remove include_blurbs attribute which isn't used. --- .../javascripts/discourse/app/lib/search.js | 2 +- app/controllers/search_controller.rb | 29 +++++++++++++++---- lib/search.rb | 11 ++++--- lib/search/grouped_search_results.rb | 22 +++++++------- spec/components/search_spec.rb | 12 ++++---- spec/requests/search_controller_spec.rb | 25 +++++++++++++--- 6 files changed, 65 insertions(+), 36 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/search.js b/app/assets/javascripts/discourse/app/lib/search.js index d0974eba057..b26e1736a31 100644 --- a/app/assets/javascripts/discourse/app/lib/search.js +++ b/app/assets/javascripts/discourse/app/lib/search.js @@ -135,7 +135,7 @@ export function searchForTerm(term, opts) { if (!opts) opts = {}; // Only include the data we have - const data = { term: term, include_blurbs: "true" }; + const data = { term: term }; if (opts.typeFilter) data.type_filter = opts.typeFilter; if (opts.searchForId) data.search_for_id = true; if (opts.restrictToArchetype) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index d0dac30e37c..79c9b98fe80 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -35,7 +35,6 @@ class SearchController < ApplicationController search_args = { type_filter: 'topic', guardian: guardian, - include_blurbs: true, blurb_length: 300, page: if params[:page].to_i <= 10 [params[:page].to_i, 1].max @@ -53,10 +52,20 @@ class SearchController < ApplicationController search_args[:user_id] = current_user.id if current_user.present? if rate_limit_errors - result = Search::GroupedSearchResults.new(search_args[:type_filter], @search_term, context, false, 0) + result = Search::GroupedSearchResults.new( + type_filter: search_args[:type_filter], + term: @search_term, + search_context: context + ) + result.error = I18n.t("rate_limiter.slow_down") elsif site_overloaded? - result = Search::GroupedSearchResults.new(search_args[:type_filter], @search_term, context, false, 0) + result = Search::GroupedSearchResults.new( + type_filter: search_args[:type_filter], + term: @search_term, + search_context: context + ) + result.error = I18n.t("search.extreme_load_error") else search = Search.new(@search_term, search_args) @@ -90,7 +99,6 @@ class SearchController < ApplicationController search_args = { guardian: guardian } search_args[:type_filter] = params[:type_filter] if params[:type_filter].present? - search_args[:include_blurbs] = params[:include_blurbs] == "true" if params[:include_blurbs].present? search_args[:search_for_id] = true if params[:search_for_id].present? context, type = lookup_search_context @@ -106,10 +114,19 @@ class SearchController < ApplicationController search_args[:restrict_to_archetype] = params[:restrict_to_archetype] if params[:restrict_to_archetype].present? if rate_limit_errors - result = Search::GroupedSearchResults.new(search_args[:type_filter], @search_term, context, false, 0) + result = Search::GroupedSearchResults.new( + type_filter: search_args[:type_filter], + term: params[:term], + search_context: context + ) + result.error = I18n.t("rate_limiter.slow_down") elsif site_overloaded? - result = GroupedSearchResults.new(search_args["type_filter"], params[:term], context, false, 0) + result = GroupedSearchResults.new( + type_filter: search_args["type_filter"], + term: params[:term], + search_context: context + ) else search = Search.new(params[:term], search_args) result = search.execute diff --git a/lib/search.rb b/lib/search.rb index 2525591282b..564c13466a3 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -154,7 +154,6 @@ class Search @opts = opts || {} @guardian = @opts[:guardian] || Guardian.new @search_context = @opts[:search_context] - @include_blurbs = @opts[:include_blurbs] || false @blurb_length = @opts[:blurb_length] @valid = true @page = @opts[:page] @@ -186,11 +185,11 @@ class Search end @results = GroupedSearchResults.new( - @opts[:type_filter], - clean_term, - @search_context, - @include_blurbs, - @blurb_length + type_filter: @opts[:type_filter], + term: clean_term, + blurb_term: term, + search_context: @search_context, + blurb_length: @blurb_length ) end diff --git a/lib/search/grouped_search_results.rb b/lib/search/grouped_search_results.rb index f5239018877..51413dccba0 100644 --- a/lib/search/grouped_search_results.rb +++ b/lib/search/grouped_search_results.rb @@ -23,19 +23,20 @@ class Search :more_users, :term, :search_context, - :include_blurbs, :more_full_page_results, :error ) attr_accessor :search_log_id - def initialize(type_filter, term, search_context, include_blurbs, blurb_length) + BLURB_LENGTH = 200 + + def initialize(type_filter:, term:, search_context:, blurb_length: nil, blurb_term: nil) @type_filter = type_filter @term = term + @blurb_term = blurb_term || term @search_context = search_context - @include_blurbs = include_blurbs - @blurb_length = blurb_length || 200 + @blurb_length = blurb_length || BLURB_LENGTH @posts = [] @categories = [] @users = [] @@ -57,7 +58,7 @@ class Search end def blurb(post) - GroupedSearchResults.blurb_for(post.cooked, @term, @blurb_length) + GroupedSearchResults.blurb_for(post.cooked, @blurb_term, @blurb_length) end def add(object) @@ -72,7 +73,7 @@ class Search end end - def self.blurb_for(cooked, term = nil, blurb_length = 200) + def self.blurb_for(cooked, term = nil, blurb_length = BLURB_LENGTH) blurb = nil cooked = SearchIndexer.scrub_html_for_search(cooked) @@ -91,14 +92,11 @@ class Search end if term - terms = term.split(/\s+/) - phrase = terms.first - - if phrase =~ Regexp.new(Search::PHRASE_MATCH_REGEXP_PATTERN) - phrase = Regexp.last_match[1] + if term =~ Regexp.new(Search::PHRASE_MATCH_REGEXP_PATTERN) + term = Regexp.last_match[1] end - blurb = TextHelper.excerpt(cooked, phrase, + blurb = TextHelper.excerpt(cooked, term, radius: blurb_length / 2 ) end diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 8bf0df4ec27..8375b27f777 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -427,7 +427,7 @@ describe Search do context 'searching the OP' do let!(:post) { Fabricate(:post_with_long_raw_content) } - let(:result) { Search.execute('hundred', type_filter: 'topic', include_blurbs: true) } + let(:result) { Search.execute('hundred', type_filter: 'topic') } it 'returns a result correctly' do expect(result.posts.length).to eq(1) @@ -449,8 +449,7 @@ describe Search do it 'returns the post' do result = Search.execute('elephant', - type_filter: 'topic', - include_blurbs: true + type_filter: 'topic' ) expect(result.posts).to contain_exactly(reply) @@ -459,8 +458,7 @@ describe Search do it 'returns the right post and blurb for searches with phrase' do result = Search.execute('"elephant"', - type_filter: 'topic', - include_blurbs: true + type_filter: 'topic' ) expect(result.posts).to contain_exactly(reply) @@ -1492,7 +1490,7 @@ describe Search do results = Search.execute('ragis', type_filter: 'topic') expect(results.posts.length).to eq(1) - results = Search.execute('Rágis', type_filter: 'topic', include_blurbs: true) + results = Search.execute('Rágis', type_filter: 'topic') expect(results.posts.length).to eq(1) # TODO: this is a test we need to fix! @@ -1514,7 +1512,7 @@ describe Search do results = Search.execute('regis', type_filter: 'topic') expect(results.posts.length).to eq(0) - results = Search.execute('Régis', type_filter: 'topic', include_blurbs: true) + results = Search.execute('Régis', type_filter: 'topic') expect(results.posts.length).to eq(1) expect(results.blurb(results.posts.first)).to include('Régis') diff --git a/spec/requests/search_controller_spec.rb b/spec/requests/search_controller_spec.rb index 6ea66b8385f..c947528b692 100644 --- a/spec/requests/search_controller_spec.rb +++ b/spec/requests/search_controller_spec.rb @@ -53,7 +53,7 @@ describe SearchController do get "/search/query.json", headers: { "HTTP_X_REQUEST_START" => "t=#{start_time.to_f}" }, params: { - term: "hi there", include_blurb: true + term: "hi there" } expect(response.status).to eq(409) @@ -80,7 +80,7 @@ describe SearchController do term = "hello\0hello" get "/search/query.json", params: { - term: term, include_blurb: true + term: term } expect(response.status).to eq(400) @@ -88,7 +88,7 @@ describe SearchController do it "can search correctly" do get "/search/query.json", params: { - term: 'awesome', include_blurb: true + term: 'awesome' } expect(response.status).to eq(200) @@ -101,6 +101,24 @@ describe SearchController do expect(data['topics'][0]['id']).to eq(awesome_post.topic_id) end + it "can search correctly with advanced search filters" do + awesome_post.update!( + raw: "#{"a" * Search::GroupedSearchResults::BLURB_LENGTH} elephant" + ) + + get "/search/query.json", params: { term: 'order:views elephant' } + + expect(response.status).to eq(200) + + data = response.parsed_body + + expect(data.dig("grouped_search_result", "term")).to eq('order:views elephant') + expect(data['posts'].length).to eq(1) + expect(data['posts'][0]['id']).to eq(awesome_post.id) + expect(data['posts'][0]['blurb']).to include('elephant') + expect(data['topics'][0]['id']).to eq(awesome_post.topic_id) + end + it 'performs the query with a type filter' do get "/search/query.json", params: { @@ -141,7 +159,6 @@ describe SearchController do end it "should return the right result" do - get "/search/query.json", params: { term: user_post.topic_id, type_filter: 'topic',