[Koha-bugs] [Bug 15896] Use Koha::Account::pay internally for makepayment

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15896

Marcel de Rooy <m.de.rooy at rijksmuseum.nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Passed QA

--- Comment #42 from Marcel de Rooy <m.de.rooy at rijksmuseum.nl> ---
QA Comment:
Since we are in a migration/consolidation process for Accounts routines, I will
not block this patch set, although Jonathan had some concerns about incomplete
test coverage and code quality.

I agree with Jonathan that we should have some more tests here, but note that
you are not doing complete new things here (just move code).
You added two tests btw. We now have calls to ->pay with and without
accountlines_id.
The statistics test in Accounts.t is quite rudimentary. Could be extended.
A test related to the call of ReturnLostItem could be added. And what about sip
type?

About the code quality. The code in sub pay is getting longer. That is true. If
we first consolidate everything in one spot, we can still improve. But maybe
this is just of matter of adding one or two private subroutines to have a more
readable sub pay?

Another note in the larger scope of making payments: The call to ReturnLostItem
was and now is only when you pay on one line. But what about paying multiple
lines including such a lost fee? Seems to be a point of attention.

On the paycollect interface: If you pay more, the interface responds with a
warn Pay less or equal. The interface should also protest against amount zero
or 0.00 btw.

And in connection to makepayment: There is still a call to makepayment in
opac/opac-account-pay-paypal-return.pl. Glancing through that code, I am
wondering if you really want to pay all accountlines without checking if the
sum matches the amount. But I may be mistaken?

In conclusion: I have no objection to pushing this code now. But in this
migration project we should try to improve later on.

Passed QA

-- 
You are receiving this mail because:
You are watching all bug changes.