Bug 15422

Summary: Number of items to order on holds ratio report will not fulfill the holds ratio
Product: Koha Reporter: Barton Chittenden <barton>
Component: ReportsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, kelly, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21941
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 23360, 25760    
Attachments: Bug 15422: Correct calculation of holds ratio report
Bug 15422: Consider partial copies needed as whole numbers
Bug 15422: Correct calculation of holds ratio report
Bug 15422: Consider partial copies needed as whole numbers
Bug 15422: Correct calculation of holds ratio report
Bug 15422: Consider partial copies needed as whole numbers
Bug 15422: Correct calculation of holds ratio report
Bug 15422: Consider partial copies needed as whole numbers

Description Barton Chittenden 2015-12-23 22:19:06 UTC
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.
Comment 1 Nick Clemens 2018-11-29 02:18:18 UTC
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
Comment 2 Owen Leonard 2018-12-04 13:50:49 UTC
>  8 - Run with HR=3, order 1, OK

I get "No items found" in this case. Is that a bug?
Comment 3 Nick Clemens 2018-12-04 14:59:09 UTC
Created attachment 82842 [details] [review]
Bug 15422: Consider partial copies needed as whole numbers
Comment 4 Owen Leonard 2018-12-05 15:19:21 UTC
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>
Comment 5 Owen Leonard 2018-12-05 15:19:24 UTC
Created attachment 82874 [details] [review]
Bug 15422: Consider partial copies needed as whole numbers

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Jonathan Druart 2018-12-13 20:27:34 UTC
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?
Comment 7 Katrin Fischer 2019-02-02 14:13:17 UTC
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.
Comment 8 Nick Clemens 2019-07-23 13:12:23 UTC
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
Comment 9 Nick Clemens 2019-07-23 13:15:24 UTC
(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
Comment 10 Katrin Fischer 2019-08-11 13:09:28 UTC
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
Comment 11 Nick Clemens 2019-08-11 19:52:55 UTC
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>
Comment 12 Nick Clemens 2019-08-11 19:53:00 UTC
Created attachment 92146 [details] [review]
Bug 15422: Consider partial copies needed as whole numbers

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Katrin Fischer 2019-10-04 07:40:56 UTC
Note: Needs strict mode turned off, the hold ratio report is broken with strict in general.
Comment 14 Katrin Fischer 2019-10-04 07:53:58 UTC
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>
Comment 15 Katrin Fischer 2019-10-04 07:54:04 UTC
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>
Comment 16 Martin Renvoize 2019-10-08 13:16:42 UTC
Nice work!

Pushed to master for 19.11.00