|
Description
Mark Tompsett
2018-01-09 15:26:15 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 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. Created attachment 70382 [details] [review] Bug 19938: Split C4/Members.pm to avoid conflict issues 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 Created attachment 70396 [details] [review] Bug 19938: Split C4/Members.pm to avoid conflict issues Signed-off-by: Lee Jamison <ldjamison@marywood.edu> 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> (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 "Bug 19938: Split C4/Members.pm to avoid conflict issues" => What does it mean? (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. (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. (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. (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 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> 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> 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. (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. 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> 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> Created attachment 70489 [details] [review] Bug 19938: Do not fetch dashboard's info if no logged in 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? (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. > (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?
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 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 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. 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 on attachment 70489 [details] [review] Bug 19938: Do not fetch dashboard's info if no logged in Finally, see bug 20068... Sorry, don't see the noise. Can you pleas recheck and amend test plan? 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. 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. :)
|