Bug 27636

Summary: Replace Koha::Account::pay with a simpler method
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24300
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement adds the new 'payin_amount' method to Koha::Account. This method utilises the well tested 'Koha::Account->add_credit' and 'Koha::Account::Line->apply' methods to achieve consistent results when paying in credit against debts. The original 'pay' method now uses payin_amount internally and will be left intact for a deprecation period.
Version(s) released in:
21.05.00
Bug Depends on: 27995    
Bug Blocks: 22435, 29139    
Attachments: Bug 27636: Add offset_amount to Koha::Accounts
Bug 27636: Add payin_amount to Koha::Account
Bug 27636: Add payin_amount to Koha::Account
Bug 27636: Update ->pay to use ->payin_amount internally
Bug 27636: Update failing test
Bug 27636: Update failing test
Bug 27636: Add payin_amount to Koha::Account
Bug 27636: Update ->pay to use ->payin_amount internally
Bug 27636: Update failing test
Bug 27636: Add payin_amount to Koha::Account
Bug 27636: Update ->pay to use ->payin_amount internally
Bug 27636: Update failing test
Bug 27636: Add payin_amount to Koha::Account
Bug 27636: Update ->pay to use ->payin_amount internally
Bug 27636: Update failing test
Bug 27636: Add payin_amount to Koha::Account
Bug 27636: Update ->pay to use ->payin_amount internally
Bug 27636: Update failing test
Bug 27636: (QA follow-up) Fix tests and validate
Bug 27636: (QA follow-up) Fix tests and validate
Bug 27636: Be explicit on the exception we expect
Bug 27636: (QA follow-up) Fix tests and validate
Bug 27636: Be explicit on the exception we expect
Bug 27636: (QA follow-up) Restore AutoReconcile assumption
Bug 27636: (QA follow-up) Restore AutoReconcile assumption
Bug 27636: (QA follow-up) Restore AutoReconcile assumption
Bug 27636: (QA follow-up) Restore force AutoReconcile
Bug 27636: (QA follow-up) Unit tests for AutoReconcile
Bug 27636: (QA follow-up) Unit tests for AutoReconcile
Bug 27636: (QA follow-up) Restore force AutoReconcile
Bug 27636: (QA follow-up) Unit tests for AutoReconcile

Description Martin Renvoize 2021-02-05 12:58:58 UTC
The pay method served us well but it can, and should, now be cleaned up or replaced by a simpler method that calls the underlying add_credit + apply methods.
Comment 1 Martin Renvoize 2021-02-05 13:10:18 UTC
Created attachment 116393 [details] [review]
Bug 27636: Add offset_amount to Koha::Accounts
Comment 2 Martin Renvoize 2021-02-05 13:11:01 UTC
This is a work in progress.. just putting the patch here for early thoughts and to display progress
Comment 3 Martin Renvoize 2021-03-18 20:31:53 UTC
Created attachment 118471 [details] [review]
Bug 27636: Add payin_amount to Koha::Account

This patch adds a `payin_amount` method to the Koha::Account as a modern
replacement to the `pay` method.

The new method uses the `add_credit` and `Koha::Account::Line->apply`
methods internally for consistency and returns the created credit
Koha::Account::Line result.

Test plan
1/ Run t/db_dependent/Koha/Account.t
Comment 4 Martin Renvoize 2021-03-19 12:02:55 UTC
Created attachment 118515 [details] [review]
Bug 27636: Add payin_amount to Koha::Account

This patch adds a `payin_amount` method to the Koha::Account as a modern
replacement to the `pay` method.

The new method uses the `add_credit` and `Koha::Account::Line->apply`
methods internally for consistency and returns the created credit
Koha::Account::Line result.

Test plan
1/ Run t/db_dependent/Koha/Account.t
Comment 5 Martin Renvoize 2021-03-19 12:02:58 UTC
Created attachment 118516 [details] [review]
Bug 27636: Update ->pay to use ->payin_amount internally

This patch is an interim patch to update Koha::Account->pay to use the
new Koha::Account->payin_amount method internally. It simply utilises
that method for the logic and acts as an input/output shim for
compatability and to allow us to use the existing tests.

Test plan
1/ Run t/db_dependent/Koha/Account.t
Comment 6 Martin Renvoize 2021-03-19 12:07:22 UTC
Created attachment 118522 [details] [review]
Bug 27636: Update failing test

It would appear that ->pay didn't fully respect AccountAutoReconcile!
Comment 7 Martin Renvoize 2021-03-19 12:09:53 UTC
Created attachment 118523 [details] [review]
Bug 27636: Update failing test

It would appear that ->pay didn't fully respect AccountAutoReconcile!
Comment 8 Martin Renvoize 2021-03-19 12:27:03 UTC
Setting NSO for feedback.. there's still work to be done here.. but I think these patches prove the way in that the new payin_amount routine can be used to replace the pay method.

Next steps I envisage

