Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
PrevPrevious commit
NextNext commit
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 `<aside>` tag outside of the result list, but the design has them interrupting the list so that sighted users will notice them. A compromise that I'm happy with is to have them nested in the list of results like so: ``` <li>result 1</li> <li>result 2</li> <li>result 3</li> <li>More results from GOV.UK <ol> <li>..</li> <li>..</li> <li>..</li> </ol> </li> <li>result 4</li> etc ``` Given this markup, and the constraint of using a logic-less templating language (mustache) the best way I could thing to achieve this HTML was to pass mustache an object that contains this structure. The responsibility for mashing up the scoped results and unscoped results falls to `presentable_result_list`. This software pattern is a bit uncomfortable because it pushes the design of the interface ("the unscoped results should be nested") onto the presenter, though it should be the view's responsibility. However it is the only way I can think to write it.
  • Loading branch information
Alice Bartlett committed Apr 10, 2015
commit 46b581a1dc9ad29b1cd6a78b9e2905d656e39dd1
36 changes: 35 additions & 1 deletion app/presenters/scoped_search_results_presenter.rb
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,16 +4,50 @@ def to_hash
super.merge({
is_scoped?: true,
scope_title: scope_title,
unscoped_results_any?: unscoped_results.any?,
unscoped_result_count: result_count_string(unscoped_result_count),
})
end

private

attr_reader :unscoped_results

def filter_fields
[]
end

def scope_title
search_response[:scope][:title]
search_response["scope"]["title"]
end

def results
result_list = search_response["results"].map{|result| build_scoped_result(result).to_hash }

if unscoped_results.any?
insertion_point = [result_list.count, 3].min
unscoped_results_sublist ={results: unscoped_results, is_multiple_results: true }

result_list.insert(insertion_point, unscoped_results_sublist)
end
result_list
end

def unscoped_result_count
search_response["unscoped_results"]["total"]
end

def unscoped_results
@unscoped_results ||= build_result_presenters

end

def build_result_presenters
search_response["unscoped_results"]["results"].map{|result| build_result(result).to_hash }
end

def build_scoped_result(result)
ScopedResult.new(search_parameters, result)
end

end
4 changes: 2 additions & 2 deletions app/presenters/search_results_presenter.rb
Original file line numberDiff line numberDiff line change
Expand Up@@ -19,7 +19,7 @@ def to_hash
result_count: result_count,
result_count_string: result_count_string(result_count),
results_any?: results.any?,
results: results.map{|result| result.to_hash },
results: results,
filter_fields: filter_fields,
debug_score: search_parameters.debug_score,
has_next_page?: has_next_page?,
Expand DownExpand Up@@ -71,7 +71,7 @@ def result_count_string(count)
end

def results
search_response["results"].map{|result| build_result(result) }
search_response["results"].map{|result| build_result(result).to_hash }
end

