From 782fc9a1b96e5e3f7b6e385fcc4b0f66b7c4735c Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Mon, 21 Oct 2024 16:42:10 +0100 Subject: [PATCH 1/2] Remove batching from payroll csv Requirement is to only produce a single payroll file with no limit on the number of payments in the file. This commit removes the zip file as we no longer need it and instead serves the generated csv directly, this lets us simplify the code quite a bit. --- .../admin/payroll_run_downloads_controller.rb | 3 +- app/models/payroll/payment_csv_row.rb | 11 +-- app/models/payroll/payments_csv.rb | 48 +++-------- app/models/payroll_run.rb | 4 - .../admin/payroll_run_downloads/show.html.erb | 8 +- .../admin/admin_payroll_run_download_spec.rb | 5 +- spec/models/payroll/payment_csv_row_spec.rb | 9 +-- spec/models/payroll/payments_csv_spec.rb | 79 ++++++------------- spec/models/payroll_run_spec.rb | 18 ----- .../admin/admin_payroll_run_downloads_spec.rb | 6 +- 10 files changed, 53 insertions(+), 138 deletions(-) diff --git a/app/controllers/admin/payroll_run_downloads_controller.rb b/app/controllers/admin/payroll_run_downloads_controller.rb index 1e4760feae..4961ac2c83 100644 --- a/app/controllers/admin/payroll_run_downloads_controller.rb +++ b/app/controllers/admin/payroll_run_downloads_controller.rb @@ -15,7 +15,8 @@ def create def show respond_to do |format| format.html - format.zip do + + format.csv do out = Payroll::PaymentsCsv.new(@payroll_run) send_data out.data, type: out.content_type, filename: out.filename end diff --git a/app/models/payroll/payment_csv_row.rb b/app/models/payroll/payment_csv_row.rb index caa2d63833..463a2a45ae 100644 --- a/app/models/payroll/payment_csv_row.rb +++ b/app/models/payroll/payment_csv_row.rb @@ -6,7 +6,6 @@ module Payroll class PaymentCsvRow < SimpleDelegator - ROW_SEPARATOR = "\r\n" DATE_FORMAT = "%d/%m/%Y" UNITED_KINGDOM = "United Kingdom" BASIC_RATE_TAX_CODE = "BR" @@ -19,19 +18,15 @@ class PaymentCsvRow < SimpleDelegator RIGHT_TO_WORK_CONFIRM_STATUS = "2" TITLE = "Prof." - def to_s - CSV.generate_line(data, row_sep: ROW_SEPARATOR) - end - - private - - def data + def to_a Payroll::PaymentsCsv::FIELDS_WITH_HEADERS.keys.map do |f| field = send(f) ExcelUtils.escape_formulas(field) end end + private + def title TITLE end diff --git a/app/models/payroll/payments_csv.rb b/app/models/payroll/payments_csv.rb index e2bf9601c7..9c795a4079 100644 --- a/app/models/payroll/payments_csv.rb +++ b/app/models/payroll/payments_csv.rb @@ -4,6 +4,8 @@ module Payroll class PaymentsCsv attr_reader :payroll_run + ROW_SEPARATOR = "\r\n" + FIELDS_WITH_HEADERS = { title: "TITLE", first_name: "FORENAME", @@ -44,49 +46,25 @@ def initialize(payroll_run) end def data - buffer = ::StringIO.new - Zip::OutputStream.write_buffer(buffer) do |file| - payroll_run.payments_in_batches.each.with_index(1) do |batch, index| - file = write_csv(file, batch, index) + CSV.generate( + row_sep: ROW_SEPARATOR, + write_headers: true, + headers: FIELDS_WITH_HEADERS.values + ) do |csv| + payroll_run.payments.includes(:claims).find_in_batches(batch_size: 1000) do |batch| + batch.each do |payment| + csv << PaymentCsvRow.new(payment).to_a + end end end - buffer.rewind - buffer.read end def content_type - "application/zip" + "text/csv" end def filename - "#{filename_base}.zip" - end - - private - - def write_csv(file, batch, index) - file.put_next_entry(filename_batch(index)) - file.write(header_row) - batch.each do |payment| - file.write(Payroll::PaymentCsvRow.new(payment).to_s) - end - file - end - - def filename_batch(index) - "#{filename_base}-batch_#{index}.csv" - end - - def filename_base - "payroll_data_#{payroll_run.created_at.to_date.iso8601}" - end - - def header_row - CSV.generate_line(csv_headers, row_sep: PaymentCsvRow::ROW_SEPARATOR) - end - - def csv_headers - FIELDS_WITH_HEADERS.values + "payroll_data_#{payroll_run.created_at.to_date.iso8601}.csv" end end end diff --git a/app/models/payroll_run.rb b/app/models/payroll_run.rb index 2c6adde813..d8b101865b 100644 --- a/app/models/payroll_run.rb +++ b/app/models/payroll_run.rb @@ -35,10 +35,6 @@ def total_claim_amount_for_policy(policy, filter: :all) line_items(policy, filter: filter).sum(&:award_amount) end - def payments_in_batches - payments.includes(:claims).find_in_batches(batch_size: MAX_BATCH_SIZE) - end - def total_batches (payments.count / MAX_BATCH_SIZE.to_f).ceil end diff --git a/app/views/admin/payroll_run_downloads/show.html.erb b/app/views/admin/payroll_run_downloads/show.html.erb index dbda8bf206..a0b9cb1d69 100644 --- a/app/views/admin/payroll_run_downloads/show.html.erb +++ b/app/views/admin/payroll_run_downloads/show.html.erb @@ -8,13 +8,9 @@ If the download does not start automatically, click on the link below.

-

- NOTE: The generated file will include <%= @payroll_run.total_batches %> - <%= "batch".pluralize(@payroll_run.total_batches) %>. -

- +
- <%= link_to "Download #{@payroll_run.created_at.strftime("%B")} payroll file", admin_payroll_run_download_path(@payroll_run, format: :zip), class: ["govuk-body", "govuk-link"], data: { "auto-follow-link": "true" } if @payroll_run.download_triggered? %> + <%= link_to "Download #{@payroll_run.created_at.strftime("%B")} payroll file", admin_payroll_run_download_path(@payroll_run, format: :csv), class: ["govuk-body", "govuk-link"], data: { "auto-follow-link": "true" } if @payroll_run.download_triggered? %>
diff --git a/spec/features/admin/admin_payroll_run_download_spec.rb b/spec/features/admin/admin_payroll_run_download_spec.rb index 22a9d46013..c65477f55e 100644 --- a/spec/features/admin/admin_payroll_run_download_spec.rb +++ b/spec/features/admin/admin_payroll_run_download_spec.rb @@ -14,10 +14,9 @@ click_on "Download #{payroll_run.created_at.strftime("%B")} payroll file" - expect(page.response_headers["Content-Type"]).to eq("application/zip") + expect(page.response_headers["Content-Type"]).to eq("text/csv") - zip = Zip::InputStream.open(::StringIO.new(body)) - csv = CSV.parse(zip.get_next_entry.get_input_stream.read, headers: true) + csv = CSV.parse(body, headers: true) expect(csv.count).to eq(9) end end diff --git a/spec/models/payroll/payment_csv_row_spec.rb b/spec/models/payroll/payment_csv_row_spec.rb index c5445b3984..d4c8e0fa94 100644 --- a/spec/models/payroll/payment_csv_row_spec.rb +++ b/spec/models/payroll/payment_csv_row_spec.rb @@ -8,8 +8,8 @@ subject { described_class.new(payment) } - describe "#to_s" do - let(:row) { CSV.parse(subject.to_s).first } + describe "#to_a" do + let(:row) { subject.to_a } let(:payment_award_amount) { BigDecimal("1234.56") } let(:payment) { create(:payment, award_amount: payment_award_amount, claims: claims) } @@ -247,7 +247,6 @@ describe "start and end dates" do it "returns the first and last dates of the month" do travel_to Date.parse "20 February 2019" do - row = CSV.parse(subject.to_s).first expect(row[7]).to eq "01/02/2019" expect(row[8]).to eq "28/02/2019" end @@ -267,9 +266,5 @@ expect(row[Payroll::PaymentsCsv::FIELDS_WITH_HEADERS.find_index { |k, _| k == :address_line_2 }]).to eq("\\#{claims.first.address_line_2}") end - - it "outputs a CRLF line feed" do - expect(subject.to_s).to end_with("\r\n") - end end end diff --git a/spec/models/payroll/payments_csv_spec.rb b/spec/models/payroll/payments_csv_spec.rb index 4b25fa0b4d..201e3545ca 100644 --- a/spec/models/payroll/payments_csv_spec.rb +++ b/spec/models/payroll/payments_csv_spec.rb @@ -4,76 +4,49 @@ subject(:generator) { described_class.new(payroll_run) } let(:payroll_run) do - create(:payroll_run, claims_counts: {Policies::StudentLoans => 1, Policies::EarlyCareerPayments => 2}, created_at: creation_date) - end - let(:creation_date) { "2023-07-17" } - - def generate_csv(payments) - csv_headers = Payroll::PaymentsCsv::FIELDS_WITH_HEADERS.values - csv_header_row = CSV.generate_line(csv_headers).chomp - - csv_payment_rows = payments.map do |payment| - Payroll::PaymentCsvRow.new(payment).to_s.chomp - end - - [csv_header_row, csv_payment_rows].join("\r\n") + "\r\n" - end - - def load_zip(buffer) - Zip::File.open_buffer(buffer) - end - - def extract(data, index) - load_zip(data).zip.flatten[index - 1] + create( + :payroll_run, + claims_counts: { + Policies::StudentLoans => 1, + Policies::EarlyCareerPayments => 2 + }, + created_at: creation_date + ) end - def extract_csv_name(data, index) - extract(data, index).name - end + let(:creation_date) { "2023-07-17" } - def extract_csv_content(data, index) - extract(data, index).get_input_stream.read + let(:rows) do + generator.data.split("\r\n").map { |row| row.split(",") } end describe "#data" do - subject(:data) { generator.data } - - let(:payments_batch_one) { payroll_run.payments.ordered.first(2) } - let(:payments_batch_two) { [payroll_run.payments.ordered.last] } - - let(:csv_batch_one) { generate_csv(payments_batch_one) } - let(:csv_batch_two) { generate_csv(payments_batch_two) } - - let(:max_batch_size) { 2 } + describe "CSV headers" do + subject(:headers) { rows.first } - before do - stub_const("PayrollRun::MAX_BATCH_SIZE", max_batch_size) + it { is_expected.to eq(described_class::FIELDS_WITH_HEADERS.values) } end - it "produces a zip file" do - expect(load_zip(data)).to be_truthy - end - - context "zip file" do - it "contains CSVs with the batch number in the filename", :aggregate_failures do - expect(extract_csv_name(data, 1)).to eq("payroll_data_#{creation_date}-batch_1.csv") - expect(extract_csv_name(data, 2)).to eq("payroll_data_#{creation_date}-batch_2.csv") - end + describe "CSV content" do + subject(:body) { rows[1..] } - it "contains CSVs with batched payment data", :aggregate_failures do - expect(extract_csv_content(data, 1)).to eq(csv_batch_one) - expect(extract_csv_content(data, 2)).to eq(csv_batch_two) + it do + is_expected.to match_array( + payroll_run.payments.map do |payment| + Payroll::PaymentCsvRow.new(payment).to_a.map(&:to_s) + end + ) end end end describe "#content_type" do - it { expect(generator.content_type).to eq("application/zip") } + subject { generator.content_type } + it { is_expected.to eq("text/csv") } end describe "#filename" do - it "returns a ZIP filename that includes the date of the payroll run" do - expect(generator.filename).to eq("payroll_data_#{creation_date}.zip") - end + subject { generator.filename } + it { is_expected.to eq("payroll_data_#{creation_date}.csv") } end end diff --git a/spec/models/payroll_run_spec.rb b/spec/models/payroll_run_spec.rb index b343ea6373..0e298dd1a0 100644 --- a/spec/models/payroll_run_spec.rb +++ b/spec/models/payroll_run_spec.rb @@ -159,24 +159,6 @@ end end - describe "#payments_in_batches" do - subject(:batches) { payroll_run.payments_in_batches } - - let(:payroll_run) { create(:payroll_run, claims_counts: {Policies::StudentLoans => 5}) } - let(:batch_size) { 2 } - let(:expected_batches) { payroll_run.payments.ordered.each_slice(batch_size).to_a } - - before do - stub_const("#{described_class}::MAX_BATCH_SIZE", batch_size) - end - - it { is_expected.to be_an(Enumerator) } - - it "returns payments in batches" do - expect(batches.to_a).to eq(expected_batches) - end - end - describe "#total_batches" do subject(:total) { payroll_run.total_batches } diff --git a/spec/requests/admin/admin_payroll_run_downloads_spec.rb b/spec/requests/admin/admin_payroll_run_downloads_spec.rb index 2636d628f4..4aa0873bed 100644 --- a/spec/requests/admin/admin_payroll_run_downloads_spec.rb +++ b/spec/requests/admin/admin_payroll_run_downloads_spec.rb @@ -53,16 +53,16 @@ get admin_payroll_run_download_path(payroll_run) - expect(response.body).to include admin_payroll_run_download_path(payroll_run, format: :zip) + expect(response.body).to include admin_payroll_run_download_path(payroll_run, format: :csv) end end context "when requesting zip" do it "allows the payroll run file to be downloaded" do payroll_run = create(:payroll_run, downloaded_at: Time.zone.now, downloaded_by: admin) - get admin_payroll_run_download_path(payroll_run, format: :zip) + get admin_payroll_run_download_path(payroll_run, format: :csv) - expect(response.headers["Content-Type"]).to eq("application/zip") + expect(response.headers["Content-Type"]).to eq("text/csv") end end end From 6ed3eddf05b725716d0255bfe6a6face810911b0 Mon Sep 17 00:00:00 2001 From: Richard Lynch Date: Tue, 22 Oct 2024 14:34:20 +0100 Subject: [PATCH 2/2] Remove limit on number of payments Now the payroll run software has been updated we no longer need to limit the number of payments in a payroll run. --- .../admin/payroll_runs_controller.rb | 13 ------ app/models/payroll_run.rb | 12 ------ .../features/admin/admin_payroll_runs_spec.rb | 19 --------- spec/models/payroll_run_spec.rb | 42 ------------------- .../requests/admin/admin_payroll_runs_spec.rb | 14 ------- 5 files changed, 100 deletions(-) diff --git a/app/controllers/admin/payroll_runs_controller.rb b/app/controllers/admin/payroll_runs_controller.rb index d1feac8886..415bc92bb4 100644 --- a/app/controllers/admin/payroll_runs_controller.rb +++ b/app/controllers/admin/payroll_runs_controller.rb @@ -11,19 +11,6 @@ def index def new @claims = Claim.payrollable.order(submitted_at: :asc) - # Due to limitations with the current payroll software we need a temporary - # cap on the number of claims that can enter payroll, especially expecting - # a high volume of approved claims in the first few months. - # - # Ideally, we should limit topups as well, as some may be related to claims - # paid in a previous payroll run, but we wouldn't have any topups at the beginning. - # - # TODO: Remove this capping once the payroll software is upgraded. - if @claims.size > PayrollRun::MAX_MONTHLY_PAYMENTS - flash[:notice] = "The number of payments entering this payrun will be capped to #{PayrollRun::MAX_MONTHLY_PAYMENTS}" - @claims = @claims.limit(PayrollRun::MAX_MONTHLY_PAYMENTS) - end - @topups = Topup.payrollable @total_award_amount = @claims.sum(&:award_amount) + @topups.sum(&:award_amount) end diff --git a/app/models/payroll_run.rb b/app/models/payroll_run.rb index d8b101865b..efe46dff20 100644 --- a/app/models/payroll_run.rb +++ b/app/models/payroll_run.rb @@ -1,7 +1,4 @@ class PayrollRun < ApplicationRecord - MAX_BATCH_SIZE = 1000 - MAX_MONTHLY_PAYMENTS = 3000 - has_many :payments, dependent: :destroy has_many :claims, through: :payments has_many :payment_confirmations, dependent: :destroy @@ -13,7 +10,6 @@ class PayrollRun < ApplicationRecord belongs_to :confirmation_report_uploaded_by, class_name: "DfeSignIn::User", optional: true validate :ensure_no_payroll_run_this_month, on: :create - validate :ensure_within_max_monthly_payments, on: :create scope :this_month, -> { where(created_at: DateTime.now.all_month) } @@ -35,10 +31,6 @@ def total_claim_amount_for_policy(policy, filter: :all) line_items(policy, filter: filter).sum(&:award_amount) end - def total_batches - (payments.count / MAX_BATCH_SIZE.to_f).ceil - end - def total_confirmed_payments payments.where.not(confirmation: nil).count end @@ -100,8 +92,4 @@ def line_items(policy, filter: :all) def ensure_no_payroll_run_this_month errors.add(:base, "There has already been a payroll run for #{Date.today.strftime("%B")}") if PayrollRun.this_month.any? end - - def ensure_within_max_monthly_payments - errors.add(:base, "This payroll run exceeds #{MAX_MONTHLY_PAYMENTS} payments") if payments.size > MAX_MONTHLY_PAYMENTS - end end diff --git a/spec/features/admin/admin_payroll_runs_spec.rb b/spec/features/admin/admin_payroll_runs_spec.rb index 29ba848608..997843d563 100644 --- a/spec/features/admin/admin_payroll_runs_spec.rb +++ b/spec/features/admin/admin_payroll_runs_spec.rb @@ -59,25 +59,6 @@ end end - scenario "Limiting the maximum number of claims entering payroll" do - click_on "Payroll" - - expected_claims = create_list(:claim, 3, :approved) - stubbed_max_payments = stub_const("PayrollRun::MAX_MONTHLY_PAYMENTS", 2) - - month_name = Date.today.strftime("%B") - click_on "Run #{month_name} payroll" - - expect(page).to have_content("The number of payments entering this payrun will be capped to #{stubbed_max_payments}") - - click_on "Confirm and submit" - - expect(page).to have_content("Approved claims 2") - - payroll_run = PayrollRun.order(:created_at).last - expect(payroll_run.claims).to match_array(expected_claims.first(stubbed_max_payments)) - end - scenario "Any claims approved in the meantime are not included" do click_on "Payroll" diff --git a/spec/models/payroll_run_spec.rb b/spec/models/payroll_run_spec.rb index 0e298dd1a0..18a212128c 100644 --- a/spec/models/payroll_run_spec.rb +++ b/spec/models/payroll_run_spec.rb @@ -23,35 +23,6 @@ end end - context "validating the number of payments entering payroll" do - let(:stubbed_max_payments) { 10 } - let(:payroll_run) { build(:payroll_run, :with_payments, count: payments_count) } - - before do - stub_const("PayrollRun::MAX_MONTHLY_PAYMENTS", stubbed_max_payments) - end - - context "when exceeding the number of maximum allowed payments" do - let(:payments_count) { stubbed_max_payments + 1 } - - it "returns a validation error", :aggregate_failures do - expect(payroll_run.valid?).to eq(false) - expect(payroll_run.errors[:base]).to eq(["This payroll run exceeds #{stubbed_max_payments} payments"]) - expect { payroll_run.save! }.to raise_error(ActiveRecord::RecordInvalid) - end - end - - context "when not exceeding the number of maximum allowed payments" do - let(:payments_count) { stubbed_max_payments } - - it "creates the payroll run", :aggregate_failures do - expect(payroll_run.valid?).to eq(true) - expect(payroll_run.errors[:base]).to be_empty - expect { payroll_run.save! }.to change { payroll_run.persisted? }.to(true) - end - end - end - describe "#total_award_amount" do it "returns the sum of the award amounts of its claims" do payment_1 = build(:payment, claims: [build(:claim, :approved, eligibility_attributes: {student_loan_repayment_amount: 1500})]) @@ -159,19 +130,6 @@ end end - describe "#total_batches" do - subject(:total) { payroll_run.total_batches } - - let(:payroll_run) { create(:payroll_run, claims_counts: {Policies::StudentLoans => 5}) } - let(:batch_size) { 2 } - - before do - stub_const("#{described_class}::MAX_BATCH_SIZE", batch_size) - end - - it { is_expected.to eq(3) } - end - describe "#total_confirmed_payments" do subject(:total) { payroll_run.total_confirmed_payments } diff --git a/spec/requests/admin/admin_payroll_runs_spec.rb b/spec/requests/admin/admin_payroll_runs_spec.rb index ba603a0af4..b0539ecf2c 100644 --- a/spec/requests/admin/admin_payroll_runs_spec.rb +++ b/spec/requests/admin/admin_payroll_runs_spec.rb @@ -13,20 +13,6 @@ expect(response).to have_http_status(:ok) expect(response.body).to include("£100") end - - it "limits the number of claims entering payroll when exceeding the maximum allowed" do - stubbed_max_payments = stub_const("PayrollRun::MAX_MONTHLY_PAYMENTS", 2) - - create(:claim, :approved, eligibility_attributes: {student_loan_repayment_amount: 100}, submitted_at: 3.days.ago) - create(:claim, :approved, eligibility_attributes: {student_loan_repayment_amount: 50}, submitted_at: 1.days.ago) - create(:claim, :approved, eligibility_attributes: {student_loan_repayment_amount: 10}, submitted_at: 2.days.ago) - - get new_admin_payroll_run_path - - expect(response).to have_http_status(:ok) - expect(response.body).to include("The number of payments entering this payrun will be capped to #{stubbed_max_payments}") - expect(response.body).to include("£110") - end end describe "admin_payroll_runs#create" do