Bug 19938 - C4::Overdues::checkoverdues has inconsistent return type
Summary: C4::Overdues::checkoverdues has inconsistent return type
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-09 15:26 UTC by Mark Tompsett
Modified: 2024-01-26 09:04 UTC (History)
7 users (show)

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


Attachments
Bug 19938: Refactor checkoverdues (4.17 KB, patch)
2018-01-09 15:49 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19938: Split C4/Members.pm to avoid conflict issues (940 bytes, patch)
2018-01-09 18:17 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19938: Refactor checkoverdues (3.52 KB, patch)
2018-01-09 18:17 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19938: Split C4/Members.pm to avoid conflict issues (992 bytes, patch)
2018-01-09 20:58 UTC, Lee Jamison
Details | Diff | Splinter Review
Bug 19938: Refactor checkoverdues (3.57 KB, patch)
2018-01-09 20:58 UTC, Lee Jamison
Details | Diff | Splinter Review
Bug 19938: Refactor checkoverdues call in C4/Members.pm (1.24 KB, patch)
2018-01-12 16:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19938: Refactor checkoverdues (3.57 KB, patch)
2018-01-12 16:16 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19938: Refactor checkoverdues call in C4/Members.pm (1.30 KB, patch)
2018-01-15 08:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19938: Refactor checkoverdues (3.65 KB, patch)
2018-01-15 08:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19938: Do not fetch dashboard's info if no logged in (2.55 KB, patch)
2018-01-15 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19938: Bad return value triggers noise (1.55 KB, patch)
2018-01-16 19:35 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2018-01-09 15:26:15 UTC
While retesting a bug, I ran a prove -r -v t, which generated a lot of log noise. One such entry was:
[Tue Jan  9 03:54:58 2018] starman: Use of uninitialized value $overdues_count in numeric gt (>) at /home/vagrant/kohaclone/opac/opac-main.pl line 76.

The $overdues_count was set by a call to checkoverdues. Reading the code I noticed someone actually commented:
# returning the count and the results is silly

So, this is a refactor of the function and all the calls. It was surprisingly small.
Comment 1 Mark Tompsett 2018-01-09 15:49:44 UTC
Created attachment 70375 [details] [review]
Bug 19938: Refactor checkoverdues

TEST PLAN
---------
0) back up plack error log if you care.

1) Run these on your kohadevbox:
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    However, the log file will have noise.

2) Run these on your kohadevbox:
    git bz apply 19938
    restart_all
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    This time the log file will have no noise.

3) run koha qa test tools
Comment 2 Mark Tompsett 2018-01-09 18:16:58 UTC
This may conflict with 19933 which removes C4::Members::patronflags if 19933 is pushed first. In which case, I believe removing C4/Member's from this patch should be sufficient. I'll split.
Comment 3 Mark Tompsett 2018-01-09 18:17:51 UTC
Created attachment 70382 [details] [review]
Bug 19938: Split C4/Members.pm to avoid conflict issues
Comment 4 Mark Tompsett 2018-01-09 18:17:54 UTC
Created attachment 70383 [details] [review]
Bug 19938: Refactor checkoverdues

TEST PLAN
---------
0) back up plack error log if you care.

1) Run these on your kohadevbox:
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    However, the log file will have noise.

2) Run these on your kohadevbox:
    git bz apply 19938
    restart_all
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    This time the log file will have no noise.

3) run koha qa test tools
Comment 5 Lee Jamison 2018-01-09 20:58:45 UTC
Created attachment 70396 [details] [review]
Bug 19938: Split C4/Members.pm to avoid conflict issues

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 6 Lee Jamison 2018-01-09 20:58:55 UTC
Created attachment 70397 [details] [review]
Bug 19938: Refactor checkoverdues

TEST PLAN
---------
0) back up plack error log if you care.

1) Run these on your kohadevbox:
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    However, the log file will have noise.

2) Run these on your kohadevbox:
    git bz apply 19938
    restart_all
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    This time the log file will have no noise.

