Created attachment 82718 [details] [review] Bug 15422: Correct calculation of holds ratio report See comment 1 for a detailed explanation of current calculations and needed calculations Also removes an unnecessary variable To test: 1 - Place 4 holds on a biblio with one item 2 - go to /cgi-bin/koha/circ/reserveratios.pl (Circulation->Holds ratios) 3 - Run with 'Hold ratio'=3, it says order 1, ok 4 - Run with HR=4, it says order 1, wrong 5 - Run with HR=2, it syas order 2, wrong 6 - Run with HR=.5, it syas order 4, wrong 7 - Apply patch 8 - Run with HR=3, order 1, OK 9 - Run with HR=4, item does not appear (0 to order), OK 10 - Run with HR=2, order 1, OK 11 - Run with HR=.5, order 7, OK > 8 - Run with HR=3, order 1, OK
I get "No items found" in this case. Is that a bug?
Created attachment 82842 [details] [review] Bug 15422: Consider partial copies needed as whole numbers Created attachment 82873 [details] [review] Bug 15422: Correct calculation of holds ratio report See comment 1 for a detailed explanation of current calculations and needed calculations Also removes an unnecessary variable To test: 1 - Place 4 holds on a biblio with one item 2 - go to /cgi-bin/koha/circ/reserveratios.pl (Circulation->Holds ratios) 3 - Run with 'Hold ratio'=3, it says order 1, ok 4 - Run with HR=4, it says order 1, wrong 5 - Run with HR=2, it syas order 2, wrong 6 - Run with HR=.5, it syas order 4, wrong 7 - Apply patch 8 - Run with HR=3, order 1, OK 9 - Run with HR=4, item does not appear (0 to order), OK 10 - Run with HR=2, order 1, OK 11 - Run with HR=.5, order 7, OK Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 82874 [details] [review] Bug 15422: Consider partial copies needed as whole numbers Signed-off-by: Owen Leonard <oleonard@myacpl.org> I would say that this is an excellent candidate for tests. We should move the existing code, provide tests to highlight that the code is wrong, then fix it! Please? I am with Jonathan on this one... it kind of screems for tests as they are practically written out in the bug and on the commit message. It will also help us to confirm in the future that this is the desired behaviour. I definitely agree. we should move this to a module and test it, however, this code has problems: 1 - It gets data that is not grouped so breaks under strict mode 2 - All our tests enforce strict mode 3 - It should be refactored or reworked to use DBIC and objects instead of straight SQL The fix here is relatively minor, the work you are asking for is more intense and will involve likely changing the report and its output and should be another report. Let's fix providing the wrong data (In reply to Katrin Fischer from comment #7) > I am with Jonathan on this one... it kind of screems for tests as they are > practically written out in the bug and on the commit message. It will also > help us to confirm in the future that this is the desired behaviour. Filed bug 23360, testing this code is problematic because of strict mode. This bug has been known for 4 years, lets patch it now Hi Nick, I agree on fixing the bug first - can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 15422: Correct calculation of holds ratio report Using index info to reconstruct a base tree... M circ/reserveratios.pl Falling back to patching base and 3-way merge... Auto-merging circ/reserveratios.pl CONFLICT (content): Merge conflict in circ/reserveratios.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 15422: Correct calculation of holds ratio report Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-15422-Correct-calculation-of-holds-ratio-repor-0dn4_I.patch Created attachment 92145 [details] [review] Bug 15422: Correct calculation of holds ratio report See comment 1 for a detailed explanation of current calculations and needed calculations Also removes an unnecessary variable To test: 1 - Place 4 holds on a biblio with one item 2 - go to /cgi-bin/koha/circ/reserveratios.pl (Circulation->Holds ratios) 3 - Run with 'Hold ratio'=3, it says order 1, ok 4 - Run with HR=4, it says order 1, wrong 5 - Run with HR=2, it syas order 2, wrong 6 - Run with HR=.5, it syas order 4, wrong 7 - Apply patch 8 - Run with HR=3, order 1, OK 9 - Run with HR=4, item does not appear (0 to order), OK 10 - Run with HR=2, order 1, OK 11 - Run with HR=.5, order 7, OK Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 92146 [details] [review] Bug 15422: Consider partial copies needed as whole numbers Signed-off-by: Owen Leonard <oleonard@myacpl.org> Note: Needs strict mode turned off, the hold ratio report is broken with strict in general. Created attachment 93670 [details] [review] Bug 15422: Correct calculation of holds ratio report See comment 1 for a detailed explanation of current calculations and needed calculations Also removes an unnecessary variable To test: 1 - Place 4 holds on a biblio with one item 2 - go to /cgi-bin/koha/circ/reserveratios.pl (Circulation->Holds ratios) 3 - Run with 'Hold ratio'=3, it says order 1, ok 4 - Run with HR=4, it says order 1, wrong 5 - Run with HR=2, it syas order 2, wrong 6 - Run with HR=.5, it syas order 4, wrong 7 - Apply patch 8 - Run with HR=3, order 1, OK 9 - Run with HR=4, item does not appear (0 to order), OK 10 - Run with HR=2, order 1, OK 11 - Run with HR=.5, order 7, OK Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 93671 [details] [review] Bug 15422: Consider partial copies needed as whole numbers Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Nice work! Pushed to master for 19.11.00 |
When running the holds ratio report ( circ/reserveratios.pl ), I have the hold ratio set to 3 (this is the threshold ratio, set in the Refine results box on the right). This is from the first row on the page: Holds : 40 Items : 3 Hold Ratio : 13.33 ... Items needed: 4 to order If we add the recommended 4 items, we have 40 holds and 7 items, which brings the hold ratio to 5.7, which is still above the threshold ratio of 3. In order to bring the actual hold ratio below the threshold ratio of 3, I would need 40 / ( 3 + x ) = 3 40 = 9 + 3x 31 = 3x 10.3 = x Or, more generally $data->{reservecount} / ( $data->{itemcount} + $needed ) = $ratio $data->{reservecount} / $ratio = $data->{itemcount} + $needed $needed = ( $data->{reservecount} / $ratio ) - $data->{itemcount} Instead at circ/reserveratios.pl line 158, we have ratiocalc => sprintf( "%.0d", $ratio_atleast1 ? ( $thisratio / $ratio ) : $thisratio ), which translates to $needed = ( $data->{reservecount} / $data->{itemcount} ) / $ratio This means that if the library orders the recommended number of copies, the title will show up again on the holds ratio report, assuming that the number of holds stays more or less constant.