From 6c3d93f52a51fcffad75234530d510f37c4b2b66 Mon Sep 17 00:00:00 2001 From: Alice Bartlett Date: Mon, 30 Mar 2015 11:54:48 +0100 Subject: [PATCH 1/8] `count` should be a string / trailing comma `count` should be a string not an integer, though there should only ever be 1 result when searching by link anyway. Add a trailing comma for cleaner diffs(tm). --- lib/search_api.rb | 4 ++-- test/unit/search_api_test.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/search_api.rb b/lib/search_api.rb index b3e1bf41ca..f4779523fa 100644 --- a/lib/search_api.rb +++ b/lib/search_api.rb @@ -33,7 +33,7 @@ def scope_info { scope: { title: scope_object.title, - } + }, } else {} @@ -45,7 +45,7 @@ def rummager_params end def scope_object - @scope_object ||= api.unified_search(filter_link: scope_object_link, count: 1, fields: %w{title}).results.first + @scope_object ||= api.unified_search(filter_link: scope_object_link, count: "1", fields: %w{title}).results.first end def is_scoped? diff --git a/test/unit/search_api_test.rb b/test/unit/search_api_test.rb index 690f4e66ba..cfd4ff7700 100644 --- a/test/unit/search_api_test.rb +++ b/test/unit/search_api_test.rb @@ -33,7 +33,7 @@ class SearchAPITest < ActiveSupport::TestCase @search_params.expects(:filter).with('manual').returns([@manual_link]) @manual_search_response = stub(results: [stub(title: @manual_title)]) - @rummager_api.expects(:unified_search).with(filter_link: @manual_link, count: 1, fields: %w{title}).returns(@manual_search_response) + @rummager_api.expects(:unified_search).with(filter_link: @manual_link, count: "1", fields: %w{title}).returns(@manual_search_response) end should "returns search results from rummager" do From dc90a5e84269df871b1d688e7ebeb325318170d0 Mon Sep 17 00:00:00 2001 From: Alice Bartlett Date: Tue, 7 Apr 2015 10:44:04 +0100 Subject: [PATCH 2/8] Unpack metaprogramming Given this is only being used for 3 things and I just spent quite a while getting lost in the meta-programming while debugging something, I've unpacked this to non-metaprogrammed methods to save the next person who comes to this some time. --- app/presenters/search_result.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/presenters/search_result.rb b/app/presenters/search_result.rb index def82cc5fc..de8fb50f38 100644 --- a/app/presenters/search_result.rb +++ b/app/presenters/search_result.rb @@ -35,18 +35,18 @@ def self.result_accessor(*keys) end end - result_accessor :link, :title, :format, :es_score + result_accessor :link, :title, :format, :es_score, :section, :subsection, :subsubsection - # Avoid the mundanity of creating these all by hand by making - # dynamic method and accessors. - %w(section subsection subsubsection).each do |key| - define_method "formatted_#{key}_name" do - mapped_name(send(key)) || humanized_name(send(key)) - end + def formatted_section_name + mapped_name(section) || humanized_name(section) + end - define_method key do - result[key] - end + def formatted_subsection_name + mapped_name(subsection) || humanized_name(subsection) + end + + def formatted_subsubsection_name + mapped_name(subsubsection) || humanized_name(subsubsection) end # External links have a truncated version of their URLs displayed on the From f9c24d718f309ca2078bacd31b3f6ee04805464c Mon Sep 17 00:00:00 2001 From: Alice Bartlett Date: Tue, 7 Apr 2015 11:46:02 +0100 Subject: [PATCH 3/8] fix typo --- test/unit/search_api_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/search_api_test.rb b/test/unit/search_api_test.rb index cfd4ff7700..3c09667703 100644 --- a/test/unit/search_api_test.rb +++ b/test/unit/search_api_test.rb @@ -36,12 +36,12 @@ class SearchAPITest < ActiveSupport::TestCase @rummager_api.expects(:unified_search).with(filter_link: @manual_link, count: "1", fields: %w{title}).returns(@manual_search_response) end - should "returns search results from rummager" do + should "return search results from rummager" do search_response = @search_api.search(@search_params) assert_equal(@search_results, search_response.fetch(:results)) end - should "returns manual from rummager" do + should "return manual from rummager" do search_response = @search_api.search(@search_params) assert_equal({title: @manual_title}, search_response.fetch(:scope)) end From 318731bd5316a7ad4039b63defe4f596723be2dc Mon Sep 17 00:00:00 2001 From: Alice Bartlett Date: Tue, 31 Mar 2015 16:23:57 +0100 Subject: [PATCH 4/8] Extract result to separate view We're about to do a lot of fiddling around with results, to keep the code nice and clean, extract `result` to a single template. --- app/views/search/_result.mustache | 85 ++++++++++++++++++++++++ app/views/search/_results_list.mustache | 86 +------------------------ 2 files changed, 86 insertions(+), 85 deletions(-) create mode 100644 app/views/search/_result.mustache diff --git a/app/views/search/_result.mustache b/app/views/search/_result.mustache new file mode 100644 index 0000000000..84222643f9 --- /dev/null +++ b/app/views/search/_result.mustache @@ -0,0 +1,85 @@ + +

