Bug 21681

Summary: Remove C4::Accounts::getcharges
Product: Koha Reporter: Josef Moravec <josef.moravec>
Component: Architecture, internals, and plumbingAssignee: Josef Moravec <josef.moravec>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21804
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21694    
Bug Blocks: 21683, 21696    
Attachments: Bug 21681: Remove C4::Accounts::getcharges
Bug 21681: Remove C4::Accounts::getcharges
Bug 21681: (follow-up) Don't use Koha::Account::Line directly
Bug 21681: (follow-up) Add tests for Koha::Account->lines
Bug 21681: Remove C4::Accounts::getcharges
Bug 21681: Add tests for GetPatronInfo
Bug 21681: Improve the account lines fetch
Bug 21681: Remove C4::Accounts::getcharges
Bug 21681: Add tests for GetPatronInfo
Bug 21681: Improve the account lines fetch

Description Josef Moravec 2018-10-26 10:04:20 UTC
C4::Accounts::getcharges is only used on one place and could be easily rewritten using objects
Comment 1 Josef Moravec 2018-10-26 10:11:34 UTC
Created attachment 81269 [details] [review]
Bug 21681: Remove C4::Accounts::getcharges

Test plan:
0) Do not apply the patch
1) Enable ILS-DI
2) Go to
<opac_url>/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_fines=1
use patron with some paid and unpaid fines
3) Save the page for later usage
4) Apply the patch
5) Repeat 2 with same patron and compare the saved result with the new
one, they should be same
6) git grep getcharges should return no occurance
7) prove t/db_dependent/Accounts.t
Comment 2 Martin Renvoize 2018-10-26 14:42:50 UTC
Created attachment 81298 [details] [review]
Bug 21681: Remove C4::Accounts::getcharges

Test plan:
0) Do not apply the patch
1) Enable ILS-DI
2) Go to
<opac_url>/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_fines=1
use patron with some paid and unpaid fines
3) Save the page for later usage
4) Apply the patch
5) Repeat 2 with same patron and compare the saved result with the new
one, they should be same
6) git grep getcharges should return no occurance
7) prove t/db_dependent/Accounts.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2018-10-26 14:44:01 UTC
Simples..
Comment 4 Martin Renvoize 2018-10-26 15:07:17 UTC
With my QA hat on.. I'm less happy with this.

1) We should not use Koha::Account::Lines directly.. the intention is to use Koha::Account as an intermediary for dealing with all account actions.
2) If we do the above, there is a method directly from the Koha::Patron object you have in ILSDI already ($patron->account->*)
3) I don't really believe ILSDI should return ALL accountlines associated to a patron.  In reality, the intention is for outstanding debits (and possibly outstanding credits) to be usable.  However, that would be a change of the api, and so is probably out of scope.  As such, we could add a 'lines' method to Koha::Account which would return all accountlines filtered by the user.
Comment 5 Martin Renvoize 2018-10-26 15:11:58 UTC
I'm working on the followup myself
Comment 6 Martin Renvoize 2018-10-26 15:24:10 UTC
Created attachment 81306 [details] [review]
Bug 21681: (follow-up) Don't use Koha::Account::Line directly
Comment 7 Josef Moravec 2018-10-26 15:41:22 UTC
(In reply to Martin Renvoize from comment #5)
> I'm working on the followup myself

Oh, you were really fast!

Thanks!

I agreee with you, that it does not make much sense to return all accountlines in getpatroninfo method of ilsdi, but it is definetaly out of scope of this report.
Comment 8 Martin Renvoize 2018-10-26 15:48:27 UTC
I just need to add a test for Koha::Account->lines now then i'll get Tomas to do a final QA on it ;)

Be really nice to clean up this area.
Comment 9 Martin Renvoize 2018-10-26 17:35:37 UTC
Created attachment 81322 [details] [review]
Bug 21681: (follow-up) Add tests for Koha::Account->lines
Comment 10 Jonathan Druart 2018-10-26 18:03:18 UTC
I will move the 2 last patches to their own bug report, see bug 21694
Comment 11 Jonathan Druart 2018-10-26 18:27:42 UTC
Created attachment 81325 [details] [review]
Bug 21681: Remove C4::Accounts::getcharges

Test plan:
0) Do not apply the patch
1) Enable ILS-DI
2) Go to
<opac_url>/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_fines=1
use patron with some paid and unpaid fines
3) Save the page for later usage
4) Apply the patch
5) Repeat 2 with same patron and compare the saved result with the new
one, they should be same
6) git grep getcharges should return no occurance
7) prove t/db_dependent/Accounts.t

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2018-10-26 18:27:48 UTC
Created attachment 81326 [details] [review]
Bug 21681: Add tests for GetPatronInfo

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2018-10-26 18:27:53 UTC
Created attachment 81327 [details] [review]
Bug 21681: Improve the account lines fetch

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2018-10-26 18:35:01 UTC
I have squashed the first 2 patches (wrong manip), sorry Martin!
Comment 15 Martin Renvoize 2018-10-26 18:36:48 UTC
Loving the code golf here.. nice teamwork!
Comment 16 Martin Renvoize 2018-10-26 18:44:24 UTC
Yup, tested and all works as expected still, code looks great after that golfing session and Jonthans added ILSDI tests too.. can't ask for more.. lets PQA it as there's now three QA team eyes here ;)
Comment 17 Martin Renvoize 2018-10-26 18:48:23 UTC
Created attachment 81329 [details] [review]
Bug 21681: Remove C4::Accounts::getcharges

Test plan:
0) Do not apply the patch
1) Enable ILS-DI
2) Go to
<opac_url>/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=51&show_fines=1
use patron with some paid and unpaid fines
3) Save the page for later usage
4) Apply the patch
5) Repeat 2 with same patron and compare the saved result with the new
one, they should be same
6) git grep getcharges should return no occurance
7) prove t/db_dependent/Accounts.t

Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2018-10-26 18:48:27 UTC
Created attachment 81330 [details] [review]
Bug 21681: Add tests for GetPatronInfo

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2018-10-26 18:48:30 UTC
Created attachment 81331 [details] [review]
Bug 21681: Improve the account lines fetch

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Nick Clemens 2018-10-31 15:04:57 UTC
Awesome work all!

Pushed to master for 18.11
Comment 21 Martin Renvoize 2018-11-09 16:11:14 UTC
Architectural enhancement, will not be backported to 18.05.x series.