https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #1 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 70375 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70375&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor --- Comment #2 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70375|0 |1 is obsolete| | --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 70383 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70383&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70382|0 |1 is obsolete| | --- Comment #5 from Lee Jamison <ldjamison@marywood.edu> --- Created attachment 70396 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70396&action=edit Bug 19938: Split C4/Members.pm to avoid conflict issues Signed-off-by: Lee Jamison <ldjamison@marywood.edu> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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/con... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #10 from Julian Maurice <julian.maurice@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 70472 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70472&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70396|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #16 from Julian Maurice <julian.maurice@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70473|0 |1 is obsolete| | --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 70486 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70486&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 70489 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70489&action=edit Bug 19938: Do not fetch dashboard's info if no logged in -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 --- Comment #21 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70485|0 |1 is obsolete| | Attachment #70486|0 |1 is obsolete| | --- Comment #23 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 70567 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70567&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70489|0 |1 is obsolete| | --- Comment #24 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 70489 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70489 Bug 19938: Do not fetch dashboard's info if no logged in Fixing the return type makes this check not necessary. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #26 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |hayleymapley@catalyst.net.n | |z --- Comment #29 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19938 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|julian.maurice@biblibre.com |testopia@bugs.koha-communit | |y.org -- You are receiving this mail because: You are watching all bug changes.