From 12da80ebd7394ebb17b96fa6c3c0b44873851e77 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Wed, 6 Sep 2023 09:57:36 +0100 Subject: [PATCH 1/8] Add action_required_by column This will be used to capture whether an application form requires some action, and who needs to perform it. --- app/models/application_form.rb | 10 ++ config/analytics.yml | 91 ++++++++++--------- ...action_required_by_to_application_forms.rb | 10 ++ db/schema.rb | 2 + spec/factories/application_forms.rb | 2 + spec/models/application_form_spec.rb | 12 +++ 6 files changed, 82 insertions(+), 45 deletions(-) create mode 100644 db/migrate/20230906085009_add_action_required_by_to_application_forms.rb diff --git a/app/models/application_form.rb b/app/models/application_form.rb index 96aecc50df..09f1bcbc43 100644 --- a/app/models/application_form.rb +++ b/app/models/application_form.rb @@ -3,6 +3,7 @@ # Table name: application_forms # # id :bigint not null, primary key +# action_required_by :string default("none"), not null # age_range_max :integer # age_range_min :integer # age_range_status :string default("not_started"), not null @@ -67,6 +68,7 @@ # # Indexes # +# index_application_forms_on_action_required_by (action_required_by) # index_application_forms_on_assessor_id (assessor_id) # index_application_forms_on_english_language_provider_id (english_language_provider_id) # index_application_forms_on_family_name (family_name) @@ -122,6 +124,14 @@ class ApplicationForm < ApplicationRecord absence: true, if: :english_language_provider_other + enum action_required_by: { + admin: "admin", + assessor: "assessor", + external: "external", + none: "none", + }, + _prefix: true + enum status: { draft: "draft", submitted: "submitted", diff --git a/config/analytics.yml b/config/analytics.yml index cb6645e4cd..e2f3696261 100644 --- a/config/analytics.yml +++ b/config/analytics.yml @@ -22,68 +22,69 @@ - blob_id - created_at :application_forms: + - action_required_by + - age_range_max + - age_range_min + - age_range_status + - alternative_family_name + - alternative_given_names + - assessor_id + - awarded_at + - confirmed_no_sanctions + - created_at + - date_of_birth + - declined_at + - dqt_match + - english_language_citizenship_exempt + - english_language_proof_method + - english_language_provider_id + - english_language_provider_other + - english_language_provider_reference + - english_language_qualification_exempt + - english_language_status + - family_name + - given_names + - has_alternative_name + - has_work_history - id - - reference - - status + - identification_document_status + - needs_registration_number + - needs_work_history + - needs_written_statement - overdue_further_information - overdue_professional_standing - overdue_qualification - overdue_reference + - personal_information_status + - qualifications_status - received_further_information - received_professional_standing - received_qualification - received_reference - - waiting_on_further_information - - waiting_on_professional_standing - - waiting_on_qualification - - waiting_on_reference - - personal_information_status - - identification_document_status - - qualifications_status - - age_range_status - - subjects_status - - work_history_status - - registration_number_status - - written_statement_status - - teacher_id - - created_at - - updated_at - - given_names - - family_name - - date_of_birth - - age_range_min - - age_range_max - - has_alternative_name - - alternative_given_names - - alternative_family_name + - reduced_evidence_accepted + - reference - region_id - registration_number - - has_work_history - - subjects - - assessor_id + - registration_number_status + - requires_preliminary_check - reviewer_id + - status + - subjects + - subjects_status - submitted_at - - awarded_at - - declined_at + - teacher_id + - teaching_authority_provides_written_statement + - updated_at + - waiting_on_further_information + - waiting_on_professional_standing + - waiting_on_qualification + - waiting_on_reference - withdrawn_at + - work_history_status - working_days_since_submission - - needs_work_history - - needs_written_statement - - needs_registration_number - - teaching_authority_provides_written_statement - written_statement_confirmation - written_statement_optional - - confirmed_no_sanctions - - english_language_status - - english_language_citizenship_exempt - - english_language_qualification_exempt - - english_language_proof_method - - english_language_provider_id - - english_language_provider_other - - english_language_provider_reference - - reduced_evidence_accepted - - requires_preliminary_check - - dqt_match + - written_statement_status :assessment_sections: - id - assessment_id diff --git a/db/migrate/20230906085009_add_action_required_by_to_application_forms.rb b/db/migrate/20230906085009_add_action_required_by_to_application_forms.rb new file mode 100644 index 0000000000..529769b1fa --- /dev/null +++ b/db/migrate/20230906085009_add_action_required_by_to_application_forms.rb @@ -0,0 +1,10 @@ +class AddActionRequiredByToApplicationForms < ActiveRecord::Migration[7.0] + def change + add_column :application_forms, + :action_required_by, + :string, + default: "none", + null: false + add_index :application_forms, :action_required_by + end +end diff --git a/db/schema.rb b/db/schema.rb index 36c0ea9369..3d2ef1fd3e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -105,6 +105,8 @@ t.boolean "overdue_reference", default: false, null: false t.jsonb "dqt_match", default: {} t.datetime "withdrawn_at" + t.string "action_required_by", default: "none", null: false + t.index ["action_required_by"], name: "index_application_forms_on_action_required_by" t.index ["assessor_id"], name: "index_application_forms_on_assessor_id" t.index ["english_language_provider_id"], name: "index_application_forms_on_english_language_provider_id" t.index ["family_name"], name: "index_application_forms_on_family_name" diff --git a/spec/factories/application_forms.rb b/spec/factories/application_forms.rb index 81660d7f1d..bd063b475c 100644 --- a/spec/factories/application_forms.rb +++ b/spec/factories/application_forms.rb @@ -3,6 +3,7 @@ # Table name: application_forms # # id :bigint not null, primary key +# action_required_by :string default("none"), not null # age_range_max :integer # age_range_min :integer # age_range_status :string default("not_started"), not null @@ -67,6 +68,7 @@ # # Indexes # +# index_application_forms_on_action_required_by (action_required_by) # index_application_forms_on_assessor_id (assessor_id) # index_application_forms_on_english_language_provider_id (english_language_provider_id) # index_application_forms_on_family_name (family_name) diff --git a/spec/models/application_form_spec.rb b/spec/models/application_form_spec.rb index 6f4a626fd5..d1b6c51b36 100644 --- a/spec/models/application_form_spec.rb +++ b/spec/models/application_form_spec.rb @@ -3,6 +3,7 @@ # Table name: application_forms # # id :bigint not null, primary key +# action_required_by :string default("none"), not null # age_range_max :integer # age_range_min :integer # age_range_status :string default("not_started"), not null @@ -67,6 +68,7 @@ # # Indexes # +# index_application_forms_on_action_required_by (action_required_by) # index_application_forms_on_assessor_id (assessor_id) # index_application_forms_on_english_language_provider_id (english_language_provider_id) # index_application_forms_on_family_name (family_name) @@ -101,6 +103,16 @@ describe "columns" do it do + is_expected.to define_enum_for(:action_required_by) + .with_values( + admin: "admin", + assessor: "assessor", + external: "external", + none: "none", + ) + .with_prefix + .backed_by_column_of_type(:string) + is_expected.to define_enum_for(:status).with_values( draft: "draft", submitted: "submitted", From 4f8b7dd5bd24529e79438fc774970e4c8cd61474 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Fri, 8 Sep 2023 13:19:54 +0100 Subject: [PATCH 2/8] Add action_required_by traits This adds some traits to the application form factory to make it easier to write tests using the action_required_by field. --- spec/factories/application_forms.rb | 32 ++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/spec/factories/application_forms.rb b/spec/factories/application_forms.rb index bd063b475c..31f30f0b19 100644 --- a/spec/factories/application_forms.rb +++ b/spec/factories/application_forms.rb @@ -124,7 +124,24 @@ written_statement_status { "completed" } end + trait :action_required_by_admin do + action_required_by { "admin" } + end + + trait :action_required_by_assessor do + action_required_by { "assessor" } + end + + trait :action_required_by_external do + action_required_by { "external" } + end + + trait :action_required_by_none do + action_required_by { "none" } + end + trait :submitted do + action_required_by_assessor status { "submitted" } submitted_at { Time.zone.now } working_days_since_submission { 0 } @@ -143,6 +160,7 @@ trait :preliminary_check do submitted + action_required_by_admin requires_preliminary_check { true } status { "preliminary_check" } end @@ -154,6 +172,7 @@ trait :waiting_on do submitted + action_required_by_external status { "waiting_on" } end @@ -172,25 +191,28 @@ status { "awarded_pending_checks" } end + trait :potential_duplicate_in_dqt do + submitted + status { "potential_duplicate_in_dqt" } + end + trait :awarded do submitted + action_required_by_none status { "awarded" } awarded_at { Time.zone.now } end trait :declined do submitted + action_required_by_none status { "declined" } declined_at { Time.zone.now } end - trait :potential_duplicate_in_dqt do - submitted - status { "potential_duplicate_in_dqt" } - end - trait :withdrawn do submitted + action_required_by_none status { "withdrawn" } withdrawn_at { Time.zone.now } end From d344f03773252bdffdcc09adf2281042397c1d32 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 7 Sep 2023 16:17:00 +0100 Subject: [PATCH 3/8] Add action_required_by_changed timeline events This adds a new type of timeline event which captures whenever the "action required by" of an application has changed. --- app/models/timeline_event.rb | 36 +++++++++++++++++------------- spec/models/timeline_event_spec.rb | 24 ++++++++++++++++++++ 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/app/models/timeline_event.rb b/app/models/timeline_event.rb index 1eb1fa7e67..c41481fde2 100644 --- a/app/models/timeline_event.rb +++ b/app/models/timeline_event.rb @@ -54,14 +54,16 @@ # class TimelineEvent < ApplicationRecord belongs_to :application_form + belongs_to :assessment, optional: true + belongs_to :assessment_section, optional: true + belongs_to :assignee, class_name: "Staff", optional: true belongs_to :creator, polymorphic: true, optional: true - - validates :creator, presence: true, unless: -> { creator_name.present? } - validates :creator_name, - presence: true, - unless: -> { creator_id.present? && creator_type.present? } + belongs_to :note, optional: true + belongs_to :requestable, polymorphic: true, optional: true + belongs_to :work_history, optional: true enum event_type: { + action_required_by_changed: "action_required_by_changed", age_range_subjects_verified: "age_range_subjects_verified", assessment_section_recorded: "assessment_section_recorded", assessor_assigned: "assessor_assigned", @@ -75,9 +77,14 @@ class TimelineEvent < ApplicationRecord reviewer_assigned: "reviewer_assigned", state_changed: "state_changed", } + + validates :creator, presence: true, unless: -> { creator_name.present? } + validates :creator_name, + presence: true, + unless: -> { creator_id.present? && creator_type.present? } + validates :event_type, inclusion: { in: event_types.values } - belongs_to :assignee, class_name: "Staff", optional: true validates :assignee, absence: true, unless: -> { assessor_assigned? || reviewer_assigned? } @@ -91,7 +98,6 @@ class TimelineEvent < ApplicationRecord absence: true, unless: -> { state_changed? || assessment_section_recorded? } - belongs_to :assessment_section, optional: true validates :assessment_section, presence: true, if: :assessment_section_recorded? @@ -99,7 +105,6 @@ class TimelineEvent < ApplicationRecord absence: true, unless: :assessment_section_recorded? - belongs_to :note, optional: true validates :note, presence: true, if: :note_created? validates :note, absence: true, unless: :note_created? @@ -114,7 +119,6 @@ class TimelineEvent < ApplicationRecord absence: true, unless: :email_sent? - belongs_to :assessment, optional: true validates :assessment, :age_range_min, :age_range_max, @@ -130,7 +134,6 @@ class TimelineEvent < ApplicationRecord absence: true, unless: :age_range_subjects_verified? - belongs_to :requestable, polymorphic: true, optional: true validates :requestable_id, presence: true, if: :requestable_event_type? validates :requestable_type, presence: true, @@ -146,16 +149,17 @@ class TimelineEvent < ApplicationRecord absence: true, unless: :requestable_event_type? - belongs_to :work_history, optional: true - validates :column_name, - :old_value, + validates :old_value, :new_value, presence: true, - if: :information_changed? + if: -> { action_required_by_changed? || information_changed? } + validates :old_value, + :new_value, + absence: true, + unless: -> { action_required_by_changed? || information_changed? } + validates :column_name, presence: true, if: :information_changed? validates :work_history_id, :column_name, - :old_value, - :new_value, absence: true, unless: :information_changed? diff --git a/spec/models/timeline_event_spec.rb b/spec/models/timeline_event_spec.rb index b6a199fd1e..2f4bea97c9 100644 --- a/spec/models/timeline_event_spec.rb +++ b/spec/models/timeline_event_spec.rb @@ -79,6 +79,7 @@ it do is_expected.to define_enum_for(:event_type).with_values( + action_required_by_changed: "action_required_by_changed", age_range_subjects_verified: "age_range_subjects_verified", assessment_section_recorded: "assessment_section_recorded", assessor_assigned: "assessor_assigned", @@ -386,5 +387,28 @@ it { is_expected.to validate_absence_of(:old_value) } it { is_expected.to validate_absence_of(:new_value) } end + + context "with an action required by changed event type" do + before { timeline_event.event_type = :action_required_by_changed } + + it { is_expected.to validate_absence_of(:assignee) } + it { is_expected.to validate_absence_of(:old_state) } + it { is_expected.to validate_absence_of(:new_state) } + it { is_expected.to validate_absence_of(:assessment_section) } + it { is_expected.to validate_absence_of(:note) } + it { is_expected.to validate_absence_of(:mailer_class_name) } + it { is_expected.to validate_absence_of(:mailer_action_name) } + it { is_expected.to validate_absence_of(:message_subject) } + it { is_expected.to validate_absence_of(:assessment) } + it { is_expected.to validate_absence_of(:age_range_min) } + it { is_expected.to validate_absence_of(:age_range_max) } + it { is_expected.to validate_absence_of(:subjects) } + it { is_expected.to validate_absence_of(:requestable_id) } + it { is_expected.to validate_absence_of(:requestable_type) } + it { is_expected.to validate_absence_of(:work_history_id) } + it { is_expected.to validate_absence_of(:column_name) } + it { is_expected.to validate_presence_of(:old_value) } + it { is_expected.to validate_presence_of(:new_value) } + end end end From f0670b6f59f4e68c005e3afd581f4d4291efc16e Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Thu, 7 Sep 2023 16:24:40 +0100 Subject: [PATCH 4/8] Show action_required_by_changed timeline events This updates the timeline entry component to be able to show the new action required by changed timeline events, explaining to the user what has changed. --- app/components/timeline_entry/component.rb | 11 +++++++++++ config/locales/components.en.yml | 4 +++- spec/components/timeline_entry_spec.rb | 14 ++++++++++++++ spec/factories/timeline_events.rb | 6 ++++++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/app/components/timeline_entry/component.rb b/app/components/timeline_entry/component.rb index 93af8c2310..d1870d1cd1 100644 --- a/app/components/timeline_entry/component.rb +++ b/app/components/timeline_entry/component.rb @@ -159,5 +159,16 @@ def information_changed_vars new_value: timeline_event.new_value, } end + + def action_required_by_changed_vars + { + action: + if timeline_event.new_value == "none" + "no" + else + timeline_event.new_value + end, + } + end end end diff --git a/config/locales/components.en.yml b/config/locales/components.en.yml index 7edb25dcf5..4091fa04e2 100644 --- a/config/locales/components.en.yml +++ b/config/locales/components.en.yml @@ -38,6 +38,7 @@ en: timeline_entry: title: + action_required_by_changed: Workflow changed assessor_assigned: Assessor assigned reviewer_assigned: Reviewer assigned state_changed: Status changed @@ -68,9 +69,10 @@ en: ReferenceRequest: Reference assessed information_changed: Information changed after submission description: + action_required_by_changed: Application requires %{action} action. assessor_assigned: "%{assignee_name} is assigned as the assessor." reviewer_assigned: "%{assignee_name} is assigned as the reviewer." - state_changed: Status changed from %{old_state} to %{new_state} + state_changed: Status changed from %{old_state} to %{new_state}. note_created: "%{text}" email_sent: "%{subject}" requestable_requested: diff --git a/spec/components/timeline_entry_spec.rb b/spec/components/timeline_entry_spec.rb index 093c9344ab..5b8d9692da 100644 --- a/spec/components/timeline_entry_spec.rb +++ b/spec/components/timeline_entry_spec.rb @@ -504,4 +504,18 @@ expect(component.text).to include(creator.name) end end + + context "action required by changed" do + let(:timeline_event) do + create(:timeline_event, :action_required_by_changed, new_value: "none") + end + + it "describes the event" do + expect(component.text.squish).to include("Application requires no action") + end + + it "attributes to the creator" do + expect(component.text).to include(creator.name) + end + end end diff --git a/spec/factories/timeline_events.rb b/spec/factories/timeline_events.rb index b201c87725..d9a2af715f 100644 --- a/spec/factories/timeline_events.rb +++ b/spec/factories/timeline_events.rb @@ -105,5 +105,11 @@ old_value { Faker::Internet.email } new_value { Faker::Internet.email } end + + trait :action_required_by_changed do + event_type { "action_required_by_changed" } + old_value { %w[admin assessor external none].sample } + new_value { %w[admin assessor external none].sample } + end end end From 6ec3e2ec61ecdedc56437e69a6125a286664c14f Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Fri, 8 Sep 2023 13:10:51 +0100 Subject: [PATCH 5/8] Update action_required_by This adds the ability for the status updater class to also update the actionable by value, ensuring that it's kept up to date with the status and captures who can perform which tasks. --- app/lib/application_form_status_updater.rb | 53 ++++++++++++---- .../application_form_status_updater_spec.rb | 62 ++++++++++++++++++- 2 files changed, 103 insertions(+), 12 deletions(-) diff --git a/app/lib/application_form_status_updater.rb b/app/lib/application_form_status_updater.rb index 0a5299be8e..30b061175f 100644 --- a/app/lib/application_form_status_updater.rb +++ b/app/lib/application_form_status_updater.rb @@ -9,8 +9,6 @@ def initialize(application_form:, user:) end def call - old_status = application_form.status - ActiveRecord::Base.transaction do application_form.update!( overdue_further_information:, @@ -27,10 +25,24 @@ def call waiting_on_reference:, ) - next if old_status == new_status + if (old_status = application_form.status) != status + application_form.update!(status:) + create_timeline_event( + event_type: "state_changed", + old_state: old_status, + new_state: status, + ) + end - application_form.update!(status: new_status) - create_timeline_event(old_state: old_status, new_state: new_status) + if (old_action_required_by = application_form.action_required_by) != + action_required_by + application_form.update!(action_required_by:) + create_timeline_event( + event_type: "action_required_by_changed", + old_value: old_action_required_by, + new_value: action_required_by, + ) + end end end @@ -117,8 +129,8 @@ def waiting_on_reference waiting_on?(requestables: reference_requests) end - def new_status - @new_status ||= + def status + @status ||= if dqt_trn_request&.potential_duplicate? "potential_duplicate_in_dqt" elsif application_form.withdrawn_at.present? @@ -150,6 +162,26 @@ def new_status end end + def action_required_by + @action_required_by ||= + if status == "preliminary_check" + "admin" + elsif %w[ + potential_duplicate_in_dqt + awarded_pending_checks + overdue + received + assessment_in_progress + submitted + ].include?(status) + "assessor" + elsif status == "waiting_on" + "external" + else + "none" + end + end + delegate :assessment, :dqt_trn_request, :region, @@ -198,17 +230,16 @@ def received?(requestables:) requestables.reject(&:reviewed?).any?(&:received?) end - def create_timeline_event(old_state:, new_state:) + def create_timeline_event(event_type:, **kwargs) creator = user.is_a?(String) ? nil : user creator_name = user.is_a?(String) ? user : "" TimelineEvent.create!( application_form:, - event_type: "state_changed", + event_type:, creator:, creator_name:, - new_state:, - old_state:, + **kwargs, ) end end diff --git a/spec/lib/application_form_status_updater_spec.rb b/spec/lib/application_form_status_updater_spec.rb index cd9aba538d..45e05b8a41 100644 --- a/spec/lib/application_form_status_updater_spec.rb +++ b/spec/lib/application_form_status_updater_spec.rb @@ -6,8 +6,42 @@ let(:application_form) { create(:application_form) } let(:user) { create(:staff) } + shared_examples "changes action required by" do |new_action_required_by| + it "changes action required by to #{new_action_required_by}" do + expect { call }.to change(application_form, :action_required_by).to( + new_action_required_by, + ) + end + + it "records a timeline event" do + expect { call }.to have_recorded_timeline_event( + :action_required_by_changed, + creator: user, + application_form:, + old_value: "none", + new_value: new_action_required_by, + ) + end + end + + shared_examples "doesn't change action required by" do + it "doesn't change action required by from none" do + expect { call }.to_not change(application_form, :action_required_by).from( + "none", + ) + end + + it "doesn't record a timeline event" do + expect { call }.to_not have_recorded_timeline_event( + :action_required_by_changed, + creator: user, + application_form:, + ) + end + end + shared_examples "changes status" do |new_status| - it "changes the status to #{new_status}" do + it "changes status to #{new_status}" do expect { call }.to change(application_form, :status).to(new_status) end @@ -31,6 +65,7 @@ create(:dqt_trn_request, :potential_duplicate, application_form:) end + include_examples "changes action required by", "assessor" include_examples "changes status", "potential_duplicate_in_dqt" end @@ -42,6 +77,7 @@ ) end + include_examples "doesn't change action required by" include_examples "changes status", "withdrawn" end @@ -53,6 +89,7 @@ ) end + include_examples "doesn't change action required by" include_examples "changes status", "declined" end @@ -64,6 +101,7 @@ ) end + include_examples "doesn't change action required by" include_examples "changes status", "awarded" end @@ -73,6 +111,7 @@ create(:dqt_trn_request, application_form:) end + include_examples "changes action required by", "assessor" include_examples "changes status", "awarded_pending_checks" end @@ -84,6 +123,7 @@ create(:further_information_request, :received, assessment:) end + include_examples "changes action required by", "assessor" include_examples "changes status", "received" it "changes received_further_information" do @@ -102,6 +142,7 @@ create(:further_information_request, :requested, assessment:) end + include_examples "changes action required by", "external" include_examples "changes status", "waiting_on" it "changes waiting_on_further_information" do @@ -120,6 +161,7 @@ create(:professional_standing_request, :requested, assessment:) end + include_examples "changes action required by", "external" include_examples "changes status", "waiting_on" it "changes waiting_on_professional_standing" do @@ -142,6 +184,7 @@ create(:professional_standing_request, :received, assessment:) end + include_examples "changes action required by", "assessor" include_examples "changes status", "submitted" it "doesn't change received_professional_standing" do @@ -158,6 +201,7 @@ create(:professional_standing_request, :received, assessment:) end + include_examples "changes action required by", "assessor" include_examples "changes status", "received" it "changes received_professional_standing" do @@ -177,6 +221,7 @@ create(:qualification_request, :received, assessment:) end + include_examples "changes action required by", "assessor" include_examples "changes status", "received" it "changes received_further_information" do @@ -195,6 +240,7 @@ create(:qualification_request, :requested, assessment:) end + include_examples "changes action required by", "external" include_examples "changes status", "waiting_on" it "changes waiting_on_qualification" do @@ -228,6 +274,7 @@ ) end + include_examples "changes action required by", "external" include_examples "changes status", "waiting_on" it "doesn't change received_reference" do @@ -256,6 +303,7 @@ end context "and it's the only reference request" do + include_examples "changes action required by", "assessor" include_examples "changes status", "received" it "changes received_reference" do @@ -269,6 +317,7 @@ context "and there are other reference requests" do before { create(:reference_request, :requested, assessment:) } + include_examples "changes action required by", "external" include_examples "changes status", "waiting_on" it "doesn't change received_reference" do @@ -297,6 +346,7 @@ ) end + include_examples "changes action required by", "assessor" include_examples "changes status", "received" it "changes received_reference" do @@ -315,6 +365,7 @@ create(:reference_request, :requested, assessment:) end + include_examples "changes action required by", "external" include_examples "changes status", "waiting_on" it "changes waiting_on_reference" do @@ -330,12 +381,14 @@ create(:assessment, :started, application_form:) end + include_examples "changes action required by", "assessor" include_examples "changes status", "assessment_in_progress" end context "with a submitted_at date" do before { application_form.update!(submitted_at: Time.zone.now) } + include_examples "changes action required by", "assessor" include_examples "changes status", "submitted" end @@ -347,6 +400,8 @@ it "doesn't record a timeline event" do expect { call }.to_not have_recorded_timeline_event(:state_changed) end + + include_examples "doesn't change action required by" end context "when preliminary check is required" do @@ -363,6 +418,7 @@ create(:assessment_section, :preliminary, assessment:) end + include_examples "changes action required by", "admin" include_examples "changes status", "preliminary_check" context "when teaching authority provides written statement" do @@ -373,11 +429,13 @@ create(:professional_standing_request, assessment:) end + include_examples "changes action required by", "admin" include_examples "changes status", "preliminary_check" context "when the preliminary check has passed" do before { preliminary_assessment_section.update!(passed: true) } + include_examples "changes action required by", "external" include_examples "changes status", "waiting_on" end @@ -390,11 +448,13 @@ preliminary_assessment_section.reload.update!(passed: false) end + include_examples "changes action required by", "admin" include_examples "changes status", "preliminary_check" context "and the application form is declined" do before { application_form.update!(declined_at: Time.zone.now) } + include_examples "doesn't change action required by" include_examples "changes status", "declined" end end From 88c18b3be37326cf11d76e6362d6c74df72fd49e Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Fri, 8 Sep 2023 13:59:04 +0100 Subject: [PATCH 6/8] Add application_form:update_statuses This is a Rake task which will go through each application form and ensure that it's got the latest accurate status. --- lib/tasks/application_forms.rake | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/tasks/application_forms.rake b/lib/tasks/application_forms.rake index e2142f060a..26448c1aab 100644 --- a/lib/tasks/application_forms.rake +++ b/lib/tasks/application_forms.rake @@ -23,4 +23,15 @@ namespace :application_forms do count = BackfillPreliminaryChecks.call(user:) puts "Updated #{count} applications." end + + desc "Update the statuses of all application forms." + task :update_statuses, %i[staff_email] => :environment do |_task, args| + user = Staff.find_by!(email: args[:staff_email]) + ApplicationForm + .order(:id) + .find_each do |application_form| + ApplicationFormStatusUpdater.call(application_form:, user:) + puts "#{application_form.reference}: #{application_form.action_required_by} - #{application_form.status}" + end + end end From e2f8412f4cfdf094de7f6c606b662fe2f2f0bfb8 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Fri, 8 Sep 2023 13:20:14 +0100 Subject: [PATCH 7/8] Add Filters::ActionRequiredBy This adds a new filter class for the action_required_by field. --- app/lib/filters/action_required_by.rb | 13 ++++++++ spec/lib/filters/action_required_by_spec.rb | 34 +++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 app/lib/filters/action_required_by.rb create mode 100644 spec/lib/filters/action_required_by_spec.rb diff --git a/app/lib/filters/action_required_by.rb b/app/lib/filters/action_required_by.rb new file mode 100644 index 0000000000..c0a8163c6b --- /dev/null +++ b/app/lib/filters/action_required_by.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class Filters::ActionRequiredBy < Filters::Base + def apply + return scope if action_required_by.empty? + + scope.where(action_required_by:) + end + + def action_required_by + Array(params[:action_required_by]).compact_blank + end +end diff --git a/spec/lib/filters/action_required_by_spec.rb b/spec/lib/filters/action_required_by_spec.rb new file mode 100644 index 0000000000..0cd2f274e3 --- /dev/null +++ b/spec/lib/filters/action_required_by_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Filters::ActionRequiredBy do + subject(:filtered_scope) { described_class.apply(scope:, params:) } + + context "the params includes a action_required_by" do + let(:params) { { action_required_by: "assessor" } } + let(:scope) { ApplicationForm.all } + + let!(:included) { create(:application_form, :action_required_by_assessor) } + let!(:excluded) { create(:application_form, :action_required_by_admin) } + + it { is_expected.to contain_exactly(included) } + end + + context "the params include multiple action_required_by" do + let(:params) { { action_required_by: %w[assessor external] } } + let(:scope) { ApplicationForm.all } + + let!(:included) { create(:application_form, :action_required_by_assessor) } + let!(:excluded) { create(:application_form, :action_required_by_admin) } + + it { is_expected.to contain_exactly(included) } + end + + context "the params don't include action_required_by" do + let(:params) { {} } + let(:scope) { double } + + it { is_expected.to eq(scope) } + end +end From bbc4220826d7fdf91ad8133524eaebc494878be9 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Fri, 8 Sep 2023 15:27:40 +0100 Subject: [PATCH 8/8] Use Filters::ActionRequiredBy This adds a new filter to the application index page for assessors to allow them to select applications which require action either by an assessor or by an admin. --- app/forms/assessor_interface/filter_form.rb | 9 +-- .../application_forms_index_view_object.rb | 55 +++++++++++++++---- .../application_forms/index.html.erb | 16 ++++-- config/locales/components.en.yml | 2 +- config/locales/helpers.en.yml | 3 +- .../assessor_interface/applications.rb | 5 ++ .../filtering_application_forms_spec.rb | 24 ++++++++ 7 files changed, 92 insertions(+), 22 deletions(-) diff --git a/app/forms/assessor_interface/filter_form.rb b/app/forms/assessor_interface/filter_form.rb index 9be8172860..dedd850a85 100644 --- a/app/forms/assessor_interface/filter_form.rb +++ b/app/forms/assessor_interface/filter_form.rb @@ -4,12 +4,13 @@ class AssessorInterface::FilterForm include ActiveModel::Model include ActiveRecord::AttributeAssignment - attr_accessor :assessor_ids, + attr_accessor :action_required_by, + :assessor_ids, + :email, :location, :name, :reference, - :email, :statuses, - :submitted_at_before, - :submitted_at_after + :submitted_at_after, + :submitted_at_before end diff --git a/app/view_objects/assessor_interface/application_forms_index_view_object.rb b/app/view_objects/assessor_interface/application_forms_index_view_object.rb index f2fba4092f..595f6e861b 100644 --- a/app/view_objects/assessor_interface/application_forms_index_view_object.rb +++ b/app/view_objects/assessor_interface/application_forms_index_view_object.rb @@ -33,6 +33,12 @@ def country_filter_options ) end + def action_required_by_filter_options + ACTION_REQUIRED_BY_OPTIONS.map do |name| + action_required_by_filter_entry(name) + end + end + def status_filter_options STATUS_FILTER_OPTIONS.each_with_object({}) do |(status, substatuses), memo| memo[status_filter_entry(status)] = substatuses.map do |sub_status| @@ -43,6 +49,8 @@ def status_filter_options private + ACTION_REQUIRED_BY_OPTIONS = %w[admin assessor external].freeze + STATUS_FILTER_OPTIONS = { preliminary_check: [], submitted: [], @@ -71,18 +79,8 @@ def filter_params (session[:filter_params] || {}).with_indifferent_access end - def application_forms_with_pagy - @application_forms_with_pagy ||= - pagy( - ::Filters::Status.apply( - scope: application_forms_without_status_filter, - params: filter_params, - ).order(submitted_at: :asc), - ) - end - - def application_forms_without_status_filter - @application_forms_without_status_filter ||= + def application_forms_without_action_required_by_filter + @application_forms_without_action_required_by_filter ||= begin filters = [ ::Filters::Assessor, @@ -98,6 +96,39 @@ def application_forms_without_status_filter end end + def application_forms_without_status_filter + @application_forms_without_status_filter ||= + ::Filters::ActionRequiredBy.apply( + scope: application_forms_without_action_required_by_filter, + params: filter_params, + ) + end + + def application_forms_with_pagy + @application_forms_with_pagy ||= + pagy( + ::Filters::Status.apply( + scope: application_forms_without_status_filter, + params: filter_params, + ).order(submitted_at: :asc), + ) + end + + def action_required_by_filter_counts + @action_required_by_filter_counts ||= + application_forms_without_action_required_by_filter.group( + :action_required_by, + ).count + end + + def action_required_by_filter_entry(name) + OpenStruct.new( + id: name, + label: + "#{name.humanize} (#{action_required_by_filter_counts.fetch(name, 0)})", + ) + end + def status_filter_counts @status_filter_counts ||= begin diff --git a/app/views/assessor_interface/application_forms/index.html.erb b/app/views/assessor_interface/application_forms/index.html.erb index 10b918ed3c..8034d3d1da 100644 --- a/app/views/assessor_interface/application_forms/index.html.erb +++ b/app/views/assessor_interface/application_forms/index.html.erb @@ -17,10 +17,10 @@
<%= f.govuk_check_boxes_fieldset :assessor_ids, legend: { size: "s" }, small: true do %> - <%- @view_object.assessor_filter_options.each do |option| -%> + <% @view_object.assessor_filter_options.each do |option| %> <%= f.govuk_check_box :assessor_ids, option.id, label: { text: option.name }, checked: @view_object.filter_form.assessor_ids&.include?(option.id.to_s) %> - <%- end -%> - <%- end -%> + <% end %> + <% end %>
@@ -48,6 +48,14 @@ <% end %> +
+ <%= f.govuk_check_boxes_fieldset :action_required_by, legend: { size: "s" }, small: true do %> + <% @view_object.action_required_by_filter_options.each do |option| %> + <%= f.govuk_check_box :action_required_by, option.id, label: { text: option.label }, checked: @view_object.filter_form.action_required_by&.include?(option.id) %> + <% end %> + <% end %> +
+
<%= f.govuk_check_boxes_fieldset :statuses, legend: { size: "s" }, small: true do %> <% @view_object.status_filter_options.each do |option, suboptions| %> @@ -67,7 +75,7 @@ <% end %> <% end %>
- <%- end -%> + <% end %>
diff --git a/config/locales/components.en.yml b/config/locales/components.en.yml index 4091fa04e2..399f9249f9 100644 --- a/config/locales/components.en.yml +++ b/config/locales/components.en.yml @@ -38,7 +38,7 @@ en: timeline_entry: title: - action_required_by_changed: Workflow changed + action_required_by_changed: Action required by changed assessor_assigned: Assessor assigned reviewer_assigned: Reviewer assigned state_changed: Status changed diff --git a/config/locales/helpers.en.yml b/config/locales/helpers.en.yml index 16f13346c2..d39d438eb2 100644 --- a/config/locales/helpers.en.yml +++ b/config/locales/helpers.en.yml @@ -101,10 +101,10 @@ en: assessor_interface_create_note_form: text: Add a note to this application assessor_interface_filter_form: + email: Applicant email location: Country trained in name: Applicant name reference: Application reference number - email: Applicant email assessor_interface_professional_standing_request_location_form: received_options: true: "Yes" @@ -277,6 +277,7 @@ en: scotland_full_registration: Does the applicant have or are they eligible for full registration? school_details_cannot_be_verified_work_history_checked: Choose the schools that need reference details to be amended assessor_interface_filter_form: + action_required_by: Action required by assessor_ids: Assessor name states: Status of application submitted_at: Created date diff --git a/spec/support/autoload/page_objects/assessor_interface/applications.rb b/spec/support/autoload/page_objects/assessor_interface/applications.rb index 5677589240..a45c7b8491 100644 --- a/spec/support/autoload/page_objects/assessor_interface/applications.rb +++ b/spec/support/autoload/page_objects/assessor_interface/applications.rb @@ -40,6 +40,11 @@ class Applications < SitePrism::Page "#assessor_interface_filter_form_submitted_at_before_1i" end + section :action_required_by_filter, + "#app-applications-filters-action-required-by" do + sections :items, GovukCheckboxItem, ".govuk-checkboxes__item" + end + section :status_filter, "#app-applications-filters-status" do sections :statuses, GovukCheckboxItem, ".govuk-checkboxes__item" end diff --git a/spec/system/assessor_interface/filtering_application_forms_spec.rb b/spec/system/assessor_interface/filtering_application_forms_spec.rb index 05f70e14d9..2710466191 100644 --- a/spec/system/assessor_interface/filtering_application_forms_spec.rb +++ b/spec/system/assessor_interface/filtering_application_forms_spec.rb @@ -36,6 +36,10 @@ and_i_apply_the_submitted_at_filter then_i_see_a_list_of_applications_filtered_by_submitted_at + when_i_clear_the_filters + and_i_apply_the_action_required_by_filter + then_i_see_a_list_of_applications_filtered_by_action_required_by + when_i_clear_the_filters and_i_apply_the_status_filter then_i_see_a_list_of_applications_filtered_by_state @@ -124,6 +128,24 @@ def then_i_see_a_list_of_applications_filtered_by_submitted_at expect(applications_page.search_results.first.name.text).to eq("John Smith") end + def and_i_apply_the_action_required_by_filter + admin_action_item = + applications_page.action_required_by_filter.items.find do |item| + item.label.text == "Admin (1)" + rescue Capybara::ElementNotFound + false + end + admin_action_item.checkbox.click + applications_page.apply_filters.click + end + + def then_i_see_a_list_of_applications_filtered_by_action_required_by + expect(applications_page.search_results.count).to eq(1) + expect(applications_page.search_results.first.name.text).to eq( + "Emma Dubois", + ) + end + def and_i_apply_the_status_filter awarded_state = applications_page.status_filter.statuses.find do |status| @@ -155,6 +177,7 @@ def application_forms create( :application_form, :submitted, + :action_required_by_admin, region: create(:region, country: create(:country, code: "FR")), given_names: "Emma", family_name: "Dubois", @@ -165,6 +188,7 @@ def application_forms create( :application_form, :submitted, + :action_required_by_assessor, region: create(:region, country: create(:country, code: "ES")), given_names: "Arnold", family_name: "Drummond",