diff --git a/app/controllers/admin/payroll_runs_controller.rb b/app/controllers/admin/payroll_runs_controller.rb index 415bc92bb4..6c5396860a 100644 --- a/app/controllers/admin/payroll_runs_controller.rb +++ b/app/controllers/admin/payroll_runs_controller.rb @@ -31,9 +31,8 @@ def create redirect_to new_admin_payroll_run_path, alert: e.message end - # NOTE: Optimisation - preload payments, claims and eligibility def show - @payroll_run = PayrollRun.where(id: params[:id]).includes({claims: [:eligibility]}, {payments: [{claims: [:eligibility]}]}).first + @payroll_run = PayrollRun.find(params[:id]) @pagy, @payments = pagy(@payroll_run.payments.ordered.includes(claims: [:eligibility]).includes(:topups)) end diff --git a/app/models/base_policy.rb b/app/models/base_policy.rb index d4442b602e..6d43ac0388 100644 --- a/app/models/base_policy.rb +++ b/app/models/base_policy.rb @@ -66,4 +66,8 @@ def approvable?(claim) def decision_deadline_date(claim) (claim.submitted_at + Claim::DECISION_DEADLINE).to_date end + + def award_amount_column + "award_amount" + end end diff --git a/app/models/claim.rb b/app/models/claim.rb index 72240ea760..a4c46fc7e7 100644 --- a/app/models/claim.rb +++ b/app/models/claim.rb @@ -221,6 +221,28 @@ class Claim < ApplicationRecord where.not(id: Claim.awaiting_further_education_provider_verification) end + scope :with_award_amounts, -> do + joins( + <<~SQL + JOIN ( + #{ + Policies::POLICIES.map do |policy| + " + SELECT + id, + #{policy.award_amount_column} AS award_amount, + '#{policy::Eligibility}' AS eligibility_type + FROM #{policy::Eligibility.table_name} + " + end.join(" UNION ALL ") + } + ) AS eligibilities + ON claims.eligibility_id = eligibilities.id + AND claims.eligibility_type = eligibilities.eligibility_type + SQL + ) + end + def onelogin_idv_full_name "#{onelogin_idv_first_name} #{onelogin_idv_last_name}" end diff --git a/app/models/payroll_run.rb b/app/models/payroll_run.rb index efe46dff20..b37959f694 100644 --- a/app/models/payroll_run.rb +++ b/app/models/payroll_run.rb @@ -36,7 +36,9 @@ def total_confirmed_payments end def all_payments_confirmed? - payment_confirmations.any? && total_confirmed_payments == payments.count + return @all_payments_confirmed if defined?(@all_payments_confirmed) + + @all_payments_confirmed = payment_confirmations.any? && total_confirmed_payments == payments_count end def self.create_with_claims!(claims, topups, attrs = {}) @@ -66,27 +68,38 @@ def download_triggered? private + def payments_count + @payments_count ||= payments.count + end + + class LineItem < Struct.new(:id, :award_amount, keyword_init: true); end + def line_items(policy, filter: :all) - @items = [] - - payments.includes(claims: [:eligibility]).includes(:topups).map do |payment| - payment.claims.each do |claim| - if policy == :all || claim.eligibility_type == policy::Eligibility.to_s - topup_claim_ids = payment.topups.pluck(:claim_id) - line_item = topup_claim_ids.include?(claim.id) ? payment.topups.find { |t| t.claim_id == claim.id } : claim - case filter - when :all - @items << line_item - when :claims - @items << line_item if line_item.is_a?(Claim) - when :topups - @items << line_item if line_item.is_a?(Topup) - end - end - end + scope = Claim + .select( + " + DISTINCT(claims.id), + COALESCE(topups.award_amount, eligibilities.award_amount) AS award_amount + " + ) + .with_award_amounts + .left_joins(payments: :topups) + .joins(payments: :payroll_run) + .where(payroll_runs: {id: id}) + + scope = scope.by_policy(policy) unless policy == :all + + case filter + when :claims + scope = scope.where(topups: {id: nil}) + when :topups + scope = scope.where.not(topups: {id: nil}) end - @items + # Claim delegates it's award amount to eligibility, so we want to return + # a non claim object ensuring the award amount is from the topup if there + # is one + ActiveRecord::Base.connection.execute(scope.to_sql).map(&LineItem.method(:new)) end def ensure_no_payroll_run_this_month diff --git a/app/models/policies/student_loans.rb b/app/models/policies/student_loans.rb index 83454eaaa4..16862dba0f 100644 --- a/app/models/policies/student_loans.rb +++ b/app/models/policies/student_loans.rb @@ -153,5 +153,9 @@ def current_financial_year(format = :default) def payroll_file_name "TSLR" end + + def award_amount_column + "student_loan_repayment_amount" + end end end diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 2f9d9d340c..aec4b96381 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -23,6 +23,29 @@ ], "note": "" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "0ac253105d2af7d0dd324537b4be4a392ded73c0f3df8ca4d32db09cac7b586d", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/claim/search.rb", + "line": 35, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "Claim.submitted.where(\"LOWER(#{attribute}) = LOWER(?)\", search_term)", + "render_path": null, + "location": { + "type": "method", + "class": "Search", + "method": "search_by" + }, + "user_input": "attribute", + "confidence": "Medium", + "cwe_id": [ + 89 + ], + "note": "" + }, { "warning_type": "Dynamic Render Path", "warning_code": 15, @@ -115,6 +138,29 @@ ], "note": "" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "cddc34b96e07d19510d574438a1ce61b69d9c1f28c0d4e5eee631d17a9b38ba6", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/claim.rb", + "line": 222, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "joins(\"JOIN (\\n #{Policies::POLICIES.map do\n \"\\n SELECT\\n id,\\n #{policy.award_amount_column} AS award_amount,\\n '#{policy::Eligibility}' AS eligibility_type\\n FROM #{policy::Eligibility.table_name}\\n \"\n end.join(\" UNION ALL \")}\\n) AS eligibilities\\nON claims.eligibility_id = eligibilities.id\\nAND claims.eligibility_type = eligibilities.eligibility_type\\n\")", + "render_path": null, + "location": { + "type": "method", + "class": "Claim", + "method": "with_award_amounts" + }, + "user_input": "Policies::POLICIES.map do\n \"\\n SELECT\\n id,\\n #{policy.award_amount_column} AS award_amount,\\n '#{policy::Eligibility}' AS eligibility_type\\n FROM #{policy::Eligibility.table_name}\\n \"\n end.join(\" UNION ALL \")", + "confidence": "Medium", + "cwe_id": [ + 89 + ], + "note": "Non of the columns used in generating the join are user provided" + }, { "warning_type": "SQL Injection", "warning_code": 0, diff --git a/spec/factories/topups.rb b/spec/factories/topups.rb index f9037d46e8..989916b722 100644 --- a/spec/factories/topups.rb +++ b/spec/factories/topups.rb @@ -1,4 +1,7 @@ FactoryBot.define do factory :topup do + claim + association :created_by, factory: :dfe_signin_user + award_amount { 100.00 } end end diff --git a/spec/models/payroll_run_spec.rb b/spec/models/payroll_run_spec.rb index 18a212128c..2ee3726f31 100644 --- a/spec/models/payroll_run_spec.rb +++ b/spec/models/payroll_run_spec.rb @@ -79,6 +79,124 @@ expect(payroll_run.total_claim_amount_for_policy(Policies::EarlyCareerPayments)).to eq(10_000) expect(payroll_run.total_claim_amount_for_policy(Policies::LevellingUpPremiumPayments)).to eq(2000) end + + context "with topups" do + it "returns the correct total accounting for top ups" do + create(:levelling_up_premium_payments_award) + + topped_up_claim_1 = build( + :claim, + :approved, + policy: Policies::LevellingUpPremiumPayments, + bank_sort_code: "123456", + bank_account_number: "12345678", + national_insurance_number: "1234567", + eligibility_attributes: {award_amount: 100} + ) + + topped_up_claim_2 = build( + :claim, + :approved, + policy: Policies::LevellingUpPremiumPayments, + bank_sort_code: "123456", + bank_account_number: "12345678", + national_insurance_number: "1234567", + eligibility_attributes: {award_amount: 100} + ) + + non_topped_up_claim_1 = build( + :claim, + :approved, + bank_sort_code: "123456", + bank_account_number: "12345678", + national_insurance_number: "1234567", + policy: Policies::LevellingUpPremiumPayments, + eligibility_attributes: {award_amount: 101} + ) + + non_topped_up_claim_2 = build( + :claim, + :approved, + bank_sort_code: "123456", + bank_account_number: "12345678", + national_insurance_number: "1234567", + policy: Policies::LevellingUpPremiumPayments, + eligibility_attributes: {award_amount: 103} + ) + + lup_payment_1 = build( + :payment, + claims: [ + topped_up_claim_1 + ] + ) + + lup_payment_2 = build( + :payment, + claims: [ + topped_up_claim_2 + ] + ) + + lup_payment_3 = build( + :payment, + claims: [ + non_topped_up_claim_1 + ] + ) + + lup_payment_4 = build( + :payment, + claims: [ + non_topped_up_claim_2 + ] + ) + + payroll_run = PayrollRun.create!( + created_by: user, + payments: [ + lup_payment_1, + lup_payment_2, + lup_payment_3, + lup_payment_4 + ] + ) + + create( + :topup, + claim: topped_up_claim_1, + award_amount: 107, + payment: lup_payment_1 + ) + + create( + :topup, + claim: topped_up_claim_2, + award_amount: 109, + payment: lup_payment_2 + ) + + expect( + payroll_run.total_claim_amount_for_policy( + Policies::LevellingUpPremiumPayments + ).to_f + ).to eq(420) # 101 + 103 + 107 + 109 + + expect( + payroll_run.total_claim_amount_for_policy( + Policies::LevellingUpPremiumPayments, + filter: :topups + ) + ).to eq(216) # 107 + 109 + + expect( + payroll_run.total_claim_amount_for_policy( + Policies::LevellingUpPremiumPayments, + filter: :claims + ) + ).to eq(204) # 103 + 101 + end + end end describe ".create_with_claims!" do