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/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/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..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,14 +31,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 - def total_confirmed_payments payments.where.not(confirmation: nil).count end @@ -104,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/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/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/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..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,37 +130,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 } - - 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_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 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