CAPT 983/slow loading payroll

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
 

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

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

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

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

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,

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

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