From 257f7aad7e9fc86cb9cbcab8f9fa13d23064b931 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 14:26:29 +0100 Subject: [PATCH 01/14] Change report all application columns order * Add rejection reason column --- app/models/reports/applications.rb | 10 ++++++---- spec/models/reports/applications_spec.rb | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/models/reports/applications.rb b/app/models/reports/applications.rb index 1b8d1927..c486b3a6 100644 --- a/app/models/reports/applications.rb +++ b/app/models/reports/applications.rb @@ -11,7 +11,7 @@ def csv def header %i[ - ip_address + urn given_name middle_name family_name @@ -32,15 +32,16 @@ def header date_of_entry start_date subject - urn visa_type + rejection_reason + ip_address ].map { _1.to_s.titleize } end def columns(application) applicant = application.applicant [ - applicant.ip_address, + application.urn, applicant.given_name, applicant.middle_name, applicant.family_name, @@ -61,8 +62,9 @@ def columns(application) application.date_of_entry, application.start_date, application.subject, - application.urn, application.visa_type, + application.application_progress.rejection_reason, + applicant.ip_address, ] end diff --git a/spec/models/reports/applications_spec.rb b/spec/models/reports/applications_spec.rb index a327be5a..4609f6ae 100644 --- a/spec/models/reports/applications_spec.rb +++ b/spec/models/reports/applications_spec.rb @@ -40,7 +40,7 @@ module Reports context "returns file with header" do let(:header) do [ - "Ip Address", + "Urn", "Given Name", "Middle Name", "Family Name", @@ -61,8 +61,9 @@ module Reports "Date Of Entry", "Start Date", "Subject", - "Urn", "Visa Type", + "Rejection Reason", + "Ip Address", ].join(",") end From 298dd3f864a2ea1dd96a2e4945348b91eb123799 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Wed, 27 Sep 2023 14:51:42 +0100 Subject: [PATCH 02/14] Create Urn model This will serve to render a urn and hold the current state of available urns ready to be used. --- app/models/urn.rb | 63 ++++++++++++----------- db/migrate/20230927092305_create_urns.rb | 12 +++++ db/schema.rb | 11 +++- spec/factories/urns.rb | 7 +++ spec/models/urn_spec.rb | 64 +++++++++++++++--------- 5 files changed, 103 insertions(+), 54 deletions(-) create mode 100644 db/migrate/20230927092305_create_urns.rb create mode 100644 spec/factories/urns.rb diff --git a/app/models/urn.rb b/app/models/urn.rb index 56330c1c..e88c85e1 100644 --- a/app/models/urn.rb +++ b/app/models/urn.rb @@ -1,38 +1,41 @@ -# frozen_string_literal: true - -# Urn represents a Uniform Resource Name (URN) generator. -# It generates a URN with a fixed prefix and a random alphanumeric suffix. -# +# == Schema Information # -# Example: +# Table name: urns # -# Urn.generate('teacher') # => "IRPTE12345" -# Urn.generate('teacher') # => "IRPTE12345" -# Urn.generate('salaried_trainee') # => "IRPST12345" +# id :bigint not null, primary key +# code :string +# prefix :string +# suffix :integer +# created_at :datetime not null +# updated_at :datetime not null # -class Urn - attr_reader :value - attr_writer :suffix - - def self.generate(applicant_type) - code = applicant_type_code(applicant_type) - PREFIX + code + Array.new(LENGTH) { CHARSET.sample }.join - end +class Urn < ApplicationRecord + class NoUrnAvailableError < StandardError; end - CHARSET = %w[0 1 2 3 4 5 6 7 8 9].freeze - PREFIX = "IRP" - LENGTH = 5 - private_constant :CHARSET, :PREFIX, :LENGTH + PREFIX = "IRP".freeze + MAX_SUFFIX = 99_999 + PADDING_SIZE = MAX_SUFFIX.to_s.size + VALID_CODES = { + "teacher" => "TE", + "salaried_trainee" => "ST", + }.freeze - def self.applicant_type_code(applicant_type) - case applicant_type - when "teacher" - "TE" - when "salaried_trainee" - "ST" - else - raise(ArgumentError, "Invalid applicant type: #{applicant_type}") + def self.next(route) + code = VALID_CODES.fetch(route) + Urn.transaction do + urn = find_by!(code:) + urn.destroy! + urn.to_s end + rescue KeyError => e + Sentry.capture_exception(e) + raise(ArgumentError, "Unknown route #{route}") + rescue ActiveRecord::RecordNotFound => e + Sentry.capture_exception(e) + raise(NoUrnAvailableError, "There no more unique URN available for #{route}") + end + + def to_s + [prefix, code, sprintf("%0#{PADDING_SIZE}d", suffix)].join end - private_methods :applicant_type_code end diff --git a/db/migrate/20230927092305_create_urns.rb b/db/migrate/20230927092305_create_urns.rb new file mode 100644 index 00000000..e13281c1 --- /dev/null +++ b/db/migrate/20230927092305_create_urns.rb @@ -0,0 +1,12 @@ +class CreateUrns < ActiveRecord::Migration[7.0] + def change + create_table :urns do |t| + t.string :prefix + t.string :code + t.integer :suffix + + t.timestamps + end + add_index :urns, :code + end +end diff --git a/db/schema.rb b/db/schema.rb index 2a356c60..a9313f30 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_09_15_100841) do +ActiveRecord::Schema[7.0].define(version: 2023_09_27_092305) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "plpgsql" @@ -173,6 +173,15 @@ t.datetime "updated_at", null: false end + create_table "urns", force: :cascade do |t| + t.string "prefix" + t.string "code" + t.integer "suffix" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["code"], name: "index_urns_on_code" + end + create_table "users", force: :cascade do |t| t.citext "email" t.datetime "created_at", null: false diff --git a/spec/factories/urns.rb b/spec/factories/urns.rb new file mode 100644 index 00000000..8112eadb --- /dev/null +++ b/spec/factories/urns.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :urn do + prefix { "IRP" } + code { %w[TE ST].sample } + suffix { Array.new(10) { _1 }.sample } + end +end diff --git a/spec/models/urn_spec.rb b/spec/models/urn_spec.rb index 4df75ded..02466d4c 100644 --- a/spec/models/urn_spec.rb +++ b/spec/models/urn_spec.rb @@ -1,39 +1,57 @@ -# frozen_string_literal: true - +# == Schema Information +# +# Table name: urns +# +# id :bigint not null, primary key +# code :string +# prefix :string +# suffix :integer +# created_at :datetime not null +# updated_at :datetime not null +# require "rails_helper" RSpec.describe Urn do - subject(:urn) { described_class.generate(applicant_type) } - describe ".generate" do - context 'when applicant type is "teacher"' do - let(:applicant_type) { "teacher" } + describe "next" do + subject(:next_urn) { described_class.next(route) } + + context "for application route teacher" do + let(:route) { "teacher" } + + before { create(:urn, code: "TE") } + + it { expect(next_urn).to match(/IRPTE\d{5}/) } + end - it "generates a URN with the correct prefix and suffix" do - expect(urn).to match(/^IRPTE[0-9]{5}$/) - end + context "for application route salaried_trainee" do + let(:route) { "salaried_trainee" } - it "generates a Urn with a suffix of only characters in the CHARSET" do - charset = %w[0 1 2 3 4 5 6 7 8 9] + before { create(:urn, code: "ST") } - expect(urn[5..9].chars).to all(be_in(charset)) - end + it { expect(next_urn).to match(/IRPST\d{5}/) } end - context 'when applicant type is "salaried_trainee"' do - let(:applicant_type) { "salaried_trainee" } + context "when bad application route" do + let(:route) { "badroute" } - it "generates a URN with the correct prefix and suffix" do - expect(urn).to match(/^IRPST[0-9]{5}$/) - end + it { expect { next_urn }.to raise_error(ArgumentError) } end - context "when an invalid applicant type is provided" do - let(:applicant_type) { "invalid_type" } + context "when there is no more urn available to assign" do + let(:route) { "salaried_trainee" } - it "raises an ArgumentError" do - expect { urn }.to raise_error(ArgumentError, "Invalid applicant type: invalid_type") - end + it { expect { next_urn }.to raise_error(Urn::NoUrnAvailableError) } end end + + describe ".to_s" do + subject(:urn) { described_class.new(prefix:, code:, suffix:) } + + let(:prefix) { "AST" } + let(:code) { "FF" } + let(:suffix) { 65 } + + it { expect(urn.to_s).to eq("ASTFF00065") } + end end From 873ff949b8378eb3ef6e98c7770d9691838bf54d Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Wed, 27 Sep 2023 14:52:43 +0100 Subject: [PATCH 03/14] Add GenerateUrns service This service will create all the available urns per application route. They will be randomized, unique and the next available urn to be taken will retreived with the Urn.next method. Once all the urn are exhausted this method will raise an error and the format of the URN will need updating ie increase the size of the suffix set. Urn::MAX_SUFFIX --- app/services/generate_urns.rb | 62 +++++++++++++++++++++++++++++ spec/services/generate_urns_spec.rb | 13 ++++++ 2 files changed, 75 insertions(+) create mode 100644 app/services/generate_urns.rb create mode 100644 spec/services/generate_urns_spec.rb diff --git a/app/services/generate_urns.rb b/app/services/generate_urns.rb new file mode 100644 index 00000000..a0a33274 --- /dev/null +++ b/app/services/generate_urns.rb @@ -0,0 +1,62 @@ +# Service responsible for the generation of all urns +# It will save the set of available urns based the current URN format +# and store it in the database URNs table. +# +# The Urn model will then be able to fetch the next available unique and +# random urn for application submition +# +# Example: +# +# Urn.next("teacher") # => "IRPTE12345" +# Urn.next("teacher") # => "IRPTE12345" +# Urn.next("salaried_trainee") # => "IRPST12345" +# +class GenerateUrns + def self.call + return if Urn.count.positive? # Do not override the current urn state + + Urn.transaction do + Urn::VALID_CODES.each_value do |code| + new(code:).generate + end + end + end + + def initialize(code:) + @code = code + end + + attr_reader :code + + def generate + data = unused_urns.map do |suffix| + { prefix: Urn::PREFIX, code: code, suffix: suffix } + end + Urn.insert_all(data) # rubocop:disable Rails/SkipsModelValidations + end + +private + + def unused_urns + generate_suffixes - existing_suffixes + end + + def generate_suffixes + Array + .new(Urn::MAX_SUFFIX) { _1 } + .drop(1) + .shuffle! + end + + def existing_suffixes + route = Urn::VALID_CODES.key(code) + Application + .where(application_route: route) + .pluck(:urn) + .map { extract_suffix(_1) } + end + + def extract_suffix(urn) + urn.match(/\d+/)[0].to_i + end +end diff --git a/spec/services/generate_urns_spec.rb b/spec/services/generate_urns_spec.rb new file mode 100644 index 00000000..ebde4c31 --- /dev/null +++ b/spec/services/generate_urns_spec.rb @@ -0,0 +1,13 @@ +require "rails_helper" + +RSpec.describe GenerateUrns do + subject(:service) { described_class } + + describe ".call" do + it { fail } + end + + describe ".generate" do + it { fail } + end +end From af003d8565b81c7d9c916200a469a2eb254e6de4 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Wed, 27 Sep 2023 14:56:23 +0100 Subject: [PATCH 04/14] Add rake task urn:generate This task is going to be used to ensure that when start the application service the URNs are ready to be picked. --- lib/tasks/urn.rake | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 lib/tasks/urn.rake diff --git a/lib/tasks/urn.rake b/lib/tasks/urn.rake new file mode 100644 index 00000000..64c42ccd --- /dev/null +++ b/lib/tasks/urn.rake @@ -0,0 +1,10 @@ +namespace :urn do + desc "generate and randomize unique urns" + task generate: :environment do + puts "running rake task urn:generate ..." + a = Urn.count + GenerateUrns.call + b = Urn.count + puts "#{b - a} URN created" + end +end From 8a93601da58457c88c897fa1e369c11cbe17bee1 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Wed, 27 Sep 2023 14:57:45 +0100 Subject: [PATCH 05/14] Update code to used `Urn.next` * startup.sh * submitform service * factories --- app/services/submit_form.rb | 2 +- bin/app-startup.sh | 2 ++ spec/factories/applications.rb | 2 +- spec/services/submit_form_spec.rb | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/services/submit_form.rb b/app/services/submit_form.rb index 939f2756..23270446 100644 --- a/app/services/submit_form.rb +++ b/app/services/submit_form.rb @@ -88,7 +88,7 @@ def create_application date_of_entry: form.date_of_entry, start_date: form.start_date, subject: SubjectStep.new(form).answer.formatted_value, - urn: Urn.generate(form.application_route), + urn: Urn.next(form.application_route), visa_type: form.visa_type, ) end diff --git a/bin/app-startup.sh b/bin/app-startup.sh index 0df4c416..9da89774 100755 --- a/bin/app-startup.sh +++ b/bin/app-startup.sh @@ -7,6 +7,8 @@ set -e # run migrations bundle exec rails db:migrate +# Front load urn generation +bundle exec rake urn:generate # add seed data in review environment if [[ "$RAILS_ENV" = "review" || "$RAILS_ENV" = "development" ]]; then diff --git a/spec/factories/applications.rb b/spec/factories/applications.rb index 49d86172..712bf90b 100644 --- a/spec/factories/applications.rb +++ b/spec/factories/applications.rb @@ -31,7 +31,7 @@ visa_type { VisaStep::VALID_ANSWERS_OPTIONS.reject { _1 == "Other" }.sample } date_of_entry { Time.zone.today } start_date { 1.month.from_now.to_date } - urn { Urn.generate(application_route) } + urn { Urn.next(application_route) } factory :teacher_application do application_route { "teacher" } diff --git a/spec/services/submit_form_spec.rb b/spec/services/submit_form_spec.rb index 3c110afe..022582e0 100644 --- a/spec/services/submit_form_spec.rb +++ b/spec/services/submit_form_spec.rb @@ -133,7 +133,7 @@ context "applicant email" do before do - allow(Urn).to receive(:generate).and_return(urn) + allow(Urn).to receive(:next).and_return(urn) end let(:urn) { "SOMEURN" } From 55205eaa76e33e0014faa248c47379310c09496e Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 17:02:41 +0100 Subject: [PATCH 06/14] Fix specs for urn --- .../admin_console/applications_list_spec.rb | 6 ++ spec/fixtures/urns.yml | 90 +++++++++++++++++++ spec/models/urn_spec.rb | 5 +- spec/rails_helper.rb | 3 +- spec/services/generate_urns_spec.rb | 22 +++-- 5 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 spec/fixtures/urns.yml diff --git a/spec/features/admin_console/applications_list_spec.rb b/spec/features/admin_console/applications_list_spec.rb index 85a51dee..8cb032cd 100644 --- a/spec/features/admin_console/applications_list_spec.rb +++ b/spec/features/admin_console/applications_list_spec.rb @@ -59,6 +59,8 @@ def given_there_are_few_applications # Create 2 specific applications for search tests + create_list(:urn, 5, code: "TE") + create_list(:urn, 5, code: "ST") unique_applicant = create(:applicant, given_name: "Unique Given Name", middle_name: "Unique Middle Name", family_name: "Unique Family Name", email_address: "unique@example.com") create(:application, applicant: unique_applicant, urn: "Unique Urn 1") @@ -70,12 +72,16 @@ def given_there_are_few_applications end def given_there_is_an_application_that_breached_sla + create_list(:urn, 5, code: "TE") + create_list(:urn, 5, code: "ST") applicant = create(:applicant) application = create(:application, applicant:) application.application_progress.update(initial_checks_completed_at: 4.days.ago) end def given_there_are_applications_with_different_dates + create_list(:urn, 5, code: "TE") + create_list(:urn, 5, code: "ST") create(:application, application_progress: build(:application_progress, :initial_checks_completed, status: :initial_checks)) create(:application, application_progress: build(:application_progress, :home_office_checks_completed, status: :home_office_checks)) end diff --git a/spec/fixtures/urns.yml b/spec/fixtures/urns.yml new file mode 100644 index 00000000..4cedf718 --- /dev/null +++ b/spec/fixtures/urns.yml @@ -0,0 +1,90 @@ +te_one: + suffix: 5668 + prefix: IRP + code: TE + +te_two: + suffix: 21368 + prefix: IRP + code: TE + +te_three: + suffix: 5 + prefix: IRP + code: TE + +te_four: + suffix: 76998 + prefix: IRP + code: TE + +te_five: + suffix: 6559 + prefix: IRP + code: TE + +te_six: + suffix: 6 + prefix: IRP + code: TE + +te_seven: + suffix: 2298 + prefix: IRP + code: TE + +te_eight: + suffix: 1159 + prefix: IRP + code: TE + +te_nine: + suffix: 79298 + prefix: IRP + code: TE + +te_ten: + suffix: 19549 + prefix: IRP + code: TE + +st_one: + suffix: 5668 + prefix: IRP + code: ST + +st_two: + suffix: 29968 + prefix: IRP + code: ST + +st_three: + suffix: 5 + prefix: IRP + code: ST + +st_four: + suffix: 76998 + prefix: IRP + code: ST + +st_five: + suffix: 6559 + prefix: IRP + code: ST + +st_six: + suffix: 6 + prefix: IRP + code: ST + +st_seven: + suffix: 28 + prefix: IRP + code: ST + +st_eight: + suffix: 159 + prefix: IRP + code: ST + diff --git a/spec/models/urn_spec.rb b/spec/models/urn_spec.rb index 02466d4c..eb150367 100644 --- a/spec/models/urn_spec.rb +++ b/spec/models/urn_spec.rb @@ -12,7 +12,6 @@ require "rails_helper" RSpec.describe Urn do - describe "next" do subject(:next_urn) { described_class.next(route) } @@ -41,6 +40,10 @@ context "when there is no more urn available to assign" do let(:route) { "salaried_trainee" } + before do + allow(described_class).to receive(:find_by!).and_raise(ActiveRecord::RecordNotFound) + end + it { expect { next_urn }.to raise_error(Urn::NoUrnAvailableError) } end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 6ba490c8..edfe2b50 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -42,7 +42,8 @@ end RSpec.configure do |config| # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures - config.fixture_path = Rails.root.join("/spec/fixtures") + config.fixture_path = Rails.root.join("spec/fixtures") + config.global_fixtures = :urns # If you're not using ActiveRecord, or you'd prefer not to run each of your # examples within a transaction, remove the following line or assign false diff --git a/spec/services/generate_urns_spec.rb b/spec/services/generate_urns_spec.rb index ebde4c31..cbe58507 100644 --- a/spec/services/generate_urns_spec.rb +++ b/spec/services/generate_urns_spec.rb @@ -1,13 +1,23 @@ require "rails_helper" RSpec.describe GenerateUrns do - subject(:service) { described_class } + describe ".generate" do + subject(:generate) { described_class.new(code:).generate } - describe ".call" do - it { fail } - end + before do + allow(Urn).to receive(:insert_all) + stub_const "Urn::MAX_SUFFIX", 3 + generate + end - describe ".generate" do - it { fail } + let(:code) { "TE" } + let(:expected_data) do + [ + { prefix: "IRP", code: code, suffix: 1 }, + { prefix: "IRP", code: code, suffix: 2 }, + ] + end + + it { expect(Urn).to have_received(:insert_all).with(match_array(expected_data)) } end end From 0d7f5ce99ef3eb71fbb87d5b7dfce11ddddfa16b Mon Sep 17 00:00:00 2001 From: Raul Gracia Date: Thu, 5 Oct 2023 15:35:10 +0100 Subject: [PATCH 07/14] Rename Rejection Details column to Comments (#282) --- app/controllers/system_admin/applicants_controller.rb | 2 +- app/models/application_progress.rb | 2 +- app/models/reports/qa_report.rb | 2 +- app/views/system_admin/applicants/edit.html.erb | 2 +- app/views/system_admin/applicants/show.html.erb | 4 ++-- .../20231002115920_rename_rejection_details_to_comments.rb | 5 +++++ db/schema.rb | 4 ++-- spec/factories/application_progresses.rb | 2 +- spec/features/admin_console/update_progress_spec.rb | 2 +- spec/models/application_progress_spec.rb | 2 +- spec/models/reports/qa_report_spec.rb | 5 ++--- 11 files changed, 18 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20231002115920_rename_rejection_details_to_comments.rb diff --git a/app/controllers/system_admin/applicants_controller.rb b/app/controllers/system_admin/applicants_controller.rb index 06bbca36..ac7da700 100644 --- a/app/controllers/system_admin/applicants_controller.rb +++ b/app/controllers/system_admin/applicants_controller.rb @@ -53,7 +53,7 @@ def applicant_params :payment_confirmation_completed_at, :rejection_completed_at, :rejection_reason, - :rejection_details, + :comments, ) end diff --git a/app/models/application_progress.rb b/app/models/application_progress.rb index 3a42614d..9e8dc4ff 100644 --- a/app/models/application_progress.rb +++ b/app/models/application_progress.rb @@ -6,11 +6,11 @@ # # id :bigint not null, primary key # banking_approval_completed_at :date +# comments :text # home_office_checks_completed_at :date # initial_checks_completed_at :date # payment_confirmation_completed_at :date # rejection_completed_at :date -# rejection_details :text # rejection_reason :integer # school_checks_completed_at :date # school_investigation_required :boolean default(FALSE), not null diff --git a/app/models/reports/qa_report.rb b/app/models/reports/qa_report.rb index 9f6871cb..af3f0db5 100644 --- a/app/models/reports/qa_report.rb +++ b/app/models/reports/qa_report.rb @@ -60,7 +60,7 @@ def rejection_rows(app) [ app.application_progress.rejection_reason&.humanize, - app.application_progress.rejection_details, + app.application_progress.comments, ] end diff --git a/app/views/system_admin/applicants/edit.html.erb b/app/views/system_admin/applicants/edit.html.erb index a9abb2a3..3cbb9e04 100644 --- a/app/views/system_admin/applicants/edit.html.erb +++ b/app/views/system_admin/applicants/edit.html.erb @@ -32,7 +32,7 @@ <%= f.govuk_date_field :payment_confirmation_completed_at, legend: { text: "Payment Confirmation (payslip)", size: "s" } %> <%= f.govuk_date_field :rejection_completed_at, legend: { text: "Rejection Date", size: "s" } %> <%= f.govuk_select :rejection_reason, rejection_reasons, label: { text: "Rejection Reason" } %> - <%= f.govuk_text_area :rejection_details, label: { text: 'Rejection Details' }, rows: 3 %> + <%= f.govuk_text_area :comments, label: { text: 'Comments' }, rows: 3 %> <%= f.govuk_submit("Update") %> <% end %> diff --git a/app/views/system_admin/applicants/show.html.erb b/app/views/system_admin/applicants/show.html.erb index e3b8fa6e..a2efeb7f 100644 --- a/app/views/system_admin/applicants/show.html.erb +++ b/app/views/system_admin/applicants/show.html.erb @@ -58,8 +58,8 @@ row.with_value(text: @progress.rejection_reason&.humanize ) end summary_list.with_row do |row| - row.with_key(text: 'Rejection Details') - row.with_value(text: @progress.rejection_details ) + row.with_key(text: 'Comments') + row.with_value(text: @progress.comments ) end end end %> diff --git a/db/migrate/20231002115920_rename_rejection_details_to_comments.rb b/db/migrate/20231002115920_rename_rejection_details_to_comments.rb new file mode 100644 index 00000000..a0294f6e --- /dev/null +++ b/db/migrate/20231002115920_rename_rejection_details_to_comments.rb @@ -0,0 +1,5 @@ +class RenameRejectionDetailsToComments < ActiveRecord::Migration[7.0] + def change + rename_column :application_progresses, :rejection_details, :comments + end +end diff --git a/db/schema.rb b/db/schema.rb index a9313f30..a17533e8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_09_27_092305) do +ActiveRecord::Schema[7.0].define(version: 2023_10_02_115920) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "plpgsql" @@ -64,7 +64,7 @@ t.date "rejection_completed_at" t.date "payment_confirmation_completed_at" t.date "banking_approval_completed_at" - t.text "rejection_details" + t.text "comments" t.integer "status", default: 0 t.integer "rejection_reason" end diff --git a/spec/factories/application_progresses.rb b/spec/factories/application_progresses.rb index 898bc3c1..a7bf61f1 100644 --- a/spec/factories/application_progresses.rb +++ b/spec/factories/application_progresses.rb @@ -4,11 +4,11 @@ # # id :bigint not null, primary key # banking_approval_completed_at :date +# comments :text # home_office_checks_completed_at :date # initial_checks_completed_at :date # payment_confirmation_completed_at :date # rejection_completed_at :date -# rejection_details :text # rejection_reason :integer # school_checks_completed_at :date # school_investigation_required :boolean default(FALSE), not null diff --git a/spec/features/admin_console/update_progress_spec.rb b/spec/features/admin_console/update_progress_spec.rb index 009bc762..4a2b8c40 100644 --- a/spec/features/admin_console/update_progress_spec.rb +++ b/spec/features/admin_console/update_progress_spec.rb @@ -93,7 +93,7 @@ def when_i_submit_a_rejection_with_reason def when_i_submit_a_rejection_with_reason_and_details fill_in_rejection_date select "Suspected fraud", from: "application_progress[rejection_reason]" - fill_in "application_progress[rejection_details]", with: "suspected_fraud" + fill_in "application_progress[comments]", with: "I suspected fraud on this application" click_button "Update" end diff --git a/spec/models/application_progress_spec.rb b/spec/models/application_progress_spec.rb index ab5bf99e..2be435eb 100644 --- a/spec/models/application_progress_spec.rb +++ b/spec/models/application_progress_spec.rb @@ -4,11 +4,11 @@ # # id :bigint not null, primary key # banking_approval_completed_at :date +# comments :text # home_office_checks_completed_at :date # initial_checks_completed_at :date # payment_confirmation_completed_at :date # rejection_completed_at :date -# rejection_details :text # rejection_reason :integer # school_checks_completed_at :date # school_investigation_required :boolean default(FALSE), not null diff --git a/spec/models/reports/qa_report_spec.rb b/spec/models/reports/qa_report_spec.rb index 1fd0a83f..e246c71b 100644 --- a/spec/models/reports/qa_report_spec.rb +++ b/spec/models/reports/qa_report_spec.rb @@ -48,8 +48,7 @@ module Reports let(:status) { "rejected" } it "returns the data including rejection reasons in CSV format" do - application = create(:application, application_progress: build(:application_progress, rejection_completed_at: Time.zone.now, status: :rejected, rejection_reason: :request_to_re_submit, rejection_details: "Some details")) - + application = create(:application, application_progress: build(:application_progress, rejection_completed_at: Time.zone.now, status: :rejected, rejection_reason: :request_to_re_submit, comments: "Some details")) expect(report.csv).to include([ application.urn, application.applicant.full_name, @@ -66,7 +65,7 @@ module Reports application.visa_type, application.date_of_entry.strftime("%d/%m/%Y"), application.application_progress.rejection_reason&.humanize, - application.application_progress.rejection_details, + application.application_progress.comments, ].join(",")) end end From 0dbc168875e4597b6ceba722d016d2ae9a7e97aa Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 13:23:49 +0100 Subject: [PATCH 08/14] Add `Event` model This model will record events with the `Event.publish` method. --- .rubocop.yml | 6 + app/models/event.rb | 61 ++++++++ config/application.rb | 1 + config/events/filtered_attributes.yml | 14 ++ db/migrate/20230928111338_create_events.rb | 13 ++ db/schema.rb | 10 ++ spec/factories/events.rb | 156 +++++++++++++++++++++ spec/factories/urns.rb | 11 ++ spec/fixtures/urns.yml | 11 ++ spec/models/event_spec.rb | 80 +++++++++++ 10 files changed, 363 insertions(+) create mode 100644 app/models/event.rb create mode 100644 config/events/filtered_attributes.yml create mode 100644 db/migrate/20230928111338_create_events.rb create mode 100644 spec/factories/events.rb create mode 100644 spec/models/event_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 02fd138c..fae89725 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -106,3 +106,9 @@ RSpec/DescribeClass: RSpec/AnyInstance: Exclude: - 'spec/requests/submission_spec.rb' + +RSpec/MultipleMemoizedHelpers: + Exclude: + - 'spec/models/event_spec.rb' + + diff --git a/app/models/event.rb b/app/models/event.rb new file mode 100644 index 00000000..2d8a92aa --- /dev/null +++ b/app/models/event.rb @@ -0,0 +1,61 @@ +# == Schema Information +# +# Table name: events +# +# id :bigint not null, primary key +# action :string +# data :jsonb +# entity_class :string +# created_at :datetime not null +# updated_at :datetime not null +# entity_id :integer +# +class Event < ApplicationRecord + ACTIONS = [ + CREATED = "created".freeze, + UPDATED = "updated".freeze, + DELETED = "deleted".freeze, + ].freeze + + validates(:action, presence: true) + validates(:action, inclusion: { in: ACTIONS }) + validates(:entity_class, presence: true) + validates(:entity_id, presence: true) + validates(:data, presence: true, unless: :deleted?) + + class << self + def publish(action, model, model_changes = {}) + create!( + action: action&.to_s, + entity_class: model&.class, + entity_id: model&.id, + data: filtered(model, model_changes), + ) + end + + private + + def filtered(model, model_changes) + model_changes.each_with_object({}) do |(attribute, diff), hsh| + hsh[attribute] = diff + hsh[attribute] = obfuscate(diff) if filtered_attribute?(model, attribute) + + hsh + end + end + + def filtered_attribute?(model, attribute) + Rails.configuration.x.events.filtered_attributes + .fetch(model.class.name, []) + .include?(attribute) + end + + def obfuscate(diff) + Array(diff).map { |e| e.blank? ? e : "[FILTERED]" } + end + end + + def deleted? + action == DELETED + end +end diff --git a/config/application.rb b/config/application.rb index ef38132e..2948f757 100644 --- a/config/application.rb +++ b/config/application.rb @@ -48,5 +48,6 @@ class Application < Rails::Application } config.x.govuk_notify.generic_email_template_id = ENV.fetch("GOVUK_NOTIFY_GENERIC_EMAIL_TEMPLATE_ID") + config.x.events.filtered_attributes = YAML.load_file(Rails.root.join("config/events/filtered_attributes.yml")) end end diff --git a/config/events/filtered_attributes.yml b/config/events/filtered_attributes.yml new file mode 100644 index 00000000..637fc440 --- /dev/null +++ b/config/events/filtered_attributes.yml @@ -0,0 +1,14 @@ +Form: + - address_line_1 + - address_line_2 + - city + - postcode + - date_of_birth + - family_name + - given_name + - middle_name + - sex + - nationality + - passport_number + - phone_number + - email_address diff --git a/db/migrate/20230928111338_create_events.rb b/db/migrate/20230928111338_create_events.rb new file mode 100644 index 00000000..ad208168 --- /dev/null +++ b/db/migrate/20230928111338_create_events.rb @@ -0,0 +1,13 @@ +class CreateEvents < ActiveRecord::Migration[7.0] + def change + create_table :events do |t| + t.string :action + t.string :entity_class + t.integer :entity_id + t.jsonb :data + + t.timestamps + end + add_index :events, :entity_class + end +end diff --git a/db/schema.rb b/db/schema.rb index a17533e8..4d5e00a1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -108,6 +108,16 @@ t.index ["user_id", "user_type"], name: "user_index" end + create_table "events", force: :cascade do |t| + t.string "action" + t.string "entity_class" + t.integer "entity_id" + t.jsonb "data" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["entity_class"], name: "index_events_on_entity_class" + end + create_table "flipper_features", force: :cascade do |t| t.string "key", null: false t.datetime "created_at", null: false diff --git a/spec/factories/events.rb b/spec/factories/events.rb new file mode 100644 index 00000000..dae03266 --- /dev/null +++ b/spec/factories/events.rb @@ -0,0 +1,156 @@ +# == Schema Information +# +# Table name: events +# +# id :bigint not null, primary key +# action :string +# data :jsonb +# entity_class :string +# created_at :datetime not null +# updated_at :datetime not null +# entity_id :integer +# +FactoryBot.define do + factory :event do + action { Event::ACTIONS.sample } + entity_class { "Form" } + entity_id { Faker::Number.number(digits: 3) } + data do + { + "id" => [nil, entity_id], + "application_route" => [nil, "teacher"], + "updated_at" => ["2023-09-29T14:40:22.857+01:00", "2023-09-29T14:40:43.388+01:00"], + } + end + + factory :form_submitted_event do + action { "deleted" } + data { {} } + end + + factory :form_updated do + action { "updated" } + + factory :form_school_details_event do + data do + { + "state_funded_secondary_school" => [nil, true], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :ineligible_form_school_details_event do + data do + { + "state_funded_secondary_school" => [nil, false], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :form_application_route_event do + data do + { + "id" => [nil, entity_id], + "application_route" => [nil, "teacher"], + "updated_at" => [nil, Time.zone.now], + "created_at" => [nil, Time.zone.now], + } + end + end + + factory :ineligible_form_application_route_event do + data do + { + "id" => [nil, entity_id], + "application_route" => [nil, "other"], + "updated_at" => [nil, Time.zone.now], + "created_at" => [nil, Time.zone.now], + } + end + end + + factory :form_start_date_event do + data do + { + "start_date" => [nil, Time.zone.today], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :ineligible_form_start_date_event do + data do + { + "start_date" => [nil, 1.year.ago.to_date], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :form_start_date_four_months_ago_event do + data do + { + "start_date" => [nil, 4.months.ago.to_date], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :form_date_of_entry_event do + data do + { + "date_of_entry" => [nil, Time.zone.today], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :ineligible_form_date_of_entry_event do + data do + { + "date_of_entry" => [nil, 1.year.ago.to_date], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :form_employment_details_event do + data do + { + "school_city" => [nil, "sth"], + "school_name" => [nil, "sths"], + "school_postcode" => [nil, "EE3 3AA"], + "school_address_line_1" => [nil, "stnh"], + "school_address_line_2" => [nil, ""], + "school_headteacher_name" => [nil, "[FILTERED]"], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + + factory :form_personal_details_event do + data do + { + "sex" => [nil, "[FILTERED]"], + "city" => [nil, "[FILTERED]"], + "postcode" => [nil, "[FILTERED]"], + "given_name" => [nil, "[FILTERED]"], + "family_name" => [nil, "[FILTERED]"], + "middle_name" => [nil, "[FILTERED]"], + "nationality" => [nil, "[FILTERED]"], + "phone_number" => [nil, "[FILTERED]"], + "student_loan" => [nil, true], + "date_of_birth" => [nil, "[FILTERED]"], + "email_address" => [nil, "[FILTERED]"], + "address_line_1" => [nil, "[FILTERED]"], + "address_line_2" => [nil, "[FILTERED]"], + "passport_number" => [nil, "[FILTERED]"], + "updated_at" => [Time.zone.now, Time.zone.now], + } + end + end + end + end +end diff --git a/spec/factories/urns.rb b/spec/factories/urns.rb index 8112eadb..5ecebff0 100644 --- a/spec/factories/urns.rb +++ b/spec/factories/urns.rb @@ -1,3 +1,14 @@ +# == Schema Information +# +# Table name: urns +# +# id :bigint not null, primary key +# code :string +# prefix :string +# suffix :integer +# created_at :datetime not null +# updated_at :datetime not null +# FactoryBot.define do factory :urn do prefix { "IRP" } diff --git a/spec/fixtures/urns.yml b/spec/fixtures/urns.yml index 4cedf718..e2cfb088 100644 --- a/spec/fixtures/urns.yml +++ b/spec/fixtures/urns.yml @@ -1,3 +1,14 @@ +# == Schema Information +# +# Table name: urns +# +# id :bigint not null, primary key +# code :string +# prefix :string +# suffix :integer +# created_at :datetime not null +# updated_at :datetime not null +# te_one: suffix: 5668 prefix: IRP diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb new file mode 100644 index 00000000..7805dcc7 --- /dev/null +++ b/spec/models/event_spec.rb @@ -0,0 +1,80 @@ +# == Schema Information +# +# Table name: events +# +# id :bigint not null, primary key +# action :string +# data :jsonb +# entity_class :string +# created_at :datetime not null +# updated_at :datetime not null +# entity_id :integer +# +require "rails_helper" + +RSpec.describe Event do + subject(:event) do + described_class.new( + action: action, + entity_class: form.class, + entity_id: form.id, + data: data, + ) + end + + let(:form) { create(:form, given_name: "Ruth") } + let(:action) { :created } + let(:data) { form.saved_changes } + + describe "publish" do + subject(:published_event) { described_class.publish(action, form, data) } + + it { expect(published_event).to be_instance_of(described_class) } + + context "when filtered attributes defined for model event data show '[FILTERED]'" do + let(:expected_config) do + { + "Form" => %w[ + address_line_1 + address_line_2 + city + postcode + date_of_birth + family_name + given_name + middle_name + sex + nationality + passport_number + phone_number + email_address + ], + } + end + let(:event_config) { Rails.configuration.x.events.filtered_attributes } + let(:event_form_given_name) { published_event.data.fetch("given_name") } + + it { expect(event_config).to eq(expected_config) } + it { expect(event_form_given_name).to contain_exactly(nil, "[FILTERED]") } + end + end + + describe "validations" do + context "presence" do + it { expect(event).to validate_presence_of(:action) } + it { expect(event).to validate_presence_of(:entity_class) } + it { expect(event).to validate_presence_of(:entity_id) } + it { expect(event).to validate_presence_of(:data) } + end + + context "when deleted action presence not required" do + let(:action) { :deleted } + + it { expect(event).not_to validate_presence_of(:data) } + end + + context "inclusion" do + it { expect(event).to validate_inclusion_of(:action).in_array(described_class::ACTIONS) } + end + end +end From 8e12f5dfff62c9fe6038447b1a912d66d7df113a Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 13:41:25 +0100 Subject: [PATCH 09/14] Update UpdateForm and SubmitForm services These service will now send some events about the forms --- app/services/submit_form.rb | 1 + app/services/update_form.rb | 9 +++++++++ spec/services/submit_form_spec.rb | 5 ++++- spec/services/update_form_spec.rb | 33 ++++++++++++++++++++++++++++++- 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/app/services/submit_form.rb b/app/services/submit_form.rb index 23270446..a6ccb4d5 100644 --- a/app/services/submit_form.rb +++ b/app/services/submit_form.rb @@ -95,6 +95,7 @@ def create_application def delete_form form.destroy! + Event.publish(:deleted, form) end def send_applicant_email diff --git a/app/services/update_form.rb b/app/services/update_form.rb index 139c6b45..a17e55ef 100644 --- a/app/services/update_form.rb +++ b/app/services/update_form.rb @@ -4,6 +4,7 @@ def self.call(...) return service unless service.valid? service.update_form! + service.capture_form_analytics service end @@ -27,6 +28,14 @@ def update_form! form.update(**parsed_params) end + def capture_form_analytics + changes = form.saved_changes + action = :updated + action = :created if changes.key?(:id) + + Event.publish(action, form, changes) if changes.present? + end + private def parsed_params diff --git a/spec/services/submit_form_spec.rb b/spec/services/submit_form_spec.rb index 022582e0..c7faac40 100644 --- a/spec/services/submit_form_spec.rb +++ b/spec/services/submit_form_spec.rb @@ -32,7 +32,10 @@ end context "returns true when form is submitted" do - before { service.submit_form! } + before do + form.save + service.submit_form! + end it { expect(service).to be_success } end diff --git a/spec/services/update_form_spec.rb b/spec/services/update_form_spec.rb index 9009f118..15ff3866 100644 --- a/spec/services/update_form_spec.rb +++ b/spec/services/update_form_spec.rb @@ -3,7 +3,7 @@ RSpec.describe UpdateForm do subject(:update_form) { described_class.new(step, params) } - let(:form) { build(:form) } + let(:form) { build(:trainee_form) } let(:step) { ApplicationRouteStep.new(form) } let(:params) { { "application_route" => application_route } } let(:application_route) { "teacher" } @@ -40,4 +40,35 @@ update_form.update_form! end end + + describe "capture_form_analytics" do + it "when the form has not changes not event is triggered" do + expect(Event).not_to receive(:publish) + + update_form.capture_form_analytics + end + + context "when the form has changes" do + before { form.save } + + it "a created event is triggered" do + expect(Event).to receive(:publish) + .with( + :created, + form, + hash_including( + "application_route" => [nil, "salaried_trainee"], + "id" => [nil, form.id], + ), + ) + update_form.capture_form_analytics + end + + it "a updated event is triggered" do + form.update(given_name: "foo") + expect(Event).to receive(:publish).with(:updated, form, hash_including("given_name" => [nil, "foo"])) + update_form.capture_form_analytics + end + end + end end From fcaa796040476a545b98d760c51ae92fdea55e21 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 13:43:16 +0100 Subject: [PATCH 10/14] Delete session when reaching an ineligible endpoint --- app/controllers/pages_controller.rb | 8 ++++++++ spec/requests/step_spec.rb | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index abc00e33..d8695425 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -5,4 +5,12 @@ class PagesController < ApplicationController def index; end def closed; end def sitemap; end + + def ineligible + session.delete("form_id") + end + + def ineligible_salaried_course + session.delete("form_id") + end end diff --git a/spec/requests/step_spec.rb b/spec/requests/step_spec.rb index b5e87d46..1e774d21 100644 --- a/spec/requests/step_spec.rb +++ b/spec/requests/step_spec.rb @@ -15,6 +15,13 @@ trainee-details visa ].each do |name| + before do + post( + "/step/application-route", + params: { application_route_step: { application_route: "teacher" } }, + ) + end + it "returns the #{name} page" do get "/step/#{name}" expect(response).to have_http_status(:ok) From 2dda0c68597b3e39a210a5e03403012bb9000173 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 13:44:37 +0100 Subject: [PATCH 11/14] Avoid jumping in the middle of an application form flow --- app/services/step_flow.rb | 1 + spec/services/step_flow_spec.rb | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/app/services/step_flow.rb b/app/services/step_flow.rb index 0dbf9190..f3ac1139 100644 --- a/app/services/step_flow.rb +++ b/app/services/step_flow.rb @@ -30,6 +30,7 @@ def matches?(request) end def current_step(form, requested_step_name) + return if form.blank? && requested_step_name != ApplicationRouteStep::ROUTE_KEY return ApplicationRouteStep.new(Form.new) unless form STEPS.fetch(requested_step_name).new(form) diff --git a/spec/services/step_flow_spec.rb b/spec/services/step_flow_spec.rb index ae872d7d..d660b320 100644 --- a/spec/services/step_flow_spec.rb +++ b/spec/services/step_flow_spec.rb @@ -82,6 +82,13 @@ let(:form) { nil } let(:step_name) { "personal-details" } + it { is_expected.to be_nil } + end + + context "when form is nil and requested step is application-route" do + let(:form) { nil } + let(:step_name) { "application-route" } + it { is_expected.to be_instance_of(ApplicationRouteStep) } end From 4fa6a72fe57240fd5edf5156c6756707eed05586 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 13:45:47 +0100 Subject: [PATCH 12/14] Add FormsFunnelQuery This will generate the data used for the eligibel and ineligible forms funnels in the system admin dashboard --- app/queries/forms_funnel_query.rb | 131 ++++++++++++++++ config/brakeman.ignore | 190 ++++++++++++++++++++++++ spec/queries/forms_funnel_query_spec.rb | 86 +++++++++++ 3 files changed, 407 insertions(+) create mode 100644 app/queries/forms_funnel_query.rb create mode 100644 config/brakeman.ignore create mode 100644 spec/queries/forms_funnel_query_spec.rb diff --git a/app/queries/forms_funnel_query.rb b/app/queries/forms_funnel_query.rb new file mode 100644 index 00000000..cd52c211 --- /dev/null +++ b/app/queries/forms_funnel_query.rb @@ -0,0 +1,131 @@ +class FormsFunnelQuery + def self.call(...) + new(...).execute + end + + def initialize(date_range: 24.hours.ago..Time.current) + @date_range = date_range + @entity_class = "Form" + @base_query = Event + .select(:entity_id) + .distinct + .where(entity_class: entity_class, created_at: date_range) + end + + attr_reader :date_range, :entity_class, :base_query + + def execute + data = StepFlow::STEPS.each_with_object({}) do |(route_key, step), hsh| + hsh[route_key] = query_dispatch(step) + hsh + end + data["submission"] = submission_query + + data + end + + def application_route_step_query(required_field) + other_query(required_field) + end + + def school_details_step_query(required_field) + boolean_query(required_field) + end + + def trainee_details_step_query(required_field) + boolean_query(required_field) + end + + def contract_details_step_query(required_field) + boolean_query(required_field) + end + + def start_date_step_query(required_field) + grouping = extract_dates(required_field) + .group_by do |(_id, start_date)| + Form::EligibilityCheck.new(Form.new).contract_start_date_eligible?(start_date.to_date) + end + + count_grouping(grouping) + end + + def subject_step_query(required_field) + other_query(required_field) + end + + def visa_step_query(required_field) + other_query(required_field) + end + + def entry_date_step_query(required_field) + date_of_entries = extract_dates(required_field) + start_dates = extract_dates("start_date") + + form_dates = date_of_entries.each_with_object([]) do |(id, date_of_entry), list| + entry = start_dates.detect { |(sid, _)| sid == id } + list << [date_of_entry.to_date, entry.last.to_date] + list + end + + grouping = form_dates.group_by do |(date_of_entry, start_date)| + Form::EligibilityCheck.new(Form.new).date_of_entry_eligible?(date_of_entry, start_date) + end + + count_grouping(grouping) + end + + def personal_details_step_query(required_field) + submitted_field_query(required_field) + end + + def employment_details_step_query(required_field) + submitted_field_query(required_field) + end + + def submission_query + { eligible: base_query.where(action: :deleted).count } + end + +private + + def count_grouping(grouping) + { eligible: grouping.fetch(true, []).count, ineligible: grouping.fetch(false, []).count } + end + + def extract_dates(field) + base_query + .where("data->'#{field}' IS NOT NULL") + .pluck(:entity_id, Arel.sql("data->'#{field}'->1")) + end + + def submitted_field_query(field) + eligible = base_query.where("data->'#{field}' IS NOT NULL").count + + { eligible: } + end + + def boolean_query(field) + ineligible = base_query.where("data->'#{field}' @> ?", "[false]").count + eligible = base_query.where("data->'#{field}' @> ?", "[true]").count + + { eligible:, ineligible: } + end + + def other_query(field) + total = base_query.where("data->'#{field}' IS NOT NULL").count + ineligible = base_query.where("data->'#{field}' @> ?", '["other"]') + .or(base_query.where("data->'#{field}' @> ?", '["Other"]')) + .count + eligible = total - ineligible + + { eligible:, ineligible: } + end + + def query_dispatch(step) + method_name = "#{step.name.underscore}_query".to_sym + required_field = step::REQUIRED_FIELDS.first + return public_send(method_name, required_field) if respond_to?(method_name) + + raise(NoMethodError, "You must define query method #{method_name}") + end +end diff --git a/config/brakeman.ignore b/config/brakeman.ignore new file mode 100644 index 00000000..b705f888 --- /dev/null +++ b/config/brakeman.ignore @@ -0,0 +1,190 @@ +{ + "ignored_warnings": [ + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "11ae20225165e9dc40bab1c95be13ca9ef9c4abd4d679f115a485d1108773615", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 102, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "base_query.where(\"data->'#{field}' IS NOT NULL\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "submitted_field_query" + }, + "user_input": "field", + "confidence": "Weak", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the method is provided by the Step class required_fields." + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "1c01eb4e2a6db1a525163bdfe3a4c6b54d05d4718ce043e8c905c83eb11290eb", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 109, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "base_query.where(\"data->'#{field}' @> ?\", \"[true]\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "boolean_query" + }, + "user_input": "field", + "confidence": "Weak", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the method is provided by the Step class required_fields." + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "2a886d32b08f7c555636327c5130a0d091103cc1a95d151531c6df62d47db647", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 116, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "base_query.where(\"data->'#{field}' @> ?\", \"[\\\"other\\\"]\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "other_query" + }, + "user_input": "field", + "confidence": "Weak", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the method is provided by the Step class required_fields." + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "552ceb7070f1b47b878b61ba6e12330df5928ca6ccae0b91d2a0bbe5c9516bec", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 115, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "base_query.where(\"data->'#{field}' IS NOT NULL\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "other_query" + }, + "user_input": "field", + "confidence": "Weak", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the method is provided by the Step class required_fields." + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "77111270d3f370d388ca700d58b3985b87e71b5716b56f526154b89d27b9f3e8", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 98, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "Arel.sql(\"data->'#{field}'->1\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "extract_dates" + }, + "user_input": "field", + "confidence": "Medium", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the method is provided by the Step class required_fields." + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "9fe4738a94be085ce81e41086b23e0fa35300a79267909d7c3f6a6083346219c", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 97, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "base_query.where(\"data->'#{field}' IS NOT NULL\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "extract_dates" + }, + "user_input": "field", + "confidence": "Weak", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the extract_dates method is provided by the Step class." + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "b205b9967732ba2d64494d600f884760570c0f38069b950bf4a82a2c030b8505", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 117, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "base_query.where(\"data->'#{field}' @> ?\", \"[\\\"Other\\\"]\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "other_query" + }, + "user_input": "field", + "confidence": "Weak", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the method is provided by the Step class required_fields." + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "e4843f22abc60f8f48a6b7b9997b49c45e69885c59f93837d5bd2a8138029d94", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/queries/forms_funnel_query.rb", + "line": 108, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "base_query.where(\"data->'#{field}' @> ?\", \"[false]\")", + "render_path": null, + "location": { + "type": "method", + "class": "FormsFunnelQuery", + "method": "boolean_query" + }, + "user_input": "field", + "confidence": "Weak", + "cwe_id": [ + 89 + ], + "note": "This is a false positive because the field argument in the method is provided by the Step class required_fields." + } + ], + "updated": "2023-10-02 14:11:36 +0100", + "brakeman_version": "6.0.1" +} diff --git a/spec/queries/forms_funnel_query_spec.rb b/spec/queries/forms_funnel_query_spec.rb new file mode 100644 index 00000000..549be5f7 --- /dev/null +++ b/spec/queries/forms_funnel_query_spec.rb @@ -0,0 +1,86 @@ +require "rails_helper" + +RSpec.describe FormsFunnelQuery, type: :service do + subject(:forms_funnel) { described_class.new(date_range:) } + + let(:date_range) { 1.day.ago..Time.current } + + describe ".submission_query" do + before do + create_list(:form_submitted_event, 2) + end + + let(:expected) { { eligible: 2 } } + + it { expect(forms_funnel.submission_query).to eq(expected) } + end + + describe ".employment_details_step_query" do + before do + create_list(:form_employment_details_event, 3) + end + + let(:expected) { { eligible: 3 } } + + it { expect(forms_funnel.employment_details_step_query("school_name")).to eq(expected) } + end + + describe ".personal_details_step_query" do + before do + create_list(:form_personal_details_event, 4) + end + + let(:expected) { { eligible: 4 } } + + it { expect(forms_funnel.personal_details_step_query("given_name")).to eq(expected) } + end + + describe ".entry_date_step_query" do + before do + old_start_date = create(:form_start_date_four_months_ago_event) + create(:ineligible_form_date_of_entry_event, entity_id: old_start_date.entity_id) + start_dates = create_list(:form_start_date_event, 6) + build_list(:form_date_of_entry_event, 5) do |event, i| + event.entity_id = start_dates[i].entity_id + event.save + end + end + + let(:expected) { { eligible: 5, ineligible: 1 } } + + it { expect(forms_funnel.entry_date_step_query("date_of_entry")).to eq(expected) } + end + + describe ".start_date_step_query" do + before do + create_list(:form_start_date_event, 6) + create(:ineligible_form_start_date_event) + end + + let(:expected) { { eligible: 6, ineligible: 1 } } + + it { expect(forms_funnel.start_date_step_query("start_date")).to eq(expected) } + end + + describe ".application_route_step_query" do + before do + create_list(:form_application_route_event, 6) + create(:ineligible_form_application_route_event) + end + + let(:expected) { { eligible: 6, ineligible: 1 } } + + it { expect(forms_funnel.application_route_step_query("application_route")).to eq(expected) } + end + + describe ".school_details_step_query" do + before do + create_list(:form_school_details_event, 3) + create(:ineligible_form_school_details_event) + end + + let(:expected) { { eligible: 3, ineligible: 1 } } + + it { expect(forms_funnel.school_details_step_query("state_funded_secondary_school")).to eq(expected) } + end +end From 589865ba689f79c8cdf2c0f54b845bbd15b75850 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Mon, 2 Oct 2023 13:47:13 +0100 Subject: [PATCH 13/14] Add Forms funnel and Ineligible forms funnel These widget have the ability to change the date_range of the formsfunnelquery but defaults to the last 24 hours. The date range filter feature is soft launched at the moment and is only usable via the url `/system-admin/dashboard?date_range[unit]=hours&date_range[range_start]=48&date_range[range_end]=24` --- .../system_admin/dashboard_controller.rb | 15 +++- app/models/kpis.rb | 57 +++++++++++++- .../system_admin/dashboard/show.html.erb | 25 ++++++ spec/models/kpis_spec.rb | 76 ++++++++++++++----- 4 files changed, 153 insertions(+), 20 deletions(-) diff --git a/app/controllers/system_admin/dashboard_controller.rb b/app/controllers/system_admin/dashboard_controller.rb index 7714bf3b..97aefbb1 100644 --- a/app/controllers/system_admin/dashboard_controller.rb +++ b/app/controllers/system_admin/dashboard_controller.rb @@ -1,7 +1,20 @@ module SystemAdmin class DashboardController < AdminController def show - @kpis = Kpis.new + @kpis = Kpis.new(**kpi_params) + rescue ArgumentError => e + flash[:alert] = e.message + redirect_to(dashboard_path) + end + + private + + def kpi_params + params + .fetch(:date_range, {}) + .permit(:unit, :range_start, :range_end) + .to_hash + .symbolize_keys end end end diff --git a/app/models/kpis.rb b/app/models/kpis.rb index 227a536b..87307493 100644 --- a/app/models/kpis.rb +++ b/app/models/kpis.rb @@ -1,8 +1,11 @@ class Kpis - def initialize - @applications = Applicant.all + def initialize(unit: "hours", range_start: "24", range_end: "0") + @date_range_params = parse_date_range(unit:, range_start:, range_end:) + @date_range = to_date_range(**@date_range_params) end + attr_reader :date_range, :date_range_params + def total_applications Application.count end @@ -66,4 +69,54 @@ def time_to_payment_confirmation def status_breakdown StatusBreakdownQuery.call end + + def forms_funnel + forms_funnel_query + end + + def ineligible_forms_funnel + forms_funnel_query.select { |_, hsh| hsh.fetch(:ineligible, nil) } + end + + def funnel_date_range_title + return funnel_title_last_n if date_range_params.fetch(:range_end).zero? + + [ + "between", + date_range_params.fetch(:range_start), + "and", + date_range_params.fetch(:range_end), + date_range_params.fetch(:unit), + "ago", + ].join(" ") + end + +private + + def funnel_title_last_n + ["last", date_range_params.fetch(:range_start), date_range_params.fetch(:unit)].join(" ") + end + + def parse_date_range(unit:, range_start:, range_end:) + raise(ArgumentError, "invalid unit value, must be hours or days") unless %w[hours days].include?(unit) + + range_end = range_end.to_i + range_start = range_start.to_i + + raise(ArgumentError, "range_end and range_start must be positive numbers") if range_start.negative? || range_end.negative? + raise(ArgumentError, "range_end must be lower than range_start") if range_end >= range_start + + { unit:, range_start:, range_end: } + end + + def to_date_range(unit:, range_start:, range_end:) + Range.new( + range_start.public_send(unit).ago, + range_end.public_send(unit).ago, + ) + end + + def forms_funnel_query + @forms_funnel_query ||= FormsFunnelQuery.call(date_range:) + end end diff --git a/app/views/system_admin/dashboard/show.html.erb b/app/views/system_admin/dashboard/show.html.erb index f2f37d87..06035df5 100644 --- a/app/views/system_admin/dashboard/show.html.erb +++ b/app/views/system_admin/dashboard/show.html.erb @@ -153,5 +153,30 @@ +
+

