Bug 27636 - Replace Koha::Account::pay with a simpler method
Summary: Replace Koha::Account::pay with a simpler method
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 27995
Blocks: 22435 29139
  Show dependency treegraph
 
Reported: 2021-02-05 12:58 UTC by Martin Renvoize
Modified: 2022-06-06 20:25 UTC (History)
5 users (show)

See Also:
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


Attachments
Bug 27636: Add offset_amount to Koha::Accounts (3.46 KB, patch)
2021-02-05 13:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Add payin_amount to Koha::Account (11.19 KB, patch)
2021-03-18 20:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Add payin_amount to Koha::Account (9.99 KB, patch)
2021-03-19 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Update ->pay to use ->payin_amount internally (10.65 KB, patch)
2021-03-19 12:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Update failing test (974 bytes, patch)
2021-03-19 12:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Update failing test (1.54 KB, patch)
2021-03-19 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Add payin_amount to Koha::Account (9.99 KB, patch)
2021-04-06 11:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Update ->pay to use ->payin_amount internally (10.67 KB, patch)
2021-04-06 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Update failing test (1.55 KB, patch)
2021-04-06 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Add payin_amount to Koha::Account (10.05 KB, patch)
2021-04-06 19:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 27636: Update ->pay to use ->payin_amount internally (10.72 KB, patch)
2021-04-06 19:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 27636: Update failing test (1.59 KB, patch)
2021-04-06 19:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 27636: Add payin_amount to Koha::Account (10.03 KB, patch)
2021-04-22 16:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Update ->pay to use ->payin_amount internally (10.75 KB, patch)
2021-04-22 16:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Update failing test (1.59 KB, patch)
2021-04-22 16:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Add payin_amount to Koha::Account (10.10 KB, patch)
2021-04-23 12:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27636: Update ->pay to use ->payin_amount internally (10.82 KB, patch)
2021-04-23 12:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27636: Update failing test (1.66 KB, patch)
2021-04-23 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Fix tests and validate (11.41 KB, patch)
2021-04-28 09:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Fix tests and validate (13.47 KB, patch)
2021-04-28 09:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: Be explicit on the exception we expect (1.05 KB, patch)
2021-04-28 09:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Fix tests and validate (13.54 KB, patch)
2021-04-28 10:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27636: Be explicit on the exception we expect (1.12 KB, patch)
2021-04-28 10:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Restore AutoReconcile assumption (1011 bytes, patch)
2021-04-28 13:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Restore AutoReconcile assumption (1.41 KB, patch)
2021-04-28 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Restore AutoReconcile assumption (1.47 KB, patch)
2021-04-28 14:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Restore force AutoReconcile (1.34 KB, patch)
2021-04-28 14:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Unit tests for AutoReconcile (7.11 KB, patch)
2021-04-29 06:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Unit tests for AutoReconcile (7.25 KB, patch)
2021-04-30 15:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Restore force AutoReconcile (1.42 KB, patch)
2021-04-30 15:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27636: (QA follow-up) Unit tests for AutoReconcile (7.25 KB, patch)
2021-04-30 15:04 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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