From 7c504b109f3b68c24428d3c7c5d1429765ca2b14 Mon Sep 17 00:00:00 2001 From: Felix Clack Date: Tue, 24 Sep 2024 09:01:15 +0100 Subject: [PATCH 1/3] Ensure to extend the expiry of a bulk search response We want the bulk search response to live for as long as someone needs to access it. The approach is based on a typical session window where 30 minutes of inactivity will cause the response to expire. There is a bug in the implementation at the moment where response always expires 30 minutes after it was originally created rather than 30 minutes from when it was last accessed. --- .../check_records/bulk_searches_controller.rb | 2 ++ .../user_searches_with_a_valid_csv_spec.rb | 17 +++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/app/controllers/check_records/bulk_searches_controller.rb b/app/controllers/check_records/bulk_searches_controller.rb index 01c776cf..76e03d2f 100644 --- a/app/controllers/check_records/bulk_searches_controller.rb +++ b/app/controllers/check_records/bulk_searches_controller.rb @@ -45,6 +45,8 @@ def show end @not_found = data['not_found'].map {|record| Hashie::Mash.new(record) } @pagy, @results = pagy_array(@results) + + @bulk_search_response.update!(expires_at: 30.minutes.from_now) end private diff --git a/spec/system/check_records/user_searches_with_a_valid_csv_spec.rb b/spec/system/check_records/user_searches_with_a_valid_csv_spec.rb index 10f63c6c..ca8510c6 100644 --- a/spec/system/check_records/user_searches_with_a_valid_csv_spec.rb +++ b/spec/system/check_records/user_searches_with_a_valid_csv_spec.rb @@ -13,6 +13,8 @@ when_i_click_find_records then_i_see_results and_my_search_is_logged + when_i_wait_for_the_search_to_expire + then_i_see_the_expired_search_message end private @@ -44,4 +46,19 @@ def and_my_search_is_logged expect(search_log.query_count).to eq 2 expect(search_log.result_count).to eq 1 end + + def when_i_wait_for_the_search_to_expire + travel 29.minutes + page.refresh + travel 2.minutes + page.refresh + expect(page).not_to have_content "Bulk search expired" + travel 31.minutes + page.refresh + end + + def then_i_see_the_expired_search_message + expect(page).to have_content "Bulk search expired" + travel_back + end end From b1a819f4960b9385036c5d836761701b719dd92b Mon Sep 17 00:00:00 2001 From: Felix Clack Date: Tue, 24 Sep 2024 11:23:29 +0100 Subject: [PATCH 2/3] Add tabbed pagination The GOVUK pagination component doesn't support being rendered in a tab. I decided to add a new component that wraps the current GOVUK component but overrides the link rendering to include a fragment. This allows the links to use the current tab as part of the URL and navigate the pagination without affecting the tab value. --- app/components/tabbed_pagination_component.rb | 38 +++++++++++++++++++ .../check_records/bulk_searches_controller.rb | 4 +- .../check_records/search/_not_found.html.erb | 19 +++++++--- 3 files changed, 55 insertions(+), 6 deletions(-) create mode 100644 app/components/tabbed_pagination_component.rb diff --git a/app/components/tabbed_pagination_component.rb b/app/components/tabbed_pagination_component.rb new file mode 100644 index 00000000..0ac54d4a --- /dev/null +++ b/app/components/tabbed_pagination_component.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +class TabbedPaginationComponent < GovukComponent::PaginationComponent + def initialize(fragment: nil, **kwargs) + self.fragment = fragment + super(**kwargs) + end + + private + + attr_accessor :fragment + + def build_items + pagy.series.map { |i| with_item(number: i, href: pagy_url_for(pagy, i, fragment:), from_pagy: true) } + end + + def build_next + return unless pagy&.next + + kwargs = { + href: pagy_url_for(pagy, pagy.next, fragment:), + text: @next_text, + } + + with_next_page(**kwargs.compact) + end + + def build_previous + return unless pagy&.prev + + kwargs = { + href: pagy_url_for(pagy, pagy.prev, fragment:), + text: @previous_text, + } + + with_previous_page(**kwargs.compact) + end +end diff --git a/app/controllers/check_records/bulk_searches_controller.rb b/app/controllers/check_records/bulk_searches_controller.rb index 76e03d2f..0398352e 100644 --- a/app/controllers/check_records/bulk_searches_controller.rb +++ b/app/controllers/check_records/bulk_searches_controller.rb @@ -44,7 +44,9 @@ def show QualificationsApi::Teacher.new(teacher['api_data']) end @not_found = data['not_found'].map {|record| Hashie::Mash.new(record) } - @pagy, @results = pagy_array(@results) + @pagy, @results = pagy_array(@results, limit: 10) + @pagy_not_found, @not_found = + pagy_array(@not_found, limit: 10, page_param: :page_not_found, anchor_string: '#records-not-found') @bulk_search_response.update!(expires_at: 30.minutes.from_now) end diff --git a/app/views/check_records/search/_not_found.html.erb b/app/views/check_records/search/_not_found.html.erb index 07abfebd..cf496553 100644 --- a/app/views/check_records/search/_not_found.html.erb +++ b/app/views/check_records/search/_not_found.html.erb @@ -1,6 +1,7 @@
- <%= govuk_table do |table| - table.with_caption(size: 'm', text: 'No records found') + <% if not_found.any? %> + <%= govuk_table do |table| + table.with_caption(size: 'm', text: 'No records found') table.with_head do |head| head.with_row do |row| @@ -13,9 +14,17 @@ not_found.each do |record| body.with_row do |row| row.with_cell(text: record.trn) - row.with_cell(text: record.date_of_birth.to_date.to_fs(:long_uk)) + row.with_cell(text: record.date_of_birth.to_date.to_fs(:long_uk)) + end end end - end - end %> + end %> + <% end %> +
+
+
+ <%= render TabbedPaginationComponent.new(pagy: @pagy_not_found, fragment: '#records-not-found') if @pagy_not_found.pages.positive? %> +
+
+
From cc115c8f843970183d1ca77ce7213ebd24d7e3c6 Mon Sep 17 00:00:00 2001 From: Felix Clack Date: Tue, 24 Sep 2024 11:30:56 +0100 Subject: [PATCH 3/3] Update copy We want to set expectations around the upload limits. --- app/views/check_records/bulk_searches/new.html.erb | 2 +- app/views/check_records/bulk_searches/show.html.erb | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/views/check_records/bulk_searches/new.html.erb b/app/views/check_records/bulk_searches/new.html.erb index 5d35a94e..e94dfd9a 100644 --- a/app/views/check_records/bulk_searches/new.html.erb +++ b/app/views/check_records/bulk_searches/new.html.erb @@ -25,7 +25,7 @@ must be in DD/MM/YYYY format.

- You can add a maximum of 20 people. + You can add a maximum of 100 people.

  • diff --git a/app/views/check_records/bulk_searches/show.html.erb b/app/views/check_records/bulk_searches/show.html.erb index 701576dc..826102a2 100644 --- a/app/views/check_records/bulk_searches/show.html.erb +++ b/app/views/check_records/bulk_searches/show.html.erb @@ -1,12 +1,15 @@ <% content_for :page_title, "Bulk Search Results" %> <% content_for :breadcrumbs do %> <%= govuk_breadcrumbs(breadcrumbs: { "Home" => check_records_search_path, "Find multiple records" => new_check_records_bulk_search_path, "Results" => nil }) %> - <%= render ActionAtComponent.new(action: "searched") %> <% end %>
    + <%= render ActionAtComponent.new(action: "searched") %>

    <%= pluralize(@total, 'teacher record') %> found

    +

    + We found <%= pluralize(@total, 'teacher record') %> out of the <%= pluralize(@results.count + @not_found.count, 'entry') %> you uploaded. +

    <%= govuk_tabs do |tabs| if @results.any? tabs.with_tab(label: "Records found", id: "records-found") do %>