Skip to content

Commit

Permalink
Add pagination for the bulk search results
Browse files Browse the repository at this point in the history
The bulk search can return hundreds of results and currently the UI
shows them all in a long table.

We want to make this a better UX by introducing pagination.

In order to make this new UX work, we need to store the API response in
a table as the API itself isn't paginated.

To avoid making the same API request over and over again to paginate, we
store the response and set an expiration value. This is effectively a
simple caching system.
  • Loading branch information
felixclack committed Sep 17, 2024
1 parent d1e8dd5 commit 62eab2f
Show file tree
Hide file tree
Showing 17 changed files with 173 additions and 25 deletions.
5 changes: 5 additions & 0 deletions app/assets/stylesheets/check_records.scss
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ h1 .govuk-tag {
}
}

.app-pagination {
display: flex;
justify-content: center;
}

@media print {
.app__no-print {
display: none;
Expand Down
25 changes: 25 additions & 0 deletions app/controllers/check_records/bulk_searches_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
require 'pagy/extras/array'

module CheckRecords
class BulkSearchesController < CheckRecordsController
include Pagy::Backend

def new
@bulk_search = BulkSearch.new
@organisation_name = current_dsi_user.dsi_user_sessions.last&.organisation_name
Expand All @@ -9,6 +13,13 @@ def create
@bulk_search = BulkSearch.new(file: bulk_search_params[:file])
@total, @results, @not_found = @bulk_search.call
if @results
@bulk_search_response = BulkSearchResponse.create!(
dsi_user: current_dsi_user,
body: { results: @results, not_found: @not_found },
total: @total,
expires_at: 30.minutes.from_now
)

BulkSearchLog.create!(
csv: @bulk_search.csv,
dsi_user_id: current_dsi_user.id,
Expand All @@ -19,7 +30,21 @@ def create
send_error_analytics_event
render :new
end
end

def show
@bulk_search_response = current_dsi_user.bulk_search_responses.find(params[:id])
if @bulk_search_response.expires_at&.past?
redirect_to new_check_records_bulk_search_path, alert: "Bulk search expired"
end

data = @bulk_search_response.body
@total = @bulk_search_response.total
@results ||= data.fetch("results", []).map do |teacher|
QualificationsApi::Teacher.new(teacher['api_data'])
end
@not_found = data['not_found'].map {|record| Hashie::Mash.new(record) }
@pagy, @results = pagy_array(@results, limit: 4)
end

private
Expand Down
2 changes: 2 additions & 0 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
module ApplicationHelper
include Pagy::Frontend

def current_namespace
request.path.split("/").second
end
Expand Down
13 changes: 13 additions & 0 deletions app/models/bulk_search_response.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class BulkSearchResponse < ApplicationRecord
belongs_to :dsi_user

before_create :expire_other_responses

private

def expire_other_responses
BulkSearchResponse.where(dsi_user:)
.where('expires_at > ?', Time.current)
.update_all(expires_at: Time.current)
end
end
1 change: 1 addition & 0 deletions app/models/dsi_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ class DsiUser < ApplicationRecord

has_many :search_logs
has_many :dsi_user_sessions, dependent: :destroy
has_many :bulk_search_responses

CURRENT_TERMS_AND_CONDITIONS_VERSION = "1.0".freeze

Expand Down
20 changes: 5 additions & 15 deletions app/views/check_records/bulk_searches/create.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,15 @@
<% 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 %>

<div class="govuk-grid-row">
<div class="govuk-grid-column-full">
<h1 class="govuk-heading-l"><%= pluralize(@total, 'teacher record') %> found</h1>
<%= govuk_tabs do |tabs|
if @results.any?
tabs.with_tab(label: "Records found", id: "records-found") do %>
<%= render "check_records/search/results_table", teachers: @results %>
<% end %>
<% end %>
<% if @not_found.any? %>
<% tabs.with_tab(label: "Not found", id: "records-not-found") do %>
<%= render "check_records/search/not_found", not_found: @not_found %>
<% end %>
<% end %>
<% end %>
<h1 class="govuk-heading-l">File summary</h1>
<p class="govuk-body">
Your CSV file includes the details of <%= pluralize(@total, 'person') %>:
</p>

<%= govuk_link_to "Search again", new_check_records_bulk_search_path %>
<%= govuk_link_to "Find records", check_records_bulk_search_path(@bulk_search_response), class: "govuk-button" %>
</div>
</div>
25 changes: 25 additions & 0 deletions app/views/check_records/bulk_searches/show.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<% 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 %>

<div class="govuk-grid-row">
<div class="govuk-grid-column-full">
<h1 class="govuk-heading-l"><%= pluralize(@total, 'teacher record') %> found</h1>
<%= govuk_tabs do |tabs|
if @results.any?
tabs.with_tab(label: "Records found", id: "records-found") do %>
<%= render "check_records/search/results_table", teachers: @results %>
<% end %>
<% end %>
<% if @not_found.any? %>
<% tabs.with_tab(label: "Not found", id: "records-not-found") do %>
<%= render "check_records/search/not_found", not_found: @not_found %>
<% end %>
<% end %>
<% end %>

<%= govuk_link_to "Search again", new_check_records_bulk_search_path %>
</div>
</div>
2 changes: 1 addition & 1 deletion app/views/check_records/search/_not_found.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
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_fs(:long_uk))
row.with_cell(text: record.date_of_birth.to_date.to_fs(:long_uk))
end
end
end
Expand Down
7 changes: 7 additions & 0 deletions app/views/check_records/search/_results_table.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,11 @@
end
end %>
<% end %>
<div class="govuk-grid-row">
<div class="govuk-grid-column-full">
<div class="app-pagination">
<%= govuk_pagination(pagy: @pagy) if @pagy.pages.positive? %>
</div>
</div>
</div>
</div>
8 changes: 8 additions & 0 deletions config/analytics_blocklist.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,11 @@
- internal
- created_at
- updated_at
:bulk_search_responses:
- id
- body
- expires_at
- dsi_user_id
- total
- created_at
- updated_at
2 changes: 1 addition & 1 deletion config/routes/check_records.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
patch "/terms-and-conditions" => "terms_and_conditions#update"

resources :teachers, only: %i[show]
resources :bulk_searches, only: %i[new create], path: 'bulk-searches'
resources :bulk_searches, only: %i[new create show], path: 'bulk-searches'

scope "/feedback" do
get "/", to: "feedbacks#new", as: :feedbacks
Expand Down
12 changes: 12 additions & 0 deletions db/migrate/20240917065402_create_bulk_search_responses.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class CreateBulkSearchResponses < ActiveRecord::Migration[7.2]
def change
create_table :bulk_search_responses, id: :uuid do |t|
t.jsonb :body
t.datetime :expires_at
t.references :dsi_user, null: false, foreign_key: true
t.integer :total

t.timestamps
end
end
end
13 changes: 12 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.2].define(version: 2024_08_29_103510) do
ActiveRecord::Schema[7.2].define(version: 2024_09_17_065402) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"

Expand Down Expand Up @@ -63,6 +63,16 @@
t.index ["dsi_user_id"], name: "index_bulk_search_logs_on_dsi_user_id"
end

create_table "bulk_search_responses", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
t.jsonb "body"
t.datetime "expires_at"
t.bigint "dsi_user_id", null: false
t.integer "total"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["dsi_user_id"], name: "index_bulk_search_responses_on_dsi_user_id"
end

create_table "console1984_commands", force: :cascade do |t|
t.text "statements"
t.bigint "sensitive_access_id"
Expand Down Expand Up @@ -211,6 +221,7 @@
add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id"
add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id"
add_foreign_key "bulk_search_logs", "dsi_users"
add_foreign_key "bulk_search_responses", "dsi_users"
add_foreign_key "date_of_birth_changes", "users"
add_foreign_key "search_logs", "dsi_users"
end
8 changes: 8 additions & 0 deletions spec/factories/bulk_search_responses.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
FactoryBot.define do
factory :bulk_search_response do
association :dsi_user
body { { results: [], not_found: [] } }
total { 0 }
expires_at { 30.minutes.from_now }
end
end
9 changes: 2 additions & 7 deletions spec/factories/dsi_users.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
FactoryBot.define do
factory :dsi_user do
sequence :email do |n|
"dsi-user-#{n}@example.com"
end

first_name { "Steven" }
last_name { "Toast" }
uid { "123" }
sequence(:email) { |n| "user#{n}@example.com" }
sequence(:uid) { |n| "uid#{n}" }
end
end
33 changes: 33 additions & 0 deletions spec/models/bulk_search_response_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
require 'rails_helper'

RSpec.describe BulkSearchResponse, type: :model do
describe 'callbacks' do
describe '#expire_other_responses' do
let(:dsi_user) { create(:dsi_user) }
let!(:old_response) { create(:bulk_search_response, dsi_user:, expires_at: 1.hour.from_now) }

it 'expires other responses for the same user' do
expect {
create(:bulk_search_response, dsi_user:)
}.to change { old_response.reload.expires_at }.to be_within(1.second).of(Time.current)
end

it 'does not expire responses for other users' do
other_user = create(:dsi_user)
other_response = create(:bulk_search_response, dsi_user: other_user, expires_at: 1.hour.from_now)

expect {
create(:bulk_search_response, dsi_user:)
}.not_to(change { other_response.reload.expires_at })
end

it 'does not affect already expired responses' do
expired_response = create(:bulk_search_response, dsi_user:, expires_at: 1.hour.ago)

expect {
create(:bulk_search_response, dsi_user:)
}.not_to(change { expired_response.reload.expires_at })
end
end
end
end
13 changes: 13 additions & 0 deletions spec/system/check_records/user_searches_with_a_valid_csv_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
when_i_sign_in_via_dsi
and_search_with_a_valid_csv
then_i_see_the_results_summary
when_i_click_find_records
then_i_see_results
and_my_search_is_logged
end

Expand All @@ -22,7 +24,18 @@ def and_search_with_a_valid_csv
end

def then_i_see_the_results_summary
expect(page).to have_content "Your CSV file includes the details of 1 person"
end

def when_i_click_find_records
click_link "Find records"
end

def then_i_see_results
expect(page).to have_content "1 teacher record found"
expect(page).to have_content "Terry Walsh"
expect(page).to have_content "Restriction"
expect(page).to have_content "No induction"
end

def and_my_search_is_logged
Expand Down

0 comments on commit 62eab2f

Please sign in to comment.