From 4331594e6da1963b46bdd0968a46afb1996fb82b Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Mon, 9 Mar 2026 15:49:05 +0000 Subject: [PATCH 01/10] Add service to send alerts to organisation admins Add a service with a method to send the appropriate email when a form is made live. The email to send depends on the previous state of the form, and whether the form is a copy if it was in the 'draft' state. --- app/services/org_admin_alerts_service.rb | 41 +++++ .../services/org_admin_alerts_service_spec.rb | 159 ++++++++++++++++++ 2 files changed, 200 insertions(+) create mode 100644 app/services/org_admin_alerts_service.rb create mode 100644 spec/services/org_admin_alerts_service_spec.rb diff --git a/app/services/org_admin_alerts_service.rb b/app/services/org_admin_alerts_service.rb new file mode 100644 index 000000000..20cd59ae8 --- /dev/null +++ b/app/services/org_admin_alerts_service.rb @@ -0,0 +1,41 @@ +class OrgAdminAlertsService + def initialize(form:, current_user:) + @form = form + @current_user = current_user + @org_admins = @form.group.organisation.admin_users.where.not(id: @current_user.id) + end + + def form_made_live + @org_admins.each do |org_admin_user| + form_made_live_email(to_email: org_admin_user.email).deliver_now + end + end + +private + + def form_made_live_email(to_email:) + previous_state = @form.state_previously_was.to_sym + + case previous_state + when :draft + new_draft_made_live_email(to_email:) + when :live_with_draft + OrgAdminAlerts::MadeLiveMailer.live_form_changes_made_live(form: @form, user: @current_user, to_email:) + when :archived + OrgAdminAlerts::MadeLiveMailer.archived_form_made_live(form: @form, user: @current_user, to_email:) + when :archived_with_draft + OrgAdminAlerts::MadeLiveMailer.archived_form_changes_made_live(form: @form, user: @current_user, to_email:) + else + raise StandardError, "Unexpected previous state: #{previous_state}" + end + end + + def new_draft_made_live_email(to_email:) + if @form.copied_from_id.present? + copied_from_form = Form.find(@form.copied_from_id) + OrgAdminAlerts::MadeLiveMailer.copied_form_made_live(form: @form, copied_from_form:, user: @current_user, to_email:) + else + OrgAdminAlerts::MadeLiveMailer.new_draft_form_made_live(form: @form, user: @current_user, to_email:) + end + end +end diff --git a/spec/services/org_admin_alerts_service_spec.rb b/spec/services/org_admin_alerts_service_spec.rb new file mode 100644 index 000000000..cdbc98b0a --- /dev/null +++ b/spec/services/org_admin_alerts_service_spec.rb @@ -0,0 +1,159 @@ +require "rails_helper" + +RSpec.describe OrgAdminAlertsService do + subject(:service) { described_class.new(form:, current_user:) } + + let(:form) { create(:form, :ready_for_live, state: previous_state) } + let(:organisation) { create(:organisation, :with_signed_mou) } + let(:group) { create(:group, organisation:) } + let(:current_user) { create(:organisation_admin_user, organisation:) } + let!(:organisation_admins) { create_list(:organisation_admin_user, 2, organisation:) } + + before do + GroupForm.create!(form: form, group:) + end + + describe "#form_made_live" do + before do + form.make_live! unless form.live? + end + + context "when the previous state is draft" do + let(:previous_state) { :draft } + + context "when form was not copied from another form" do + it "sends the new draft form made live email to the organisation admins" do + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:new_draft_form_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:new_draft_form_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.form_made_live + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + + it "does not send an email to the current user" do + allow(OrgAdminAlerts::MadeLiveMailer).to receive(:new_draft_form_made_live).and_call_original + service.form_made_live + + expect(OrgAdminAlerts::MadeLiveMailer).not_to have_received(:new_draft_form_made_live).with( + form: form, + user: current_user, + to_email: current_user.email, + ) + end + end + + context "when form was copied from another form" do + let(:copied_from_form) { create(:form, :ready_for_live) } + let(:form) { create(:form, :ready_for_live, state: previous_state, copied_from_id: copied_from_form.id) } + + it "sends the copied form made live email to the organisation admins" do + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:copied_form_made_live).with( + form: form, + copied_from_form: copied_from_form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:copied_form_made_live).with( + form: form, + copied_from_form: copied_from_form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.form_made_live + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + end + end + + context "when the previous state is live with draft" do + let(:previous_state) { :live_with_draft } + + it "sends the live form changes made live email to the organisation admins" do + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:live_form_changes_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:live_form_changes_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.form_made_live + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + end + + context "when the previous state is archived" do + let(:previous_state) { :archived } + + it "sends the archived form made live email to the organisation admins" do + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:archived_form_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:archived_form_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.form_made_live + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + end + + context "when the previous state is archived with draft" do + let(:previous_state) { :archived_with_draft } + + it "sends the archived form changes made live email to the organisation admins" do + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:archived_form_changes_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::MadeLiveMailer).to receive(:archived_form_changes_made_live).with( + form: form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.form_made_live + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + end + + context "when the previous state is unexpected" do + let(:previous_state) { :live } + + it "raises an error and does not send any emails" do + expect { + service.form_made_live + }.to raise_error(StandardError, "Unexpected previous state: live") + + expect(ActionMailer::Base.deliveries.size).to eq(0) + end + end + end +end From 733da3409a319dace3fa571dc257788704c43f21 Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Mon, 9 Mar 2026 16:00:10 +0000 Subject: [PATCH 02/10] Send alerts to org admins when a form is made live When a form is made live, call the OrgAdminAlertsService to send the appropriate alerts to org admins. This is done in the controller as we need to know the user making the change to include in the alert. --- app/controllers/forms/make_live_controller.rb | 4 ++++ .../forms/make_live_controller_spec.rb | 24 +++++++++++++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/app/controllers/forms/make_live_controller.rb b/app/controllers/forms/make_live_controller.rb index d969588c0..b8a9a2b93 100644 --- a/app/controllers/forms/make_live_controller.rb +++ b/app/controllers/forms/make_live_controller.rb @@ -17,6 +17,10 @@ def create @make_form_live_service = MakeFormLiveService.call(current_form:, current_user:) @make_form_live_service.make_live unless current_form.live? + if current_form.state_previously_changed? + OrgAdminAlertsService.new(form: current_form, current_user:).form_made_live + end + render "confirmation", locals: { current_form:, confirmation_page_title: @make_form_live_service.page_title, confirmation_page_body: @make_form_live_service.confirmation_page_body } end diff --git a/spec/requests/forms/make_live_controller_spec.rb b/spec/requests/forms/make_live_controller_spec.rb index 55d0a508a..566b5b0f6 100644 --- a/spec/requests/forms/make_live_controller_spec.rb +++ b/spec/requests/forms/make_live_controller_spec.rb @@ -1,14 +1,15 @@ require "rails_helper" RSpec.describe Forms::MakeLiveController, type: :request do - let(:user) { build :user } + let(:user) { build :user, organisation: } let(:form) { create(:form, :ready_for_live) } let(:id) { form.id } let(:form_params) { nil } + let(:organisation) { test_org } let(:group_role) { :group_admin } - let(:group) { create(:group, organisation: user.organisation, status: :active) } + let(:group) { create(:group, organisation:, status: :active) } describe "#new" do before do @@ -59,6 +60,7 @@ before do Membership.create!(group_id: group.id, user:, added_by: user, role: group_role) GroupForm.create!(form_id: form.id, group_id: group.id) + create(:organisation_admin_user, organisation:) login_as user end @@ -76,6 +78,14 @@ expect(response).to render_template(:confirmation) end + it "sends an email to the organisation admins" do + post(make_live_path(form_id: form.id), params: form_params) + expect(ActionMailer::Base.deliveries.count).to eq(1) + + template_id = Settings.govuk_notify.org_admin_alerts.new_draft_form_made_live_template_id + expect(ActionMailer::Base.deliveries.last.govuk_notify_template).to eq(template_id) + end + context "and that form has not been made live before" do it "has the page title 'Your form is live'" do post(make_live_path(form_id: form.id), params: form_params) @@ -108,6 +118,11 @@ post(make_live_path(form_id: form.id), params: form_params) }.not_to(change { form.reload.live_form_document.updated_at }) end + + it "does not send an email to the organisation admins" do + post(make_live_path(form_id: form.id), params: form_params) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end end context "and has draft changes" do @@ -145,6 +160,11 @@ it "redirects you to the form page" do expect(response).to redirect_to(form_path(form.id)) end + + it "does not send an email to the organisation admins" do + post(make_live_path(form_id: form.id), params: form_params) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end end context "when all tasks are not complete" do From 9a624622a21a04f8d4d25ca640c0ceabdb4781fa Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Wed, 11 Mar 2026 16:10:23 +0000 Subject: [PATCH 03/10] Send alerts to org admins when a form is unarchived There is a separate controller for making an archived form live agin without changes. When the form is unarchived, send the appropriate alerts to org admins. --- app/controllers/forms/unarchive_controller.rb | 4 ++++ .../forms/unarchive_controller_spec.rb | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/app/controllers/forms/unarchive_controller.rb b/app/controllers/forms/unarchive_controller.rb index 95a1c72fd..4c806b4e9 100644 --- a/app/controllers/forms/unarchive_controller.rb +++ b/app/controllers/forms/unarchive_controller.rb @@ -17,6 +17,10 @@ def create @make_form_live_service = MakeFormLiveService.call(current_form:, current_user:) @make_form_live_service.make_live + if current_form.state_previously_changed? + OrgAdminAlertsService.new(form: current_form, current_user:).form_made_live + end + render "forms/make_live/confirmation", locals: { current_form:, confirmation_page_title: @make_form_live_service.page_title, confirmation_page_body: @make_form_live_service.confirmation_page_body } end diff --git a/spec/requests/forms/unarchive_controller_spec.rb b/spec/requests/forms/unarchive_controller_spec.rb index 2502110b8..abc3f07c0 100644 --- a/spec/requests/forms/unarchive_controller_spec.rb +++ b/spec/requests/forms/unarchive_controller_spec.rb @@ -39,6 +39,7 @@ before do Membership.create!(group_id: group.id, user: standard_user, added_by: standard_user, role: :group_admin) GroupForm.create!(form_id: form.id, group_id: group.id) + create(:organisation_admin_user, organisation: standard_user.organisation) login_as user end @@ -61,6 +62,14 @@ post(unarchive_create_path(form_id: form.id), params: form_params) expect(response.body).to include "Your form is live" end + + it "sends an email to the organisation admins" do + post(unarchive_create_path(form_id: form.id), params: form_params) + expect(ActionMailer::Base.deliveries.count).to eq(1) + + template_id = Settings.govuk_notify.org_admin_alerts.archived_form_made_live_template_id + expect(ActionMailer::Base.deliveries.last.govuk_notify_template).to eq(template_id) + end end context "when deciding not to make a form live again" do @@ -76,6 +85,11 @@ post(unarchive_create_path(form_id: form.id), params: form_params) expect(response).to redirect_to(archived_form_path(form.id)) end + + it "does not send an email to the organisation admins" do + post(make_live_path(form_id: form.id), params: form_params) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end end context "when no option is selected" do @@ -97,6 +111,11 @@ expect(response).to render_template("unarchive_form") expect(response.body).to include("You must choose an option") end + + it "does not send an email to the organisation admins" do + post(make_live_path(form_id: form.id), params: form_params) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end end context "when current user does not belong to the forms group" do From a94241148ee470acf3e19a7ccadea0aec441a4a7 Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Mon, 9 Mar 2026 16:15:37 +0000 Subject: [PATCH 04/10] Add method to alert admins when a new draft form is created Add a method that calls the correct mailer to send an email to organisation admins when a new draft form is created. The mailer to user depends on whether the form is a copy of another form or not. --- app/services/org_admin_alerts_service.rb | 23 +++++- .../services/org_admin_alerts_service_spec.rb | 77 ++++++++++++++++++- 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/app/services/org_admin_alerts_service.rb b/app/services/org_admin_alerts_service.rb index 20cd59ae8..c027927dc 100644 --- a/app/services/org_admin_alerts_service.rb +++ b/app/services/org_admin_alerts_service.rb @@ -11,6 +11,14 @@ def form_made_live end end + def new_draft_form_created + return unless @form.group.active? + + @org_admins.each do |org_admin_user| + new_draft_form_created_email(to_email: org_admin_user.email).deliver_now + end + end + private def form_made_live_email(to_email:) @@ -31,11 +39,22 @@ def form_made_live_email(to_email:) end def new_draft_made_live_email(to_email:) - if @form.copied_from_id.present? - copied_from_form = Form.find(@form.copied_from_id) + if copied_from_form OrgAdminAlerts::MadeLiveMailer.copied_form_made_live(form: @form, copied_from_form:, user: @current_user, to_email:) else OrgAdminAlerts::MadeLiveMailer.new_draft_form_made_live(form: @form, user: @current_user, to_email:) end end + + def new_draft_form_created_email(to_email:) + if copied_from_form + OrgAdminAlerts::DraftCreatedMailer.copied_draft_form_created(form: @form, copied_from_form:, user: @current_user, to_email:) + else + OrgAdminAlerts::DraftCreatedMailer.new_draft_form_created(form: @form, user: @current_user, to_email:) + end + end + + def copied_from_form + Form.find_by(id: @form.copied_from_id) + end end diff --git a/spec/services/org_admin_alerts_service_spec.rb b/spec/services/org_admin_alerts_service_spec.rb index cdbc98b0a..e0c4edeba 100644 --- a/spec/services/org_admin_alerts_service_spec.rb +++ b/spec/services/org_admin_alerts_service_spec.rb @@ -3,9 +3,8 @@ RSpec.describe OrgAdminAlertsService do subject(:service) { described_class.new(form:, current_user:) } - let(:form) { create(:form, :ready_for_live, state: previous_state) } let(:organisation) { create(:organisation, :with_signed_mou) } - let(:group) { create(:group, organisation:) } + let(:group) { create(:group, organisation:, status: :active) } let(:current_user) { create(:organisation_admin_user, organisation:) } let!(:organisation_admins) { create_list(:organisation_admin_user, 2, organisation:) } @@ -14,6 +13,8 @@ end describe "#form_made_live" do + let(:form) { create(:form, :ready_for_live, state: previous_state) } + before do form.make_live! unless form.live? end @@ -156,4 +157,76 @@ end end end + + describe "#new_draft_form_created" do + let(:form) { create(:form) } + + context "when form was not copied from another form" do + it "sends the new draft form created email to the organisation admins" do + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_draft_form_created).with( + form: form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_draft_form_created).with( + form: form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.new_draft_form_created + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + + it "does not send an email to the current user" do + allow(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_draft_form_created).and_call_original + service.new_draft_form_created + + expect(OrgAdminAlerts::DraftCreatedMailer).not_to have_received(:new_draft_form_created).with( + form: form, + user: current_user, + to_email: current_user.email, + ) + end + end + + context "when form was copied from another form" do + let(:copied_from_form) { create(:form, :ready_for_live) } + let(:form) { create(:form, :ready_for_live, copied_from_id: copied_from_form.id) } + + it "sends the copied draft form created email to the organisation admins" do + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:copied_draft_form_created).with( + form: form, + copied_from_form: copied_from_form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:copied_draft_form_created).with( + form: form, + copied_from_form: copied_from_form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.new_draft_form_created + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + end + + context "when the group is not active" do + before do + group.trial! + end + + it "does not send any emails" do + service.new_draft_form_created + + expect(ActionMailer::Base.deliveries.size).to eq(0) + end + end + end end From 536683c166899405abbe9d44a93d3cf33e720678 Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Mon, 9 Mar 2026 16:26:01 +0000 Subject: [PATCH 05/10] Send alerts to org admins when a new form is created When a new form is successfully created, call the OrgAdminAlerService to send an email to all organisation admins. The controller does not need to check if the group is active, as the service does this. --- app/controllers/group_forms_controller.rb | 1 + spec/requests/group_forms_controller_spec.rb | 20 +++++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/controllers/group_forms_controller.rb b/app/controllers/group_forms_controller.rb index b3ba63311..4225783e1 100644 --- a/app/controllers/group_forms_controller.rb +++ b/app/controllers/group_forms_controller.rb @@ -19,6 +19,7 @@ def create if @name_input.valid? @form = CreateFormService.new.create!(creator: @current_user, group: @group, name: @name_input.name) + OrgAdminAlertsService.new(form: @form, current_user:).new_draft_form_created redirect_to form_path(@form.id) else diff --git a/spec/requests/group_forms_controller_spec.rb b/spec/requests/group_forms_controller_spec.rb index ed78a2982..458da8bc1 100644 --- a/spec/requests/group_forms_controller_spec.rb +++ b/spec/requests/group_forms_controller_spec.rb @@ -1,7 +1,8 @@ require "rails_helper" RSpec.describe "/groups/:group_id/forms", type: :request do - let(:group) { create :group } + let(:group) { create :group, organisation:, status: :active } + let(:organisation) { test_org } let(:nonexistent_group) { "foobar" } before do @@ -106,6 +107,10 @@ { name: "" } end + before do + create(:organisation_admin_user, organisation:) + end + context "with valid parameters" do it "creates a form" do expect { @@ -125,6 +130,14 @@ post group_forms_url(group), params: { forms_name_input: valid_attributes } expect(response).to redirect_to(form_url(Form.last.id)) end + + it "sends an email to the organisation admins" do + post group_forms_url(group), params: { forms_name_input: valid_attributes } + expect(ActionMailer::Base.deliveries.count).to eq(1) + + template_id = Settings.govuk_notify.org_admin_alerts.new_draft_form_created_template_id + expect(ActionMailer::Base.deliveries.last.govuk_notify_template).to eq(template_id) + end end context "with invalid parameters" do @@ -147,6 +160,11 @@ expect(response).to render_template("input_objects/forms/_name_input") expect(response.body).to include I18n.t("error_summary.heading") end + + it "does not send an email to the organisation admins", :feature_org_admin_alerts_enabled do + post group_forms_url(group), params: { forms_name_input: invalid_attributes } + expect(ActionMailer::Base.deliveries.count).to eq(0) + end end context "when the current user does not have access to the group" do From 490694f8a2c64a40c7131b5facebd97f8ecaad2f Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Mon, 9 Mar 2026 17:04:35 +0000 Subject: [PATCH 06/10] Send alerts to org admins when a form is copied When a form is successfully copied, call the OrgAdminAlerService to send an email to all organisation admins. The controller does not need to check if the group is active, as the service does this. --- app/controllers/forms/copy_controller.rb | 3 +++ spec/requests/forms/copy_controller_spec.rb | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/controllers/forms/copy_controller.rb b/app/controllers/forms/copy_controller.rb index ef54fa778..c58adfb22 100644 --- a/app/controllers/forms/copy_controller.rb +++ b/app/controllers/forms/copy_controller.rb @@ -25,6 +25,9 @@ def create if @copy_input.submit copied_form = FormCopyService.new(current_form, current_user).copy(tag: @copy_input.tag) copied_form.update!(name: @copy_input.name) + + OrgAdminAlertsService.new(form: copied_form, current_user:).new_draft_form_created + redirect_to form_path(copied_form.id), success: t("banner.success.form.copied") else set_back_link(current_form) diff --git a/spec/requests/forms/copy_controller_spec.rb b/spec/requests/forms/copy_controller_spec.rb index 8b1654ae7..361d525fc 100644 --- a/spec/requests/forms/copy_controller_spec.rb +++ b/spec/requests/forms/copy_controller_spec.rb @@ -4,7 +4,7 @@ let(:id) { form.id } let(:form) { create(:form) } - let(:group) { create(:group, organisation: standard_user.organisation) } + let(:group) { create(:group, organisation: standard_user.organisation, status: :active) } before do Membership.create!(group_id: group.id, user: standard_user, added_by: standard_user) @@ -54,6 +54,10 @@ end describe "#create" do + before do + create(:organisation_admin_user, organisation: standard_user.organisation) + end + context "when the copy is successful" do it "redirects to the form page" do post create_copy_form_path(id), params: { forms_copy_input: { name: "Copied Form", tag: "draft" } } @@ -61,6 +65,14 @@ expect(response).to have_http_status(:found) expect(response).to redirect_to(form_path(Form.last.id)) end + + it "sends an email to the organisation admins" do + post create_copy_form_path(id), params: { forms_copy_input: { name: "Copied Form", tag: "draft" } } + expect(ActionMailer::Base.deliveries.count).to eq(1) + + template_id = Settings.govuk_notify.org_admin_alerts.copied_draft_form_created_template_id + expect(ActionMailer::Base.deliveries.last.govuk_notify_template).to eq(template_id) + end end context "when the copy fails" do @@ -70,6 +82,11 @@ expect(response).to have_http_status(:ok) expect(response).to render_template(:confirm) end + + it "does not send an email to the organisation admins", :feature_org_admin_alerts_enabled do + post create_copy_form_path(id), params: { forms_copy_input: { name: "", tag: "draft" } } + expect(ActionMailer::Base.deliveries.count).to eq(0) + end end end end From 9eaa1d827b5bb838032a8deafc706856b8509124 Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Mon, 9 Mar 2026 17:59:03 +0000 Subject: [PATCH 07/10] Add method to alert admins when a draft of an existing form is created Add a method that sends an email to all organisation admins when a draft of a live or archived form is created. We do not need to check whether the group is active in this case, as the group would need to be active for the form to have been made live. --- app/services/org_admin_alerts_service.rb | 17 +++++ .../services/org_admin_alerts_service_spec.rb | 69 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/app/services/org_admin_alerts_service.rb b/app/services/org_admin_alerts_service.rb index c027927dc..620bf4f94 100644 --- a/app/services/org_admin_alerts_service.rb +++ b/app/services/org_admin_alerts_service.rb @@ -19,6 +19,12 @@ def new_draft_form_created end end + def draft_of_existing_form_created + @org_admins.each do |org_admin_user| + draft_of_existing_form_created_email(to_email: org_admin_user.email).deliver_now + end + end + private def form_made_live_email(to_email:) @@ -54,6 +60,17 @@ def new_draft_form_created_email(to_email:) end end + def draft_of_existing_form_created_email(to_email:) + case @form.state.to_sym + when :live_with_draft + OrgAdminAlerts::DraftCreatedMailer.new_live_form_draft_created(form: @form, user: @current_user, to_email:) + when :archived_with_draft + OrgAdminAlerts::DraftCreatedMailer.new_archived_form_draft_created(form: @form, user: @current_user, to_email:) + else + raise StandardError, "Unexpected form state: #{@form.state}" + end + end + def copied_from_form Form.find_by(id: @form.copied_from_id) end diff --git a/spec/services/org_admin_alerts_service_spec.rb b/spec/services/org_admin_alerts_service_spec.rb index e0c4edeba..3d1a1c7b5 100644 --- a/spec/services/org_admin_alerts_service_spec.rb +++ b/spec/services/org_admin_alerts_service_spec.rb @@ -229,4 +229,73 @@ end end end + + describe "#draft_of_existing_form_created" do + context "when the form state is live with draft" do + let(:form) { create(:form, :live_with_draft) } + + it "sends the live form changes made live email to the organisation admins" do + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_live_form_draft_created).with( + form: form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_live_form_draft_created).with( + form: form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.draft_of_existing_form_created + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + + it "does not send an email to the current user" do + allow(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_live_form_draft_created).and_call_original + service.draft_of_existing_form_created + + expect(OrgAdminAlerts::DraftCreatedMailer).not_to have_received(:new_live_form_draft_created).with( + form: form, + user: current_user, + to_email: current_user.email, + ) + end + end + + context "when the form state is archived with draft" do + let(:form) { create(:form, :archived_with_draft) } + + it "sends the archived form changes made live email to the organisation admins" do + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_archived_form_draft_created).with( + form: form, + user: current_user, + to_email: organisation_admins.first.email, + ).and_call_original + + expect(OrgAdminAlerts::DraftCreatedMailer).to receive(:new_archived_form_draft_created).with( + form: form, + user: current_user, + to_email: organisation_admins.second.email, + ).and_call_original + + service.draft_of_existing_form_created + + expect(ActionMailer::Base.deliveries.size).to eq(2) + end + end + + context "when the form state is unexpected" do + let(:form) { create(:form) } + + it "raises an error and does not send any emails" do + expect { + service.draft_of_existing_form_created + }.to raise_error(StandardError, "Unexpected form state: draft") + + expect(ActionMailer::Base.deliveries.size).to eq(0) + end + end + end end From 84b5eeb4bb275a8e148fe4bcc57d183ca2511233 Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Tue, 10 Mar 2026 15:09:18 +0000 Subject: [PATCH 08/10] Send alerts to org admins when a draft is created for an existing form Send an email to organisation admins when a draft is created for an existing live or archived form. This is added as an after_action in the FormsController base controller as any controller action that updates a form will create a draft if the form is currently in a live or archived state. We need to store the initial state of the form in the controller to compare to in the after_action to determine if a draft was created. This is because the form could have been updated in a separate instance of the Form model, which is the case when adding a new question to the form - so we cannot use the `state_previously_was` helper method provided by ActiveModel::Dirty. As all controllers that inherit from FormsController use the `current_form`, we can set the instance variable in a before_action along with the @initial_form_state. This makes the controller tests for FormsController redundant as they were just checking the that form was only retrieved once. --- app/controllers/forms_controller.rb | 20 +++++-- app/models/form.rb | 9 ++++ spec/controllers/forms_controller_spec.rb | 26 --------- spec/models/form_spec.rb | 39 ++++++++++++++ .../submission_attachments_controller_spec.rb | 6 ++- spec/requests/forms_controller_spec.rb | 54 +++++++++++++++++++ 6 files changed, 124 insertions(+), 30 deletions(-) delete mode 100644 spec/controllers/forms_controller_spec.rb create mode 100644 spec/requests/forms_controller_spec.rb diff --git a/app/controllers/forms_controller.rb b/app/controllers/forms_controller.rb index 92a5007d1..41199fe8e 100644 --- a/app/controllers/forms_controller.rb +++ b/app/controllers/forms_controller.rb @@ -1,9 +1,9 @@ class FormsController < WebController + before_action :load_form after_action :verify_authorized + after_action :alert_org_admins_if_draft_created - def current_form - @current_form ||= Form.find(params[:form_id]) - end + attr_reader :current_form def current_live_form @current_live_form ||= FormDocument::Content.from_form_document(current_form.live_form_document) @@ -20,4 +20,18 @@ def current_archived_form def current_archived_welsh_form @current_archived_welsh_form ||= FormDocument::Content.from_form_document(current_form.archived_welsh_form_document) end + +private + + def load_form + @current_form = Form.find(params[:form_id]) + @initial_form_state = @current_form.state + end + + def alert_org_admins_if_draft_created + return if current_form.destroyed? + return unless @current_form.reload.draft_created?(@initial_form_state) + + OrgAdminAlertsService.new(form: current_form, current_user:).draft_of_existing_form_created + end end diff --git a/app/models/form.rb b/app/models/form.rb index 115dff259..650e5ff43 100644 --- a/app/models/form.rb +++ b/app/models/form.rb @@ -211,6 +211,15 @@ def normalise_welsh! pages.each(&:normalise_welsh!) end + # Pass in the previous state rather than getting it from #state_previously_was as the Form may have been updated in a + # separate instance + def draft_created?(previous_state) + return false if state.to_sym == previous_state.to_sym + + (previous_state.to_sym == :live && live_with_draft?) || + (previous_state.to_sym == :archived && archived_with_draft?) + end + private def set_external_id diff --git a/spec/controllers/forms_controller_spec.rb b/spec/controllers/forms_controller_spec.rb deleted file mode 100644 index 05725be06..000000000 --- a/spec/controllers/forms_controller_spec.rb +++ /dev/null @@ -1,26 +0,0 @@ -require "rails_helper" - -RSpec.describe FormsController, type: :controller do - subject(:controller) { described_class.new } - - let(:form) { create :form } - - describe "#current_form" do - before do - controller.params = ActionController::Parameters.new(form_id: form.id) - end - - it "returns the current form" do - expect(controller.current_form).to eq form - end - - it "memorizes the find form request so it doesn't have to repeat the calls" do - allow(Form).to receive(:find).with(form.id).and_return(form) - - controller.current_form - controller.current_form - - expect(Form).to have_received(:find).exactly(1).times - end - end -end diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb index 29089d2e1..2572195c2 100644 --- a/spec/models/form_spec.rb +++ b/spec/models/form_spec.rb @@ -1306,6 +1306,45 @@ } end + describe "#draft_created?" do + subject(:draft_created?) { form.draft_created?(initial_state) } + + context "when a draft has been created for a live form" do + let(:form) { create(:form, :live_with_draft) } + let(:initial_state) { "live" } + + it { is_expected.to be true } + end + + context "when a draft has been created for an archived form" do + let(:form) { create(:form, :archived_with_draft) } + let(:initial_state) { "archived" } + + it { is_expected.to be true } + end + + context "when the state has not changed" do + let(:form) { create(:form, :live_with_draft) } + let(:initial_state) { "live_with_draft" } + + it { is_expected.to be false } + end + + context "when the state has been updated to live" do + let(:form) { create(:form, :live) } + let(:initial_state) { "live_with_draft" } + + it { is_expected.to be false } + end + + context "when the state has been updated from live with draft to archived with draft" do + let(:form) { create(:form, :archived_with_draft) } + let(:initial_state) { "live_with_draft" } + + it { is_expected.to be false } + end + end + context "when Welsh is not an available language" do let(:languages) { %w[en] } diff --git a/spec/requests/forms/submission_attachments_controller_spec.rb b/spec/requests/forms/submission_attachments_controller_spec.rb index 00f5b143d..9c21c83fc 100644 --- a/spec/requests/forms/submission_attachments_controller_spec.rb +++ b/spec/requests/forms/submission_attachments_controller_spec.rb @@ -2,10 +2,14 @@ RSpec.describe Forms::SubmissionAttachmentsController, type: :request do let(:form) { create(:form, :live, submission_format: original_submission_format) } + let(:user) { standard_user } let(:original_submission_format) { [] } + let(:group) { create(:group, organisation: user.organisation) } before do - login_as_super_admin_user + Membership.create!(group_id: group.id, user:, added_by: user, role: :group_admin) + GroupForm.create!(form:, group_id: group.id) + login_as user end describe "#new" do diff --git a/spec/requests/forms_controller_spec.rb b/spec/requests/forms_controller_spec.rb new file mode 100644 index 000000000..1beee069c --- /dev/null +++ b/spec/requests/forms_controller_spec.rb @@ -0,0 +1,54 @@ +require "rails_helper" + +RSpec.describe FormsController, type: :request do + let(:form) { create(:form, name: "Form name", creator_id: 123) } + let(:user) { standard_user } + let(:group) { create(:group, organisation: user.organisation, status: :active) } + + before do + Membership.create!(group_id: group.id, user:, added_by: user, role: :group_admin) + GroupForm.create!(form:, group_id: group.id) + create(:organisation_admin_user, organisation: user.organisation) + login_as user + end + + describe "#alert_org_admins_if_draft_created" do + before do + # Adding a new question loads in the form again from the database during creation. Post to this route to test that + # this triggers an alert email for the status change even though the Form model loaded in by the controller isn't + # directly updated. + create :draft_question_for_new_page, user:, form_id: form.id + post(create_question_path(form.id), params: { + submit_type: "save", + pages_question_input: { + question_text: "Some question", + hint_text: "", + is_optional: false, + is_repeatable: false, + }, + }) + end + + context "when submitting to a route creates a draft of an existing form" do + let(:form) { create(:form, :live) } + + it "sends an email to the organisation admins" do + expect(response).to have_http_status(:redirect) + expect(form.reload).to be_live_with_draft + expect(ActionMailer::Base.deliveries.count).to eq(1) + + template_id = Settings.govuk_notify.org_admin_alerts.new_live_form_draft_created_template_id + expect(ActionMailer::Base.deliveries.last.govuk_notify_template).to eq(template_id) + end + end + + context "when submitting to a route does not create a draft" do + let(:form) { create(:form, :live_with_draft) } + + it "does not send an email to the organisation admins" do + expect(response).to have_http_status(:redirect) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + end + end +end From 0e6a3877c0a568ca81aaab2285b85e1a7b1b6c2d Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Tue, 10 Mar 2026 15:22:05 +0000 Subject: [PATCH 09/10] Only send org admin alerts if the feature flag is enabled --- app/services/org_admin_alerts_service.rb | 9 +++++++ spec/requests/forms/copy_controller_spec.rb | 4 +-- .../forms/make_live_controller_spec.rb | 2 +- .../forms/unarchive_controller_spec.rb | 2 +- spec/requests/forms_controller_spec.rb | 2 +- spec/requests/group_forms_controller_spec.rb | 4 +-- .../services/org_admin_alerts_service_spec.rb | 27 ++++++++++++++++++- 7 files changed, 42 insertions(+), 8 deletions(-) diff --git a/app/services/org_admin_alerts_service.rb b/app/services/org_admin_alerts_service.rb index 620bf4f94..ea1fd5e3f 100644 --- a/app/services/org_admin_alerts_service.rb +++ b/app/services/org_admin_alerts_service.rb @@ -6,12 +6,15 @@ def initialize(form:, current_user:) end def form_made_live + return unless feature_enabled? + @org_admins.each do |org_admin_user| form_made_live_email(to_email: org_admin_user.email).deliver_now end end def new_draft_form_created + return unless feature_enabled? return unless @form.group.active? @org_admins.each do |org_admin_user| @@ -20,6 +23,8 @@ def new_draft_form_created end def draft_of_existing_form_created + return unless feature_enabled? + @org_admins.each do |org_admin_user| draft_of_existing_form_created_email(to_email: org_admin_user.email).deliver_now end @@ -74,4 +79,8 @@ def draft_of_existing_form_created_email(to_email:) def copied_from_form Form.find_by(id: @form.copied_from_id) end + + def feature_enabled? + FeatureService.enabled?(:org_admin_alerts_enabled) + end end diff --git a/spec/requests/forms/copy_controller_spec.rb b/spec/requests/forms/copy_controller_spec.rb index 361d525fc..e9d0f33ba 100644 --- a/spec/requests/forms/copy_controller_spec.rb +++ b/spec/requests/forms/copy_controller_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe Forms::CopyController, type: :request do +RSpec.describe Forms::CopyController, :feature_org_admin_alerts_enabled, type: :request do let(:id) { form.id } let(:form) { create(:form) } @@ -83,7 +83,7 @@ expect(response).to render_template(:confirm) end - it "does not send an email to the organisation admins", :feature_org_admin_alerts_enabled do + it "does not send an email to the organisation admins" do post create_copy_form_path(id), params: { forms_copy_input: { name: "", tag: "draft" } } expect(ActionMailer::Base.deliveries.count).to eq(0) end diff --git a/spec/requests/forms/make_live_controller_spec.rb b/spec/requests/forms/make_live_controller_spec.rb index 566b5b0f6..97a83e820 100644 --- a/spec/requests/forms/make_live_controller_spec.rb +++ b/spec/requests/forms/make_live_controller_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe Forms::MakeLiveController, type: :request do +RSpec.describe Forms::MakeLiveController, :feature_org_admin_alerts_enabled, type: :request do let(:user) { build :user, organisation: } let(:form) { create(:form, :ready_for_live) } let(:id) { form.id } diff --git a/spec/requests/forms/unarchive_controller_spec.rb b/spec/requests/forms/unarchive_controller_spec.rb index abc3f07c0..1f99e8c71 100644 --- a/spec/requests/forms/unarchive_controller_spec.rb +++ b/spec/requests/forms/unarchive_controller_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe Forms::UnarchiveController, type: :request do +RSpec.describe Forms::UnarchiveController, :feature_org_admin_alerts_enabled, type: :request do let(:user) { standard_user } let(:form) { create(:form, :archived) } diff --git a/spec/requests/forms_controller_spec.rb b/spec/requests/forms_controller_spec.rb index 1beee069c..787652871 100644 --- a/spec/requests/forms_controller_spec.rb +++ b/spec/requests/forms_controller_spec.rb @@ -12,7 +12,7 @@ login_as user end - describe "#alert_org_admins_if_draft_created" do + describe "#alert_org_admins_if_draft_created", :feature_org_admin_alerts_enabled do before do # Adding a new question loads in the form again from the database during creation. Post to this route to test that # this triggers an alert email for the status change even though the Form model loaded in by the controller isn't diff --git a/spec/requests/group_forms_controller_spec.rb b/spec/requests/group_forms_controller_spec.rb index 458da8bc1..94922c125 100644 --- a/spec/requests/group_forms_controller_spec.rb +++ b/spec/requests/group_forms_controller_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe "/groups/:group_id/forms", type: :request do +RSpec.describe "/groups/:group_id/forms", :feature_org_admin_alerts_enabled, type: :request do let(:group) { create :group, organisation:, status: :active } let(:organisation) { test_org } let(:nonexistent_group) { "foobar" } @@ -161,7 +161,7 @@ expect(response.body).to include I18n.t("error_summary.heading") end - it "does not send an email to the organisation admins", :feature_org_admin_alerts_enabled do + it "does not send an email to the organisation admins" do post group_forms_url(group), params: { forms_name_input: invalid_attributes } expect(ActionMailer::Base.deliveries.count).to eq(0) end diff --git a/spec/services/org_admin_alerts_service_spec.rb b/spec/services/org_admin_alerts_service_spec.rb index 3d1a1c7b5..d99ece72b 100644 --- a/spec/services/org_admin_alerts_service_spec.rb +++ b/spec/services/org_admin_alerts_service_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -RSpec.describe OrgAdminAlertsService do +RSpec.describe OrgAdminAlertsService, :feature_org_admin_alerts_enabled do subject(:service) { described_class.new(form:, current_user:) } let(:organisation) { create(:organisation, :with_signed_mou) } @@ -156,6 +156,15 @@ expect(ActionMailer::Base.deliveries.size).to eq(0) end end + + context "when the feature is disabled", feature_org_admin_alerts_enabled: false do + let(:previous_state) { :draft } + + it "does not send any emails" do + service.form_made_live + expect(ActionMailer::Base.deliveries.size).to eq(0) + end + end end describe "#new_draft_form_created" do @@ -228,6 +237,13 @@ expect(ActionMailer::Base.deliveries.size).to eq(0) end end + + context "when the feature is disabled", feature_org_admin_alerts_enabled: false do + it "does not send any emails" do + service.new_draft_form_created + expect(ActionMailer::Base.deliveries.size).to eq(0) + end + end end describe "#draft_of_existing_form_created" do @@ -297,5 +313,14 @@ expect(ActionMailer::Base.deliveries.size).to eq(0) end end + + context "when the feature is disabled", feature_org_admin_alerts_enabled: false do + let(:form) { create(:form, :live_with_draft) } + + it "does not send any emails" do + service.draft_of_existing_form_created + expect(ActionMailer::Base.deliveries.size).to eq(0) + end + end end end From 3799172f37dab8a1ebbdde029daa0bcdc5aa800f Mon Sep 17 00:00:00 2001 From: Stephen Daly Date: Tue, 10 Mar 2026 15:29:55 +0000 Subject: [PATCH 10/10] Update checking feature_daily_submission_emails_enabled flag in tests The FeatureService stubbing didn't work with the new feature flag that had been added, so set the feature flag using RSpec metadata instead. --- .../forms/daily_submission_batch_controller_spec.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/spec/requests/forms/daily_submission_batch_controller_spec.rb b/spec/requests/forms/daily_submission_batch_controller_spec.rb index da01affea..fbea53ac9 100644 --- a/spec/requests/forms/daily_submission_batch_controller_spec.rb +++ b/spec/requests/forms/daily_submission_batch_controller_spec.rb @@ -1,15 +1,12 @@ require "rails_helper" -RSpec.describe Forms::DailySubmissionBatchController, type: :request do +RSpec.describe Forms::DailySubmissionBatchController, :feature_daily_submission_emails_enabled, type: :request do let(:form) { create(:form, :live, send_daily_submission_batch: send_daily_submission_batch_original_value) } let(:send_daily_submission_batch_original_value) { false } let(:current_user) { standard_user } let(:group) { create(:group, organisation: standard_user.organisation) } - let(:daily_submission_emails_enabled) { true } before do - allow(FeatureService).to receive(:enabled?).with(:daily_submission_emails_enabled).and_return(daily_submission_emails_enabled) - Membership.create!(group_id: group.id, user: standard_user, added_by: standard_user) GroupForm.create!(form_id: form.id, group_id: group.id) @@ -37,9 +34,7 @@ end end - context "when the feature flag is disabled" do - let(:daily_submission_emails_enabled) { false } - + context "when the feature flag is disabled", feature_daily_submission_emails_enabled: false do it "returns 404" do expect(response).to have_http_status(:not_found) end