Bug 21727

Summary: Add Koha::Account::Line->adjust
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, josef.moravec, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21759
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21980    
Bug Blocks: 21747, 25596    
Attachments: Bug 21727: Add ->adjust to Koha::Account::Line
Bug 21727: Add tests for Koha::Account::Line->adjust
Bug 21727: Add ->adjust to Koha::Account::Line
Bug 21727: Add tests for Koha::Account::Line->adjust
Bug 21727: Add ->adjust to Koha::Account::Line
Bug 21727: (QA follow-up) Test lastincrement changes
Bug 21727: Add clarifications to the POD of adjust
Bug 21727: Add tests for Koha::Account::Line->adjust
Bug 21727: Add ->adjust to Koha::Account::Line
Bug 21727: (QA follow-up) Test lastincrement changes
Bug 21727: Add clarifications to the POD of adjust
Bug 21727: Add tests for Koha::Account::Line->adjust
Bug 21727: Add ->adjust to Koha::Account::Line
Bug 21727: (QA follow-up) Test lastincrement changes
Bug 21727: Add clarifications to the POD of adjust
Bug 21727: Add handling for cases requireing patron refunds
Bug 21727: Add handling for cases requireing patron refunds
Bug 21727: Unit tests for the refund case
Bug 21727: Unit tests for the refund case
Bug 21727: Add handling for cases requireing patron refunds
Bug 21727: (QA follow-up) Fix POD encoding

Description Martin Renvoize 2018-10-30 18:52:20 UTC
There should be a high-level method for update existing debits, similar to Koha::Account::add_credit and Koha::Account::add_debit

It will handle updating the line, and adding an offset.

Envisaged use case is for debit amendments and fine increments.
Comment 1 Martin Renvoize 2018-11-02 10:46:29 UTC
Created attachment 81879 [details] [review]
Bug 21727: Add ->adjust to Koha::Account::Line
Comment 2 Martin Renvoize 2018-11-02 10:49:30 UTC
Proof Of Concept... tests to come
Comment 3 Martin Renvoize 2018-11-30 11:54:19 UTC
Created attachment 82777 [details] [review]
Bug 21727: Add tests for Koha::Account::Line->adjust
Comment 4 Martin Renvoize 2018-11-30 11:54:22 UTC
Created attachment 82778 [details] [review]
Bug 21727: Add ->adjust to Koha::Account::Line
Comment 5 Josef Moravec 2018-12-05 10:05:15 UTC
When running test I got this: 

Can't locate object method "throw" via package "Koha::Exceptions::Account::UnrecognisedType" (perhaps you forgot to load "Koha::Exceptions::Account::UnrecognisedType"?) at /home/vagrant/kohaclone/Koha/Account/Line.pm line 231