* Migrate pay tests into payin_amount tests
* Migrate pay calls to payin_amount
Comment 9 Martin Renvoize 2021-04-06 11:13:59 UTC
Created attachment 119205 [details] [review]
Bug 27636: Add payin_amount to Koha::Account

This patch adds a `payin_amount` method to the Koha::Account as a modern
replacement to the `pay` method.

The new method uses the `add_credit` and `Koha::Account::Line->apply`
methods internally for consistency and returns the created credit
Koha::Account::Line result.

Test plan
1/ Run t/db_dependent/Koha/Account.t
Comment 10 Martin Renvoize 2021-04-06 11:14:02 UTC
Created attachment 119206 [details] [review]
Bug 27636: Update ->pay to use ->payin_amount internally

This patch is an interim patch to update Koha::Account->pay to use the
new Koha::Account->payin_amount method internally. It simply utilises
that method for the logic and acts as an input/output shim for
compatability and to allow us to use the existing tests.

Test plan
1/ Run t/db_dependent/Koha/Account.t
Comment 11 Martin Renvoize 2021-04-06 11:14:05 UTC
Created attachment 119207 [details] [review]
Bug 27636: Update failing test

It would appear that ->pay didn't fully respect AccountAutoReconcile!
Comment 12 David Nind 2021-04-06 19:38:30 UTC
Created attachment 119242 [details] [review]
Bug 27636: Add payin_amount to Koha::Account

This patch adds a `payin_amount` method to the Koha::Account as a modern
replacement to the `pay` method.

The new method uses the `add_credit` and `Koha::Account::Line->apply`
methods internally for consistency and returns the created credit
Koha::Account::Line result.

Test plan
1/ Run t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2021-04-06 19:38:33 UTC
Created attachment 119243 [details] [review]
Bug 27636: Update ->pay to use ->payin_amount internally

This patch is an interim patch to update Koha::Account->pay to use the
new Koha::Account->payin_amount method internally. It simply utilises
that method for the logic and acts as an input/output shim for
compatability and to allow us to use the existing tests.

Test plan
1/ Run t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2021-04-06 19:38:37 UTC
Created attachment 119244 [details] [review]
Bug 27636: Update failing test

It would appear that ->pay didn't fully respect AccountAutoReconcile!

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Martin Renvoize 2021-04-22 16:21:23 UTC
Created attachment 120022 [details] [review]
Bug 27636: Add payin_amount to Koha::Account

This patch adds a `payin_amount` method to the Koha::Account as a modern
replacement to the `pay` method.

The new method uses the `add_credit` and `Koha::Account::Line->apply`
methods internally for consistency and returns the created credit
Koha::Account::Line result.

Test plan
1/ Run t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 Martin Renvoize 2021-04-22 16:21:27 UTC
Created attachment 120023 [details] [review]
Bug 27636: Update ->pay to use ->payin_amount internally

This patch is an interim patch to update Koha::Account->pay to use the
new Koha::Account->payin_amount method internally. It simply utilises
that method for the logic and acts as an input/output shim for
compatability and to allow us to use the existing tests.

Test plan
1/ Run t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Martin Renvoize 2021-04-22 16:21:31 UTC
Created attachment 120024 [details] [review]
Bug 27636: Update failing test

It would appear that ->pay didn't fully respect AccountAutoReconcile!

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 Martin Renvoize 2021-04-22 16:21:43 UTC
Rebased.
Comment 19 Kyle M Hall 2021-04-23 12:55:43 UTC
Created attachment 120089 [details] [review]
Bug 27636: Add payin_amount to Koha::Account

This patch adds a `payin_amount` method to the Koha::Account as a modern
replacement to the `pay` method.

The new method uses the `add_credit` and `Koha::Account::Line->apply`
methods internally for consistency and returns the created credit
Koha::Account::Line result.

Test plan
1/ Run t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2021-04-23 12:55:58 UTC
Created attachment 120090 [details] [review]
Bug 27636: Update ->pay to use ->payin_amount internally

This patch is an interim patch to update Koha::Account->pay to use the
new Koha::Account->payin_amount method internally. It simply utilises
that method for the logic and acts as an input/output shim for
compatability and to allow us to use the existing tests.

Test plan
1/ Run t/db_dependent/Koha/Account.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2021-04-23 12:56:03 UTC
Created attachment 120091 [details] [review]
Bug 27636: Update failing test

It would appear that ->pay didn't fully respect AccountAutoReconcile!

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Jonathan Druart 2021-04-27 14:25:45 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 23 Martin Renvoize 2021-04-28 09:23:50 UTC
Created attachment 120257 [details] [review]
Bug 27636: (QA follow-up) Fix tests and validate

This patch fixes the tests for when a negative amount is passed to the
pay method.  Prior to now, a negative amount would have been passed
through and recorded. This was inconsistent with all other accounts
methods and has been deprecated to ensure consistent amounts handling.

This patch also introduces basic validation to prevent negatives being
entered into the UI.
Comment 24 Martin Renvoize 2021-04-28 09:27:38 UTC
Created attachment 120258 [details] [review]
Bug 27636: (QA follow-up) Fix tests and validate

This patch fixes the tests for when a negative amount is passed to the
pay method.  Prior to now, a negative amount would have been passed
through and recorded. This was inconsistent with all other accounts
methods and has been deprecated to ensure consistent amounts handling.

This patch also introduces basic validation to prevent negatives being
entered into the UI.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Jonathan Druart 2021-04-28 09:32:30 UTC
Created attachment 120259 [details] [review]
Bug 27636: Be explicit on the exception we expect
Comment 26 Kyle M Hall 2021-04-28 10:26:10 UTC
Created attachment 120262 [details] [review]
Bug 27636: (QA follow-up) Fix tests and validate

This patch fixes the tests for when a negative amount is passed to the
pay method.  Prior to now, a negative amount would have been passed
through and recorded. This was inconsistent with all other accounts
methods and has been deprecated to ensure consistent amounts handling.

This patch also introduces basic validation to prevent negatives being
entered into the UI.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2021-04-28 10:29:12 UTC
Created attachment 120263 [details] [review]
Bug 27636: Be explicit on the exception we expect

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Martin Renvoize 2021-04-28 13:38:50 UTC
Created attachment 120270 [details] [review]
Bug 27636: (QA follow-up) Restore AutoReconcile assumption

This patch restores the behaviour of paycollect which relied on
Koha::Account->pay ignoring the value of the AutoReconcile system
preference.

We now explicitly fall back to selecting all outstanding debit lines if
a selection of lines is not passed into the controller.
Comment 29 Martin Renvoize 2021-04-28 13:44:35 UTC
Created attachment 120271 [details] [review]
Bug 27636: (QA follow-up) Restore AutoReconcile assumption

This patch restores the behaviour of paycollect which relied on
Koha::Account->pay ignoring the value of the AutoReconcile system
preference.

We now explicitly fall back to selecting all outstanding debit lines if
a selection of lines is not passed into the controller.
Comment 30 Nick Clemens 2021-04-28 14:02:11 UTC
Created attachment 120272 [details] [review]
Bug 27636: (QA follow-up) Restore AutoReconcile assumption

This patch restores the behaviour of paycollect which relied on
Koha::Account->pay ignoring the value of the AutoReconcile system
preference.

We now explicitly fall back to selecting all outstanding debit lines if
a selection of lines is not passed into the controller.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Martin Renvoize 2021-04-28 14:48:46 UTC
Created attachment 120276 [details] [review]
Bug 27636: (QA follow-up) Restore force AutoReconcile

This patch restores the behaviour ->pay always acting as though
`AccountAutoReconcile` was enabled.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2021-04-28 14:50:40 UTC
That final follow-up needs some tests writing.. they're on my radar and in progress.. but I wanted to put that patch here to get the dashboard green again should you want to push these followups
Comment 33 Martin Renvoize 2021-04-29 06:39:58 UTC
Created attachment 120294 [details] [review]
Bug 27636: (QA follow-up) Unit tests for AutoReconcile
Comment 34 Jonathan Druart 2021-04-29 07:59:41 UTC
I get:
t/db_dependent/Accounts.t .. 1/27 Global symbol "$payment" requires explicit package name (did you forget to declare "my $payment"?) at t/db_dependent/Accounts.t line 240.

Then, when the $payment=>$accountline change is done I get:

t/db_dependent/Accounts.t .. 6/27 Odd number of elements in anonymous hash at /kohadevbox/koha/Koha/Account.pm line 109.
    # Looks like you planned 17 tests but ran 3.

#   Failed test 'Koha::Account::pay - always AutoReconcile + notes tests'
#   at t/db_dependent/Accounts.t line 241.
{UNKNOWN}: Not an ARRAY reference at /kohadevbox/koha/Koha/Account/Line.pm line 626. at /kohadevbox/koha/Koha/Account/Line.pm line 690
Comment 35 Fridolin Somers 2021-04-30 09:01:14 UTC
Enhancement not pushed to 20.11.x
Comment 36 Martin Renvoize 2021-04-30 15:00:45 UTC
Created attachment 120343 [details] [review]
Bug 27636: (QA follow-up) Unit tests for AutoReconcile
Comment 37 Martin Renvoize 2021-04-30 15:04:21 UTC
Created attachment 120344 [details] [review]
Bug 27636: (QA follow-up) Restore force AutoReconcile

This patch restores the behaviour ->pay always acting as though
`AccountAutoReconcile` was enabled.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2021-04-30 15:04:25 UTC
Created attachment 120345 [details] [review]
Bug 27636: (QA follow-up) Unit tests for AutoReconcile
Comment 39 Jonathan Druart 2021-04-30 15:18:10 UTC
Last patches pushed to master.
Comment 40 Fridolin Somers 2021-05-05 13:17:08 UTC
Enhancement not pushed to 20.11.x