Bug 21727 - Add Koha::Account::Line->adjust
Summary: Add Koha::Account::Line->adjust
Status: RESOLVED 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: Josef Moravec
URL:
Keywords:
Depends on: 21980
Blocks: 21747
  Show dependency treegraph
 
Reported: 2018-10-30 18:52 UTC by Martin Renvoize
Modified: 2019-06-27 09:24 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21727: Add ->adjust to Koha::Account::Line (4.04 KB, patch)
2018-11-02 10:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add tests for Koha::Account::Line->adjust (4.29 KB, patch)
2018-11-30 11:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add ->adjust to Koha::Account::Line (3.99 KB, patch)
2018-11-30 11:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add tests for Koha::Account::Line->adjust (4.34 KB, patch)
2018-12-11 15:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21727: Add ->adjust to Koha::Account::Line (4.53 KB, patch)
2018-12-11 15:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21727: (QA follow-up) Test lastincrement changes (3.36 KB, patch)
2018-12-11 15:38 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21727: Add clarifications to the POD of adjust (1.03 KB, patch)
2018-12-11 17:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add tests for Koha::Account::Line->adjust (4.37 KB, patch)
2018-12-12 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add ->adjust to Koha::Account::Line (4.56 KB, patch)
2018-12-12 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: (QA follow-up) Test lastincrement changes (3.42 KB, patch)
2018-12-12 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add clarifications to the POD of adjust (1.05 KB, patch)
2018-12-12 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add tests for Koha::Account::Line->adjust (4.43 KB, patch)
2018-12-12 11:36 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21727: Add ->adjust to Koha::Account::Line (4.62 KB, patch)
2018-12-12 11:36 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21727: (QA follow-up) Test lastincrement changes (3.48 KB, patch)
2018-12-12 11:36 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21727: Add clarifications to the POD of adjust (1.10 KB, patch)
2018-12-12 11:36 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21727: Add handling for cases requireing patron refunds (1.70 KB, patch)
2018-12-12 12:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21727: Add handling for cases requireing patron refunds (1.75 KB, patch)
2018-12-12 13:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21727: Unit tests for the refund case (3.06 KB, patch)
2018-12-12 13:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21727: Unit tests for the refund case (3.11 KB, patch)
2018-12-12 14:16 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21727: Add handling for cases requireing patron refunds (1.81 KB, patch)
2018-12-12 14:16 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21727: (QA follow-up) Fix POD encoding (593 bytes, patch)
2019-01-11 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

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