Forms funnel

+
<%= @kpis.funnel_date_range_title %>
+ + <% @kpis.forms_funnel.each do |path,v|%> + + + + + <% end %> +
<%= path %><%= v[:eligible] %>
+
+ +
+

Ineligible forms funnel

+
<%= @kpis.funnel_date_range_title %>
+ + <% @kpis.ineligible_forms_funnel.each do |path,v|%> + + + + + <% end %> +
<%= path %><%= v[:ineligible] %>
+
diff --git a/spec/models/kpis_spec.rb b/spec/models/kpis_spec.rb index c1ed016a..58abc3ed 100644 --- a/spec/models/kpis_spec.rb +++ b/spec/models/kpis_spec.rb @@ -1,35 +1,77 @@ require "rails_helper" RSpec.describe Kpis do - describe "#total_applications" do - it "returns the total number of applications" do - create_list(:application, 5) + subject(:kpis) { described_class.new(unit:, range_start:, range_end:) } + + let(:unit) { "hours" } + let(:range_start) { "12" } + let(:range_end) { "0" } - kpis = described_class.new + let(:application) { create(:application) } - expect(kpis.total_applications).to eq(5) + describe "argument error on initialize" do + context "when unknown unit" do + let(:unit) { "bad" } + + it { expect { kpis }.to raise_error(ArgumentError) } end - end - describe "#total_rejections" do - it "returns the total number of applications rejected" do - application = create(:application) - create_list(:application_progress, 5, :rejection_completed, application:) + context "when bad range_start and range_end" do + let(:range_start) { "bad" } + let(:range_end) { "bad" } + + it { expect { kpis }.to raise_error(ArgumentError) } + end + + context "when range_start negative" do + let(:range_start) { "-15" } + + it { expect { kpis }.to raise_error(ArgumentError) } + end + + context "when range_end negative" do + let(:range_end) { "-15" } + + it { expect { kpis }.to raise_error(ArgumentError) } + end - stats = described_class.new + context "when range_end value greater than range_start" do + let(:range_start) { "15" } + let(:range_end) { "24" } - expect(stats.total_rejections).to eq(5) + it { expect { kpis }.to raise_error(ArgumentError) } end end + describe "#total_applications" do + before { create_list(:application, 5) } + + it { expect(kpis.total_applications).to eq(5) } + end + + describe "#total_rejections" do + before { create_list(:application_progress, 5, :rejection_completed, application:) } + + it { expect(kpis.total_rejections).to eq(5) } + end + describe "#total_paid" do - it "returns the total number of applications paid" do - application = create(:application) - create_list(:application_progress, 5, :banking_approval_completed, application:) + before { create_list(:application_progress, 5, :banking_approval_completed, application:) } + + it { expect(kpis.total_paid).to eq(5) } + end + + describe "funnel_date_range_title" do + context "when range_end is set to 0" do + let(:range_end) { 0 } + + it { expect(kpis.funnel_date_range_title).to eq("last 12 hours") } + end - stats = described_class.new + context "when range_end is not 0" do + let(:range_end) { 6 } - expect(stats.total_paid).to eq(5) + it { expect(kpis.funnel_date_range_title).to eq("between 12 and 6 hours ago") } end end end From 87025bc76c452b695efe9b0d3701c256e8923242 Mon Sep 17 00:00:00 2001 From: fumimowdan Date: Thu, 5 Oct 2023 15:31:36 +0100 Subject: [PATCH 14/14] Add feature flag to disploy form analytics --- app/views/system_admin/dashboard/show.html.erb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/views/system_admin/dashboard/show.html.erb b/app/views/system_admin/dashboard/show.html.erb index 06035df5..20ab633e 100644 --- a/app/views/system_admin/dashboard/show.html.erb +++ b/app/views/system_admin/dashboard/show.html.erb @@ -153,6 +153,7 @@ + <% if Flipper.enabled? :display_form_analytics, current_user %>

Forms funnel

<%= @kpis.funnel_date_range_title %>
@@ -178,5 +179,6 @@ <% end %>
+ <% end %>