Bug 21896

Summary: Add Koha::Account::reconcile_balance
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: cbrannon, fridolin.somers, jesse, josef.moravec, katrin.fischer, kyle, marjorie.barry-vila, martin.renvoize, mtompset, mustaqeemabd, nick, sally.healey, tomascohen
Version: master   
Target Milestone: 19.05   
Hardware: All   
OS: All   
Whiteboard:
Change sponsored?: --- Patch complexity: ---
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Adds a business logic level routine for reconciling user account balances.
Version(s) released in:
Bug Depends on: 20997    
Bug Blocks: 18805, 21915    
Attachments: Bug 21896: Add unit tests for Koha::Account::normalize_balance
[SIGNED OFF]Bug 21896: Add Koha::Account::normalize_balance
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance
[SIGNED OFF]Bug 21896: Add Koha::Account::normalize_balance
Bug 21896: Add unit tests for Koha::Account::normalize_balance
Bug 21896: Add unit tests for Koha::Account::normalize_balance
Bug 21896: Add Koha::Account::normalize_balance
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename
Bug 21896: Add unit tests for Koha::Account::normalize_balance
Bug 21896: Add Koha::Account::normalize_balance
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename
Bug 21896: Add unit tests for Koha::Account::normalize_balance
Bug 21896: Add Koha::Account::normalize_balance
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour
Bug 21896: (QA follow-up) Add tests for FIFO behaviour
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour
[18.05.x] Bug 21896: Add unit tests for Koha::Account::normalize_balance
[18.05.x] Bug 21896: Add Koha::Account::normalize_balance
[18.05.x] Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename
[18.05.x] Bug 21896: (QA follow-up) Add tests for FIFO behaviour
[18.05.x] Bug 21896: (QA follow-up) Document and Test for FIFO behaviour
Bug 21896: RM follow-up - fix tests

Description Tomás Cohen Arazi 2018-11-27 22:55:40 UTC
We need a method that automatically applies outstanding credits to outstanding debits.
Comment 1 Tomás Cohen Arazi 2018-11-27 22:59:54 UTC
Created attachment 82684 [details] [review]
Bug 21896: Add unit tests for Koha::Account::normalize_balance
Comment 2 Tomás Cohen Arazi 2018-11-27 23:00:01 UTC
Created attachment 82685 [details] [review]
[SIGNED OFF]Bug 21896: Add Koha::Account::normalize_balance

This patch adds the normalize_balance() method to Koha::Account. Its
purpose is to apply outstanding credits (i.e. manual ones ore remaining
amounts like in the case of refunds) to outstanding debts.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 3 Christopher Brannon 2018-11-28 00:21:07 UTC
Created attachment 82688 [details] [review]
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance
Comment 4 Christopher Brannon 2018-11-28 00:22:37 UTC
Created attachment 82689 [details] [review]
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance
Comment 5 Christopher Brannon 2018-11-28 00:24:19 UTC
Created attachment 82690 [details] [review]
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance
Comment 6 Christopher Brannon 2018-11-28 00:54:48 UTC
Created attachment 82691 [details] [review]
[SIGNED OFF]Bug 21896: Add Koha::Account::normalize_balance

This patch adds the normalize_balance() method to Koha::Account. Its
purpose is to apply outstanding credits (i.e. manual ones ore remaining
amounts like in the case of refunds) to outstanding debts.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 7 Christopher Brannon 2018-11-28 00:57:33 UTC
Created attachment 82692 [details] [review]
Bug 21896: Add unit tests for Koha::Account::normalize_balance
Comment 8 Christopher Brannon 2018-11-28 01:00:39 UTC
Sorry, I am struggling with the sign off process still.  :/  I hope I didn't screw anything up.
Comment 9 Martin Renvoize 2018-11-28 08:07:07 UTC
First comment is that I think I prefer the term 'reconcile' to the term 'normalize'.. 

To me reconcile means to apply one action against each other to tend towards a 'happy place', i.e. apply credits against debits to work towards balance zero, in the accounting sense... whereas normalize means to 'make the same for comparison' so you can continue with calculations, so the accounting example here would be to convert all accounts in various currencies into one 'normalized' currency before doing calculations upon them.

Hope that makes sense.. maybe it's just my scientific background speaking out.

Second comment is a question.. do we need to do anything logical in terms of the order of application of credits against debits.. i.e. should it be oldest debts first, smallest debts first, blocking debts first for example?
Comment 10 Tomás Cohen Arazi 2018-11-28 14:27:22 UTC
Created attachment 82701 [details] [review]
Bug 21896: Add unit tests for Koha::Account::normalize_balance

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 11 Tomás Cohen Arazi 2018-11-28 14:27:31 UTC
Created attachment 82702 [details] [review]
Bug 21896: Add Koha::Account::normalize_balance

This patch adds the normalize_balance() method to Koha::Account. Its
purpose is to apply outstanding credits (i.e. manual ones ore remaining
amounts like in the case of refunds) to outstanding debts.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 12 Tomás Cohen Arazi 2018-11-28 14:27:38 UTC
Created attachment 82703 [details] [review]
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename

This was requested on the QA review and I agree.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 M. Tompsett 2018-11-29 04:28:15 UTC
Created attachment 82720 [details] [review]
Bug 21896: Add unit tests for Koha::Account::normalize_balance

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 14 M. Tompsett 2018-11-29 04:28:19 UTC
Created attachment 82721 [details] [review]
Bug 21896: Add Koha::Account::normalize_balance

This patch adds the normalize_balance() method to Koha::Account. Its
purpose is to apply outstanding credits (i.e. manual ones ore remaining
amounts like in the case of refunds) to outstanding debts.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 15 M. Tompsett 2018-11-29 04:28:23 UTC
Created attachment 82722 [details] [review]
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename

This was requested on the QA review and I agree.

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

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 Martin Renvoize 2018-11-29 13:51:22 UTC
Created attachment 82735 [details] [review]
Bug 21896: Add unit tests for Koha::Account::normalize_balance

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2018-11-29 13:51:26 UTC
Created attachment 82736 [details] [review]
Bug 21896: Add Koha::Account::normalize_balance

This patch adds the normalize_balance() method to Koha::Account. Its
purpose is to apply outstanding credits (i.e. manual ones ore remaining
amounts like in the case of refunds) to outstanding debts.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2018-11-29 13:51:30 UTC
Created attachment 82737 [details] [review]
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename

This was requested on the QA review and I agree.

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

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2018-11-29 13:51:33 UTC
Created attachment 82738 [details] [review]
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour

A short dicussion lead to the decision to make it explictly clear that
this method will implicitly apply credits against debits in a 'First In
First Out' manor, meaning oldest outstanding debits will be paid off
first.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2018-11-29 13:55:13 UTC
I've added a followup to document the FIFO behaviour of this function and a supplemental test to guarantee the current implicit behaviour.

Otherwise, I like this now.. Passing QA
Comment 21 Tomás Cohen Arazi 2018-11-29 14:04:52 UTC
Created attachment 82739 [details] [review]
Bug 21896: (QA follow-up) Add tests for FIFO behaviour

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2018-11-29 14:04:57 UTC
Created attachment 82740 [details] [review]
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour

A short dicussion lead to the decision to make it explictly clear that
this method will implicitly apply credits against debits in a 'First In
First Out' manor, meaning oldest outstanding debits will be paid off
first.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Martin Renvoize 2018-11-29 14:18:29 UTC
Created attachment 82742 [details] [review]
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour

A short dicussion lead to the decision to make it explictly clear that
this method will implicitly apply credits against debits in a 'First In
First Out' manor, meaning oldest outstanding debits will be paid off
first.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Nick Clemens 2018-11-29 19:15:27 UTC
Awesome work everyone!

Pushed to master for 19.05
Comment 25 Tomás Cohen Arazi 2018-11-30 20:11:38 UTC
Created attachment 82811 [details] [review]
[18.05.x] Bug 21896: Add unit tests for Koha::Account::normalize_balance

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Tomás Cohen Arazi 2018-11-30 20:11:42 UTC
Created attachment 82812 [details] [review]
[18.05.x] Bug 21896: Add Koha::Account::normalize_balance

This patch adds the normalize_balance() method to Koha::Account. Its
purpose is to apply outstanding credits (i.e. manual ones ore remaining
amounts like in the case of refunds) to outstanding debts.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Account.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Tomás Cohen Arazi 2018-11-30 20:11:46 UTC
Created attachment 82813 [details] [review]
[18.05.x] Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename

This was requested on the QA review and I agree.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Tomás Cohen Arazi 2018-11-30 20:11:51 UTC
Created attachment 82814 [details] [review]
[18.05.x] Bug 21896: (QA follow-up) Add tests for FIFO behaviour

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2018-11-30 20:11:55 UTC
Created attachment 82815 [details] [review]
[18.05.x] Bug 21896: (QA follow-up) Document and Test for FIFO behaviour

A short dicussion lead to the decision to make it explictly clear that
this method will implicitly apply credits against debits in a 'First In
First Out' manor, meaning oldest outstanding debits will be paid off
first.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Nick Clemens 2018-11-30 21:43:22 UTC
Created attachment 82820 [details] [review]
Bug 21896: RM follow-up - fix tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Martin Renvoize 2018-12-10 09:57:34 UTC
Pushed to 18.11.x for 18.11.01.

This is early enough in the release to be pushed even though it's technically an enhancement and as it is required as part of the fix for the critical bug 18805 I believe the justification is sound.
Comment 32 Jesse Maseto 2018-12-18 18:20:19 UTC
Will not be backported to 18.05
Comment 33 Katrin Fischer 2019-01-30 17:43:16 UTC
*** Bug 22233 has been marked as a duplicate of this bug. ***
Comment 34 Katrin Fischer 2019-01-30 17:44:36 UTC
We had a question on the mailing list and another in a bug report about this one for 18.05. Not sure how hard backporting would bea (lots of missing dependencies)?
Comment 35 Tomás Cohen Arazi 2019-01-30 18:05:49 UTC
(In reply to Katrin Fischer from comment #34)
> We had a question on the mailing list and another in a bug report about this
> one for 18.05. Not sure how hard backporting would bea (lots of missing
> dependencies)?

I don't think it is that bad, and we could have a 18.05 version of this if required.

It was the RMaint call not to backport it. Maybe the new RMaints change their mindS?