From bd23bfe6985b875891869751938f7a92c535d038 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 21 Aug 2023 14:33:06 +0200 Subject: [PATCH 1/9] Add change_name_permission column This adds a new column to the staff table which records whether the staff user has permission to change application names. --- app/models/staff.rb | 1 + config/analytics.yml | 1 + ...20230821122601_add_change_name_permission_to_staff.rb | 9 +++++++++ db/schema.rb | 1 + spec/factories/staff.rb | 5 +++++ spec/models/staff_spec.rb | 1 + 6 files changed, 18 insertions(+) create mode 100644 db/migrate/20230821122601_add_change_name_permission_to_staff.rb diff --git a/app/models/staff.rb b/app/models/staff.rb index 9e6a1895a3..83c3bbe72b 100644 --- a/app/models/staff.rb +++ b/app/models/staff.rb @@ -5,6 +5,7 @@ # id :bigint not null, primary key # award_decline_permission :boolean default(FALSE) # azure_ad_uid :string +# change_name_permission :boolean default(FALSE), not null # change_work_history_permission :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string diff --git a/config/analytics.yml b/config/analytics.yml index f30409e697..570a4373b2 100644 --- a/config/analytics.yml +++ b/config/analytics.yml @@ -324,6 +324,7 @@ :staff: - award_decline_permission - azure_ad_uid + - change_name_permission - change_work_history_permission - confirmation_sent_at - confirmed_at diff --git a/db/migrate/20230821122601_add_change_name_permission_to_staff.rb b/db/migrate/20230821122601_add_change_name_permission_to_staff.rb new file mode 100644 index 0000000000..b0a0402c36 --- /dev/null +++ b/db/migrate/20230821122601_add_change_name_permission_to_staff.rb @@ -0,0 +1,9 @@ +class AddChangeNamePermissionToStaff < ActiveRecord::Migration[7.0] + def change + add_column :staff, + :change_name_permission, + :boolean, + default: false, + null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 363034bb31..9a5050a4e8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -441,6 +441,7 @@ t.boolean "change_work_history_permission", default: false, null: false t.boolean "reverse_decision_permission", default: false, null: false t.boolean "withdraw_permission", default: false, null: false + t.boolean "change_name_permission", default: false, null: false t.index "lower((email)::text)", name: "index_staff_on_lower_email", unique: true t.index ["confirmation_token"], name: "index_staff_on_confirmation_token", unique: true t.index ["invitation_token"], name: "index_staff_on_invitation_token", unique: true diff --git a/spec/factories/staff.rb b/spec/factories/staff.rb index 76614e80bd..6536728ffc 100644 --- a/spec/factories/staff.rb +++ b/spec/factories/staff.rb @@ -5,6 +5,7 @@ # id :bigint not null, primary key # award_decline_permission :boolean default(FALSE) # azure_ad_uid :string +# change_name_permission :boolean default(FALSE), not null # change_work_history_permission :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string @@ -62,6 +63,10 @@ award_decline_permission { true } end + trait :with_change_name_permission do + change_name_permission { true } + end + trait :with_change_work_history_permission do change_work_history_permission { true } end diff --git a/spec/models/staff_spec.rb b/spec/models/staff_spec.rb index 4c494469f3..003094f4bd 100644 --- a/spec/models/staff_spec.rb +++ b/spec/models/staff_spec.rb @@ -5,6 +5,7 @@ # id :bigint not null, primary key # award_decline_permission :boolean default(FALSE) # azure_ad_uid :string +# change_name_permission :boolean default(FALSE), not null # change_work_history_permission :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string From 88402b9eab134536e417de59f5e498a4c1870761 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 21 Aug 2023 14:35:45 +0200 Subject: [PATCH 2/9] Add change name permission to personas This adds the ability to select a user with the change name permission in the personas (and makes sure that one is generated in the example data). --- app/views/personas/index.html.erb | 6 ++++-- lib/tasks/example_data.rake | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/views/personas/index.html.erb b/app/views/personas/index.html.erb index e760228b0d..0ca82f77a0 100644 --- a/app/views/personas/index.html.erb +++ b/app/views/personas/index.html.erb @@ -11,20 +11,21 @@ <%= govuk_table do |table| table.with_colgroup do |colgroup| colgroup.with_col(span: 1) - colgroup.with_col(span: 5) + colgroup.with_col(span: 6) colgroup.with_col(span: 1) end table.with_head do |head| head.with_row do |row| row.with_cell(scope: false) - row.with_cell(header: true, text: "Permissions", colspan: 4) + row.with_cell(header: true, text: "Permissions", colspan: 5) row.with_cell(scope: false) end head.with_row do |row| row.with_cell(header: true, text: "Email") row.with_cell(header: true, text: "Award/decline") + row.with_cell(header: true, text: "Change names") row.with_cell(header: true, text: "Change work history") row.with_cell(header: true, text: "Reverse decisions") row.with_cell(header: true, text: "Support console") @@ -39,6 +40,7 @@ row.with_cell(text: staff.email) row.with_cell { govuk_boolean_tag(staff.award_decline_permission) } + row.with_cell { govuk_boolean_tag(staff.change_name_permission) } row.with_cell { govuk_boolean_tag(staff.change_work_history_permission) } row.with_cell { govuk_boolean_tag(staff.reverse_decision_permission) } row.with_cell { govuk_boolean_tag(staff.support_console_permission) } diff --git a/lib/tasks/example_data.rake b/lib/tasks/example_data.rake index 62d8eab2af..a7657cb2c0 100644 --- a/lib/tasks/example_data.rake +++ b/lib/tasks/example_data.rake @@ -56,6 +56,7 @@ def staff_members name: "Dave Assessor", email: "assessor-dave@example.com", award_decline_permission: true, + change_name_permission: false, change_work_history_permission: false, reverse_decision_permission: false, support_console_permission: false, @@ -65,6 +66,7 @@ def staff_members name: "Beryl Assessor", email: "assessor-beryl@example.com", award_decline_permission: true, + change_name_permission: false, change_work_history_permission: false, reverse_decision_permission: false, support_console_permission: false, @@ -74,6 +76,7 @@ def staff_members name: "Sally Manager", email: "manager-sally@example.com", award_decline_permission: false, + change_name_permission: true, change_work_history_permission: true, reverse_decision_permission: true, support_console_permission: true, @@ -83,6 +86,7 @@ def staff_members name: "Antonio Helpdesk", email: "helpdesk-antonio@example.com", award_decline_permission: false, + change_name_permission: false, change_work_history_permission: false, reverse_decision_permission: false, support_console_permission: false, From ef6bff7d00cf768cdb47ae26f8dc8f0769572f6c Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 21 Aug 2023 14:40:57 +0200 Subject: [PATCH 3/9] Add change name permission to staff views This allows us to invite users with that permission, and gives the ability for support users to give others that permission. --- app/controllers/staff/invitations_controller.rb | 1 + app/controllers/support_interface/staff_controller.rb | 1 + .../support_interface/staff/_permissions_fields.html.erb | 3 +++ app/views/support_interface/staff/index.html.erb | 9 +++++++++ config/locales/en.yml | 1 + spec/system/support_interface/staff_spec.rb | 2 +- 6 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/controllers/staff/invitations_controller.rb b/app/controllers/staff/invitations_controller.rb index aa9f70e375..05442fc159 100644 --- a/app/controllers/staff/invitations_controller.rb +++ b/app/controllers/staff/invitations_controller.rb @@ -14,6 +14,7 @@ def configure_permitted_parameters keys: %i[ name award_decline_permission + change_name_permission change_work_history_permission reverse_decision_permission support_console_permission diff --git a/app/controllers/support_interface/staff_controller.rb b/app/controllers/support_interface/staff_controller.rb index 3d1a7e4967..1568473697 100644 --- a/app/controllers/support_interface/staff_controller.rb +++ b/app/controllers/support_interface/staff_controller.rb @@ -29,6 +29,7 @@ def load_staff def staff_params params.require(:staff).permit( :award_decline_permission, + :change_name_permission, :change_work_history_permission, :reverse_decision_permission, :support_console_permission, diff --git a/app/views/support_interface/staff/_permissions_fields.html.erb b/app/views/support_interface/staff/_permissions_fields.html.erb index 402cbc6d91..d7e08d4e46 100644 --- a/app/views/support_interface/staff/_permissions_fields.html.erb +++ b/app/views/support_interface/staff/_permissions_fields.html.erb @@ -2,6 +2,9 @@ <%= f.govuk_check_box :award_decline_permission, 1, 0, multiple: false, label: { text: t("activerecord.attributes.staff.award_decline_permission") } %> + <%= f.govuk_check_box :change_name_permission, 1, 0, multiple: false, + label: { text: t("activerecord.attributes.staff.change_name_permission") } %> + <%= f.govuk_check_box :change_work_history_permission, 1, 0, multiple: false, label: { text: t("activerecord.attributes.staff.change_work_history_permission") } %> diff --git a/app/views/support_interface/staff/index.html.erb b/app/views/support_interface/staff/index.html.erb index 32cc718c2b..76fde50ca4 100644 --- a/app/views/support_interface/staff/index.html.erb +++ b/app/views/support_interface/staff/index.html.erb @@ -29,6 +29,15 @@ ) end + summary_list.with_row do |row| + row.with_key { t("activerecord.attributes.staff.change_name_permission") } + row.with_value { govuk_boolean_tag(staff.change_name_permission) } + row.with_action( + href: edit_support_interface_staff_path(staff), + visually_hidden_text: t("activerecord.attributes.staff.change_name_permission") + ) + end + summary_list.with_row do |row| row.with_key { t("activerecord.attributes.staff.change_work_history_permission") } row.with_value { govuk_boolean_tag(staff.change_work_history_permission) } diff --git a/config/locales/en.yml b/config/locales/en.yml index d562e9ec06..c8887b4c0d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -47,6 +47,7 @@ en: attributes: staff: award_decline_permission: Award/decline (assessor) + change_name_permission: Change applicant’s name change_work_history_permission: Change work history references reverse_decision_permission: Reverse decisions support_console_permission: Support console access diff --git a/spec/system/support_interface/staff_spec.rb b/spec/system/support_interface/staff_spec.rb index 79786bc681..e0b81e026d 100644 --- a/spec/system/support_interface/staff_spec.rb +++ b/spec/system/support_interface/staff_spec.rb @@ -168,7 +168,7 @@ def and_i_see_the_helpdesk_user end def when_i_click_on_the_helpdesk_user - find(:xpath, "(//a[text()='Change'])[6]").click + find(:xpath, "(//a[text()='Change'])[7]").click end def then_i_see_the_staff_edit_form From ddeeb0d641235d3716ac32db2ad194f4ef670854 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 7 Aug 2023 11:47:18 +0200 Subject: [PATCH 4/9] Update ApplicationFormPolicy I've update the policy to allow updating or editing an application form if the user has the right permission. I've also refactored the tests to use some shared examples to reduce duplication. --- .../application_form_policy.rb | 4 + .../application_form_policy_spec.rb | 30 +------ .../assessment_policy_spec.rb | 48 +---------- .../work_history_policy_spec.rb | 26 +----- spec/policies/assessor_policy_spec.rb | 82 ++----------------- spec/policies/note_policy_spec.rb | 3 +- spec/support/shared_examples/policy.rb | 60 ++++++++++++++ 7 files changed, 83 insertions(+), 170 deletions(-) diff --git a/app/policies/assessor_interface/application_form_policy.rb b/app/policies/assessor_interface/application_form_policy.rb index 8c186dbfb3..5f5c83a18f 100644 --- a/app/policies/assessor_interface/application_form_policy.rb +++ b/app/policies/assessor_interface/application_form_policy.rb @@ -9,6 +9,10 @@ def show? true end + def update? + user.change_name_permission + end + def destroy? user.withdraw_permission end diff --git a/spec/policies/assessor_interface/application_form_policy_spec.rb b/spec/policies/assessor_interface/application_form_policy_spec.rb index 9b0fcd67a2..c7db7fb8b1 100644 --- a/spec/policies/assessor_interface/application_form_policy_spec.rb +++ b/spec/policies/assessor_interface/application_form_policy_spec.rb @@ -47,40 +47,16 @@ describe "#edit?" do subject(:edit?) { policy.edit? } - - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } + it_behaves_like "a policy method requiring the change name permission" end describe "#destroy?" do subject(:destroy?) { policy.destroy? } - - let(:record) { create(:assessment, :award) } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_withdraw_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the withdraw permission" end describe "#withdraw?" do subject(:rollback?) { policy.withdraw? } - - let(:record) { create(:assessment, :award) } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_withdraw_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the withdraw permission" end end diff --git a/spec/policies/assessor_interface/assessment_policy_spec.rb b/spec/policies/assessor_interface/assessment_policy_spec.rb index c9794eec1f..d6654cd0d9 100644 --- a/spec/policies/assessor_interface/assessment_policy_spec.rb +++ b/spec/policies/assessor_interface/assessment_policy_spec.rb @@ -40,61 +40,21 @@ describe "#update?" do subject(:update?) { policy.update? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the award decline permission" end describe "#edit?" do subject(:edit?) { policy.edit? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the award decline permission" end describe "#destroy?" do subject(:destroy?) { policy.destroy? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_reverse_decision_permission) - end - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the reverse decision permission" end describe "#rollback?" do subject(:rollback?) { policy.rollback? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_reverse_decision_permission) - end - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the reverse decision permission" end end diff --git a/spec/policies/assessor_interface/work_history_policy_spec.rb b/spec/policies/assessor_interface/work_history_policy_spec.rb index 94a9aced25..f8abb593f5 100644 --- a/spec/policies/assessor_interface/work_history_policy_spec.rb +++ b/spec/policies/assessor_interface/work_history_policy_spec.rb @@ -40,34 +40,12 @@ describe "#update?" do subject(:update?) { policy.update? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_change_work_history_permission) - end - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring change the work history permission" end describe "#edit?" do subject(:edit?) { policy.edit? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) do - create(:staff, :confirmed, :with_change_work_history_permission) - end - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring change the work history permission" end describe "#destroy?" do diff --git a/spec/policies/assessor_policy_spec.rb b/spec/policies/assessor_policy_spec.rb index a57392fc0a..440858c59b 100644 --- a/spec/policies/assessor_policy_spec.rb +++ b/spec/policies/assessor_policy_spec.rb @@ -5,106 +5,42 @@ RSpec.describe AssessorPolicy do it_behaves_like "a policy" - let(:user) { nil } - let(:record) { nil } + let(:user) { create(:staff, :confirmed) } - subject(:policy) { described_class.new(user, record) } + subject(:policy) { described_class.new(user, nil) } describe "#index?" do subject(:index?) { policy.index? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be true } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it { is_expected.to be true } end describe "#show?" do subject(:show?) { policy.show? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be true } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it { is_expected.to be true } end describe "#create?" do subject(:create?) { policy.create? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the award decline permission" end describe "#new?" do subject(:new?) { policy.new? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the award decline permission" end describe "#update?" do subject(:update?) { policy.update? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the award decline permission" end describe "#edit?" do subject(:edit?) { policy.edit? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the award decline permission" end describe "#destroy?" do subject(:destroy?) { policy.destroy? } - - context "without permission" do - let(:user) { create(:staff, :confirmed) } - it { is_expected.to be false } - end - - context "with permission" do - let(:user) { create(:staff, :confirmed, :with_award_decline_permission) } - it { is_expected.to be true } - end + it_behaves_like "a policy method requiring the award decline permission" end end diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index ba808c3c5c..1a78b6fbd0 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -6,9 +6,8 @@ it_behaves_like "a policy" let(:user) { create(:staff, :confirmed) } - let(:record) { nil } - subject(:policy) { described_class.new(user, record) } + subject(:policy) { described_class.new(user, nil) } describe "#index?" do subject(:index?) { policy.index? } diff --git a/spec/support/shared_examples/policy.rb b/spec/support/shared_examples/policy.rb index 11ee1f793d..c1872f4e67 100644 --- a/spec/support/shared_examples/policy.rb +++ b/spec/support/shared_examples/policy.rb @@ -16,6 +16,66 @@ end end +RSpec.shared_examples "a policy method requiring the award decline permission" do + context "without permission" do + let(:user) { create(:staff) } + it { is_expected.to be false } + end + + context "with permission" do + let(:user) { create(:staff, :with_award_decline_permission) } + it { is_expected.to be true } + end +end + +RSpec.shared_examples "a policy method requiring the change name permission" do + context "without permission" do + let(:user) { create(:staff) } + it { is_expected.to be false } + end + + context "with permission" do + let(:user) { create(:staff, :with_change_name_permission) } + it { is_expected.to be true } + end +end + +RSpec.shared_examples "a policy method requiring change the work history permission" do + context "without permission" do + let(:user) { create(:staff) } + it { is_expected.to be false } + end + + context "with permission" do + let(:user) { create(:staff, :with_change_work_history_permission) } + it { is_expected.to be true } + end +end + +RSpec.shared_examples "a policy method requiring the reverse decision permission" do + context "without permission" do + let(:user) { create(:staff) } + it { is_expected.to be false } + end + + context "with permission" do + let(:user) { create(:staff, :with_reverse_decision_permission) } + it { is_expected.to be true } + end +end + +RSpec.shared_examples "a policy method requiring the withdraw permission" do + context "without permission" do + let(:user) { create(:staff) } + it { is_expected.to be false } + end + + context "with permission" do + let(:user) { create(:staff, :with_withdraw_permission) } + it { is_expected.to be true } + end +end + RSpec.shared_examples "a policy method requiring the support console permission" do context "without permission" do let(:user) { create(:staff) } From 96e1af8e4c6b12f36039a49b44d6208560d5ce9e Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 7 Aug 2023 11:50:43 +0200 Subject: [PATCH 5/9] Add ApplicationFormNameForm This is a form which uses the new UpdateApplicationFormName service to allow assessors to change the name on an application form. --- .../application_form_name_form.rb | 25 ++++++++++++++ config/locales/helpers.en.yml | 6 ++++ .../application_form_name_form_spec.rb | 34 +++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 app/forms/assessor_interface/application_form_name_form.rb create mode 100644 spec/forms/assessor_interface/application_form_name_form_spec.rb diff --git a/app/forms/assessor_interface/application_form_name_form.rb b/app/forms/assessor_interface/application_form_name_form.rb new file mode 100644 index 0000000000..b46bfed5b2 --- /dev/null +++ b/app/forms/assessor_interface/application_form_name_form.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class AssessorInterface::ApplicationFormNameForm + include ActiveModel::Model + include ActiveModel::Attributes + + attr_accessor :application_form, :user + attribute :given_names, :string + attribute :family_name, :string + + validates :application_form, :user, presence: true + + def save + return false if invalid? + + UpdateApplicationFormName.call( + application_form:, + user:, + given_names:, + family_name:, + ) + + true + end +end diff --git a/config/locales/helpers.en.yml b/config/locales/helpers.en.yml index b98a3700c2..e8d53d8203 100644 --- a/config/locales/helpers.en.yml +++ b/config/locales/helpers.en.yml @@ -2,6 +2,9 @@ en: helpers: caption: {} hint: + assessor_interface_application_form_name_form: + given_names: Type the updated given names in the text box below + family_name: Type the updated family name in the text box below assessor_interface_assessment_section_form: selected_failure_reasons: Select all options that are relevant to you. failure_reason_notes: @@ -58,6 +61,9 @@ en: start_date: For example, 3 2020, If you cannot remember the exact month or year, enter an estimate. end_date: For example, 3 2020, If you cannot remember the exact month or year, enter an estimate. label: + assessor_interface_application_form_name_form: + given_names: Change given names + family_name: Change family name assessor_interface_assessment_recommendation_form: recommendation_options: award: Award QTS diff --git a/spec/forms/assessor_interface/application_form_name_form_spec.rb b/spec/forms/assessor_interface/application_form_name_form_spec.rb new file mode 100644 index 0000000000..e5562b1a49 --- /dev/null +++ b/spec/forms/assessor_interface/application_form_name_form_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe AssessorInterface::ApplicationFormNameForm, type: :model do + let(:application_form) { create(:application_form) } + let(:user) { create(:staff) } + + let(:given_names) { "A new given name" } + let(:family_name) { "A new family name" } + + subject(:form) do + described_class.new(application_form:, user:, given_names:, family_name:) + end + + describe "validations" do + it { is_expected.to validate_presence_of(:application_form) } + it { is_expected.to validate_presence_of(:user) } + end + + describe "#save" do + subject(:save) { form.save } + + it "calls the UpdateApplicationFormName service" do + expect(UpdateApplicationFormName).to receive(:call).with( + application_form:, + user:, + given_names:, + family_name:, + ) + expect(save).to be true + end + end +end From 4f398ddb78c69d26f83d50f2a435bff10f4f2409 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 7 Aug 2023 13:59:33 +0200 Subject: [PATCH 6/9] Refactor application_form_summary_rows This removes an argument which was always set to true, and simplifies the actions. --- .../application_form_overview/component.rb | 1 - .../component.rb | 2 +- app/helpers/application_form_helper.rb | 172 +++++++++++------- spec/helpers/application_form_helper_spec.rb | 60 ++---- 4 files changed, 114 insertions(+), 121 deletions(-) diff --git a/app/components/application_form_overview/component.rb b/app/components/application_form_overview/component.rb index 1cd7d473e7..236329fbc4 100644 --- a/app/components/application_form_overview/component.rb +++ b/app/components/application_form_overview/component.rb @@ -16,7 +16,6 @@ def summary_rows application_form_summary_rows( application_form, include_name: true, - include_reference: true, highlight_email:, ) + [ diff --git a/app/components/application_form_search_result/component.rb b/app/components/application_form_search_result/component.rb index 1ffeb09393..70265380db 100644 --- a/app/components/application_form_search_result/component.rb +++ b/app/components/application_form_search_result/component.rb @@ -17,8 +17,8 @@ def summary_rows application_form_summary_rows( application_form, include_name: false, - include_reference: true, include_reviewer: application_form.reviewer.present?, + class_context: "app-search-result__item", ) end diff --git a/app/helpers/application_form_helper.rb b/app/helpers/application_form_helper.rb index 5a6c392aa6..f2556b0d33 100644 --- a/app/helpers/application_form_helper.rb +++ b/app/helpers/application_form_helper.rb @@ -13,97 +13,129 @@ def application_form_full_name(application_form) def application_form_summary_rows( application_form, include_name:, - include_reference:, include_reviewer: true, - highlight_email: false + highlight_email: false, + class_context: nil ) [ ( if include_name - [ - I18n.t("application_form.summary.name"), - application_form_full_name(application_form), - ] + { + key: { + text: I18n.t("application_form.summary.name"), + }, + value: { + text: application_form_full_name(application_form), + }, + } end ), - [ - I18n.t("application_form.summary.country"), - CountryName.from_country(application_form.region.country), - ], - [ - I18n.t("application_form.summary.email"), - ( - if highlight_email - "#{ERB::Util.html_escape(application_form.teacher.email)}".html_safe - else - application_form.teacher.email - end - ), - ], + { + key: { + text: I18n.t("application_form.summary.country"), + }, + value: { + text: CountryName.from_country(application_form.region.country), + }, + }, ( if application_form.region.name.present? - [ - I18n.t("application_form.summary.region"), - application_form.region.name, - ] + { + key: { + text: I18n.t("application_form.summary.region"), + }, + value: { + text: application_form.region.name, + }, + } end ), - [ - I18n.t("application_form.summary.submitted_at"), - application_form.submitted_at.strftime("%e %B %Y"), - ], - [ - I18n.t("application_form.summary.days_since_submission"), - pluralize(application_form.working_days_since_submission, "day"), - ], - [ - I18n.t("application_form.summary.assessor"), - application_form.assessor&.name || - I18n.t("application_form.summary.unassigned"), - [ + { + key: { + text: I18n.t("application_form.summary.email"), + }, + value: { + text: + ( + if highlight_email + "#{ERB::Util.html_escape(application_form.teacher.email)}".html_safe + else + application_form.teacher.email + end + ), + }, + }, + { + key: { + text: I18n.t("application_form.summary.submitted_at"), + }, + value: { + text: application_form.submitted_at.strftime("%e %B %Y"), + }, + }, + { + key: { + text: I18n.t("application_form.summary.days_since_submission"), + }, + value: { + text: + pluralize(application_form.working_days_since_submission, "day"), + }, + }, + { + key: { + text: I18n.t("application_form.summary.assessor"), + }, + value: { + text: + application_form.assessor&.name || + I18n.t("application_form.summary.unassigned"), + }, + actions: [ { - href: - assessor_interface_application_form_assign_assessor_path( - application_form, - ), + visually_hidden_text: I18n.t("application_form.summary.assessor"), + href: [:assessor_interface, application_form, :assign_assessor], }, ], - ], + }, ( if include_reviewer - [ - I18n.t("application_form.summary.reviewer"), - application_form.reviewer&.name || - I18n.t("application_form.summary.unassigned"), - [ + { + key: { + text: I18n.t("application_form.summary.reviewer"), + }, + value: { + text: + application_form.reviewer&.name || + I18n.t("application_form.summary.unassigned"), + }, + actions: [ { - href: - assessor_interface_application_form_assign_reviewer_path( - application_form, - ), + visually_hidden_text: + I18n.t("application_form.summary.reviewer"), + href: [:assessor_interface, application_form, :assign_reviewer], }, ], - ] - end - ), - ( - if include_reference - [ - I18n.t("application_form.summary.reference"), - application_form.reference, - ] + } end ), - [ - I18n.t("application_form.summary.status"), - application_form_status_tags( - application_form, - class_context: "app-search-result__item", - ), - ], - ].compact.map do |key, value, actions| - { key: { text: key }, value: { text: value }, actions: actions || [] } - end + { + key: { + text: I18n.t("application_form.summary.reference"), + }, + value: { + text: application_form.reference, + }, + }, + { + key: { + text: I18n.t("application_form.summary.status"), + }, + value: { + text: application_form_status_tags(application_form, class_context:), + }, + }, + ].compact end def application_form_display_work_history_before_teaching_qualification_banner?( diff --git a/spec/helpers/application_form_helper_spec.rb b/spec/helpers/application_form_helper_spec.rb index 0ad91c1400..81c25f2f45 100644 --- a/spec/helpers/application_form_helper_spec.rb +++ b/spec/helpers/application_form_helper_spec.rb @@ -33,25 +33,13 @@ describe "#application_form_summary_rows" do subject(:summary_rows) do - application_form_summary_rows( - application_form, - include_name: true, - include_reference: true, - ) + application_form_summary_rows(application_form, include_name: true) end it do is_expected.to eq( [ - { - key: { - text: "Name", - }, - value: { - text: "Given Family", - }, - actions: [], - }, + { key: { text: "Name" }, value: { text: "Given Family" } }, { key: { text: "Country trained in", @@ -59,16 +47,6 @@ value: { text: "United States", }, - actions: [], - }, - { - key: { - text: "Email", - }, - value: { - text: application_form.teacher.email, - }, - actions: [], }, { key: { @@ -77,17 +55,16 @@ value: { text: "Region", }, - actions: [], }, { key: { - text: "Created on", + text: "Email", }, value: { - text: " 1 January 2020", + text: application_form.teacher.email, }, - actions: [], }, + { key: { text: "Created on" }, value: { text: " 1 January 2020" } }, { key: { text: "Working days since submission", @@ -95,7 +72,6 @@ value: { text: "0 days", }, - actions: [], }, { key: { @@ -106,10 +82,8 @@ }, actions: [ { - href: - assessor_interface_application_form_assign_assessor_path( - application_form, - ), + visually_hidden_text: "Assigned to", + href: [:assessor_interface, application_form, :assign_assessor], }, ], }, @@ -122,31 +96,20 @@ }, actions: [ { - href: - assessor_interface_application_form_assign_reviewer_path( - application_form, - ), + visually_hidden_text: "Reviewer", + href: [:assessor_interface, application_form, :assign_reviewer], }, ], }, - { - key: { - text: "Reference", - }, - value: { - text: "0000001", - }, - actions: [], - }, + { key: { text: "Reference" }, value: { text: "0000001" } }, { key: { text: "Status", }, value: { text: - "Not started\n", + "Not started\n", }, - actions: [], }, ], ) @@ -157,7 +120,6 @@ application_form_summary_rows( application_form, include_name: true, - include_reference: true, include_reviewer: false, ) end From 08a2f808a45bdf419616d818d44c74e2fc23649b Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 7 Aug 2023 14:17:37 +0200 Subject: [PATCH 7/9] Add link to change name This adds a link to the summary list rows if the staff user has the right permission, allowing them to change the given names and family name of the applicant. --- .../application_form_overview/component.rb | 10 ++--- .../component.rb | 8 +++- app/helpers/application_form_helper.rb | 12 +++++ .../application_forms/index.html.erb | 8 ++-- .../application_forms/show.html.erb | 4 +- ...pplication_form_overview_component_spec.rb | 5 ++- ...ation_form_search_result_component_spec.rb | 5 ++- spec/helpers/application_form_helper_spec.rb | 44 ++++++++++++++++++- 8 files changed, 80 insertions(+), 16 deletions(-) diff --git a/app/components/application_form_overview/component.rb b/app/components/application_form_overview/component.rb index 236329fbc4..c78731ec21 100644 --- a/app/components/application_form_overview/component.rb +++ b/app/components/application_form_overview/component.rb @@ -2,9 +2,10 @@ module ApplicationFormOverview class Component < ViewComponent::Base - def initialize(application_form, highlight_email: false) + def initialize(application_form, current_staff:, highlight_email: false) super @application_form = application_form + @current_staff = current_staff @highlight_email = highlight_email end @@ -15,6 +16,7 @@ def title def summary_rows application_form_summary_rows( application_form, + current_staff:, include_name: true, highlight_email:, ) + @@ -27,9 +29,7 @@ def summary_rows text: govuk_link_to( I18n.t("application_form.overview.view_timeline"), - assessor_interface_application_form_timeline_events_path( - application_form, - ), + [:assessor_interface, application_form, :timeline_events], ), }, }, @@ -38,7 +38,7 @@ def summary_rows private - attr_reader :application_form, :highlight_email + attr_reader :application_form, :current_staff, :highlight_email delegate :application_form_summary_rows, to: :helpers end diff --git a/app/components/application_form_search_result/component.rb b/app/components/application_form_search_result/component.rb index 70265380db..2862a547d6 100644 --- a/app/components/application_form_search_result/component.rb +++ b/app/components/application_form_search_result/component.rb @@ -1,8 +1,11 @@ +# frozen_string_literal: true + module ApplicationFormSearchResult class Component < ViewComponent::Base - def initialize(application_form:) + def initialize(application_form, current_staff:) super @application_form = application_form + @current_staff = current_staff end def full_name @@ -16,6 +19,7 @@ def href def summary_rows application_form_summary_rows( application_form, + current_staff:, include_name: false, include_reviewer: application_form.reviewer.present?, class_context: "app-search-result__item", @@ -24,7 +28,7 @@ def summary_rows private - attr_reader :application_form + attr_reader :application_form, :current_staff delegate :application_form_full_name, to: :helpers delegate :application_form_summary_rows, to: :helpers diff --git a/app/helpers/application_form_helper.rb b/app/helpers/application_form_helper.rb index f2556b0d33..fbbf085e56 100644 --- a/app/helpers/application_form_helper.rb +++ b/app/helpers/application_form_helper.rb @@ -12,6 +12,7 @@ def application_form_full_name(application_form) def application_form_summary_rows( application_form, + current_staff:, include_name:, include_reviewer: true, highlight_email: false, @@ -27,6 +28,17 @@ def application_form_summary_rows( value: { text: application_form_full_name(application_form), }, + actions: [ + if AssessorInterface::ApplicationFormPolicy.new( + current_staff, + application_form, + ).edit? + { + visually_hidden_text: I18n.t("application_form.summary.name"), + href: [:edit, :assessor_interface, application_form], + } + end, + ].compact, } end ), diff --git a/app/views/assessor_interface/application_forms/index.html.erb b/app/views/assessor_interface/application_forms/index.html.erb index 6eb7429ca3..10b918ed3c 100644 --- a/app/views/assessor_interface/application_forms/index.html.erb +++ b/app/views/assessor_interface/application_forms/index.html.erb @@ -71,11 +71,11 @@
- <% if @view_object.application_forms_records.any? %> + <% if (records = @view_object.application_forms_records).present? %>
    - <%- @view_object.application_forms_records.each do |application_form| -%> - <%= render(ApplicationFormSearchResult::Component.new(application_form:)) %> - <%- end -%> + <% records.each do |application_form| %> + <%= render(ApplicationFormSearchResult::Component.new(application_form, current_staff:)) %> + <% end %>