def build_result(result)
Expand Down
11 changes: 9 additions & 2 deletions app/views/search/_results_list.mustache
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,7 +2,9 @@
{{#is_scoped?}}
<strong>{{result_count_string }} found in</strong><br>
{{scope_title}}<br>
<a href='/search?q={{query}}'>Display results from all of GOV.UK</a>
{{#unscoped_results_any?}}
<a href='/search?q={{query}}'>Display{{unscoped_result_count}} from all of GOV.UK</a>
{{/unscoped_results_any?}}
{{/is_scoped?}}
{{^is_scoped?}}
{{result_count_string }} found
Expand All@@ -12,7 +14,12 @@
{{#results_any?}}
<ol class="results-list{{#debug_score}} debug{{/debug_score}}" id="js-live-search-results" start="{{first_result_number}}">
{{#results}}
{{>search/_result}}
{{#is_multiple_results}}
{{>search/_sublist}}
{{/is_multiple_results}}
{{^is_multiple_results}}
{{>search/_result}}
{{/is_multiple_results}}
{{/results}}
</ol>

Expand Down
11 changes: 11 additions & 0 deletions app/views/search/_sublist.mustache
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
<li class='descoped-results'>
<div class='descope-message'>
<a href='/search?q={{query}}'>Display{{unscoped_result_count}} from all of GOV.UK</a>
</div>
<ol>
{{#results}}
{{>search/_result}}
{{/results}}
</ol>
</li>

115 changes: 115 additions & 0 deletions test/unit/presenters/scoped_search_results_presenter_test.rb
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
require_relative "../../test_helper"

class ScopedSearchResultsPresenterTest < ActiveSupport::TestCase
setup do
@scope_title = stub
@unscoped_result_count = stub

@scoped_results = [{"title"=> "scoped_result_1"},
{"title"=> "scoped_result_2"},
{"title"=> "scoped_result_3"},
{"title"=> "scoped_result_4"},
]
@unscoped_results = [{"title"=> "unscoped_result_1"},
{"title"=> "unscoped_result_2"},
{"title"=> "unscoped_result_3"},
]

@search_response ={"result_count" => "50",
"results" => @scoped_results,
"scope" =>{
"title" => @scope_title,
},
"unscoped_results" =>{
"total" => @unscoped_result_count,
"results" => @unscoped_results,
},
}


@search_parameters = stub(search_term: 'words',
debug_score: 1,
start: 1,
count: 1,
build_link: 1,
)
end

should "return a hash that has is_scoped set to true" do
results = ScopedSearchResultsPresenter.new(@search_response, @search_parameters)
assert_equal true, results.to_hash[:is_scoped?]
end

should "return a hash with the scope_title set to the scope title from the @search_response" do
results = ScopedSearchResultsPresenter.new(@search_response, @search_parameters)
assert_equal @scope_title, results.to_hash[:scope_title]
end

should "return a hash result count set to the scope title from the @search_response" do
results = ScopedSearchResultsPresenter.new(@search_response, @search_parameters)
assert_equal "#{@unscoped_result_count} results", results.to_hash[:unscoped_result_count]
end

context "presentable result list" do

should "return all scoped results with unscoped results inserted at position 4" do
results = ScopedSearchResultsPresenter.new(@search_response, @search_parameters).to_hash

##
# This test is asserting that the format of `presentable_list` is:
# [result, result, result,{results: list_of_results, is_multiple_results: true}, result ...]
# Where list_of_results are the top three results from an unscoped request to rummager
# and a flag `is_multiple_results` set to true.
##

simplified_expected_results_list = [{"title"=> "scoped_result_1"},
{"title"=> "scoped_result_2"},
{"title"=> "scoped_result_3"},
{"is_multiple_results" => true,
"results" => [{"title"=> "unscoped_result_1"},
{"title"=> "unscoped_result_2"},
{"title"=> "unscoped_result_3"},
]
},
{"title"=> "scoped_result_4"},
]


# Scoped results
simplified_expected_results_list[0..2].each_with_index do | result, i |
assert_equal result["title"], results[:results][i][:title]
end

# Check un-scoped sub-list has flag
assert_equal true, results[:results][3][:is_multiple_results]

# iterate unscoped sublist of results
simplified_expected_results_list[3]["results"].each_with_index do | result, i |
assert_equal result["title"], results[:results][3][:results][i][:title]
end

# check remaining result
assert_equal simplified_expected_results_list[4]["title"], results[:results][4][:title]
end
end

context "no scoped results returned" do
setup do
@no_results = []
@search_response["unscoped_results"]["results"] = @no_results
end

should "not not include unscoped results in the presentable_list if there aren't any" do
results = ScopedSearchResultsPresenter.new(@search_response, @search_parameters).to_hash

@scoped_results.each_with_index do | result, i |
assert_equal result["title"], results[:results][i][:title]
end
end

should "not set unscoped_results_any? to false" do
results = ScopedSearchResultsPresenter.new(@search_response, @search_parameters).to_hash
assert_equal false, results.to_hash[:unscoped_results_any?]
end
end
end