Fix LoanReport #129

Merged
merged 12 commits into from Mar 21, 2013

2 participants

@olly

Include Legacy SFLG loans when the modified_by_legacy_id is NULL.

@olly

Don't merge this in please. Needs some further changes based on feedback from CfE.

@olly

This is ready now.

This turned into a much bigger re-factor to support filtering by "loan type" instead of a (confusing) combination of loan scheme and loan source.

The responsibility of LoanReport has been split into two. The new LoanReport and the LoanReportPresenter.

LoanReport is just responsible for building the query now, given the criteria. LoanReportPresenter is responsible for parsing the form fields, validating the data & presenting varying interfaces depending on the permissions of the user.

@jabley jabley commented on the diff Mar 18, 2013
app/presenters/loan_report_presenter.rb
+ end
+
+ private
+ def loan_types_are_allowed
+ if loan_types.present? && loan_types.any? { |type| !ALLOWED_LOAN_TYPES.include?(type) }
+ errors.add(:loan_types, :inclusion)
+ end
+ end
+
+ def loan_states_are_allowed
+ if states.present? && states.any? { |state| !ALLOWED_LOAN_STATES.include?(state) }
+ errors.add(:states, :inclusion)
+ end
+ end
+
+ # See http://stackoverflow.com/questions/8929230/why-is-the-first-element-always-blank-in-my-rails-multi-select
@jabley
jabley added a note Mar 18, 2013

This seems to crop up a lot. Is there a better way of fixing it?

@jabley
jabley added a note Mar 18, 2013

Ah, was it just moved from elsewhere in the codebase?

@olly
olly added a note Mar 18, 2013

Yeah, just a refactor. We do use something similar in a couple of other places in the codebase. But not enough to abstract out just yet, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jabley jabley commented on the diff Mar 18, 2013
app/reports/loan_report.rb
- 'loans.guarantee_rate AS loan_guarantee_rate',
- 'loans.premium_rate AS loan_premium_rate',
- 'created_by_user.username AS created_by_username',
- 'modified_by_user.username AS modified_by_username',
- '(SELECT organisation_reference_code FROM lenders WHERE id = loans.lender_id) AS lender_organisation_reference_code',
- '(SELECT recovered_on FROM recoveries WHERE loan_id = loans.id ORDER BY recoveries.id DESC LIMIT 1) AS last_recovery_on',
- '(SELECT SUM(amount_due_to_dti) FROM recoveries WHERE loan_id = loans.id) AS total_recoveries',
- '(SELECT created_at FROM loan_realisations WHERE realised_loan_id = loans.id ORDER BY loan_realisations.id DESC LIMIT 1) AS last_realisation_at',
- '(SELECT SUM(realised_amount) FROM loan_realisations WHERE realised_loan_id = loans.id) AS total_loan_realisations',
- '(SELECT SUM(amount_drawn) FROM loan_modifications WHERE loan_id = loans.id) AS total_amount_drawn',
- '(SELECT SUM(lump_sum_repayment) FROM loan_modifications WHERE loan_id = loans.id) AS total_lump_sum_repayment'
- ].join(',')
- )
+ if loan_types.present?
+ # Tried to use Arel here, but it was segfaulting. Falling back to string
+ # concatination - what web developers do best...
@jabley
jabley added a note Mar 18, 2013

Concatenation like this kills teh kitties.

The Arel code that was segfaulting is presumably what is commented out below? Very odd that what is presumably walking an AST to generate SQL would segfault.

@olly
olly added a note Mar 18, 2013

It was very weird. It would reliably crash the server process. I didn't investigate any further - I just resorted to something else.

I've actually removed the lines that do the special case for Legacy SFLG loans (due to a conversation with Linda). This shortens it to the following, which is a little nicer (e7013bf).

  type_condition = ->(type) { "(loans.loan_scheme = '#{type.scheme}' AND loans.loan_source = '#{type.source}')" }
  scope = scope.where(loan_types.map(&type_condition).join(' OR '))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jabley jabley merged commit 88f369f into master Mar 21, 2013

1 check passed

Details default The Travis build passed
@jabley jabley deleted the loan-report-fix branch Mar 21, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment