Bug 21896 - Add Koha::Account::reconcile_balance
Summary: Add Koha::Account::reconcile_balance
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: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
: 22233 (view as bug list)
Depends on: 20997
Blocks: 18805 21915
  Show dependency treegraph
 
Reported: 2018-11-27 22:55 UTC by Tomás Cohen Arazi
Modified: 2020-06-04 20:32 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Adds a business logic level routine for reconciling user account balances.
Version(s) released in:


Attachments
Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.43 KB, patch)
2018-11-27 22:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF]Bug 21896: Add Koha::Account::normalize_balance (1.50 KB, patch)
2018-11-27 23:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.43 KB, patch)
2018-11-28 00:21 UTC, Christopher Brannon
Details | Diff | Splinter Review
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.43 KB, patch)
2018-11-28 00:22 UTC, Christopher Brannon
Details | Diff | Splinter Review
[SIGNED OFF]Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.43 KB, patch)
2018-11-28 00:24 UTC, Christopher Brannon
Details | Diff | Splinter Review
[SIGNED OFF]Bug 21896: Add Koha::Account::normalize_balance (1.50 KB, patch)
2018-11-28 00:54 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.43 KB, patch)
2018-11-28 00:57 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.49 KB, patch)
2018-11-28 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21896: Add Koha::Account::normalize_balance (1.56 KB, patch)
2018-11-28 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename (2.80 KB, patch)
2018-11-28 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.55 KB, patch)
2018-11-29 04:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21896: Add Koha::Account::normalize_balance (1.61 KB, patch)
2018-11-29 04:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename (2.86 KB, patch)
2018-11-29 04:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.60 KB, patch)
2018-11-29 13:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21896: Add Koha::Account::normalize_balance (1.67 KB, patch)
2018-11-29 13:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename (2.92 KB, patch)
2018-11-29 13:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour (3.88 KB, patch)
2018-11-29 13:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21896: (QA follow-up) Add tests for FIFO behaviour (2.63 KB, patch)
2018-11-29 14:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour (2.55 KB, patch)
2018-11-29 14:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21896: (QA follow-up) Document and Test for FIFO behaviour (2.54 KB, patch)
2018-11-29 14:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
[18.05.x] Bug 21896: Add unit tests for Koha::Account::normalize_balance (6.62 KB, patch)
2018-11-30 20:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[18.05.x] Bug 21896: Add Koha::Account::normalize_balance (1.70 KB, patch)
2018-11-30 20:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[18.05.x] Bug 21896: (QA follow-up) normalize_balance -> reconcile_balance rename (2.95 KB, patch)
2018-11-30 20:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[18.05.x] Bug 21896: (QA follow-up) Add tests for FIFO behaviour (2.64 KB, patch)
2018-11-30 20:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[18.05.x] Bug 21896: (QA follow-up) Document and Test for FIFO behaviour (2.57 KB, patch)
2018-11-30 20:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21896: RM follow-up - fix tests (1.21 KB, patch)
2018-11-30 21:43 UTC, Nick Clemens
Details | Diff | Splinter Review

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