Skip to content

Commit

Permalink
Adds support for configuring specific TIMDEX index
Browse files Browse the repository at this point in the history
Why are these changes being introduced:

* Being able to choose an index to search is required to be able to
  provide an RDI specific instance of this app

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/RDI-205

How does this address that need:

* Pulls in the latest schema from GraphQL which enables optional setting
  of the index
* Provdes `TIMDEX_INDEX` value in GraphQL query if it is set in ENV, if
  it is not set, the TIMDEX endpoint will default to whatever default
  value it is configured for
* Updates cassettes since the schema changed

Side effect:

- Analyzer#hits private method was created to allow for stubbing hits in
  tests to avoid having to use cassettes to test the Analyzer. This
  removes a fragile test.
- Added mocha gem to facilitate the stubbing noted above
- Some cassettes were shared between controller and model tests for
  Record and they now need their own cassettes because the GraphQL is slightly different (we also could have updated the model tests but
  this seemed easier and less fragile down the road to not couple unrelated tests to the same cassette)
  • Loading branch information
JPrevost committed Jul 14, 2022
1 parent 3ece046 commit 32b3822
Show file tree
Hide file tree
Showing 49 changed files with 685 additions and 431 deletions.
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ end
group :test do
gem 'capybara'
gem 'climate_control'
gem 'mocha'
gem 'selenium-webdriver'
gem 'simplecov'
gem 'simplecov-lcov'
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ GEM
mitlibraries-theme (0.7.0)
rails (>= 5, < 8)
sassc (~> 2)
mocha (1.14.0)
msgpack (1.5.2)
net-imap (0.2.3)
digest
Expand Down Expand Up @@ -317,6 +318,7 @@ DEPENDENCIES
importmap-rails
jbuilder
mitlibraries-theme (~> 0.7.0)
mocha
pg
puma (~> 5.0)
rails (~> 7.0.2, >= 7.0.2.3)
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ change as part of the work.

- `GLOBAL_ALERT`: The main functionality for this comes from our theme gem, but when set the value will be rendered as
safe html above the main header of the site.
- `TIMDEX_INDEX`: Name of the index, or alias, to provide to the GraphQL endpoint. Defaults to `nil` which will let TIMDEX determine the best index to use. Wildcard values can be set, for example `rdi*` would search any indexes that begin with `rdi` in the underlying OpenSearch instance behind TIMDEX.
- `TIMDEX_SOURCES`: Comma-separated list of sources to display in the advanced-search source selection element. This
overrides the default which is set in ApplicationHelper.

Expand Down
3 changes: 2 additions & 1 deletion app/controllers/record_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ class RecordController < ApplicationController

def view
id = params[:id]
index = ENV.fetch('TIMDEX_INDEX', nil)

response = TimdexBase::Client.query(TimdexRecord::Query, variables: { id: })
response = TimdexBase::Client.query(TimdexRecord::Query, variables: { id:, index: })

# Detection of unexpected response from the API would go here...

Expand Down
6 changes: 5 additions & 1 deletion app/models/analyzer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ class Analyzer

def initialize(enhanced_query, response)
@pagination = {}
@pagination[:hits] = response&.data&.search&.to_h&.dig('hits')
@pagination[:hits] = hits(response)
@pagination[:page] = enhanced_query[:page]
@pagination[:prev] = enhanced_query[:page] - 1 if enhanced_query[:page] > 1
@pagination[:next] = next_page(enhanced_query[:page], @pagination[:hits])
end

private

def hits(response)
response&.data&.search&.to_h&.dig('hits')
end

def next_page(page, hits)
page + 1 if page * RESULTS_PER_PAGE < hits
end
Expand Down
1 change: 1 addition & 0 deletions app/models/query_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ def initialize(enhanced_query)
@query['from'] = calculate_from(enhanced_query[:page])
extract_query(enhanced_query)
extract_filters(enhanced_query)
@query['index'] = ENV.fetch('TIMDEX_INDEX', nil)
@query.compact!
end

Expand Down
4 changes: 2 additions & 2 deletions app/models/timdex_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

