From 2d9c4b76075c79b470b6ada2fb576ef8302c8b7b Mon Sep 17 00:00:00 2001 From: Jonathan Young Date: Fri, 8 Sep 2023 15:00:47 +0100 Subject: [PATCH] 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