3) run koha qa test tools

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 7 Jonathan Druart 2018-01-10 11:54:17 UTC
(In reply to M. Tompsett from comment #0)
> While retesting a bug, I ran a prove -r -v t, which generated a lot of log
> noise. One such entry was:
> [Tue Jan  9 03:54:58 2018] starman: Use of uninitialized value
> $overdues_count in numeric gt (>) at
> /home/vagrant/kohaclone/opac/opac-main.pl line 76.

Prove generated a log entry from opac-main.pl? How?

https://jenkins.koha-community.org/job/Koha_Master_D8/lastCompletedBuild/console
Comment 8 Jonathan Druart 2018-01-10 11:55:16 UTC
"Bug 19938: Split C4/Members.pm to avoid conflict issues"

 => What does it mean?
Comment 9 Mark Tompsett 2018-01-10 13:54:33 UTC
(In reply to Jonathan Druart from comment #8)
> "Bug 19938: Split C4/Members.pm to avoid conflict issues"
> 
>  => What does it mean?

because 19933 might trigger a conflict in C4/Members with the removal of patronflags, so put the possible conflict area elsewhere. I split it from a single commit, so as to avoid the conflict as needed.
Comment 10 Julian Maurice 2018-01-12 11:25:31 UTC
(In reply to M. Tompsett from comment #9)
> (In reply to Jonathan Druart from comment #8)
> > "Bug 19938: Split C4/Members.pm to avoid conflict issues"
> > 
> >  => What does it mean?
> 
> because 19933 might trigger a conflict in C4/Members with the removal of
> patronflags, so put the possible conflict area elsewhere. I split it from a
> single commit, so as to avoid the conflict as needed.

I don't see how they can conflict. Bug 19933 only adds a comment to C4/Members.pm, and not very close to the lines modified by this bug's patches.
I think it's safe to squash the patches.
Comment 11 Julian Maurice 2018-01-12 12:07:55 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to M. Tompsett from comment #0)
> > While retesting a bug, I ran a prove -r -v t, which generated a lot of log
> > noise. One such entry was:
> > [Tue Jan  9 03:54:58 2018] starman: Use of uninitialized value
> > $overdues_count in numeric gt (>) at
> > /home/vagrant/kohaclone/opac/opac-main.pl line 76.
> 
> Prove generated a log entry from opac-main.pl? How?
> 
> https://jenkins.koha-community.org/job/Koha_Master_D8/lastCompletedBuild/
> console

t/db_dependent/www/search_utf8.t use Test::WWW::Mechanize to do various HTTP requests to Koha, that's how.

I can confirm that warnings disappear after applying the patches, but before I mark this as Passed QA, please rewrite the commit message. It's not really a refactoring because it changes behaviour.
Comment 12 Julian Maurice 2018-01-12 12:11:42 UTC
(In reply to Julian Maurice from comment #10)
> I don't see how they can conflict. Bug 19933 only adds a comment to
> C4/Members.pm, and not very close to the lines modified by this bug's
> patches.
> I think it's safe to squash the patches.

I didn't realize the code modified by this patch was inside patronflags(), so feel free to squash the patches or not, but if you decide to keep both patches separated, please rewrite the first commit message too, it doesn't describe what the patch does
Comment 13 Mark Tompsett 2018-01-12 16:15:58 UTC
Created attachment 70472 [details] [review]
Bug 19938: Refactor checkoverdues call in C4/Members.pm

Due to concerns that another bug may conflict, this was
purposefully split. The other bug removes patronflags().
It is this function that has the C4::Overdues::checkoverdues
call refactored.

TEST PLAN
---------
Apply both patches and follow the test plan in the larger one.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 14 Mark Tompsett 2018-01-12 16:16:02 UTC
Created attachment 70473 [details] [review]
Bug 19938: Refactor checkoverdues

TEST PLAN
---------
0) back up plack error log if you care.

1) Run these on your kohadevbox:
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    However, the log file will have noise.

2) Run these on your kohadevbox:
    git bz apply 19938
    restart_all
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    This time the log file will have no noise.

3) run koha qa test tools

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Comment 15 Mark Tompsett 2018-01-12 16:20:39 UTC
It seemed from comment #11 and comment #12, that the FailedQA was for the git commit messages, not the actual code.

Changing the return value and tweaking code based on that is a refactor, just not in a huge rewrite from scratch sense. Perhaps there is a better word, but I can't think of it right now.

As I only changed the commit message, I'm moving this back to Signed off.
Comment 16 Julian Maurice 2018-01-15 08:49:38 UTC
(In reply to M. Tompsett from comment #15)
> It seemed from comment #11 and comment #12, that the FailedQA was for the
> git commit messages, not the actual code.
> 
> Changing the return value and tweaking code based on that is a refactor,
> just not in a huge rewrite from scratch sense. Perhaps there is a better
> word, but I can't think of it right now.
According to Wikipedia,
> Code refactoring is the process of restructuring existing computer code
> without changing its external behavior.
IMO the return value is part of the external behavior of a subroutine, so it's not a refactor.

But I won't block this any longer for such a minor issue.
Comment 17 Julian Maurice 2018-01-15 08:51:23 UTC
Created attachment 70485 [details] [review]
Bug 19938: Refactor checkoverdues call in C4/Members.pm

Due to concerns that another bug may conflict, this was
purposefully split. The other bug removes patronflags().
It is this function that has the C4::Overdues::checkoverdues
call refactored.

TEST PLAN
---------
Apply both patches and follow the test plan in the larger one.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 18 Julian Maurice 2018-01-15 08:51:28 UTC
Created attachment 70486 [details] [review]
Bug 19938: Refactor checkoverdues

TEST PLAN
---------
0) back up plack error log if you care.

1) Run these on your kohadevbox:
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    However, the log file will have noise.

2) Run these on your kohadevbox:
    git bz apply 19938
    restart_all
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    This time the log file will have no noise.