class TimdexRecord < TimdexBase
Query = TimdexBase::Client.parse <<-'GRAPHQL'
query($id: String!) {
recordId(id: $id) {
query($id: String!, $index: String) {
recordId(id: $id, index: $index) {
alternateTitles {
kind
value
Expand Down
2 changes: 2 additions & 0 deletions app/models/timdex_search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class TimdexSearch < TimdexBase
$subjects: String
$title: String
$sourceFacet: [String!]
$index: String
$from: String
$contentType: String
) {
Expand All @@ -26,6 +27,7 @@ class TimdexSearch < TimdexBase
subjects: $subjects
title: $title
sourceFacet: $sourceFacet
index: $index
from: $from
contentTypeFacet: $contentType
) {
Expand Down
20 changes: 20 additions & 0 deletions config/schema/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,16 @@
}
},
"defaultValue": null
},
{
"name": "index",
"description": "It is not recommended to provide an index value unless we have provided you with one for your specific use case",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": "null"
}
],
"type": {
Expand Down Expand Up @@ -1195,6 +1205,16 @@
},
"defaultValue": "\"0\""
},
{
"name": "index",
"description": "It is not recommended to provide an index value unless we have provided you with one for your specific use case",
"type": {
"kind": "SCALAR",
"name": "String",
"ofType": null
},
"defaultValue": "null"
},
{
"name": "collectionFacet",
"description": null,
Expand Down
6 changes: 3 additions & 3 deletions test/controllers/record_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class RecordControllerTest < ActionDispatch::IntegrationTest
end

test 'full record path with an id does not return an error' do
VCR.use_cassette('timdex record sample',
VCR.use_cassette('timdex controller record sample',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
get '/record/jpal:doi:10.7910-DVN-MNIBOL'
Expand All @@ -18,7 +18,7 @@ class RecordControllerTest < ActionDispatch::IntegrationTest

test 'record ids can include multiple periods' do
needle_id = 'there.is.no.record'
VCR.use_cassette('timdex record no record',
VCR.use_cassette('timdex controller record no record',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
get "/record/#{needle_id}"
Expand All @@ -28,7 +28,7 @@ class RecordControllerTest < ActionDispatch::IntegrationTest

test 'full record display where no record exists displays an error' do
needle_id = 'there.is.no.record'
VCR.use_cassette('timdex record no record',
VCR.use_cassette('timdex controller record no record',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
get "/record/#{needle_id}"
Expand Down
88 changes: 41 additions & 47 deletions test/models/analyzer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,63 +2,57 @@

class AnalyzerTest < ActiveSupport::TestCase
test 'analyzer pagination does not include previous page value on first page of results' do
VCR.use_cassette('data',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
eq = {
q: 'data',
page: 1
}
query = { 'q' => 'data', 'from' => '0' }
response = TimdexBase::Client.query(TimdexSearch::Query, variables: query)
pagination = Analyzer.new(eq, response).pagination
Analyzer.any_instance.stubs(:hits).returns(100)
mocking_hits_so_this_is_empty = {}

assert pagination.key?(:hits)
assert pagination.key?(:next)
assert pagination.key?(:page)
eq = {
q: 'data',
page: 1
}
query = { 'q' => 'data', 'from' => '0' }

refute pagination.key?(:prev)
end
pagination = Analyzer.new(eq, mocking_hits_so_this_is_empty).pagination

assert pagination.key?(:hits)
assert pagination.key?(:next)
assert pagination.key?(:page)

refute pagination.key?(:prev)
end

test 'analyzer pagination includes all values when not on first or last page of results' do
VCR.use_cassette('data page 2',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
eq = {
q: 'data',
page: 2
}
query = { 'q' => 'data', 'from' => '20' }
response = TimdexBase::Client.query(TimdexSearch::Query, variables: query)
pagination = Analyzer.new(eq, response).pagination
Analyzer.any_instance.stubs(:hits).returns(100)
mocking_hits_so_this_is_empty = {}

eq = {
q: 'data',
page: 2
}
query = { 'q' => 'data', 'from' => '20' }

assert pagination.key?(:hits)
assert pagination.key?(:next)
assert pagination.key?(:prev)
assert pagination.key?(:page)
end
pagination = Analyzer.new(eq, mocking_hits_so_this_is_empty).pagination

assert pagination.key?(:hits)
assert pagination.key?(:next)
assert pagination.key?(:prev)
assert pagination.key?(:page)
end

# When regenerating cassettes, the page param of the eq hash in this test must be updated to match the current last
# page returned by the API.
test 'analyzer pagination does not include last page value on last page of results' do
VCR.use_cassette('data last page',
allow_playback_repeats: true,
match_requests_on: %i[method uri body]) do
eq = {
q: 'data',
page: 28
}
query = { 'q' => 'data', 'from' => '320' }
response = TimdexBase::Client.query(TimdexSearch::Query, variables: query)
pagination = Analyzer.new(eq, response).pagination
Analyzer.any_instance.stubs(:hits).returns(100)

mocking_hits_so_this_is_empty = {}
eq = {
q: 'data',
page: 28
}

pagination = Analyzer.new(eq, mocking_hits_so_this_is_empty).pagination

assert pagination.key?(:hits)
assert pagination.key?(:prev)
assert pagination.key?(:page)
assert pagination.key?(:hits)
assert pagination.key?(:prev)
assert pagination.key?(:page)

assert_nil pagination[:next]
end
assert_nil pagination[:next]
end
end
12 changes: 12 additions & 0 deletions test/models/query_builder_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,16 @@ class QueryBuilderTest < ActiveSupport::TestCase
}
assert_equal(expected, QueryBuilder.new(search).query)
end

test 'query builder can read TIMDEX_INDEX from env' do
ClimateControl.modify TIMDEX_INDEX: 'rdi*' do
search = { q: 'blah' }
assert_equal('rdi*', QueryBuilder.new(search).query['index'])
end
end

test 'query builder index is nil if TIMDEX_INDEX not provided in env' do
search = { q: 'blah' }
assert_nil(QueryBuilder.new(search).query['index'])
end
end
1 change: 1 addition & 0 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
SimpleCov.start('rails')
require_relative '../config/environment'
require 'rails/test_help'
require 'mocha/minitest'

VCR.configure do |config|
config.ignore_localhost = true
Expand Down
Loading

0 comments on commit 32b3822

Please sign in to comment.