{{title}}

+ + {{#debug_score}} +

+ {{link}} +

+

+ Score: {{es_score}} + Format: {{#government}}government{{/government}} {{format}} +

+ {{/debug_score}} + + {{#external}} +

+ Part of + {{ display_link }} +

+ {{/external}} + + {{#section}} +

+ Part of + {{formatted_section_name}} + {{#formatted_subsection_name}} + , + {{ formatted_subsection_name }} + {{/formatted_subsection_name}} + {{#formatted_subsubsection_name}} + , + {{ formatted_subsubsection_name }} + {{/formatted_subsubsection_name}} +

+ {{/section}} + + {{#metadata_any?}} +
    + {{#metadata}} +
  • {{{.}}}
    • + {{/metadata}} +
+ {{/metadata_any?}} + + {{#historic?}} + {{#government_name}} +

+ First published during the {{government_name}} +

+ {{/government_name}} + {{/historic?}} + +

{{description}}

+ + {{#sections_present?}} +
    + {{#sections}} +
  • {{title}}
    • + {{/sections}} +
+ {{/sections_present?}} + + {{#examples_present?}} + + {{/examples_present?}} + + {{^examples_present?}} + {{#suggested_filter_present?}} +

{{suggested_filter_title}}

+ {{/suggested_filter_present?}} + {{/examples_present?}} + + diff --git a/app/views/search/_results_list.mustache b/app/views/search/_results_list.mustache index b34c41b1ac..1c34e0b401 100644 --- a/app/views/search/_results_list.mustache +++ b/app/views/search/_results_list.mustache @@ -12,91 +12,7 @@ {{#results_any?}}
    {{#results}} - -

    {{title}}

    - - {{#debug_score}} -

    - {{link}} -

    -

    - Score: {{es_score}} - Format: {{#government}}government{{/government}} {{format}} -

    - {{/debug_score}} - - {{#external}} -

    - Part of - {{ display_link }} -

    - {{/external}} - - {{#section}} -

    - Part of - {{formatted_section_name}} - {{#formatted_subsection_name}} - , - {{ formatted_subsection_name }} - {{/formatted_subsection_name}} - {{#formatted_subsubsection_name}} - , - {{ formatted_subsubsection_name }} - {{/formatted_subsubsection_name}} -

    - {{/section}} - - {{#metadata_any?}} -
      - {{#metadata}} -
    • {{{.}}}
      • - {{/metadata}} -
    - {{/metadata_any?}} - - {{#historic?}} - {{#government_name}} -

    - First published during the {{government_name}} -

    - {{/government_name}} - {{/historic?}} - -

    {{description}}

    - - {{#sections_present?}} -
      - {{#sections}} -
    • {{title}}
      • - {{/sections}} -
    - {{/sections_present?}} - - {{#examples_present?}} - - {{/examples_present?}} - - {{^examples_present?}} - {{#suggested_filter_present?}} -

    {{suggested_filter_title}}

    - {{/suggested_filter_present?}} - {{/examples_present?}} - - + {{>search/_result}} {{/results}}

From 962a37c73cd66d9204c1f26250c5ffa4cc4fcadd Mon Sep 17 00:00:00 2001 From: Alice Bartlett Date: Mon, 30 Mar 2015 12:07:10 +0100 Subject: [PATCH 5/8] Add request for extra results When users search within a manual (this is doing a "scoped" search) we want to show them some results from the rest of GOV.UK (ie some "un-scoped" results) so that they are aware that here are other results available that might be useful. This commit adds a third rummager request for scoped searches which returns three un-scoped results. This request is built by removing the scoping `filer_manual[]=this/manual` and adding an extra parameter `reject_manual[]=this/manual` which tells rummager we don't want any results that occur in this manual. --- lib/search_api.rb | 15 ++++++++++++--- test/unit/search_api_test.rb | 15 ++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/lib/search_api.rb b/lib/search_api.rb index f4779523fa..9ba89a0f20 100644 --- a/lib/search_api.rb +++ b/lib/search_api.rb @@ -31,9 +31,10 @@ def search_results def scope_info if is_scoped? && scope_object.present? { - scope: { - title: scope_object.title, + "scope" => { + "title" => scope_object.title, }, + "unscoped_results" => unscoped_results, } else {} @@ -53,7 +54,15 @@ def is_scoped? end def scope_object_link - params.filter('manual').first + @scope_object_link ||= params.filter('manual').first + end + + def unscoped_results + @unscoped_results ||= api.unified_search(unscoped_rummager_request).to_hash + end + + def unscoped_rummager_request + rummager_params.except(:filter_manual).merge(count: "3", reject_manual: scope_object_link) end end end diff --git a/test/unit/search_api_test.rb b/test/unit/search_api_test.rb index 3c09667703..e090f034df 100644 --- a/test/unit/search_api_test.rb +++ b/test/unit/search_api_test.rb @@ -5,7 +5,7 @@ class SearchAPITest < ActiveSupport::TestCase setup do @rummager_api = stub - @rummager_params = stub + @rummager_params = stub(except: {}) @search_params = stub(rummager_parameters: @rummager_params) @search_api = SearchAPI.new(@rummager_api) @search_results = stub @@ -24,15 +24,19 @@ class SearchAPITest < ActiveSupport::TestCase end end - context "given an search scoped to a manual" do + context "given a search scoped to a manual" do setup do @manual_link = 'manual/manual-name' @manual_title = 'Manual Title' + @govuk_result_title = "GOV.UK result" + @search_params.expects(:filtered_by?).with('manual').returns(true) @search_params.expects(:filter).with('manual').returns([@manual_link]) @manual_search_response = stub(results: [stub(title: @manual_title)]) + @unscoped_search_response = stub(to_hash: {title: @govuk_result_title}) + @rummager_api.expects(:unified_search).with(count: "3", reject_manual: @manual_link).returns(@unscoped_search_response) @rummager_api.expects(:unified_search).with(filter_link: @manual_link, count: "1", fields: %w{title}).returns(@manual_search_response) end @@ -43,7 +47,12 @@ class SearchAPITest < ActiveSupport::TestCase should "return manual from rummager" do search_response = @search_api.search(@search_params) - assert_equal({title: @manual_title}, search_response.fetch(:scope)) + assert_equal({ "title" => @manual_title }, search_response.fetch("scope")) + end + + should "return three results for the whole of gov.uk from rummager" do + search_response = @search_api.search(@search_params) + assert_equal({ title: @govuk_result_title }, search_response.fetch("unscoped_results")) end end end From 01e73e10b1dc9cf591ec3c1306c88d1c50dc926f Mon Sep 17 00:00:00 2001 From: Alice Bartlett Date: Tue, 7 Apr 2015 18:25:36 +0100 Subject: [PATCH 6/8] Extract scoped search to separate presenter Since out code for scoped searches is getting a bit more involved with the inclusion of un-scoped results, let's extract this functionality to a new presenter `scoped_search_results_presenter`. This commit just extracts the existing functionality into a new presenter, no new features are added. --- app/controllers/search_controller.rb | 8 +++- .../scoped_search_results_presenter.rb | 19 ++++++++ app/presenters/search_results_presenter.rb | 44 ++++++------------- 3 files changed, 39 insertions(+), 32 deletions(-) create mode 100644 app/presenters/scoped_search_results_presenter.rb diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index df3d6794a0..9f46cc4081 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -17,7 +17,13 @@ def index search_response = search_client.search(search_params) @search_term = search_params.search_term - @results = SearchResultsPresenter.new(search_response, search_params) + + if (search_response["scope"].present?) + @results = ScopedSearchResultsPresenter.new(search_response, search_params) + else + @results = SearchResultsPresenter.new(search_response, search_params) + end + @facets = search_response["facets"] @spelling_suggestion = @results.spelling_suggestion diff --git a/app/presenters/scoped_search_results_presenter.rb b/app/presenters/scoped_search_results_presenter.rb new file mode 100644 index 0000000000..ac5cd9aba9 --- /dev/null +++ b/app/presenters/scoped_search_results_presenter.rb @@ -0,0 +1,19 @@ +class ScopedSearchResultsPresenter < SearchResultsPresenter + + def to_hash + super.merge({ + is_scoped?: true, + scope_title: scope_title, + }) + end + +private + + def filter_fields + end + + def scope_title + search_response[:scope][:title] + end + +end diff --git a/app/presenters/search_results_presenter.rb b/app/presenters/search_results_presenter.rb index 1f48a1ba99..d9679c8fb4 100644 --- a/app/presenters/search_results_presenter.rb +++ b/app/presenters/search_results_presenter.rb @@ -17,7 +17,7 @@ def to_hash { query: search_parameters.search_term, result_count: result_count, - result_count_string: result_count_string, + result_count_string: result_count_string(result_count), results_any?: results.any?, results: results.map { |result| result.to_hash }, filter_fields: filter_fields, @@ -29,25 +29,19 @@ def to_hash previous_page_link: previous_page_link, previous_page_label: previous_page_label, first_result_number: (search_parameters.start + 1), - is_scoped?: is_scoped?, - scope_title: scope_title, } end def filter_fields - if is_scoped? - [] - else - search_response["facets"].map do |field, value| - external = SearchParameters::external_field_name(field) - facet_params = search_parameters.filter(external) - facet = SearchFacetPresenter.new(value, facet_params) - { - field: external, - field_title: FACET_TITLES.fetch(field, field), - options: facet.to_hash, - } - end + search_response["facets"].map do |field, value| + external = SearchParameters::external_field_name(field) + facet_params = search_parameters.filter(external) + facet = SearchFacetPresenter.new(value, facet_params) + { + field: external, + field_title: FACET_TITLES.fetch(field, field), + options: facet.to_hash, + } end end @@ -72,8 +66,8 @@ def result_count search_response["total"].to_i end - def result_count_string - pluralize(number_with_delimiter(result_count), "result") + def result_count_string(count) + pluralize(number_with_delimiter(count), "result") end def results @@ -81,9 +75,7 @@ def results end def build_result(result) - if is_scoped? - ScopedResult.new(search_parameters, result) - elsif result["document_type"] == "group" + if result["document_type"] == "group" GroupResult.new(search_parameters, result) elsif result["document_type"] && result["document_type"] != "edition" NonEditionResult.new(search_parameters, result) @@ -127,16 +119,6 @@ def previous_page_label end end - def is_scoped? - search_response[:scope].present? - end - - def scope_title - if is_scoped? - search_response[:scope][:title] - end - end - private attr_reader :search_parameters, :search_response From 46b581a1dc9ad29b1cd6a78b9e2905d656e39dd1 Mon Sep 17 00:00:00 2001 From: Alice Bartlett Date: Tue, 7 Apr 2015 18:47:32 +0100 Subject: [PATCH 7/8] Expose unscoped results to mustache view This commit: - adds methods to `scoped_search_results_presenter` to show unscoped results from rummager in the view. - Amends `search_results_presenter` to put the `.to_hash` call inside the `.map` on the result builder. So now whenever `results` is called the returned array is read to be sent to the view. This is because we call `results` from `scoped_search_results_presenter` now too. - Adds a template for unscoped results - adds tests for `scoped_search_results_presenter` `presentable_result_list` is the starting point for the biggest change here. I'm not sure the implementation choices here are very obvious so I'll explain them. The design (results with an embedded list of other results) is difficult to represent semantically in HTML. Ideally the extra results would be in an `