|
Description
Martin Renvoize
2019-03-20 11:48:57 UTC
Created attachment 86782 [details] [review] Bug 22547: Add tests for C4::Overdues->UpdateFine Add tests for UpdateFine, in particular testing the functionality of MaxFine. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Question here.. should I write the tests to test all the current behaviour or write them to test what I believe the behaviour should be.. Example questions raised: 1) Should we really be counting 'Sundry'/'Manual Invoice' in the total fines amount outstanding? 2) If the above is correct, should we be counting other fee's in the total fines amount outstanding calculation? 3) We don't explicitly order the accountlines lookup, as such we can't be sure we're actually updating the correct fine for the renewals case... Should I test for this. That final one.. actually I don't think we record distinct fines after renewal any more.. this was an inadvertent change of functionality at some point I believe... maybe I'm reading the code wrong. Created attachment 86790 [details] [review] Bug 22547: Add tests for C4::Overdues->UpdateFine Add tests for UpdateFine, in particular testing the functionality of MaxFine. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Test Plan: * Read the test and confirm it all makes sense.. run the test and confirm it passes. Created attachment 87051 [details] [review] Bug 22547: Add tests for C4::Overdues->UpdateFine Add tests for UpdateFine, in particular testing the functionality of MaxFine. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> QA: Looking here Martin,
I am having some doubts about doing these kind of things in the test:
$fine->amountoutstanding('50')->store;
This obviously is a shortcut for doing the payment via 'nice' Koha calls, but I think it would be better to use the regular calls in order to find integration bugs and not give a false sense of security.
I tested a bit with something like:
t::lib::Mocks::mock_userenv({ branchcode => $patron->branchcode });
Koha::Account->new({ patron_id => $patron->borrowernumber })->pay({
amount => 50, lines => [ $fine ],
});
But it needs more attention..
This is no absolute blocker btw. It is great to have more tests here. But if you could improve now, please do.
Hmm, I've been debating this a bit today. I think I was really aiming for a strict Unit Test here and as such wanted minimal reliance on thinks like the pay functions. I do agree, we really want to also have a level of integration tests but I'm not sure it's appropriate in this case. I'll ponder how/where to add such an integration tests too. Setting back to SO as I believe these tests are OK as is at the moment.. perhaps we could do further improvements and more formal integration tests down the line. Created attachment 87643 [details] [review] Bug 22547: Add tests for C4::Overdues->UpdateFine Add tests for UpdateFine, in particular testing the functionality of MaxFine. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 87644 [details] [review] Bug 22547: (QA follow-up) Warn in debug mode Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88072 [details] [review] Bug 22547: Add tests for C4::Overdues->UpdateFine Add tests for UpdateFine, in particular testing the functionality of MaxFine. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 88073 [details] [review] Bug 22547: (QA follow-up) Warn in debug mode Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Awesome work all! Pushed to master for 19.05 Enhancement will not be backported to 18.11.x series. |