3) run koha qa test tools

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 19 Jonathan Druart 2018-01-15 15:02:32 UTC
Created attachment 70489 [details] [review]
Bug 19938: Do not fetch dashboard's info if no logged in
Comment 20 Jonathan Druart 2018-01-15 15:02:58 UTC
So actually we are trying to remove a warning because there are wrong calls in opac-main.
The dashboard's info should be fetched only if a patron is logged in.
I did this change on bug 12001 already.
I am suggesting a new patch to avoid the change of the subroutine's prototype (which is wrong instead, but let remove the subroutine instead) and future conflict (with bug 19933).

Deal?
Comment 21 Mark Tompsett 2018-01-16 00:46:41 UTC
(In reply to Jonathan Druart from comment #20)
> So actually we are trying to remove a warning because there are wrong calls
> in opac-main.
> The dashboard's info should be fetched only if a patron is logged in.
> I did this change on bug 12001 already.
> I am suggesting a new patch to avoid the change of the subroutine's
> prototype (which is wrong instead, but let remove the subroutine instead)
> and future conflict (with bug 19933).
> 
> Deal?

While the problem that triggered this code change is that, and your simpler change would suffice, I was hoping for a more thorough fix of the checkoverdues function. Granted, this is only an intermediary step towards one that deals with Koha::Checkouts filtered on is_overdue.

If you prefer, I could attempt to Koha::Checkouts the checkoverdues accessing areas of code.
Comment 22 Mark Tompsett 2018-01-16 05:01:00 UTC
> (In reply to Jonathan Druart from comment #20)
> > The dashboard's info should be fetched only if a patron is logged in.

Re-read checkoverdue code...
The problem case is "or return".
Tweak to return (0,{}); perhaps?
Comment 23 Mark Tompsett 2018-01-16 19:35:43 UTC
Created attachment 70567 [details] [review]
Bug 19938: Bad return value triggers noise

TEST PLAN
---------
0) back up plack error log if you care.

1) Run these on your kohadevbox:
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    However, the log file will have noise.

2) Run these on your kohadevbox:
    git bz apply 19938
    restart_all
    echo | sudo tee /var/log/koha/kohadev/plack-error.log
    sudo koha-shell -c bash kohadev
    prove t/db_dependent/Circulation/dateexpiry.t
    prove t/db_dependent/www/search_utf8.t
    ./misc/cronjobs/staticfines.pl
    exit

    They will all pass or run without error.
    This time the log file will have no noise.

3) run koha qa test tools
Comment 24 Mark Tompsett 2018-01-16 19:39:45 UTC
Comment on attachment 70489 [details] [review]
Bug 19938: Do not fetch dashboard's info if no logged in

Fixing the return type makes this check not necessary.
Comment 25 Mark Tompsett 2018-01-16 19:58:05 UTC
Comment on attachment 70489 [details] [review]
Bug 19938: Do not fetch dashboard's info if no logged in

Sorry. Let's include this optimization which will reduce output times.
Comment 26 Mark Tompsett 2018-01-16 20:02:23 UTC
Both can be applied together or independently. Both solve the warning issue. However, JD's has the added advantage of also trimming unnecessary calls, meaning fast page output time. Mine is short and pretty, bringing consistency to the checkoverdues return value, but does nothing to trim times. Test plan is comment #23.
Comment 27 Jonathan Druart 2018-01-23 12:31:14 UTC
Comment on attachment 70489 [details] [review]
Bug 19938: Do not fetch dashboard's info if no logged in

Finally, see bug 20068...
Comment 28 Katrin Fischer 2018-07-08 16:36:17 UTC
Sorry, don't see the noise. Can you pleas recheck and amend test plan?
Comment 29 Hayley Pelham 2019-03-08 00:52:26 UTC
t/db_dependent/www/search_utf8.t failed -

t/db_dependent/www/search_utf8.t .. Warning: <retrieval> element has non-unique value in 'name' key attribute: F at t/db_dependent/www/search_utf8.t line 43.
Warning: <retrieval> element has non-unique value in 'name' key attribute: B at t/db_dependent/www/search_utf8.t line 43.
t/db_dependent/www/search_utf8.t .. 1/99 Link not found at t/db_dependent/www/search_utf8.t line 145.
# Looks like your test exited with 255 just after 3.
t/db_dependent/www/search_utf8.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 96/99 subtests 

Test Summary Report
-------------------
t/db_dependent/www/search_utf8.t (Wstat: 65280 Tests: 3 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 99 tests but ran 3.
Files=1, Tests=3,  4 wallclock secs ( 0.01 usr  0.00 sys +  0.83 cusr  0.10 csys =  0.94 CPU)
Result: FAIL

As this test passing is part of the outlined test plan, I have marked as Failed QA.
Comment 30 Mark Tompsett 2019-07-12 01:36:54 UTC
This is called in C4::Members::patronflags
-- this function has been deprecated, and is only used in C4::SIP::ILS::Patron.
And while I would think it should be:
    my $flags     = C4::Members::patronflags( $kp->{borrowernumber} );
Fixing that is not in this scope. And because borrowernumber is magically determined, checkoverdues doesn't just return, which is the problem this bug addresses.

In fact, looking at the other few places that call checkoverdues, it is clear that it should always be called with a borrower number. So while is a valid fix, there is no way to test it short of adding a test. That, a new test, will be the next attachment. :)