New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show other results from GOV.UK in a scoped search #784

Merged
merged 8 commits into from Apr 13, 2015

Conversation

Projects
None yet
8 participants
@alicebartlett
Contributor

alicebartlett commented Apr 9, 2015

Pivotal: https://www.pivotaltracker.com/story/show/

What
This PR exposes the top three search results from the whole of GOV.UK in a search that has been scoped to a single document.

Before:
screen shot 2015-04-09 at 14 06 30

After:
screen shot 2015-04-09 at 14 07 45

Summary of commits:

  • 6c3d93f, dc90a5e, f9c24d7 and 318731b (the first three commits) are refactoring done as part of the prep work for adding this feature.
  • 5b7049b modifies search_api.rb to add an the additional call to rummager for three GOV.UK wide search results. Tests for this request are also included in this commit.
  • 804af74 refactors out scoped search functionality to a new class ScopedSearchResultsPresenter and adds tests for this new presenter.
  • e4c0154 is where the bulk of the changes are. This commit takes the new search results returned by the changes to search_api.rb in 5b7049b, and formats them properly and displays them in the view.
  • 248d2e8 is CSS for the new results pull out.

How to review this PR
I've done quite a lot of hindsight based refactoring so each commit should make sense in isolation. Review however you prefer.

Who should review this PR

  • Anyone in the core team is especially encouraged to pile in.
  • I paired with @evilstreak, @jennyd, and @binaryberry on different bits of this at various times.
  • @rivalee / @markhurrell if you could check you're happy with the design and the micro-copy is what was agreed with the content team, I know it changed quite a bit and I'm not sure I got the right stuff in the end.
  • Anyone else with an interest in search/rummager/manuals

Alice Bartlett added some commits Mar 30, 2015