<%= govuk_pagination(pagy: @view_object.application_forms_pagy) %> diff --git a/app/views/assessor_interface/application_forms/show.html.erb b/app/views/assessor_interface/application_forms/show.html.erb index 97dea122e5..e16d43ce8c 100644 --- a/app/views/assessor_interface/application_forms/show.html.erb +++ b/app/views/assessor_interface/application_forms/show.html.erb @@ -21,7 +21,9 @@ <%= render "shared/assessor_header", title: "Application", application_form: %> -<%= render(ApplicationFormOverview::Component.new(application_form, highlight_email: @view_object.highlight_email?)) %> +<%= render(ApplicationFormOverview::Component.new( + application_form, current_staff:, highlight_email: @view_object.highlight_email? +)) %>

Assessment

diff --git a/spec/components/application_form_overview_component_spec.rb b/spec/components/application_form_overview_component_spec.rb index ff66ec2ba7..87abbb5c1e 100644 --- a/spec/components/application_form_overview_component_spec.rb +++ b/spec/components/application_form_overview_component_spec.rb @@ -3,9 +3,12 @@ require "rails_helper" RSpec.describe ApplicationFormOverview::Component, type: :component do - subject(:component) { render_inline(described_class.new(application_form)) } + subject(:component) do + render_inline(described_class.new(application_form, current_staff:)) + end let(:application_form) { create(:application_form, :submitted) } + let(:current_staff) { create(:staff) } describe "heading" do subject(:text) { component.at_css("h2").text.strip } diff --git a/spec/components/application_form_search_result_component_spec.rb b/spec/components/application_form_search_result_component_spec.rb index b9e8a719c1..8a525c7a08 100644 --- a/spec/components/application_form_search_result_component_spec.rb +++ b/spec/components/application_form_search_result_component_spec.rb @@ -3,7 +3,9 @@ require "rails_helper" RSpec.describe ApplicationFormSearchResult::Component, type: :component do - subject(:component) { render_inline(described_class.new(application_form:)) } + subject(:component) do + render_inline(described_class.new(application_form, current_staff:)) + end let(:application_form) do create( @@ -14,6 +16,7 @@ family_name: "Family", ) end + let(:current_staff) { create(:staff) } describe "heading text" do subject(:text) { component.at_css("h2").text.strip } diff --git a/spec/helpers/application_form_helper_spec.rb b/spec/helpers/application_form_helper_spec.rb index 81c25f2f45..77cdb189ba 100644 --- a/spec/helpers/application_form_helper_spec.rb +++ b/spec/helpers/application_form_helper_spec.rb @@ -33,13 +33,27 @@ describe "#application_form_summary_rows" do subject(:summary_rows) do - application_form_summary_rows(application_form, include_name: true) + application_form_summary_rows( + application_form, + current_staff:, + include_name: true, + ) end + let(:current_staff) { create(:staff) } + it do is_expected.to eq( [ - { key: { text: "Name" }, value: { text: "Given Family" } }, + { + key: { + text: "Name", + }, + value: { + text: "Given Family", + }, + actions: [], + }, { key: { text: "Country trained in", @@ -115,10 +129,36 @@ ) end + context "user has change name permission" do + let(:current_staff) { create(:staff, :with_change_name_permission) } + + it "has an action to change the name" do + name_row = summary_rows.find { |row| row[:key][:text] == "Name" } + + expect(name_row).to eq( + { + key: { + text: "Name", + }, + value: { + text: "Given Family", + }, + actions: [ + { + visually_hidden_text: "Name", + href: [:edit, :assessor_interface, application_form], + }, + ], + }, + ) + end + end + context "include_reviewer false" do subject(:summary_rows_without_reviewer) do application_form_summary_rows( application_form, + current_staff:, include_name: true, include_reviewer: false, ) From 0f5c5df7849bbd94cd25a5fed4c6cff81e387bc7 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 7 Aug 2023 14:50:09 +0200 Subject: [PATCH 8/9] Refactor application page object This changes the page object to use the summary list object rather than look for the items directly. --- .../assessor_interface/application.rb | 24 +++++++++++++------ .../page_objects/govuk_summary_list.rb | 2 ++ .../assigning_assessor_spec.rb | 4 ++-- .../awaiting_professional_standing_spec.rb | 6 +++-- .../change_application_form_name_spec.rb | 0 .../completing_assessment_spec.rb | 10 +++++--- ...firming_english_language_exemption_spec.rb | 2 +- ...plicate_applicant_application_form_spec.rb | 2 +- .../pre_assessment_tasks_spec.rb | 2 +- .../verifying_professional_standing_spec.rb | 4 ++-- .../verifying_qualifications_spec.rb | 8 ++++--- .../verifying_references_spec.rb | 2 +- .../view_application_form_spec.rb | 2 +- .../view_timeline_events_spec.rb | 2 +- 14 files changed, 45 insertions(+), 25 deletions(-) create mode 100644 spec/system/assessor_interface/change_application_form_name_spec.rb diff --git a/spec/support/autoload/page_objects/assessor_interface/application.rb b/spec/support/autoload/page_objects/assessor_interface/application.rb index b561881941..7539de9b6b 100644 --- a/spec/support/autoload/page_objects/assessor_interface/application.rb +++ b/spec/support/autoload/page_objects/assessor_interface/application.rb @@ -7,19 +7,29 @@ class Application < SitePrism::Page element :add_note_button, ".app-inline-action .govuk-button" - section :overview, "#app-application-overview" do - element :name, "div:nth-of-type(1) > dd:nth-of-type(1)" - element :assessor_name, "div:nth-of-type(7) > dd:nth-of-type(1)" - element :reviewer_name, "div:nth-of-type(8) > dd:nth-of-type(1)" - element :status, "div:nth-of-type(10) > dd:nth-of-type(1)" - end - + section :summary_list, GovukSummaryList, ".govuk-summary-list" section :task_list, TaskList, ".app-task-list" section :management_tasks, ".app-task-list + .govuk-warning-text" do elements :links, ".govuk-link" end + def name_summary + summary_list.find_row(key: "Name") + end + + def assessor_summary + summary_list.find_row(key: "Assigned to") + end + + def reviewer_summary + summary_list.find_row(key: "Reviewer") + end + + def status_summary + summary_list.find_row(key: "Status") + end + def preliminary_check_task task_list.find_item("Preliminary check (qualifications)") end diff --git a/spec/support/autoload/page_objects/govuk_summary_list.rb b/spec/support/autoload/page_objects/govuk_summary_list.rb index d63421d986..1a5199a120 100644 --- a/spec/support/autoload/page_objects/govuk_summary_list.rb +++ b/spec/support/autoload/page_objects/govuk_summary_list.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module PageObjects class GovukSummaryList < SitePrism::Section sections :rows, ".govuk-summary-list__row" do diff --git a/spec/system/assessor_interface/assigning_assessor_spec.rb b/spec/system/assessor_interface/assigning_assessor_spec.rb index 9916442499..7993f3cd15 100644 --- a/spec/system/assessor_interface/assigning_assessor_spec.rb +++ b/spec/system/assessor_interface/assigning_assessor_spec.rb @@ -51,7 +51,7 @@ def and_i_select_an_assessor end def and_the_assessor_is_assigned_to_the_application_form - expect(assessor_application_page.overview.assessor_name.text).to eq( + expect(assessor_application_page.assessor_summary.value).to have_text( assessor.name, ) end @@ -66,7 +66,7 @@ def and_i_select_a_reviewer end def and_the_assessor_is_assigned_as_reviewer_to_the_application_form - expect(assessor_application_page.overview.reviewer_name.text).to eq( + expect(assessor_application_page.reviewer_summary.value).to have_text( assessor.name, ) end diff --git a/spec/system/assessor_interface/awaiting_professional_standing_spec.rb b/spec/system/assessor_interface/awaiting_professional_standing_spec.rb index 37117bd0fd..2b853f6db3 100644 --- a/spec/system/assessor_interface/awaiting_professional_standing_spec.rb +++ b/spec/system/assessor_interface/awaiting_professional_standing_spec.rb @@ -31,7 +31,7 @@ def given_there_is_an_application_form_with_professional_standing_request end def and_i_see_a_waiting_on_status - expect(assessor_application_page.overview.status.text).to eq( + expect(assessor_application_page.status_summary.value).to have_text( "WAITING ON PROFESSIONAL STANDING", ) end @@ -49,7 +49,9 @@ def when_i_fill_in_the_form end def and_i_see_a_not_started_status - expect(assessor_application_page.overview.status.text).to eq("NOT STARTED") + expect(assessor_application_page.status_summary.value).to have_text( + "NOT STARTED", + ) end def and_the_teacher_receives_a_professional_standing_received_email diff --git a/spec/system/assessor_interface/change_application_form_name_spec.rb b/spec/system/assessor_interface/change_application_form_name_spec.rb new file mode 100644 index 0000000000..e69de29bb2 diff --git a/spec/system/assessor_interface/completing_assessment_spec.rb b/spec/system/assessor_interface/completing_assessment_spec.rb index 9c661916e7..e3ce1e0236 100644 --- a/spec/system/assessor_interface/completing_assessment_spec.rb +++ b/spec/system/assessor_interface/completing_assessment_spec.rb @@ -374,17 +374,21 @@ def when_i_click_on_overview_button end def then_the_application_form_is_awarded - expect(assessor_application_page.overview.status.text).to eq("AWARDED") + expect(assessor_application_page.status_summary.value).to have_text( + "AWARDED", + ) end def then_the_application_form_is_waiting_on - expect(assessor_application_page.overview.status.text).to include( + expect(assessor_application_page.status_summary.value.text).to include( "WAITING ON", ) end def then_the_application_form_is_declined - expect(assessor_application_page.overview.status.text).to eq("DECLINED") + expect(assessor_application_page.status_summary.value).to have_text( + "DECLINED", + ) end def application_id diff --git a/spec/system/assessor_interface/confirming_english_language_exemption_spec.rb b/spec/system/assessor_interface/confirming_english_language_exemption_spec.rb index 8a25b12bd9..2930b111eb 100644 --- a/spec/system/assessor_interface/confirming_english_language_exemption_spec.rb +++ b/spec/system/assessor_interface/confirming_english_language_exemption_spec.rb @@ -167,7 +167,7 @@ def and_the_application_english_language_proof_method_is_moi end def then_i_see_the_application - expect(assessor_application_page.overview.name.text).to eq( + expect(assessor_application_page.name_summary.value).to have_text( "#{application_form.given_names} #{application_form.family_name}", ) end diff --git a/spec/system/assessor_interface/duplicate_applicant_application_form_spec.rb b/spec/system/assessor_interface/duplicate_applicant_application_form_spec.rb index 94af4833f4..e468712d6e 100644 --- a/spec/system/assessor_interface/duplicate_applicant_application_form_spec.rb +++ b/spec/system/assessor_interface/duplicate_applicant_application_form_spec.rb @@ -24,7 +24,7 @@ def when_i_click_back_link end def then_i_see_the_application - expect(assessor_application_page.overview.name.text).to eq( + expect(assessor_application_page.name_summary.value).to have_text( "#{application_form.given_names} #{application_form.family_name}", ) end diff --git a/spec/system/assessor_interface/pre_assessment_tasks_spec.rb b/spec/system/assessor_interface/pre_assessment_tasks_spec.rb index 9680f8fdb8..05edea049f 100644 --- a/spec/system/assessor_interface/pre_assessment_tasks_spec.rb +++ b/spec/system/assessor_interface/pre_assessment_tasks_spec.rb @@ -49,7 +49,7 @@ def given_there_is_an_application_form_with_professional_standing_request end def and_i_see_a_waiting_on_status - expect(assessor_application_page.overview.status.text).to eq( + expect(assessor_application_page.status_summary.value).to have_text( "PRELIMINARY CHECK", ) end diff --git a/spec/system/assessor_interface/verifying_professional_standing_spec.rb b/spec/system/assessor_interface/verifying_professional_standing_spec.rb index b6e5b3aa76..0ed8b851e5 100644 --- a/spec/system/assessor_interface/verifying_professional_standing_spec.rb +++ b/spec/system/assessor_interface/verifying_professional_standing_spec.rb @@ -39,7 +39,7 @@ def given_there_is_an_application_form_with_professional_standing_request end def and_i_see_a_waiting_on_status - expect(assessor_application_page.overview.status.text).to eq( + expect(assessor_application_page.status_summary.value).to have_text( "WAITING ON PROFESSIONAL STANDING", ) end @@ -74,7 +74,7 @@ def when_i_fill_in_the_review_form end def and_i_see_a_received_status - expect(assessor_application_page.overview.status.text).to eq( + expect(assessor_application_page.status_summary.value).to have_text( "RECEIVED PROFESSIONAL STANDING", ) end diff --git a/spec/system/assessor_interface/verifying_qualifications_spec.rb b/spec/system/assessor_interface/verifying_qualifications_spec.rb index 0a0b63c313..08c41983c0 100644 --- a/spec/system/assessor_interface/verifying_qualifications_spec.rb +++ b/spec/system/assessor_interface/verifying_qualifications_spec.rb @@ -80,7 +80,7 @@ def given_there_is_an_application_form_with_qualification_request end def and_i_see_a_waiting_on_status - expect(assessor_application_page.overview.status.text).to eq( + expect(assessor_application_page.status_summary.value).to have_text( "WAITING ON QUALIFICATION", ) end @@ -139,11 +139,13 @@ def when_i_go_back_to_overview end def and_i_see_a_received_status - expect(assessor_application_page.overview.status.text).to eq("RECEIVED") + expect(assessor_application_page.status_summary.value).to have_text( + "RECEIVED", + ) end def and_i_see_an_in_progress_status - expect(assessor_application_page.overview.status.text).to eq( + expect(assessor_application_page.status_summary.value).to have_text( "ASSESSMENT IN PROGRESS", ) end diff --git a/spec/system/assessor_interface/verifying_references_spec.rb b/spec/system/assessor_interface/verifying_references_spec.rb index ff20338f4a..94ddbabbd4 100644 --- a/spec/system/assessor_interface/verifying_references_spec.rb +++ b/spec/system/assessor_interface/verifying_references_spec.rb @@ -38,7 +38,7 @@ def given_there_is_an_application_form_with_reference_request end def and_i_see_a_waiting_on_status - expect(assessor_application_page.overview.status.text).to eq( + expect(assessor_application_page.status_summary.value).to have_text( "WAITING ON REFERENCE", ) end diff --git a/spec/system/assessor_interface/view_application_form_spec.rb b/spec/system/assessor_interface/view_application_form_spec.rb index aba5fb0f96..a3f9b4c0a9 100644 --- a/spec/system/assessor_interface/view_application_form_spec.rb +++ b/spec/system/assessor_interface/view_application_form_spec.rb @@ -25,7 +25,7 @@ def when_i_click_back_link end def then_i_see_the_application - expect(assessor_application_page.overview.name.text).to eq( + expect(assessor_application_page.name_summary.value).to have_text( "#{application_form.given_names} #{application_form.family_name}", ) end diff --git a/spec/system/assessor_interface/view_timeline_events_spec.rb b/spec/system/assessor_interface/view_timeline_events_spec.rb index 95e82d90d8..a14eee26aa 100644 --- a/spec/system/assessor_interface/view_timeline_events_spec.rb +++ b/spec/system/assessor_interface/view_timeline_events_spec.rb @@ -25,7 +25,7 @@ def given_an_application_form_exists end def and_i_click_view_timeline - assessor_application_page.overview.click_link( + assessor_application_page.click_link( "View timeline of this applications actions", ) end From ffcab97cebd6eaa6e34529d1ed7d216807c13e96 Mon Sep 17 00:00:00 2001 From: Thomas Leese Date: Mon, 7 Aug 2023 14:52:12 +0200 Subject: [PATCH 9/9] Add ability to change applicant name This adds the ability for an assessor with the right permissions to change the applicant name associated with an application. --- .../application_forms_controller.rb | 28 ++++++++ .../application_forms/edit.html.erb | 34 +++++++++ config/routes.rb | 4 +- .../assessor_interface/edit_application.rb | 17 +++++ spec/support/page_helpers.rb | 5 ++ .../change_application_form_name_spec.rb | 72 +++++++++++++++++++ 6 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 app/views/assessor_interface/application_forms/edit.html.erb create mode 100644 spec/support/autoload/page_objects/assessor_interface/edit_application.rb diff --git a/app/controllers/assessor_interface/application_forms_controller.rb b/app/controllers/assessor_interface/application_forms_controller.rb index 07a0c0d04e..6df1b0ef26 100644 --- a/app/controllers/assessor_interface/application_forms_controller.rb +++ b/app/controllers/assessor_interface/application_forms_controller.rb @@ -32,6 +32,27 @@ def status @view_object = show_view_object end + def edit + authorize [:assessor_interface, application_form] + + @form = ApplicationFormNameForm.new + end + + def update + authorize [:assessor_interface, application_form] + + @form = + ApplicationFormNameForm.new( + form_params.merge(application_form:, user: current_staff), + ) + + if @form.save + redirect_to [:assessor_interface, application_form] + else + render :edit, status: :unprocessable_entity + end + end + def withdraw authorize [:assessor_interface, application_form] end @@ -65,5 +86,12 @@ def show_view_object def application_form @application_form ||= show_view_object.application_form end + + def form_params + params.require(:assessor_interface_application_form_name_form).permit( + :given_names, + :family_name, + ) + end end end diff --git a/app/views/assessor_interface/application_forms/edit.html.erb b/app/views/assessor_interface/application_forms/edit.html.erb new file mode 100644 index 0000000000..19eaf45ce6 --- /dev/null +++ b/app/views/assessor_interface/application_forms/edit.html.erb @@ -0,0 +1,34 @@ +<% title = "Change details for ‘#{application_form_full_name(@application_form)}’" %> + +<% content_for :page_title, "#{"Error: " if @form.errors.any?}#{title}" %> +<% content_for :back_link_url, assessor_interface_application_form_path(@application_form) %> + +<%= form_with model: @form, url: [:assessor_interface, @application_form], method: :put do |f| %> + <%= f.govuk_error_summary %> + +

<%= title %>

+ +

Current name

+ + <%= govuk_summary_list(actions: false) do |summary_list| + summary_list.with_row do |row| + row.with_key { "Given names" } + row.with_value { @application_form.given_names } + end + + summary_list.with_row do |row| + row.with_key { "Family name" } + row.with_value { @application_form.family_name } + end + end %> + +

Change name

+

Use this form to change one or more of the names.

+ + <%= f.govuk_text_field :given_names %> + <%= f.govuk_text_field :family_name %> + + <%= f.govuk_submit do %> + <%= govuk_link_to "Cancel", [:assessor_interface, @application_form] %> + <% end %> +<% end %> diff --git a/config/routes.rb b/config/routes.rb index 0f4ba1bd7d..7579986f51 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -14,9 +14,7 @@ namespace :assessor_interface, path: "/assessor" do root to: redirect("/assessor/applications") - resources :application_forms, - path: "/applications", - only: %i[index show destroy] do + resources :application_forms, path: "/applications", except: :new do collection do post "filters/apply", to: "application_forms#apply_filters" get "filters/clear", to: "application_forms#clear_filters" diff --git a/spec/support/autoload/page_objects/assessor_interface/edit_application.rb b/spec/support/autoload/page_objects/assessor_interface/edit_application.rb new file mode 100644 index 0000000000..a690df1cd8 --- /dev/null +++ b/spec/support/autoload/page_objects/assessor_interface/edit_application.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module PageObjects + module AssessorInterface + class EditApplication < SitePrism::Page + set_url "/assessor/applications/{application_form_id}/edit" + + section :form, "form" do + element :given_names_field, + "#assessor-interface-application-form-name-form-given-names-field" + element :family_name_field, + "#assessor-interface-application-form-name-form-family-name-field" + element :submit_button, ".govuk-button" + end + end + end +end diff --git a/spec/support/page_helpers.rb b/spec/support/page_helpers.rb index 935f59f849..f67c1e11ab 100644 --- a/spec/support/page_helpers.rb +++ b/spec/support/page_helpers.rb @@ -27,6 +27,11 @@ def assessor_assessment_section_page PageObjects::AssessorInterface::AssessmentSection.new end + def assessor_edit_application_page + @assessor_edit_application_page ||= + PageObjects::AssessorInterface::EditApplication.new + end + def assessor_edit_professional_standing_request_location_page @assessor_edit_professional_standing_request_location_page ||= PageObjects::AssessorInterface::EditProfessionalStandingRequestLocation.new diff --git a/spec/system/assessor_interface/change_application_form_name_spec.rb b/spec/system/assessor_interface/change_application_form_name_spec.rb index e69de29bb2..0bbc3d7190 100644 --- a/spec/system/assessor_interface/change_application_form_name_spec.rb +++ b/spec/system/assessor_interface/change_application_form_name_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe "Assessor change application form name", type: :system do + before do + given_the_service_is_open + given_there_is_an_application_form + end + + it "checks manage applications permission" do + given_i_am_authorized_as_a_user(assessor) + + when_i_visit_the(:assessor_edit_application_page, application_form_id:) + then_i_see_the_forbidden_page + end + + it "allows changing application form name" do + given_i_am_authorized_as_a_user(manager) + + when_i_visit_the(:assessor_application_page, application_id:) + then_i_see_the(:assessor_application_page) + + when_i_click_on_change_name + then_i_see_the(:assessor_edit_application_page, application_form_id:) + + when_i_fill_in_the_name + then_i_see_the(:assessor_application_page, application_id:) + end + + def given_there_is_an_application_form + application_form + end + + def when_i_click_on_change_name + assessor_application_page + .summary_list + .find_row(key: "Name") + .actions + .link + .click + end + + def when_i_fill_in_the_name + assessor_edit_application_page.form.given_names_field.fill_in with: + "New given names" + assessor_edit_application_page.form.family_name_field.fill_in with: + "New family name" + assessor_edit_application_page.form.submit_button.click + end + + def application_form + @application_form ||= + create( + :application_form, + :submitted, + :with_personal_information, + :with_assessment, + ) + end + + delegate :id, to: :application_form, prefix: true + alias_method :application_id, :application_form_id + + def assessor + create(:staff, :confirmed, :with_award_decline_permission) + end + + def manager + create(:staff, :confirmed, :with_change_name_permission) + end +end