Description
Josef Moravec
2018-10-26 10:04:20 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 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> Simples.. 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. I'm working on the followup myself Created attachment 81306 [details] [review] Bug 21681: (follow-up) Don't use Koha::Account::Line directly (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. 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. Created attachment 81322 [details] [review] Bug 21681: (follow-up) Add tests for Koha::Account->lines I will move the 2 last patches to their own bug report, see bug 21694 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> Created attachment 81326 [details] [review] Bug 21681: Add tests for GetPatronInfo Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 81327 [details] [review] Bug 21681: Improve the account lines fetch Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> I have squashed the first 2 patches (wrong manip), sorry Martin! Loving the code golf here.. nice teamwork! 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 ;) 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> 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> 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> Awesome work all! Pushed to master for 18.11 Architectural enhancement, will not be backported to 18.05.x series. |