Alice Bartlett
`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).
Alice Bartlett
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.
Alice Bartlett
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.
@binaryberry

This comment has been minimized.

Contributor

binaryberry commented Apr 9, 2015

This PR is too advanced for me to review but 👍 on the detailed commit messages!

@alicebartlett alicebartlett force-pushed the surface-other-results branch 2 times, most recently from 73f3d42 to 0db47e9 Apr 9, 2015

@edds

This comment has been minimized.

Contributor

edds commented Apr 9, 2015

I have had a quick look and this looks great to me. Thanks for taking the time to make the commits really easy to follow.

@rboulton

This comment has been minimized.

Contributor

rboulton commented Apr 9, 2015

I'm very excited to see this, it looks shiny. .

end
# iterate remaining results
@scoped_results[3..-1].each_with_index do | result, i |

This comment has been minimized.

@benilovj

benilovj Apr 9, 2015

Contributor

this bit of the test is quite involved. I wonder whether it'd be more obvious if you checked the actual (hard-coded) values instead of variables.

This comment has been minimized.

@alicebartlett

alicebartlett Apr 9, 2015

Contributor

The hard-coded values are more like:

<[{:attributes=>[],
    :debug_score=>1,
    :description=>nil,
    :display_link=>nil,
    :es_score=>nil,
    :examples=>nil,
    :examples_present?=>false,
    :external=>false,
    :format=>nil,
    :formatted_section_name=>nil,
    :formatted_subsection_name=>nil,
    :formatted_subsubsection_name=>nil,
    :link=>nil,
    :section=>nil,
    :suggested_filter_link=>nil,
    :suggested_filter_present?=>false,
    :suggested_filter_title=>nil,
    :title=>"scoped_result_1"},
   {:attributes=>[],
    :debug_score=>1,
    :description=>nil,
    :display_link=>nil,
    :es_score=>nil,
    :examples=>nil,
    :examples_present?=>false,
    :external=>false,
    :format=>nil,
    :formatted_section_name=>nil,
    :formatted_subsection_name=>nil,
    :formatted_subsubsection_name=>nil,
    :link=>nil,
    :section=>nil,
    :suggested_filter_link=>nil,
    :suggested_filter_present?=>false,
    :suggested_filter_title=>nil,
    :title=>"scoped_result_2"},
   {:attributes=>[],
    :debug_score=>1,
    :description=>nil,
    :display_link=>nil,
    :es_score=>nil,
    :examples=>nil,
    :examples_present?=>false,
    :external=>false,
    :format=>nil,
    :formatted_section_name=>nil,
    :formatted_subsection_name=>nil,
    :formatted_subsubsection_name=>nil,
    :link=>nil,
    :section=>nil,
    :suggested_filter_link=>nil,
    :suggested_filter_present?=>false,
    :suggested_filter_title=>nil,
    :title=>"scoped_result_3"},
   {:is_multiple_results=>true,
    :results=>
     [{:attributes=>[],
       :debug_score=>1,
       :description=>nil,
       :display_link=>nil,
       :es_score=>nil,
       :examples=>nil,
       :examples_present?=>false,
       :external=>false,
       :format=>nil,
       :formatted_section_name=>nil,
       :formatted_subsection_name=>nil,
       :formatted_subsubsection_name=>nil,
       :link=>nil,
       :section=>nil,
       :suggested_filter_link=>nil,
       :suggested_filter_present?=>false,
       :suggested_filter_title=>nil,
       :title=>"unscoped_result_1"},
      {:attributes=>[],
       :debug_score=>1,
       :description=>nil,
       :display_link=>nil,
       :es_score=>nil,
       :examples=>nil,
       :examples_present?=>false,
       :external=>false,
       :format=>nil,
       :formatted_section_name=>nil,
       :formatted_subsection_name=>nil,
       :formatted_subsubsection_name=>nil,
       :link=>nil,
       :section=>nil,
       :suggested_filter_link=>nil,
       :suggested_filter_present?=>false,
       :suggested_filter_title=>nil,
       :title=>"unscoped_result_2"},
      {:attributes=>[],
       :debug_score=>1,
       :description=>nil,
       :display_link=>nil,
       :es_score=>nil,
       :examples=>nil,
       :examples_present?=>false,
       :external=>false,
       :format=>nil,
       :formatted_section_name=>nil,
       :formatted_subsection_name=>nil,
       :formatted_subsubsection_name=>nil,
       :link=>nil,
       :section=>nil,
       :suggested_filter_link=>nil,
       :suggested_filter_present?=>false,
       :suggested_filter_title=>nil,
       :title=>"unscoped_result_3"}]},
   {:attributes=>[],
    :debug_score=>1,
    :description=>nil,
    :display_link=>nil,
    :es_score=>nil,
    :examples=>nil,
    :examples_present?=>false,
    :external=>false,
    :format=>nil,
    :formatted_section_name=>nil,
    :formatted_subsection_name=>nil,
    :formatted_subsubsection_name=>nil,
    :link=>nil,
    :section=>nil,
    :suggested_filter_link=>nil,
    :suggested_filter_present?=>false,
    :suggested_filter_title=>nil,
    :title=>"scoped_result_4"}]>.

which is why I've written it this way, though I understand your point. Do you think this is clearer:

     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" },
                              ]

      results.

      # Scoped results
     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
      expected_results_list[3]["results"].each_with_index do | result, i |
        assert_equal result["title"], results[:results][3][:results][i][:title]
      end

      # iterate remaining results
      expected_results_list[3..-1].each_with_index do | result, i |
        assert_equal result["title"], results[:results][i+4][:title]
      end
    end
presentable_result_list = results
if unscoped_results.any?
insertion_point = results.count > 3 ? 3 : results.count

This comment has been minimized.

@boffbowsh

boffbowsh Apr 9, 2015

Contributor

[results.count, 3].min would work here, there's debate of course about which is more readable :)

This comment has been minimized.

@alicebartlett

alicebartlett Apr 9, 2015

Contributor

Good shout, I'll change and rebase.

@rivalee

This comment has been minimized.

rivalee commented Apr 9, 2015

Design looks good to me and the copy is correct!

@alicebartlett alicebartlett force-pushed the surface-other-results branch 2 times, most recently from 9fe22cf to 4c2d431 Apr 9, 2015

search_response["scope"]["title"]
end
def presentable_result_list

This comment has been minimized.

@boffbowsh

boffbowsh Apr 10, 2015

Contributor

I wonder if this would be better as an override of the results method?

def results
  presentable_result_list = super # possibly a better local var name than this, but I'm currently unable to brain

  if unscoped_results.any?
    insertion_point = [results.count, 3].min
    unscoped_results_sublist = { results: unscoped_results, is_multiple_results: true }
     presentable_result_list.insert(insertion_point, unscoped_results_sublist)
  end
  presentable_result_list
end

Then you wouldn't need to change results in to_hash above, and intent is clearer.

This comment has been minimized.

@alicebartlett

alicebartlett Apr 10, 2015

Contributor

Aaaaactually, I'm already overwriting results in this class to only dish out ScopedResults which I don't want the super to have to know about, so this won't work. There's probably still a clearer way though, want to come pair on it when you're free?

@@ -53,7 +54,15 @@ def is_scoped?
end
def scope_object_link
params.filter('manual').first
@link ||= params.filter('manual').first

This comment has been minimized.

@boffbowsh

boffbowsh Apr 10, 2015

Contributor

Better to have memoisation variables named the same as the method they're memoising.

This comment has been minimized.

@alicebartlett

alicebartlett Apr 10, 2015

Contributor

YES you're right will update this

assert_equal result["title"], results[:results][3][:results][i][:title]
end
# iterate check remaining result

This comment has been minimized.

@boffbowsh

boffbowsh Apr 10, 2015

Contributor

Actually a double negative, or typo?

@boffbowsh

This comment has been minimized.

Contributor

boffbowsh commented Apr 10, 2015

Pedantic style comments, but otherwise seems to do the trick 👍

Alice Bartlett added some commits Mar 30, 2015

Alice Bartlett
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.
Alice Bartlett
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.

@alicebartlett alicebartlett force-pushed the surface-other-results branch from 4c2d431 to 3c05577 Apr 10, 2015

Alice Bartlett added some commits Apr 7, 2015

Alice Bartlett
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.
Alice Bartlett

@alicebartlett alicebartlett force-pushed the surface-other-results branch from 3c05577 to a933fc9 Apr 10, 2015

@alicebartlett

This comment has been minimized.

Contributor

alicebartlett commented Apr 13, 2015

OK I think I've responded to all the comments now, thanks everyone!

@boffbowsh

This comment has been minimized.

Contributor

boffbowsh commented Apr 13, 2015

Looks good to me 👍

@evilstreak

This comment has been minimized.

Contributor

evilstreak commented Apr 13, 2015

👍

evilstreak added a commit that referenced this pull request Apr 13, 2015

Merge pull request #784 from alphagov/surface-other-results
Show other results from GOV.UK in a scoped search

@evilstreak evilstreak merged commit 1387536 into master Apr 13, 2015

1 check passed

default "Build #704 succeeded on Jenkins"
Details

@evilstreak evilstreak deleted the surface-other-results branch Apr 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment