Bug 22547 - C4::Overdues - UpdateFine is barely tested
Summary: C4::Overdues - UpdateFine is barely tested
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 22521
  Show dependency treegraph
 
Reported: 2019-03-20 11:48 UTC by Martin Renvoize
Modified: 2020-01-06 20:17 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00


Attachments
Bug 22547: Add tests for C4::Overdues->UpdateFine (4.29 KB, patch)
2019-03-20 11:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22547: Add tests for C4::Overdues->UpdateFine (7.98 KB, patch)
2019-03-20 13:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22547: Add tests for C4::Overdues->UpdateFine (8.04 KB, patch)
2019-03-27 10:02 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22547: Add tests for C4::Overdues->UpdateFine (8.09 KB, patch)
2019-04-09 16:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22547: (QA follow-up) Warn in debug mode (1.43 KB, patch)
2019-04-09 16:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22547: Add tests for C4::Overdues->UpdateFine (8.09 KB, patch)
2019-04-16 14:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22547: (QA follow-up) Warn in debug mode (1.43 KB, patch)
2019-04-16 14:03 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-03-20 11:48:57 UTC
UpdateFine is not tested at the unit level, it should be to help prevent regressions (and to allow more confidence in refactoring efforts).
Comment 1 Martin Renvoize 2019-03-20 11:51:04 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>
Comment 2 Martin Renvoize 2019-03-20 12:19:14 UTC
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.
Comment 3 Martin Renvoize 2019-03-20 12:21:48 UTC
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.
Comment 4 Martin Renvoize 2019-03-20 13:33:23 UTC
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>
Comment 5 Martin Renvoize 2019-03-20 13:34:04 UTC
Test Plan:
* Read the test and confirm it all makes sense.. run the test and confirm it passes.
Comment 6 Josef Moravec 2019-03-27 10:02:17 UTC
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>
Comment 7 Marcel de Rooy 2019-03-29 09:13:57 UTC
QA: Looking here
Comment 8 Marcel de Rooy 2019-03-29 10:13:47 UTC
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.
Comment 9 Martin Renvoize 2019-03-29 22:04:08 UTC
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.
Comment 10 Martin Renvoize 2019-04-09 16:21:19 UTC
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.
Comment 11 Tomás Cohen Arazi 2019-04-09 16:59:23 UTC
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>
Comment 12 Tomás Cohen Arazi 2019-04-09 16:59:27 UTC
Created attachment 87644 [details] [review]
Bug 22547: (QA follow-up) Warn in debug mode

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Martin Renvoize 2019-04-16 14:03:13 UTC
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>
Comment 14 Martin Renvoize 2019-04-16 14:03:16 UTC
Created attachment 88073 [details] [review]
Bug 22547: (QA follow-up) Warn in debug mode

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Nick Clemens 2019-04-17 17:29:48 UTC
Awesome work all!

Pushed to master for 19.05
Comment 16 Martin Renvoize 2019-04-26 13:24:49 UTC
Enhancement will not be backported to 18.11.x series.