From f08b919e7b2eb35c8c5bd1eb89431a2afc99e385 Mon Sep 17 00:00:00 2001 From: Jonathan Young Date: Fri, 8 Sep 2023 15:00:47 +0100 Subject: [PATCH 1/2] Refactor email handling I believe these changes will make it easier to extend the `annual_leave_request#update_status` method to accomodate other actions, such as editing and withdrawing annual leave requests. The present solution is very rigid, requiring new EmailsHelper methods to be written for each new update action. The new solution simply needs a new StatusUpdate class to be added for the new action, and for that action to be added to the case options in the private `status_update` method in AnnualLeaveRequestController. This brings the code much closer to being open for extension. --- .../annual_leave_requests_controller.rb | 47 ++++++++++--- app/helpers/emails_helper.rb | 68 ------------------- lib/status_update.rb | 66 ++++++++++++++++++ .../annual_leave_requests_controller_spec.rb | 4 +- .../approve_annual_leave_request_spec.rb | 2 +- .../deny_annual_leave_request_spec.rb | 2 +- .../user/request_annual_leave_spec.rb | 2 +- spec/helpers/emails_helper_spec.rb | 45 ------------ 8 files changed, 108 insertions(+), 128 deletions(-) delete mode 100644 app/helpers/emails_helper.rb create mode 100644 lib/status_update.rb delete mode 100644 spec/helpers/emails_helper_spec.rb diff --git a/app/controllers/annual_leave_requests_controller.rb b/app/controllers/annual_leave_requests_controller.rb index e5c96a3..23f2fbb 100644 --- a/app/controllers/annual_leave_requests_controller.rb +++ b/app/controllers/annual_leave_requests_controller.rb @@ -1,3 +1,5 @@ +require "status_update" + class AnnualLeaveRequestsController < ApplicationController def new @annual_leave_request = AnnualLeaveRequest.new @@ -13,7 +15,7 @@ def create @annual_leave_request = current_user.annual_leave_requests.build(annual_leave_request_params) if @annual_leave_request.save - helpers.send_new_request_email(@annual_leave_request) + notify_client.send_email(new_request_email_hash) redirect_to annual_leave_request_confirmation_path else render "new" @@ -37,11 +39,10 @@ def update_status @annual_leave_request = line_reports_leave_requests.find(params[:annual_leave_request_id]) if @annual_leave_request.update(annual_leave_request_params) - helpers.send_status_updated_email(@annual_leave_request) - redirect_to status_update_confirmation_page + notify_client.send_email(status_update.email_hash) + redirect_to status_update.confirmation_page_path else - render "approve" if annual_leave_request_params[:status] == "approved" - render "deny" if annual_leave_request_params[:status] == "denied" + render status_update.action end end @@ -63,12 +64,38 @@ def annual_leave_request_params params.require(:annual_leave_request).permit(:date_from, :date_to, :days_required, :status, :confirm_approval, :denial_reason) end - def status_update_confirmation_page - case annual_leave_request_params[:status] + def status_update + case @annual_leave_request.status when "approved" - confirm_annual_leave_request_approval_path + ApprovedStatusUpdate when "denied" - confirm_annual_leave_request_denial_path - end + DeniedStatusUpdate + end.new(@annual_leave_request) + end + + def new_request_email_hash + { + email_address: line_manager.email, + template_id: "1587d50b-c12e-4698-b10e-cf414de26f36", + personalisation: { + line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", + name: "#{user.given_name} #{user.family_name}", + date_from: @annual_leave_request.date_from.to_fs(:rfc822), + date_to: @annual_leave_request.date_to.to_fs(:rfc822), + days_required: @annual_leave_request.days_required, + }, + } + end + + def notify_client + @notify_client ||= Notifications::Client.new(ENV["NOTIFY_API_KEY"]) + end + + def line_manager + @annual_leave_request.user.line_manager + end + + def user + @annual_leave_request.user end end diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb deleted file mode 100644 index 64c6cd4..0000000 --- a/app/helpers/emails_helper.rb +++ /dev/null @@ -1,68 +0,0 @@ -module EmailsHelper - def send_new_request_email(annual_leave_request) - user = annual_leave_request.user - line_manager = user.line_manager - - client.send_email( - email_address: line_manager.email, - template_id: "1587d50b-c12e-4698-b10e-cf414de26f36", - personalisation: { - line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", - name: "#{user.given_name} #{user.family_name}", - date_from: annual_leave_request.date_from.to_fs(:rfc822), - date_to: annual_leave_request.date_to.to_fs(:rfc822), - days_required: annual_leave_request.days_required, - }, - ) - end - - def send_status_updated_email(annual_leave_request) - case annual_leave_request.status - when "approved" - client.send_email(approval_email_hash(annual_leave_request)) - when "denied" - client.send_email(denial_email_hash(annual_leave_request)) - end - end - -private - - def client - @client ||= Notifications::Client.new(notify_api_key) - end - - def notify_api_key - ENV["NOTIFY_API_KEY"] - end - - def approval_email_hash(annual_leave_request) - user = annual_leave_request.user - line_manager = user.line_manager - { - email_address: user.email, - template_id: "34542d49-8b91-412c-9393-c186a04a7d1c", - personalisation: { - line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", - name: "#{user.given_name} #{user.family_name}", - date_from: annual_leave_request.date_from.to_fs(:rfc822), - date_to: annual_leave_request.date_to.to_fs(:rfc822), - }, - } - end - - def denial_email_hash(annual_leave_request) - user = annual_leave_request.user - line_manager = user.line_manager - { - email_address: user.email, - template_id: "ec9035df-9c98-4e0e-8826-47768c311745", - personalisation: { - line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", - name: "#{user.given_name} #{user.family_name}", - date_from: annual_leave_request.date_from.to_fs(:rfc822), - date_to: annual_leave_request.date_to.to_fs(:rfc822), - denial_reason: annual_leave_request.denial_reason, - }, - } - end -end diff --git a/lib/status_update.rb b/lib/status_update.rb new file mode 100644 index 0000000..8ccc23e --- /dev/null +++ b/lib/status_update.rb @@ -0,0 +1,66 @@ +class ApprovedStatusUpdate + include Rails.application.routes.url_helpers + + attr_reader :annual_leave_request, :user, :line_manager + + def initialize(annual_leave_request) + @annual_leave_request = annual_leave_request + @user = annual_leave_request.user + @line_manager = user.line_manager + end + + def confirmation_page_path + confirm_annual_leave_request_approval_path + end + + def action + "approve" + end + + def email_hash + { + email_address: user.email, + template_id: "34542d49-8b91-412c-9393-c186a04a7d1c", + personalisation: { + line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", + name: "#{user.given_name} #{user.family_name}", + date_from: annual_leave_request.date_from.to_fs(:rfc822), + date_to: annual_leave_request.date_to.to_fs(:rfc822), + }, + } + end +end + +class DeniedStatusUpdate + include Rails.application.routes.url_helpers + + attr_reader :annual_leave_request, :user, :line_manager + + def initialize(annual_leave_request) + @annual_leave_request = annual_leave_request + @user = annual_leave_request.user + @line_manager = user.line_manager + end + + def confirmation_page_path + confirm_annual_leave_request_denial_path + end + + def action + "deny" + end + + def email_hash + { + email_address: user.email, + template_id: "ec9035df-9c98-4e0e-8826-47768c311745", + personalisation: { + line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", + name: "#{user.given_name} #{user.family_name}", + date_from: annual_leave_request.date_from.to_fs(:rfc822), + date_to: annual_leave_request.date_to.to_fs(:rfc822), + denial_reason: annual_leave_request.denial_reason, + }, + } + end +end diff --git a/spec/controllers/annual_leave_requests_controller_spec.rb b/spec/controllers/annual_leave_requests_controller_spec.rb index b5565f0..6691725 100644 --- a/spec/controllers/annual_leave_requests_controller_spec.rb +++ b/spec/controllers/annual_leave_requests_controller_spec.rb @@ -5,7 +5,7 @@ describe "POST create" do setup do - allow(Notifications::Client).to receive(:new).and_return(notify_fake_client) + allow(controller).to receive(:notify_client).and_return(notify_fake_client) sign_in user end @@ -48,7 +48,7 @@ let(:leave_request) { create(:annual_leave_request, user_id: user.id) } setup do - allow(Notifications::Client).to receive(:new).and_return(notify_fake_client) + allow(controller).to receive(:notify_client).and_return(notify_fake_client) sign_in line_manager end diff --git a/spec/features/line_manager/approve_annual_leave_request_spec.rb b/spec/features/line_manager/approve_annual_leave_request_spec.rb index c3f7ed4..3d4c004 100644 --- a/spec/features/line_manager/approve_annual_leave_request_spec.rb +++ b/spec/features/line_manager/approve_annual_leave_request_spec.rb @@ -3,7 +3,7 @@ RSpec.feature "Line manager can approve an annual leave request" do setup do notify_test_client = Notifications::Client.new(ENV["NOTIFY_TEST_API_KEY"]) - allow_any_instance_of(EmailsHelper).to receive(:client).and_return(notify_test_client) # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(AnnualLeaveRequestsController).to receive(:notify_client).and_return(notify_test_client) # rubocop:disable RSpec/AnyInstance end scenario do diff --git a/spec/features/line_manager/deny_annual_leave_request_spec.rb b/spec/features/line_manager/deny_annual_leave_request_spec.rb index be472b1..4580344 100644 --- a/spec/features/line_manager/deny_annual_leave_request_spec.rb +++ b/spec/features/line_manager/deny_annual_leave_request_spec.rb @@ -3,7 +3,7 @@ RSpec.feature "Line manager can deny an annual leave request" do setup do notify_test_client = Notifications::Client.new(ENV["NOTIFY_TEST_API_KEY"]) - allow_any_instance_of(EmailsHelper).to receive(:client).and_return(notify_test_client) # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(AnnualLeaveRequestsController).to receive(:notify_client).and_return(notify_test_client) # rubocop:disable RSpec/AnyInstance end scenario do diff --git a/spec/features/user/request_annual_leave_spec.rb b/spec/features/user/request_annual_leave_spec.rb index c3de5f0..6b5eec0 100644 --- a/spec/features/user/request_annual_leave_spec.rb +++ b/spec/features/user/request_annual_leave_spec.rb @@ -6,7 +6,7 @@ setup do notify_test_client = Notifications::Client.new(ENV["NOTIFY_TEST_API_KEY"]) - allow_any_instance_of(EmailsHelper).to receive(:client).and_return(notify_test_client) # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(AnnualLeaveRequestsController).to receive(:notify_client).and_return(notify_test_client) # rubocop:disable RSpec/AnyInstance end scenario do diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb deleted file mode 100644 index 6eef6c0..0000000 --- a/spec/helpers/emails_helper_spec.rb +++ /dev/null @@ -1,45 +0,0 @@ -require "spec_helper" - -RSpec.describe EmailsHelper, type: :helper do - let(:annual_leave_request) { create(:annual_leave_request, user_id: user.id) } - let(:user) { create(:user, line_manager_id: line_manager.id) } - let(:line_manager) { create(:user, email: "line_manager@digital.cabinet-office.gov.uk") } - - describe "#send_new_request_email" do - it "sends an email to the users line manager" do - @client = Notifications::Client.new(ENV["NOTIFY_TEST_API_KEY"]) - user_full_name = "#{user.given_name} #{user.family_name}" - - response_notification = helper.send_new_request_email(annual_leave_request) - response = client.get_notification(response_notification.id) - - expect(response.email_address).to eq(line_manager.email) - expect(response.subject).to eq("GOV.UK Holiday Logger – #{user_full_name} – New Annual Leave Request") - end - end - - describe "#send_status_updated_email" do - it "sends a denial email to the user when status is updated to 'denied'" do - @client = Notifications::Client.new(ENV["NOTIFY_TEST_API_KEY"]) - annual_leave_request.status = "denied" - annual_leave_request.denial_reason = "some reason" - - response_notification = helper.send_status_updated_email(annual_leave_request) - response = client.get_notification(response_notification.id) - - expect(response.email_address).to eq(user.email) - expect(response.subject).to eq("GOV.UK Holiday Logger – Annual Leave Request Denied") - end - - it "sends an approval email to the user when status is updated to 'approved'" do - @client = Notifications::Client.new(ENV["NOTIFY_TEST_API_KEY"]) - annual_leave_request.status = "approved" - - response_notification = helper.send_status_updated_email(annual_leave_request) - response = client.get_notification(response_notification.id) - - expect(response.email_address).to eq(user.email) - expect(response.subject).to eq("GOV.UK Holiday Logger – Annual Leave Request Approved") - end - end -end From 576f4156a68ee13a6f1bd0b1d40d3bc977c464b9 Mon Sep 17 00:00:00 2001 From: Jonathan Young Date: Fri, 8 Sep 2023 15:20:13 +0100 Subject: [PATCH 2/2] Update refactor to test emails Updates the AnnualLeaveRequestController tests to check emails are sent to the correct person with the correct template used. --- .../annual_leave_requests_controller.rb | 4 +- .../annual_leave_requests_controller_spec.rb | 65 +++++++------------ 2 files changed, 25 insertions(+), 44 deletions(-) diff --git a/app/controllers/annual_leave_requests_controller.rb b/app/controllers/annual_leave_requests_controller.rb index 23f2fbb..84d4a03 100644 --- a/app/controllers/annual_leave_requests_controller.rb +++ b/app/controllers/annual_leave_requests_controller.rb @@ -15,7 +15,7 @@ def create @annual_leave_request = current_user.annual_leave_requests.build(annual_leave_request_params) if @annual_leave_request.save - notify_client.send_email(new_request_email_hash) + @email_response_notification = notify_client.send_email(new_request_email_hash) redirect_to annual_leave_request_confirmation_path else render "new" @@ -39,7 +39,7 @@ def update_status @annual_leave_request = line_reports_leave_requests.find(params[:annual_leave_request_id]) if @annual_leave_request.update(annual_leave_request_params) - notify_client.send_email(status_update.email_hash) + @email_response_notification = notify_client.send_email(status_update.email_hash) redirect_to status_update.confirmation_page_path else render status_update.action diff --git a/spec/controllers/annual_leave_requests_controller_spec.rb b/spec/controllers/annual_leave_requests_controller_spec.rb index 6691725..1889fbf 100644 --- a/spec/controllers/annual_leave_requests_controller_spec.rb +++ b/spec/controllers/annual_leave_requests_controller_spec.rb @@ -1,27 +1,17 @@ RSpec.describe AnnualLeaveRequestsController do let(:user) { create(:user, line_manager_id: line_manager.id) } + let(:user_full_name) { "#{user.given_name} #{user.family_name}" } let(:line_manager) { create(:user, email: "line_manager@digital.cabinet-office.gov.uk") } - let(:notify_fake_client) { instance_double(Notifications::Client, send_email: "FakeNotificationResponse") } + let(:notify_test_client) { Notifications::Client.new(ENV["NOTIFY_TEST_API_KEY"]) } describe "POST create" do setup do - allow(controller).to receive(:notify_client).and_return(notify_fake_client) + allow(controller).to receive(:notify_client).and_return(notify_test_client) sign_in user end it "emails line manager and redirects to confirmation page if annual leave request valid" do valid_request = build(:annual_leave_request, user_id: user.id) - new_request_email_hash = { - email_address: line_manager.email, - template_id: "1587d50b-c12e-4698-b10e-cf414de26f36", - personalisation: { - line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", - name: "#{user.given_name} #{user.family_name}", - date_from: valid_request.date_from.to_fs(:rfc822), - date_to: valid_request.date_to.to_fs(:rfc822), - days_required: valid_request.days_required, - }, - } post :create, params: { annual_leave_request: { date_from: valid_request.date_from, @@ -29,7 +19,11 @@ days_required: valid_request.days_required, } } - expect(notify_fake_client).to have_received(:send_email).with(new_request_email_hash) + email_response_notification = assigns(:email_response_notification) + email_response = notify_test_client.get_notification(email_response_notification.id) + + expect(email_response.email_address).to eq(line_manager.email) + expect(email_response.subject).to eq("GOV.UK Holiday Logger – #{user_full_name} – New Annual Leave Request") expect(response).to redirect_to(annual_leave_request_confirmation_path) end @@ -48,7 +42,7 @@ let(:leave_request) { create(:annual_leave_request, user_id: user.id) } setup do - allow(controller).to receive(:notify_client).and_return(notify_fake_client) + allow(controller).to receive(:notify_client).and_return(notify_test_client) sign_in line_manager end @@ -76,18 +70,7 @@ expect(response).to render_template(:deny) end - it "sends an email to the line report when status is updated to approved" do - approved_request_email_hash = { - email_address: user.email, - template_id: "34542d49-8b91-412c-9393-c186a04a7d1c", - personalisation: { - line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", - name: "#{user.given_name} #{user.family_name}", - date_from: leave_request.date_from.to_fs(:rfc822), - date_to: leave_request.date_to.to_fs(:rfc822), - }, - } - + it "sends an email to the line report and redirects to confirmatioon page when status is updated to 'approved'" do patch :update_status, params: { annual_leave_request_id: leave_request.id, annual_leave_request: { @@ -96,10 +79,15 @@ }, } - expect(notify_fake_client).to have_received(:send_email).with(approved_request_email_hash) + email_response_notification = assigns(:email_response_notification) + email_response = notify_test_client.get_notification(email_response_notification.id) + + expect(email_response.email_address).to eq(user.email) + expect(email_response.subject).to eq("GOV.UK Holiday Logger – Annual Leave Request Approved") + expect(response).to redirect_to(confirm_annual_leave_request_approval_path) end - it "sends an email to the line report when status is updated to 'denied'" do + it "sends an email to the line report and redirects to confirmatioon page when status is updated to 'denied'" do patch :update_status, params: { annual_leave_request_id: leave_request.id, annual_leave_request: { @@ -107,20 +95,13 @@ denial_reason: "some valid reason", }, } - leave_request.reload - denied_request_email_hash = { - email_address: user.email, - template_id: "ec9035df-9c98-4e0e-8826-47768c311745", - personalisation: { - line_manager_name: "#{line_manager.given_name} #{line_manager.family_name}", - name: "#{user.given_name} #{user.family_name}", - date_from: leave_request.date_from.to_fs(:rfc822), - date_to: leave_request.date_to.to_fs(:rfc822), - denial_reason: leave_request.denial_reason, - }, - } - expect(notify_fake_client).to have_received(:send_email).with(denied_request_email_hash) + email_response_notification = assigns(:email_response_notification) + email_response = notify_test_client.get_notification(email_response_notification.id) + + expect(email_response.email_address).to eq(user.email) + expect(email_response.subject).to eq("GOV.UK Holiday Logger – Annual Leave Request Denied") + expect(response).to redirect_to(confirm_annual_leave_request_denial_path) end end end