From ab23c830b0138266ca8162be0887ae878f8cde07 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Mon, 11 Nov 2024 15:05:37 +0000 Subject: [PATCH] Move payroll run to background job Creating the November payroll run timed out (October's too). This commit moves creating the payment records into a background job. We need to pass the claim ids and topup ids to the background job, this is because once the admin has reviewed the claims that will be part of this payroll run (shown in the `new` action) we can't include any claims created in the interim. We may want to consider someother method of indicating what claims should be part of this payroll run (create pending payments? flag claims / topups as reviewed for payroll?) so we don't have to pass such a large amount of information to the background job. --- .../admin/payroll_runs_controller.rb | 4 +- app/jobs/payroll_run_job.rb | 24 +++ app/models/payroll_run.rb | 23 +-- .../admin/payroll_runs/_complete.html.erb | 157 ++++++++++++++++ app/views/admin/payroll_runs/_failed.html.erb | 15 ++ .../admin/payroll_runs/_pending.html.erb | 14 ++ app/views/admin/payroll_runs/show.html.erb | 176 ++---------------- config/analytics.yml | 1 + ...241111114155_add_status_to_payroll_runs.rb | 7 + db/schema.rb | 3 +- spec/factories/payroll_runs.rb | 2 + .../features/admin/admin_payroll_runs_spec.rb | 32 +++- spec/jobs/payroll_run_job_spec.rb | 65 +++++++ spec/models/payroll_run_spec.rb | 40 ---- 14 files changed, 338 insertions(+), 225 deletions(-) create mode 100644 app/jobs/payroll_run_job.rb create mode 100644 app/views/admin/payroll_runs/_complete.html.erb create mode 100644 app/views/admin/payroll_runs/_failed.html.erb create mode 100644 app/views/admin/payroll_runs/_pending.html.erb create mode 100644 db/migrate/20241111114155_add_status_to_payroll_runs.rb create mode 100644 spec/jobs/payroll_run_job_spec.rb diff --git a/app/controllers/admin/payroll_runs_controller.rb b/app/controllers/admin/payroll_runs_controller.rb index 6c5396860a..4216cf1f00 100644 --- a/app/controllers/admin/payroll_runs_controller.rb +++ b/app/controllers/admin/payroll_runs_controller.rb @@ -24,7 +24,9 @@ def create return end - payroll_run = PayrollRun.create_with_claims!(claims, topups, created_by: admin_user) + payroll_run = PayrollRun.create!(created_by: admin_user) + + PayrollRunJob.perform_later(payroll_run, claims.ids, topups.ids) redirect_to [:admin, payroll_run], notice: "Payroll run created" rescue ActiveRecord::RecordInvalid => e diff --git a/app/jobs/payroll_run_job.rb b/app/jobs/payroll_run_job.rb new file mode 100644 index 0000000000..f52f6ebff0 --- /dev/null +++ b/app/jobs/payroll_run_job.rb @@ -0,0 +1,24 @@ +class PayrollRunJob < ApplicationJob + def perform(payroll_run, claim_ids, topup_ids) + claims = Claim.where(id: claim_ids) + topups = Topup.where(id: topup_ids) + + ActiveRecord::Base.transaction do + [claims, topups].reduce([], :concat).group_by(&:national_insurance_number).each_value do |grouped_items| + # associates the claim to the payment, for Topup that's its associated claim + grouped_claims = grouped_items.map { |i| i.is_a?(Topup) ? i.claim : i } + + # associates the payment to the Topup, so we know it's payrolled + group_topups = grouped_items.select { |i| i.is_a?(Topup) } + + award_amount = grouped_items.map(&:award_amount).compact.sum(0) + Payment.create!(payroll_run: payroll_run, claims: grouped_claims, topups: group_topups, award_amount: award_amount) + end + + payroll_run.complete! + end + rescue => e + payroll_run.failed! + raise e + end +end diff --git a/app/models/payroll_run.rb b/app/models/payroll_run.rb index b37959f694..2d5a4992be 100644 --- a/app/models/payroll_run.rb +++ b/app/models/payroll_run.rb @@ -9,6 +9,8 @@ class PayrollRun < ApplicationRecord # backfill existing payroll runs and payments with a payment confirmation belongs_to :confirmation_report_uploaded_by, class_name: "DfeSignIn::User", optional: true + enum :status, %w[pending complete failed].index_with(&:itself) + validate :ensure_no_payroll_run_this_month, on: :create scope :this_month, -> { where(created_at: DateTime.now.all_month) } @@ -41,27 +43,6 @@ def all_payments_confirmed? @all_payments_confirmed = payment_confirmations.any? && total_confirmed_payments == payments_count end - def self.create_with_claims!(claims, topups, attrs = {}) - ActiveRecord::Base.transaction do - PayrollRun.create!(attrs).tap do |payroll_run| - [claims, topups].reduce([], :concat).group_by { |obj| group_by_field(obj) }.each_value do |grouped_items| - # associates the claim to the payment, for Topup that's its associated claim - grouped_claims = grouped_items.map { |i| i.is_a?(Topup) ? i.claim : i } - - # associates the payment to the Topup, so we know it's payrolled - group_topups = grouped_items.select { |i| i.is_a?(Topup) } - - award_amount = grouped_items.map(&:award_amount).compact.sum(0) - Payment.create!(payroll_run: payroll_run, claims: grouped_claims, topups: group_topups, award_amount: award_amount) - end - end - end - end - - def self.group_by_field(obj) - obj.national_insurance_number - end - def download_triggered? downloaded_at.present? && downloaded_by.present? end diff --git a/app/views/admin/payroll_runs/_complete.html.erb b/app/views/admin/payroll_runs/_complete.html.erb new file mode 100644 index 0000000000..78a7097786 --- /dev/null +++ b/app/views/admin/payroll_runs/_complete.html.erb @@ -0,0 +1,157 @@ +
+
+

+ <%= @payroll_run.created_at.strftime("%B") %> payroll run +

+ +
+
+
+ Approved claims +
+ +
+ <%= @payroll_run.number_of_claims_for_policy(:all, filter: :claims) %> +
+
+
+
+ Top ups +
+ +
+ <%= @payroll_run.number_of_claims_for_policy(Policies::LevellingUpPremiumPayments, filter: :topups) %> +
+
+
+
+ Total award amount +
+ +
+ <%= number_to_currency(@payroll_run.total_award_amount) %> +
+
+
+
+ Created by +
+ +
+ <%= user_details(@payroll_run.created_by) %> +
+
+
+
+ Downloaded +
+ +
+ <%= @payroll_run.download_triggered? ? l(@payroll_run.downloaded_at) : "No" %> +
+
+ <% if @payroll_run.download_triggered? %> +
+
+ Downloaded by +
+ +
+ <%= user_details(@payroll_run.downloaded_by) %> +
+
+ <% end %> +
+
+ +
+

Summary of claim amounts by service

+ + + + + + + + + <% Policies.all.each do |policy| %> + + + + + + <% end %> + + + + + + + + +
ServiceNumber of claimsTotal claimed amount
<%= policy.short_name %><%= @payroll_run.number_of_claims_for_policy(policy, filter: :claims) %><%= number_to_currency(@payroll_run.total_claim_amount_for_policy(policy, filter: :claims)) %>
<%= I18n.t("levelling_up_premium_payments.policy_short_name") %> Top Ups<%= @payroll_run.number_of_claims_for_policy(Policies::LevellingUpPremiumPayments, filter: :topups) %><%= number_to_currency(@payroll_run.total_claim_amount_for_policy(Policies::LevellingUpPremiumPayments, filter: :topups)) %>
+
+ + <% unless @payroll_run.all_payments_confirmed? %> +
+
+ + <%= text_field_tag "payroll_run_download_link", new_admin_payroll_run_download_url(@payroll_run), data: {"copy-to-clipboard": :true}, readonly: true, class: ["govuk-input"] %> +
+
+ <% end %> + +
+

Payments

+ + + + + + + + + + + <% unless @payroll_run.all_payments_confirmed? %> + + <% end %> + + + + <% @payments.each do |payment| %> + <% payment.claims.each_with_index do |claim, index| %> + <% number_of_claims = payment.claims.size %> + <% topup_claim_ids = payment.topups.pluck(:claim_id) %> + <% line_item = topup_claim_ids.include?(claim.id) ? payment.topups.select { |t| t.claim_id == claim.id }.first : claim %> + + + <% if index == 0 %> + + + <% end %> + + + + <% if index == 0 %> + + <% unless @payroll_run.all_payments_confirmed? %> + + <% end %> + <% end %> + + <% end %> + <% end %> + +
Payment IDPayee NameClaim ReferenceServiceClaim AmountPayment AmountActions
<%= payment.id %><%= payment.banking_name %><%= link_to claim.reference, admin_claim_path(claim), class: "govuk-link" %><%= line_item.is_a?(Topup) ? "#{I18n.t("levelling_up_premium_payments.policy_short_name")} (top up)" : claim.policy.short_name %><%= number_to_currency(line_item.award_amount) %><%= number_to_currency(payment.award_amount) %> + <% unless payment.confirmation.present? %> + <%= link_to remove_admin_payroll_run_payment_path(id: payment.id, payroll_run_id: payment.payroll_run.id), class: "govuk-link" do %> + Remove payment row + <% end %> + <% end %> +
+ + <%== render partial: 'pagination', locals: { pagy: @pagy } %> +
diff --git a/app/views/admin/payroll_runs/_failed.html.erb b/app/views/admin/payroll_runs/_failed.html.erb new file mode 100644 index 0000000000..1d21183c73 --- /dev/null +++ b/app/views/admin/payroll_runs/_failed.html.erb @@ -0,0 +1,15 @@ +
+
+

+ <%= @payroll_run.created_at.strftime("%B") %> payroll run +

+ +
+ + + Warning + This payroll run errored, please contact tech support + +
+
+
diff --git a/app/views/admin/payroll_runs/_pending.html.erb b/app/views/admin/payroll_runs/_pending.html.erb new file mode 100644 index 0000000000..b7ab477f63 --- /dev/null +++ b/app/views/admin/payroll_runs/_pending.html.erb @@ -0,0 +1,14 @@ +
+
+

+ <%= @payroll_run.created_at.strftime("%B") %> payroll run +

+ +

+ This payroll run is in progress +

+ + <%= govuk_button_link_to "Refresh", admin_payroll_run_path(@payroll_run) %> +
+
+ diff --git a/app/views/admin/payroll_runs/show.html.erb b/app/views/admin/payroll_runs/show.html.erb index fee1b631b5..0ebda801f0 100644 --- a/app/views/admin/payroll_runs/show.html.erb +++ b/app/views/admin/payroll_runs/show.html.erb @@ -1,164 +1,18 @@ <% content_for(:page_title) { page_title("#{@payroll_run.created_at.strftime("%B")} payroll run") } %> -
-
-

- <%= @payroll_run.created_at.strftime("%B") %> payroll run -

- -
-
-
- Approved claims -
- -
- <%= @payroll_run.number_of_claims_for_policy(:all, filter: :claims) %> -
-
-
-
- Top ups -
- -
- <%= @payroll_run.number_of_claims_for_policy(Policies::LevellingUpPremiumPayments, filter: :topups) %> -
-
-
-
- Total award amount -
- -
- <%= number_to_currency(@payroll_run.total_award_amount) %> -
-
-
-
- Created by -
- -
- <%= user_details(@payroll_run.created_by) %> -
-
-
-
- Downloaded -
- -
- <%= @payroll_run.download_triggered? ? l(@payroll_run.downloaded_at) : "No" %> -
-
- <% if @payroll_run.download_triggered? %> -
-
- Downloaded by -
- -
- <%= user_details(@payroll_run.downloaded_by) %> -
-
- <% end %> -
-
- -
-

Summary of claim amounts by service

- - - - - - - - - <% Policies.all.each do |policy| %> - - - - - - <% end %> - - - - - - - - -
ServiceNumber of claimsTotal claimed amount
<%= policy.short_name %><%= @payroll_run.number_of_claims_for_policy(policy, filter: :claims) %><%= number_to_currency(@payroll_run.total_claim_amount_for_policy(policy, filter: :claims)) %>
<%= I18n.t("levelling_up_premium_payments.policy_short_name") %> Top Ups<%= @payroll_run.number_of_claims_for_policy(Policies::LevellingUpPremiumPayments, filter: :topups) %><%= number_to_currency(@payroll_run.total_claim_amount_for_policy(Policies::LevellingUpPremiumPayments, filter: :topups)) %>
-
- - <% unless @payroll_run.all_payments_confirmed? %> -
-
- - <%= text_field_tag "payroll_run_download_link", new_admin_payroll_run_download_url(@payroll_run), data: {"copy-to-clipboard": :true}, readonly: true, class: ["govuk-input"] %> -
-
- <% end %> - -
-

Payments

- - - - - - - - - - - <% unless @payroll_run.all_payments_confirmed? %> - - <% end %> - - - - <% @payments.each do |payment| %> - <% payment.claims.each_with_index do |claim, index| %> - <% number_of_claims = payment.claims.size %> - <% topup_claim_ids = payment.topups.pluck(:claim_id) %> - <% line_item = topup_claim_ids.include?(claim.id) ? payment.topups.select { |t| t.claim_id == claim.id }.first : claim %> - - - <% if index == 0 %> - - - <% end %> - - - - <% if index == 0 %> - - <% unless @payroll_run.all_payments_confirmed? %> - - <% end %> - <% end %> - - <% end %> - <% end %> - -
Payment IDPayee NameClaim ReferenceServiceClaim AmountPayment AmountActions
<%= payment.id %><%= payment.banking_name %><%= link_to claim.reference, admin_claim_path(claim), class: "govuk-link" %><%= line_item.is_a?(Topup) ? "#{I18n.t("levelling_up_premium_payments.policy_short_name")} (top up)" : claim.policy.short_name %><%= number_to_currency(line_item.award_amount) %><%= number_to_currency(payment.award_amount) %> - <% unless payment.confirmation.present? %> - <%= link_to remove_admin_payroll_run_payment_path(id: payment.id, payroll_run_id: payment.payroll_run.id), class: "govuk-link" do %> - Remove payment row - <% end %> - <% end %> -
- - <%== render partial: 'pagination', locals: { pagy: @pagy } %> -
- - <% if PayrollRun.allow_destroy? %> +<% case @payroll_run.status %> +<% when "pending" %> + <%= render "pending" %> +<% when "complete" %> + <%= render "complete" %> +<% when "failed" %> + <%= render "failed" %> +<% else %> + <% fail "Unknown payroll run status #{@payroll_run.status}" %> +<% end %> + +<% if PayrollRun.allow_destroy? %> +
<%= button_to( "Delete payroll run", @@ -167,5 +21,5 @@ class: "govuk-button govuk-button--warning", ) %>
- <% end %> -
+
+<% end %> diff --git a/config/analytics.yml b/config/analytics.yml index 5035c974c6..e9426d8897 100644 --- a/config/analytics.yml +++ b/config/analytics.yml @@ -139,6 +139,7 @@ shared: - downloaded_by_id - scheduled_payment_date - confirmation_report_uploaded_by_id + - status :student_loans_eligibilities: - id - created_at diff --git a/db/migrate/20241111114155_add_status_to_payroll_runs.rb b/db/migrate/20241111114155_add_status_to_payroll_runs.rb new file mode 100644 index 0000000000..18fae6a56c --- /dev/null +++ b/db/migrate/20241111114155_add_status_to_payroll_runs.rb @@ -0,0 +1,7 @@ +class AddStatusToPayrollRuns < ActiveRecord::Migration[7.0] + def change + add_column :payroll_runs, :status, :string, default: "pending", null: false + + PayrollRun.update_all(status: "complete") + end +end diff --git a/db/schema.rb b/db/schema.rb index a8c856cb8c..519c33940f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_11_07_164046) do +ActiveRecord::Schema[7.0].define(version: 2024_11_11_114155) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "pg_trgm" @@ -410,6 +410,7 @@ t.uuid "downloaded_by_id" t.date "scheduled_payment_date" t.uuid "confirmation_report_uploaded_by_id" + t.string "status", default: "pending", null: false t.index ["confirmation_report_uploaded_by_id"], name: "index_payroll_runs_on_confirmation_report_uploaded_by_id" t.index ["created_at"], name: "index_payroll_runs_on_created_at" t.index ["created_by_id"], name: "index_payroll_runs_on_created_by_id" diff --git a/spec/factories/payroll_runs.rb b/spec/factories/payroll_runs.rb index 0879430cef..d7845b02e1 100644 --- a/spec/factories/payroll_runs.rb +++ b/spec/factories/payroll_runs.rb @@ -3,6 +3,8 @@ association :created_by, factory: :dfe_signin_user association :downloaded_by, factory: :dfe_signin_user + status { :complete } + transient do # The claim_counts attribute provides a convenient way to create a # payroll run with associated payment objects and associated claim diff --git a/spec/features/admin/admin_payroll_runs_spec.rb b/spec/features/admin/admin_payroll_runs_spec.rb index 997843d563..25735793b0 100644 --- a/spec/features/admin/admin_payroll_runs_spec.rb +++ b/spec/features/admin/admin_payroll_runs_spec.rb @@ -34,13 +34,19 @@ click_on "Confirm and submit" + expect(page).to have_content("Payroll run created") + expect(page).to have_content("This payroll run is in progress") + + perform_enqueued_jobs + + click_on "Refresh" + payroll_run = PayrollRun.order(:created_at).last expect(page).to have_content("Approved claims 4") expect(page).to have_content("Top ups 1") expect(page).to have_content("Created by #{@signed_in_user.full_name}") expect(page).to have_content("Total award amount £9,500.00") - expect(page).to have_content("Payroll run created") expect(page).to have_field("payroll_run_download_link", with: new_admin_payroll_run_download_url(payroll_run)) expect(page).to have_content("Student Loans 2 £2,000.00") @@ -71,6 +77,10 @@ click_on "Confirm and submit" + perform_enqueued_jobs + + click_on "Refresh" + expect(page).to have_content("Approved claims 3") payroll_run = PayrollRun.order(:created_at).last @@ -269,4 +279,24 @@ expect(page).to have_content "Next" end end + + scenario "failed payroll run job show an error message" do + create(:claim, :approved, policy: Policies::EarlyCareerPayments) + + click_on "Payroll" + + month_name = Date.today.strftime("%B") + + click_on "Run #{month_name} payroll" + + click_on "Confirm and submit" + + allow(Payment).to receive(:create!).and_raise(StandardError) + + expect { perform_enqueued_jobs }.to raise_error(StandardError) + + click_on "Refresh" + + expect(page).to have_content("This payroll run errored") + end end diff --git a/spec/jobs/payroll_run_job_spec.rb b/spec/jobs/payroll_run_job_spec.rb new file mode 100644 index 0000000000..8d2989def4 --- /dev/null +++ b/spec/jobs/payroll_run_job_spec.rb @@ -0,0 +1,65 @@ +require "rails_helper" + +RSpec.describe PayrollRunJob, type: :job do + let(:user) { create(:dfe_signin_user) } + let(:payroll_run) { create(:payroll_run, created_by: user) } + + describe "#perform" do + context "when successful" do + let(:claims) { Policies.all.map { |policy| create(:claim, :approved, policy: policy) } } + let(:topups) { [] } + + before do + described_class.perform_now(payroll_run, claims.map(&:id), topups.map(&:id)) + end + + it "creates a payroll run with payments and populates the award_amount" do + expect(payroll_run.reload.created_by.id).to eq(user.id) + expect(payroll_run.claims).to match_array(claims) + expect(claims[0].payments.first.award_amount).to eq(claims[0].award_amount) + expect(claims[1].payments.first.award_amount).to eq(claims[1].award_amount) + end + + context "with multiple claims from the same teacher reference number" do + let(:personal_details) do + { + national_insurance_number: generate(:national_insurance_number), + eligibility_attributes: {teacher_reference_number: generate(:teacher_reference_number)}, + email_address: generate(:email_address), + bank_sort_code: "112233", + bank_account_number: "95928482", + address_line_1: "64 West Lane", + student_loan_plan: StudentLoan::PLAN_1 + } + end + let(:matching_claims) do + [ + create(:claim, :approved, personal_details.merge(policy: Policies::StudentLoans)), + create(:claim, :approved, personal_details.merge(policy: Policies::EarlyCareerPayments)) + ] + end + let(:other_claim) { create(:claim, :approved) } + let(:claims) { matching_claims + [other_claim] } + + it "groups them into a single payment and populates the award_amount" do + expect(payroll_run.payments.map(&:claims)).to match_array([match_array(matching_claims), [other_claim]]) + expect(matching_claims[0].reload.payments.first.award_amount).to eq(matching_claims.sum(&:award_amount)) + end + end + end + + context "when errored" do + before do + allow(Payment).to receive(:create!).and_raise(StandardError) + end + + it "marks the payroll run as failed and reraises the error" do + expect do + described_class.perform_now(payroll_run, [create(:claim).id], []) + end.to raise_error(StandardError) + + expect(payroll_run.reload.failed?).to be true + end + end + end +end diff --git a/spec/models/payroll_run_spec.rb b/spec/models/payroll_run_spec.rb index 2ee3726f31..c28550c27f 100644 --- a/spec/models/payroll_run_spec.rb +++ b/spec/models/payroll_run_spec.rb @@ -199,46 +199,6 @@ end end - describe ".create_with_claims!" do - let(:claims) { Policies.all.map { |policy| create(:claim, :approved, policy: policy) } } - let(:topups) { [] } - subject!(:payroll_run) { PayrollRun.create_with_claims!(claims, topups, created_by: user) } - - it "creates a payroll run with payments and populates the award_amount" do - expect(payroll_run.reload.created_by.id).to eq(user.id) - expect(payroll_run.claims).to match_array(claims) - expect(claims[0].payments.first.award_amount).to eq(claims[0].award_amount) - expect(claims[1].payments.first.award_amount).to eq(claims[1].award_amount) - end - - context "with multiple claims from the same teacher reference number" do - let(:personal_details) do - { - national_insurance_number: generate(:national_insurance_number), - eligibility_attributes: {teacher_reference_number: generate(:teacher_reference_number)}, - email_address: generate(:email_address), - bank_sort_code: "112233", - bank_account_number: "95928482", - address_line_1: "64 West Lane", - student_loan_plan: StudentLoan::PLAN_1 - } - end - let(:matching_claims) do - [ - create(:claim, :approved, personal_details.merge(policy: Policies::StudentLoans)), - create(:claim, :approved, personal_details.merge(policy: Policies::EarlyCareerPayments)) - ] - end - let(:other_claim) { create(:claim, :approved) } - let(:claims) { matching_claims + [other_claim] } - - it "groups them into a single payment and populates the award_amount" do - expect(payroll_run.payments.map(&:claims)).to match_array([match_array(matching_claims), [other_claim]]) - expect(matching_claims[0].reload.payments.first.award_amount).to eq(matching_claims.sum(&:award_amount)) - end - end - end - describe ".this_month" do it "only includes payroll runs created in this calendar month" do create(:payroll_run, created_at: 1.month.ago)