Bug 13640 - Holds To Pull List includes items unreserveable items
Summary: Holds To Pull List includes items unreserveable items
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major with 10 votes (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 22431 23175
  Show dependency treegraph
 
Reported: 2015-01-29 06:09 UTC by David Cook
Modified: 2019-07-16 06:17 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.02


Attachments
Bug 13640: Do not display unreserveable items on the Holds to pull screen (1.81 KB, patch)
2019-05-13 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13640: Do not display unreserveable items on the Holds to pull screen (1.88 KB, patch)
2019-06-17 07:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13640: Do not display unreserveable items on the Holds to pull screen (1.97 KB, patch)
2019-06-21 07:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13640: (QA follow-up) Allow NULL values for default_branch_item_rules.holdallowed (1.28 KB, patch)
2019-06-21 07:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2015-01-29 06:09:36 UTC
Currently, the Holds To Pull List includes items that are unreserveable or cannot be placed on hold.

For instance:

Bib Record A has 2 items:

Item Record X (can be placed on hold)
Item Record Y (cannot be placed on hold)

If you place a bib-level hold on Bib Record A, both Item Record X and Item Record Y show up as available items in the Holds To Pull List.

--

Fixing this will actually probably involve re-writing the whole of pendingreserves.pl, which is probably long overdue anyway, since it includes a lot of SQL in the script itself.
Comment 1 Katrin Fischer 2015-07-23 07:31:09 UTC
We are observing this as well. 

For a record with 2 items:
1 item is not for loan (via itemtype) and on the shelf
1 item is checked out

The holds to pull list shows the not for loan item.
Comment 2 Lisette Scheer 2016-12-08 19:21:32 UTC
We have this as well. If 1 item has no holds allowed and the other is checked out the no holds allowed will show up on the holds list.
Comment 3 David Cook 2017-01-12 06:06:13 UTC
Just a note that I have no plans to work on this currently, so I'm happy for someone else to work on fixing this bug.
Comment 4 Fiona Borthwick 2017-04-25 10:09:39 UTC
This has been raised by one of our customers and I can see it being a big problem for the larger libraries.

In our customer's case, they have marked certain item types - e.g. Reference as not for loan in the item type policy. This means that the 'not for loan' subfield in the item record has not been populated.  It appears that the SQL for the Holds to Pull report is looking at the not for loan item attribute and therefore not considering the item type at all.

There are 'Default holds policy by item type' set up to mark these item types with the value of No Holds Allowed.

The customer is therefore confused as to why the Holds to Pull report should list these items.  Of course the Reference items will not satisfy the hold so they can be ignored but in larger libraries they could have a large list.

It seems that perhaps a solution would be to modify the SQL to add a JOIN to look at the item type policies too.
Comment 5 Magnus Enger 2018-09-20 12:55:02 UTC
I have a customer expeiencing problems with this too. 

Could an easy solution be to have a checkbox on the itemtypes that says "Do not include in 'Holds to pull' report"? And then exlude items with that setting when the "Holds to pull" is generated?
Comment 6 Katrin Fischer 2018-09-21 05:41:29 UTC
Hm, I don't think we need more configuration options - we need the report to use the ones we have correctly.
Comment 7 Magnus Enger 2018-09-21 06:13:38 UTC
Yeah, probably a good idea!
Comment 8 Magnus Enger 2018-09-21 07:11:20 UTC
(In reply to Katrin Fischer from comment #6)
> Hm, I don't think we need more configuration options - we need the report to
> use the ones we have correctly.

...but I can't convince myself 100% that it will work. 

For example, inter library loans. When you start an ILL request you want to put a hold on the item, so there is a connection between the patron and the record/item. So if you use an itemtype for ILL that itemtype must be "holdable". At the same time the item will not be on loan, and so it will look like an "on the shelf hold" to the Holds to pull report, I think? 

Not sure I can see all the consequences, there might be ways to work around this, but perhaps something to consider when the time comes. 

BTW, this is the SQL that generates the holds to pull report:
http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=circ/pendingreserves.pl;h=7f03d88cc10a3f6af4c14711242201adb41dfb80;hb=HEAD#l174
Comment 9 Magnus Enger 2018-09-21 07:31:34 UTC
I think I managed to take the 'Default holds policy by item type' into account in this report:

SELECT min(reservedate) as l_reservedate,
    reserves.reserve_id,
    reserves.borrowernumber as borrowernumber,

    GROUP_CONCAT(DISTINCT items.holdingbranch 
            ORDER BY items.itemnumber SEPARATOR '|') l_holdingbranch,
    reserves.biblionumber,
    reserves.branchcode as l_branch,
    reserves.itemnumber,
    items.holdingbranch,
    items.homebranch,
    GROUP_CONCAT(DISTINCT items.itype
            ORDER BY items.itemnumber SEPARATOR '|') l_item_type,
    GROUP_CONCAT(DISTINCT items.location 
            ORDER BY items.itemnumber SEPARATOR '|') l_location,
    GROUP_CONCAT(DISTINCT items.itemcallnumber 
            ORDER BY items.itemnumber SEPARATOR '<br/>') l_itemcallnumber,
    GROUP_CONCAT(DISTINCT items.enumchron
            ORDER BY items.itemnumber SEPARATOR '<br/>') l_enumchron,
    GROUP_CONCAT(DISTINCT items.copynumber
            ORDER BY items.itemnumber SEPARATOR '<br/>') l_copynumber,
    biblio.title,
    biblio.author,
    count(DISTINCT items.itemnumber) as icount,
    count(DISTINCT reserves.borrowernumber) as rcount,
    borrowers.firstname,
    borrowers.surname
FROM  reserves
    LEFT JOIN items ON items.biblionumber=reserves.biblionumber 
    LEFT JOIN biblio ON reserves.biblionumber=biblio.biblionumber
    LEFT JOIN biblioitems ON biblio.biblionumber=biblioitems.biblionumber
    LEFT JOIN branchtransfers ON items.itemnumber=branchtransfers.itemnumber
    LEFT JOIN issues ON items.itemnumber=issues.itemnumber
    LEFT JOIN borrowers ON reserves.borrowernumber=borrowers.borrowernumber
    LEFT JOIN default_branch_item_rules ON items.itype=default_branch_item_rules.itemtype
WHERE
    reserves.found IS NULL
    AND (reserves.itemnumber IS NULL OR reserves.itemnumber = items.itemnumber)
    AND items.itemnumber NOT IN (SELECT itemnumber FROM branchtransfers where datearrived IS NULL)
    AND items.itemnumber NOT IN (select itemnumber FROM reserves where found IS NOT NULL)
    AND issues.itemnumber IS NULL
    AND reserves.priority <> 0 
    AND reserves.suspend = 0
    AND notforloan = 0 AND damaged = 0 AND itemlost = 0 AND withdrawn = 0
    AND default_branch_item_rules.holdallowed != 0
GROUP BY reserves.biblionumber ORDER BY biblio.title

What is changed compared to the "Holds to pull" SQL are these two lines:

- LEFT JOIN default_branch_item_rules ON items.itype=default_branch_item_rules.itemtype
- AND default_branch_item_rules.holdallowed != 0

However, default_branch_item_rules.holdallowed is of the "bit" type:
http://schema.koha-community.org/18_05/tables/default_branch_item_rules.html
I gotta confess I'm not familiar with that. Any special tricks to consider? When I look in the database, the value of that column is 0 or 2. Why not a boolean "0 or 1" value?
Comment 10 Jonathan Druart 2019-05-13 14:57:44 UTC
(In reply to Magnus Enger from comment #9)
> What is changed compared to the "Holds to pull" SQL are these two lines:
> 
> - LEFT JOIN default_branch_item_rules ON
> items.itype=default_branch_item_rules.itemtype
> - AND default_branch_item_rules.holdallowed != 0

This seems to fix the issue.

> However, default_branch_item_rules.holdallowed is of the "bit" type:
> http://schema.koha-community.org/18_05/tables/default_branch_item_rules.html
> I gotta confess I'm not familiar with that. Any special tricks to consider?
> When I look in the database, the value of that column is 0 or 2. Why not a
> boolean "0 or 1" value?

Because there are 3 values: From any library, From home library and not allowed (0)
Comment 11 Jonathan Druart 2019-05-13 15:02:50 UTC
Created attachment 89665 [details] [review]
Bug 13640: Do not display unreserveable items on the Holds to pull screen

We should only display the items that meet the hold policies

Test plan:
It would be good to have a huge list of holds displayed on the "Holds to
pull" and confirm that the display is now correct.

One of the possible test plan has been let in a comment on the bug
report:
Bib Record A has 2 items:

Item Record X (can be placed on hold)
Item Record Y (cannot be placed on hold)

If you place a bib-level hold on Bib Record A, both Item Record X and
Item Record Y show up as available items in the Holds To Pull List.

With this patch applied you must not see Y
Comment 12 Martin Renvoize 2019-06-17 07:45:21 UTC
(In reply to Jonathan Druart from comment #10)
> Because there are 3 values: From any library, From home library and not
> allowed (0)

I'm wondering if we're taking into account the 1, and 2 options correctly then in that case?
Comment 13 Martin Renvoize 2019-06-17 07:46:14 UTC
Created attachment 90639 [details] [review]
Bug 13640: Do not display unreserveable items on the Holds to pull screen

We should only display the items that meet the hold policies

Test plan:
It would be good to have a huge list of holds displayed on the "Holds to
pull" and confirm that the display is now correct.

One of the possible test plan has been let in a comment on the bug
report:
Bib Record A has 2 items:

Item Record X (can be placed on hold)
Item Record Y (cannot be placed on hold)

If you place a bib-level hold on Bib Record A, both Item Record X and
Item Record Y show up as available items in the Holds To Pull List.

With this patch applied you must not see Y

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-06-17 07:47:34 UTC
Signing off as in a testing it appears to work as expected.. 

I'd love to see this query moved into a module and unit tested really, but this fix works for now in my opinion.. good luck QAers.
Comment 15 Jonathan Druart 2019-06-19 02:46:33 UTC
(In reply to Martin Renvoize from comment #12)
> (In reply to Jonathan Druart from comment #10)
> > Because there are 3 values: From any library, From home library and not
> > allowed (0)
> 
> I'm wondering if we're taking into account the 1, and 2 options correctly
> then in that case?

Why not? The patch adds the holdallowed != 0 condition, which matches 1 and 2 :)
Comment 16 Martin Renvoize 2019-06-19 07:02:40 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Martin Renvoize from comment #12)
> > (In reply to Jonathan Druart from comment #10)
> > > Because there are 3 values: From any library, From home library and not
> > > allowed (0)
> > 
> > I'm wondering if we're taking into account the 1, and 2 options correctly
> > then in that case?
> 
> Why not? The patch adds the holdallowed != 0 condition, which matches 1 and
> 2 :)

It was more of a 'note to self' to make sure I improved my understanding of how the value is used before Signing Off.

It looks sane enough to me, but I'm still not 100%. What I'm trying to work out is if the clause is in the right place and is indeed correct allowing both 1 and 2 options. I note that with independent branches switched on we limit by holdingbranch being the local branch.. hence wondering if we aught to be handling library limitations in this part of the query too...  It's one monster query.
Comment 17 Marcel de Rooy 2019-06-21 06:33:04 UTC
QA: Looking here
Comment 18 Marcel de Rooy 2019-06-21 06:59:51 UTC
Fails on strict sql mode:

DBD::mysql::st execute failed: 'koha_master.reserves.reserve_id' isn't in GROUP BY [for Statement [ ... etc ]
Comment 19 Marcel de Rooy 2019-06-21 07:07:58 UTC
The where condition is wrong. It should also incorporate the case that default_branch_item_rules is NULL (for istance, no corresponding record found for a biblio level hold).
Will add a follow-up.
Comment 20 Marcel de Rooy 2019-06-21 07:11:03 UTC
(In reply to Martin Renvoize from comment #14)
> Signing off as in a testing it appears to work as expected.. 
> 
> I'd love to see this query moved into a module and unit tested really, but
> this fix works for now in my opinion.. good luck QAers.

It does not work, but I agree about the module thing ;)
Comment 21 Marcel de Rooy 2019-06-21 07:13:39 UTC
Created attachment 90871 [details] [review]
Bug 13640: Do not display unreserveable items on the Holds to pull screen

We should only display the items that meet the hold policies

Test plan:
It would be good to have a huge list of holds displayed on the "Holds to
pull" and confirm that the display is now correct.

One of the possible test plan has been let in a comment on the bug
report:
Bib Record A has 2 items:

Item Record X (can be placed on hold)
Item Record Y (cannot be placed on hold)

If you place a bib-level hold on Bib Record A, both Item Record X and
Item Record Y show up as available items in the Holds To Pull List.

With this patch applied you must not see Y

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2019-06-21 07:13:44 UTC
Created attachment 90872 [details] [review]
Bug 13640: (QA follow-up) Allow NULL values for default_branch_item_rules.holdallowed

If a rule has not been defined for an itemtype or you submit a biblio level
hold, you cannot filter on holdallowed<>0.

Test plan:
Test with a biblio level hold. Have two items; one item should be blocked
with a policy.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2019-06-21 07:15:11 UTC
@RM: Passing QA with a follow-up. Note that the current query also fails on strict SQL mode. So not blocking for that. And yes, it could be moved to a module on a new report.
Comment 24 Martin Renvoize 2019-06-21 09:16:10 UTC
Nice work!

Pushed to master for 19.11.00
Comment 25 Fridolin SOMERS 2019-07-16 06:17:02 UTC
Pushed to 19.05.x for 19.05.02