From 6b3056b5fc82cfa65f9b05027e607fce38503c46 Mon Sep 17 00:00:00 2001 From: jazairi <16103405+jazairi@users.noreply.github.com> Date: Tue, 21 Oct 2025 17:22:36 -0400 Subject: [PATCH] Isolate search forms for visual design changes Why these changes are being introduced: GeoData and USE UI currently share search form input and submit fields. These fields should be isolated for UXWS to easily make tweaks to the form for USE UI that won't affect GeoData. Relevant ticket(s): * [USE-127](https://mitlibraries.atlassian.net/browse/USE-127) How this addresses that need: This creates separate form partials for USE and GeoData to faciliate changes to the USE form without affecting GeoData. It also moves the USE search form to the header, which is where UXWS' designs have it. Side effects of this change: It's possible we may want to move the GeoData form to the header as well, but given the tight timeline for USE UI beta, it makes sense to focus on the USE side of things for now. --- app/assets/stylesheets/partials/_search.scss | 4 +- app/views/basic_search/index.html.erb | 2 +- app/views/layouts/_site_header.html.erb | 1 + app/views/search/_form.html.erb | 186 +--------------- app/views/search/_form_geo.html.erb | 205 ++++++++++++++++++ app/views/search/results.html.erb | 1 - app/views/search/results_geo.html.erb | 2 +- .../controllers/search_controller_geo_test.rb | 172 +++++++++------ test/controllers/search_controller_test.rb | 28 ++- 9 files changed, 346 insertions(+), 255 deletions(-) create mode 100644 app/views/search/_form_geo.html.erb diff --git a/app/assets/stylesheets/partials/_search.scss b/app/assets/stylesheets/partials/_search.scss index 0cb9ed3e..daeca2a8 100644 --- a/app/assets/stylesheets/partials/_search.scss +++ b/app/assets/stylesheets/partials/_search.scss @@ -3,7 +3,7 @@ // ------------------------ /* basic search bar */ -.basic-search { +.search-form { background-color: #989898; margin-bottom: 0rem; padding: 2.4rem 2rem 1.6rem 2rem; @@ -114,7 +114,7 @@ } } - .basic-search-submit { + .geo-search-submit { width: 150px; margin-top: 0.8rem; .btn { diff --git a/app/views/basic_search/index.html.erb b/app/views/basic_search/index.html.erb index 1de2af85..7b2c7996 100644 --- a/app/views/basic_search/index.html.erb +++ b/app/views/basic_search/index.html.erb @@ -2,6 +2,6 @@
<%= render partial: "shared/site_title" %> - <%= render partial: "search/form" %> + <%= render partial: "search/form_geo" if Flipflop.enabled?(:gdt) %> <%= render partial: "static/about" if ENV.fetch('ABOUT_APP', nil) %>
diff --git a/app/views/layouts/_site_header.html.erb b/app/views/layouts/_site_header.html.erb index 4792c7c1..a8651f43 100644 --- a/app/views/layouts/_site_header.html.erb +++ b/app/views/layouts/_site_header.html.erb @@ -29,5 +29,6 @@ <% end %> + <%= render partial: 'search/form' unless Flipflop.enabled?(:gdt) %> \ No newline at end of file diff --git a/app/views/search/_form.html.erb b/app/views/search/_form.html.erb index 81f02734..deacab7f 100644 --- a/app/views/search/_form.html.erb +++ b/app/views/search/_form.html.erb @@ -1,185 +1,13 @@ -<% -# Initial form setup is determined by the advanced parameter. Thereafter it is altered by javascript. -advanced_label = "Search by title, author, etc." -search_required = true -if params[:advanced] == "true" - search_required = false -end - -geobox_label = "Geospatial bounding box search" -if params[:geobox] == "true" - geobox_required = true - search_required = false -end - -geodistance_label = "Geospatial distance search" -if params[:geodistance] == "true" - geodistance_required = true - search_required = false -end - -# Placeholder text for the keyword input changes if any of the search panels are open. -keyword_placeholder = search_required ? "Enter your search" : "Keyword anywhere" -%> - - diff --git a/app/views/search/_form_geo.html.erb b/app/views/search/_form_geo.html.erb new file mode 100644 index 00000000..9ac0a6a0 --- /dev/null +++ b/app/views/search/_form_geo.html.erb @@ -0,0 +1,205 @@ +<% +# Initial form setup is determined by the advanced parameter. Thereafter it is altered by javascript. +advanced_label = "Search by title, author, etc." +search_required = true +if params[:advanced] == "true" + search_required = false +end + +geobox_label = "Geospatial bounding box search" +if params[:geobox] == "true" + geobox_required = true + search_required = false +end + +geodistance_label = "Geospatial distance search" +if params[:geodistance] == "true" + geodistance_required = true + search_required = false +end + +# Placeholder text for the keyword input changes if any of the search panels are open. +keyword_placeholder = search_required ? "Enter your search" : "Keyword anywhere" +%> + + + +<% if Flipflop.enabled?(:boolean_picker) %> + +<% end %> + +<%= javascript_include_tag "search_form" %> diff --git a/app/views/search/results.html.erb b/app/views/search/results.html.erb index 09fbdcbc..e4b9253b 100644 --- a/app/views/search/results.html.erb +++ b/app/views/search/results.html.erb @@ -8,7 +8,6 @@ <%= render partial: "shared/site_title" %> - <%= render partial: "form" %> <%= render partial: "search_summary" %> <%= render(partial: 'shared/error', collection: @errors) %> diff --git a/app/views/search/results_geo.html.erb b/app/views/search/results_geo.html.erb index 3ec91925..1d025023 100644 --- a/app/views/search/results_geo.html.erb +++ b/app/views/search/results_geo.html.erb @@ -8,7 +8,7 @@ <%= render partial: "shared/site_title" %> - <%= render partial: "form" %> + <%= render partial: "form_geo" %> <%= render partial: "search_summary_geo" %> <%= render(partial: 'shared/error', collection: @errors) %> diff --git a/test/controllers/search_controller_geo_test.rb b/test/controllers/search_controller_geo_test.rb index 70d71304..6e6908fa 100644 --- a/test/controllers/search_controller_geo_test.rb +++ b/test/controllers/search_controller_geo_test.rb @@ -23,9 +23,45 @@ def setup assert_select 'input#advanced-identifiers', count: 0 end + test 'index shows geo form when GDT is enabled' do + get '/' + + # Should show geo form with all geo elements, no basic form in header + assert_select 'form#search-form', { count: 0 } # No basic form in header when GDT enabled + assert_select 'form#search-form-geo', { count: 1 } # Geo form in main content + assert_select 'details#geobox-search-panel', { count: 1 } + assert_select 'details#geodistance-search-panel', { count: 1 } + assert_select 'details#advanced-search-panel', { count: 1 } + end + + test 'results page shows geo form when GDT is enabled' do + VCR.use_cassette('geobox and geodistance', + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do + query = { + geobox: 'true', + geodistance: 'true', + geoboxMinLongitude: 40.5, + geoboxMinLatitude: 60.0, + geoboxMaxLongitude: 78.2, + geoboxMaxLatitude: 80.0, + geodistanceLatitude: 36.1, + geodistanceLongitude: 62.6, + geodistanceDistance: '50mi' + }.to_query + get "/results?#{query}" + assert_response :success + assert_select 'form#search-form', { count: 0 } # No basic form in header when GDT enabled + assert_select 'form#search-form-geo', { count: 1 } + assert_select 'details#geobox-search-panel', { count: 1 } + assert_select 'details#geodistance-search-panel', { count: 1 } + assert_select 'details#advanced-search-panel', { count: 1 } + end + end + test 'contributors label is renamed to authors in GDT' do get '/' - assert_select 'label', text: "Authors" + assert_select 'label', text: 'Authors' end test 'index shows geobox form, closed by default' do @@ -84,8 +120,8 @@ def setup test 'GDT lists applied geospatial search terms' do VCR.use_cassette('geobox and geodistance', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geobox: 'true', geodistance: 'true', @@ -113,8 +149,8 @@ def setup test 'can query geobox' do VCR.use_cassette('geobox', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geobox: 'true', geoboxMinLongitude: 40.5, @@ -130,8 +166,8 @@ def setup test 'can query geodistance' do VCR.use_cassette('geodistance', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geodistance: 'true', geodistanceLatitude: 36.1, @@ -146,8 +182,8 @@ def setup test 'both geospatial fields can be queried at once' do VCR.use_cassette('geobox and geodistance', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geobox: 'true', geodistance: 'true', @@ -167,8 +203,8 @@ def setup test 'geo forms are open on results page with URL parameter' do VCR.use_cassette('geobox and geodistance', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geobox: 'true', geodistance: 'true', @@ -191,8 +227,8 @@ def setup test 'coordinates can include decimals or not' do VCR.use_cassette('geobox and geodistance many decimals', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geobox: 'true', geodistance: 'true', @@ -213,8 +249,8 @@ def setup end VCR.use_cassette('geobox and geodistance no decimals', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geobox: 'true', geodistance: 'true', @@ -237,8 +273,8 @@ def setup test 'geospatial arguments retain values in search form with correct data types' do VCR.use_cassette('geobox and geodistance', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { geobox: 'true', geodistance: 'true', @@ -248,7 +284,7 @@ def setup geoboxMaxLatitude: 80.0, geodistanceLatitude: 36.1, geodistanceLongitude: 62.6, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{query}" assert_response :success @@ -266,8 +302,8 @@ def setup test 'geospatial search can combine with basic and advanced inputs' do VCR.use_cassette('geo all', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do query = { q: 'hi', title: 'hey', @@ -280,7 +316,7 @@ def setup geoboxMaxLatitude: 80.0, geodistanceLatitude: 36.1, geodistanceLongitude: 62.6, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{query}" assert_response :success @@ -294,7 +330,7 @@ def setup }.to_query get "/results?#{query}" assert_response :redirect - assert_equal flash[:error], "All bounding box fields are required." + assert_equal flash[:error], 'All bounding box fields are required.' end test 'geodistance arguments are non-nullable' do @@ -303,7 +339,7 @@ def setup }.to_query get "/results?#{query}" assert_response :redirect - assert_equal flash[:error], "All geospatial distance fields are required." + assert_equal flash[:error], 'All geospatial distance fields are required.' end test 'geobox param is required for geobox search' do @@ -315,18 +351,18 @@ def setup }.to_query get "/results?#{query}" assert_response :redirect - assert_equal flash[:error], "A search term is required." + assert_equal flash[:error], 'A search term is required.' end test 'geodistance param is required for geobox search' do query = { geodistanceLatitude: 36.1, geodistanceLongitude: 62.6, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{query}" assert_response :redirect - assert_equal flash[:error], "A search term is required." + assert_equal flash[:error], 'A search term is required.' end test 'geobox lat cannot fall below lower limit' do @@ -339,7 +375,7 @@ def setup }.to_query get "/results?#{low_query}" assert_response :redirect - assert_equal flash[:error], "Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0." + assert_equal flash[:error], 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.' end test 'geobox long cannot fall below lower limit' do @@ -352,7 +388,7 @@ def setup }.to_query get "/results?#{low_query}" assert_response :redirect - assert_equal flash[:error], "Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0." + assert_equal flash[:error], 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.' end test 'geobox lat/long cannot exceed upper limit' do @@ -365,13 +401,13 @@ def setup }.to_query get "/results?#{high_query}" assert_response :redirect - assert_equal flash[:error], "Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0." + assert_equal flash[:error], 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.' end test 'geobox does not error with minimum lat value' do VCR.use_cassette('geobox min lat range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geobox: 'true', geoboxMinLongitude: -45.0, @@ -387,8 +423,8 @@ def setup test 'geobox does not error with minimum long value' do VCR.use_cassette('geobox min long range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geobox: 'true', geoboxMinLongitude: -180.0, @@ -404,8 +440,8 @@ def setup test 'geobox does not error with maximum lat value' do VCR.use_cassette('geobox max lat range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geobox: 'true', geoboxMinLongitude: 45.0, @@ -421,8 +457,8 @@ def setup test 'geobox does not error with maximum long value' do VCR.use_cassette('geobox max long range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geobox: 'true', geoboxMinLongitude: 45.0, @@ -446,7 +482,7 @@ def setup }.to_query get "/results?#{bad_values}" assert_response :redirect - assert_equal flash[:error], "Maximum latitude cannot exceed minimum latitude." + assert_equal flash[:error], 'Maximum latitude cannot exceed minimum latitude.' end test 'geodistance lat cannot fall below lower limit' do @@ -454,11 +490,11 @@ def setup geodistance: 'true', geodistanceLatitude: -90.000001, geodistanceLongitude: -180.0, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{low_query}" assert_response :redirect - assert_equal flash[:error], "Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0." + assert_equal flash[:error], 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.' end test 'geodistance long cannot fall below lower limit' do @@ -466,11 +502,11 @@ def setup geodistance: 'true', geodistanceLatitude: -90.0, geodistanceLongitude: -180.000001, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{low_query}" assert_response :redirect - assert_equal flash[:error], "Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0." + assert_equal flash[:error], 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.' end test 'geodistance lat cannot exceed upper limit' do @@ -478,11 +514,11 @@ def setup geodistance: 'true', geodistanceLatitude: 90.000001, geodistanceLongitude: 180.0, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{high_query}" assert_response :redirect - assert_equal flash[:error], "Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0." + assert_equal flash[:error], 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.' end test 'geodistance long cannot exceed upper limit' do @@ -490,22 +526,22 @@ def setup geodistance: 'true', geodistanceLatitude: 90.0, geodistanceLongitude: 180.000001, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{high_query}" assert_response :redirect - assert_equal flash[:error], "Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0." + assert_equal flash[:error], 'Latitude must be between -90.0 and 90.0, and longitude must be -180.0 and 180.0.' end test 'geodistance does not error with minimum lat value' do VCR.use_cassette('geodistance min lat range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geodistance: 'true', geodistanceLatitude: -90.0, geodistanceLongitude: -43.33, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{acceptable_query}" assert_response :success @@ -515,13 +551,13 @@ def setup test 'geodistance does not error with minimum long value' do VCR.use_cassette('geodistance min long range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geodistance: 'true', geodistanceLatitude: -21.1724, geodistanceLongitude: -180.0, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{acceptable_query}" assert_response :success @@ -531,13 +567,13 @@ def setup test 'geodistance does not error with maximum lat value' do VCR.use_cassette('geodistance max lat range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geodistance: 'true', geodistanceLatitude: 90.0, geodistanceLongitude: -43.33, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{acceptable_query}" assert_response :success @@ -547,30 +583,30 @@ def setup test 'geodistance does not error with maximum long value' do VCR.use_cassette('geodistance max long range limit', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geodistance: 'true', geodistanceLatitude: -21.1724, geodistanceLongitude: 180.0, - geodistanceDistance: '50mi' + geodistanceDistance: '50mi' }.to_query get "/results?#{acceptable_query}" assert_response :success assert_nil flash[:error] end - end + end test 'geodistance cannot be negative' do zero_distance = { geodistance: 'true', geodistanceLatitude: 90.0, geodistanceLongitude: 180.0, - geodistanceDistance: '-50mi' + geodistanceDistance: '-50mi' }.to_query get "/results?#{zero_distance}" assert_response :redirect - assert_equal flash[:error], "Distance must include an integer greater than 0." + assert_equal flash[:error], 'Distance must include an integer greater than 0.' end test 'geodistance cannot be 0' do @@ -578,17 +614,17 @@ def setup geodistance: 'true', geodistanceLatitude: 90.0, geodistanceLongitude: 180.0, - geodistanceDistance: '0mi' + geodistanceDistance: '0mi' }.to_query get "/results?#{negative_distance}" assert_response :redirect - assert_equal flash[:error], "Distance must include an integer greater than 0." + assert_equal flash[:error], 'Distance must include an integer greater than 0.' end test 'geodistance can contain units or not (default is meters)' do VCR.use_cassette('geodistance units', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do acceptable_query = { geodistance: 'true', geodistanceLatitude: -21.1724, @@ -601,8 +637,8 @@ def setup end VCR.use_cassette('geodistance no units', - allow_playback_repeats: true, - match_requests_on: %i[method uri body]) do + allow_playback_repeats: true, + match_requests_on: %i[method uri body]) do another_acceptable_query = { geodistance: 'true', geodistanceLatitude: -21.1724, @@ -624,7 +660,7 @@ def setup }.to_query get "/results?#{bad_units}" assert_response :redirect - assert_equal flash[:error], "Distance units must be one of the following: mi, km, yd, ft, in, m, cm, mm, NM/nmi" + assert_equal flash[:error], 'Distance units must be one of the following: mi, km, yd, ft, in, m, cm, mm, NM/nmi' end test 'view full record link appears as expected for GDT records' do diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 3b5cea00..3cbdc1ca 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -75,7 +75,7 @@ def mock_timdex_search_success get '/' assert_response :success - assert_select 'form#basic-search', { count: 1 } + assert_select 'form#search-form', { count: 1 } assert_select 'details#advanced-search-panel', count: 0 end @@ -90,6 +90,19 @@ def mock_timdex_search_success end end + test 'index shows basic search form when GDT is disabled' do + # GDT is disabled by default in test setup + get '/' + assert_response :success + + # Should show basic form without geo elements + assert_select 'form#search-form', { count: 1 } + assert_select 'form#search-form-geo', { count: 0 } + assert_select 'details#geobox-search-panel', { count: 0 } + assert_select 'details#geodistance-search-panel', { count: 0 } + assert_select 'details#advanced-search-panel', { count: 0 } + end + test 'advanced search form appears on results page with URL parameter' do if Flipflop.enabled?(:gdt) VCR.use_cassette('advanced', @@ -183,7 +196,7 @@ def mock_timdex_search_success get '/results?q=hallo' assert_response :success - assert_select 'form#basic-search', { count: 1 } + assert_select 'form#search-form', { count: 1 } end test 'timdex results with valid query shows search form' do @@ -191,7 +204,7 @@ def mock_timdex_search_success get '/results?q=hallo&tab=timdex' assert_response :success - assert_select 'form#basic-search', { count: 1 } + assert_select 'form#search-form', { count: 1 } end test 'primo results with valid query populates search form with query' do @@ -210,6 +223,15 @@ def mock_timdex_search_success assert_select '#basic-search-main[value=data]' end + test 'results page shows basic USE search form when GDT is disabled' do + # GDT is disabled by default in test setup + mock_primo_search_success + get '/results?q=test' + assert_response :success + assert_select 'form#search-form', { count: 1 } + assert_select 'form#search-form-geo', { count: 0 } + end + test 'results with valid query has div for filters which is populated' do skip('Filters not implemented in USE UI') VCR.use_cassette('data basic controller',