Maybe you forgot one patch? Or is there a dependency bug report?
Comment 6 Tomás Cohen Arazi 2018-12-09 19:56:57 UTC
(In reply to Josef Moravec from comment #5)
> When running test I got this: 
> 
> Can't locate object method "throw" via package
> "Koha::Exceptions::Account::UnrecognisedType" (perhaps you forgot to load
> "Koha::Exceptions::Account::UnrecognisedType"?) at
> /home/vagrant/kohaclone/Koha/Account/Line.pm line 231
> 
> Maybe you forgot one patch? Or is there a dependency bug report?

This one depends on a couple exceptions that are added in bug 21002. Adding the dependency now.
Comment 7 Martin Renvoize 2018-12-10 11:47:21 UTC
Thanks Tomas, that's it.. sorry I missed the dependency chain in the first place.
Comment 8 Tomás Cohen Arazi 2018-12-11 15:38:03 UTC
Created attachment 83054 [details] [review]
Bug 21727: Add tests for Koha::Account::Line->adjust

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2018-12-11 15:38:08 UTC
Created attachment 83055 [details] [review]
Bug 21727: Add ->adjust to Koha::Account::Line

This patch introduces a high-level method for updating existing debits,
similar to Koha::Account::add_credit and Koha::Account::add_debit.

It updates the Koha::Account::Line object, and adds an offset. It also
logs if required.

It whitelists the account types that are allowed to be adjusted, just in
case some others require adjusting, having a single source for truth.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2018-12-11 15:38:14 UTC
Created attachment 83056 [details] [review]
Bug 21727: (QA follow-up) Test lastincrement changes

'adjust' changes the 'lastincrement column, and this behaviour should be
tested. This patch just adds tests for that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Martin Renvoize 2018-12-11 16:43:34 UTC
Awesome.. thanks for testing (and enhancing the test coverage) Tomas.

For Josef's benefit I've also updated the dependancies.. splitting out the exception class additions into their own bug so we're not dependant on the larger and less related bug 21002
Comment 12 Martin Renvoize 2018-12-11 16:54:53 UTC
Also to note.. the only use case that exists so far for this method is the adjustment of the non-definitive 'FU' type accountline which is a 'still incrementing' charge.  

I think the majority of accounts should be handled by other methods adding credit/debit pairs and only 'non-definitive/in-progress' incrementing fines should utilise this method.. 

One such incrementing fine I could think of perhaps wishing to use this in the future that might make sense would be the introduction of a more granular lost item charging scheme that acted as a buffer to 'overdue' and would charge at a different rate whilst the user either found the item or the system decided it was well and truly lost. (in such a case I would expect an LU accountype to get paired with the existing L type).
Comment 13 Tomás Cohen Arazi 2018-12-11 17:17:05 UTC
(In reply to Martin Renvoize from comment #12)
> Also to note.. the only use case that exists so far for this method is the
> adjustment of the non-definitive 'FU' type accountline which is a 'still
> incrementing' charge.  
> 
> I think the majority of accounts should be handled by other methods adding
> credit/debit pairs and only 'non-definitive/in-progress' incrementing fines
> should utilise this method.. 
> 
> One such incrementing fine I could think of perhaps wishing to use this in
> the future that might make sense would be the introduction of a more
> granular lost item charging scheme that acted as a buffer to 'overdue' and
> would charge at a different rate whilst the user either found the item or
> the system decided it was well and truly lost. (in such a case I would
> expect an LU accountype to get paired with the existing L type).

I would add the fact that this fee is either a fixed about (by itemtype) or the one specified in the lost item itself. On the second case, that value can get easily outdated compared to real bookseller prices in a changing world, so having a way to adjust that value makes sense in a future.
Comment 14 Martin Renvoize 2018-12-11 17:45:54 UTC
Created attachment 83058 [details] [review]
Bug 21727: Add clarifications to the POD of adjust

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2018-12-12 09:07:35 UTC
Created attachment 83088 [details] [review]
Bug 21727: Add tests for Koha::Account::Line->adjust

Sponsored-by: PTFS Europe
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Martin Renvoize 2018-12-12 09:07:38 UTC
Created attachment 83089 [details] [review]
Bug 21727: Add ->adjust to Koha::Account::Line

This patch introduces a high-level method for updating existing debits,
similar to Koha::Account::add_credit and Koha::Account::add_debit.

It updates the Koha::Account::Line object, and adds an offset. It also
logs if required.

It whitelists the account types that are allowed to be adjusted, just in
case some others require adjusting, having a single source for truth.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!

Sponsored-by: PTFS Europe
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Martin Renvoize 2018-12-12 09:07:42 UTC
Created attachment 83090 [details] [review]
Bug 21727: (QA follow-up) Test lastincrement changes

'adjust' changes the 'lastincrement column, and this behaviour should be
tested. This patch just adds tests for that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2018-12-12 09:07:45 UTC
Created attachment 83091 [details] [review]
Bug 21727: Add clarifications to the POD of adjust

Sponsored-by: PTFS Europe
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Josef Moravec 2018-12-12 11:36:26 UTC
Created attachment 83095 [details] [review]
Bug 21727: Add tests for Koha::Account::Line->adjust

Sponsored-by: PTFS Europe
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Josef Moravec 2018-12-12 11:36:31 UTC
Created attachment 83096 [details] [review]
Bug 21727: Add ->adjust to Koha::Account::Line

This patch introduces a high-level method for updating existing debits,
similar to Koha::Account::add_credit and Koha::Account::add_debit.

It updates the Koha::Account::Line object, and adds an offset. It also
logs if required.

It whitelists the account types that are allowed to be adjusted, just in
case some others require adjusting, having a single source for truth.

To test:
- Apply this patches
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: Tests pass!

Sponsored-by: PTFS Europe
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Josef Moravec 2018-12-12 11:36:35 UTC
Created attachment 83097 [details] [review]
Bug 21727: (QA follow-up) Test lastincrement changes

'adjust' changes the 'lastincrement column, and this behaviour should be
tested. This patch just adds tests for that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 22 Josef Moravec 2018-12-12 11:36:39 UTC
Created attachment 83098 [details] [review]
Bug 21727: Add clarifications to the POD of adjust

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 23 Martin Renvoize 2018-12-12 12:12:45 UTC
Created attachment 83101 [details] [review]
Bug 21727: Add handling for cases requireing patron refunds

As it is possible to partially pay off a fine that is still incrementing
we can end up in a position (when dropbox mode is enabled) where the
patron may be owed money by the library should they have paid off the
fine between dropping the item off in dropbox mode and the increment
that reverts the increase on the dropbox day.
Comment 24 Martin Renvoize 2018-12-12 12:14:51 UTC
Marking as Failed QA.. Tomas has identified a case where the adjust may lead to a situation where the library owes the patron a refund and we agree we should never allow the 'amountoutstanding' to become negative.

As such, I have produced a further patch to handle this case, adding a credit to the patrons account, but I still need to add a test for this case to the tests.
Comment 25 Tomás Cohen Arazi 2018-12-12 13:51:35 UTC
Created attachment 83117 [details] [review]
Bug 21727: Add handling for cases requireing patron refunds

As it is possible to partially pay off a fine that is still incrementing
we can end up in a position (when dropbox mode is enabled) where the
patron may be owed money by the library should they have paid off the
fine between dropping the item off in dropbox mode and the increment
that reverts the increase on the dropbox day.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2018-12-12 13:51:43 UTC
Created attachment 83118 [details] [review]
Bug 21727: Unit tests for the refund case

This patch introduces tests for the behaviour when the negative adjust
exceedes the amount that was already paid. In this case, the line amount
is expected to be set to 0, and a credit with the payed amount created.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Josef Moravec 2018-12-12 14:16:06 UTC
Created attachment 83121 [details] [review]
Bug 21727: Unit tests for the refund case

This patch introduces tests for the behaviour when the negative adjust
exceedes the amount that was already paid. In this case, the line amount
is expected to be set to 0, and a credit with the payed amount created.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account/Lines.t
=> SUCCESS: tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2018-12-12 14:16:10 UTC
Created attachment 83122 [details] [review]
Bug 21727: Add handling for cases requireing patron refunds

As it is possible to partially pay off a fine that is still incrementing
we can end up in a position (when dropbox mode is enabled) where the
patron may be owed money by the library should they have paid off the
fine between dropping the item off in dropbox mode and the increment
that reverts the increase on the dropbox day.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Josef Moravec 2018-12-12 14:17:27 UTC
Great catch
Comment 30 Nick Clemens 2019-01-11 13:32:07 UTC
Awesome work all!

Pushed to master for 19.05
Comment 31 Tomás Cohen Arazi 2019-01-11 18:19:18 UTC
Created attachment 83818 [details] [review]
Bug 21727: (QA follow-up) Fix POD encoding
Comment 32 Nick Clemens 2019-01-11 18:52:24 UTC
(In reply to Tomás Cohen Arazi from comment #31)
> Created attachment 83818 [details] [review] [review]
> Bug 21727: (QA follow-up) Fix POD encoding

Follow-up pushed to master for 19.05
Comment 33 Martin Renvoize 2019-01-14 14:02:45 UTC
Enhancement, will not be backported to 18.11.x series.
Comment 34 Kyle M Hall 2020-01-30 12:32:51 UTC
*** Bug 24431 has been marked as a duplicate of this bug. ***