Bug 25726 - Holds to Pull made empty by pathological holds
Summary: Holds to Pull made empty by pathological holds
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Andrew Fuerste-Henry
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-12 13:43 UTC by Andrew Fuerste-Henry
Modified: 2020-09-18 17:36 UTC (History)
8 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:
20.11.00, 20.05.03, 19.11.09, 19.05.15


Attachments
Bug 25726: make holds to pull ignore holds where found is not null and itemnumber is null (1.32 KB, patch)
2020-06-12 13:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25726: make holds to pull ignore holds where found is not null and itemnumber is null (1.38 KB, patch)
2020-06-12 14:23 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25726: make holds to pull ignore holds where found is not null and itemnumber is null (2.07 KB, patch)
2020-07-03 09:38 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 Andrew Fuerste-Henry 2020-06-12 13:43:11 UTC
After upgrading to 19.11, we're seeing libraries where pendingreserves.pl comes up empty when it shouldn't be. After some digging, we found it was because the library had some bad holds in their reserves table -- lines where reserves.itemnumber is null but reserves.found is not null. That runs into an issue on this line of the Holds to Pull query:
    AND items.itemnumber NOT IN (select itemnumber FROM reserves where found IS NOT NULL)

I don't know exactly what changed between 19.05 and 19.11 to change how Koha thinks about this. I also don't know how libraries have ended up with these pathological holds. But that's a separate bug.

To recreate:
1- place 2 bib-level holds on available items
2- confirm they both show on Holds to Pull
3- edit one hold from the database to set found='T'
4- reload Holds to Pull, confirm it is now empty
Comment 1 Andrew Fuerste-Henry 2020-06-12 13:54:48 UTC
Created attachment 105800 [details] [review]
Bug 25726: make holds to pull ignore holds where found is not null and itemnumber is null

To test:
1- place 2 bib-level holds on available items
2- confirm they both show on Holds to Pull
3- edit one hold from the database to set found='T'
4- reload Holds to Pull, confirm it is now empty
5- apply patch
6- Reload Holds to Pull
7- confirm it now shows the hold you did not edit
Comment 2 ByWater Sandboxes 2020-06-12 14:23:18 UTC
Created attachment 105821 [details] [review]
Bug 25726: make holds to pull ignore holds where found is not null and itemnumber is null

To test:
1- place 2 bib-level holds on available items
2- confirm they both show on Holds to Pull
3- edit one hold from the database to set found='T'
4- reload Holds to Pull, confirm it is now empty
5- apply patch
6- Reload Holds to Pull
7- confirm it now shows the hold you did not edit

Signed-off-by: donnab <donna@bywatersolutions.com>
Comment 3 Katrin Fischer 2020-06-13 18:16:20 UTC
Hi Andrew,

I am a bit worried about the cause of these strange holds. You say reserves.found is not null - what was the found value for these strange holds?
Comment 4 Andrew Fuerste-Henry 2020-06-13 20:05:23 UTC
Me, too! We've only seen a couple examples and haven't yet tracked down how they got in that state. In both cases the hold had a reserves.found value of T, but was not actually in transit. In one of those cases, the patron had 2 holds on the same record, which suggests maybe bug 18958 is involved? But, yes, for sure, there is a larger issue of where this bad data came from, my patch is just alleviating the major symptom caused by the data's existence.

(In reply to Katrin Fischer from comment #3)
> Hi Andrew,
> 
> I am a bit worried about the cause of these strange holds. You say
> reserves.found is not null - what was the found value for these strange
> holds?
Comment 5 Katrin Fischer 2020-06-14 12:38:45 UTC
Hi Andrew,

I am actually a bit confused by the SQL change and worried about unwanted side effects:

-    AND items.itemnumber NOT IN (select itemnumber FROM reserves where found IS NOT NULL)
+    AND items.itemnumber NOT IN (select itemnumber FROM reserves where found IS NOT NULL AND itemnumber IS NOT NULL)

If I understand correctly this will make the items in 'Transit' status show up in the list. But if they really are in transit - they can't be pulled... so should they show up? I think the logic of the SQL might be right and we need to take care to fix the holds, if the state of found = "T" and itemnumber empty is unwanted.

I've tried several things to get a 'pathological' hold, but haven't succeded other than in turning up more oddities (see bug 25748).
Comment 6 Marcel de Rooy 2020-07-03 09:37:37 UTC
(In reply to Katrin Fischer from comment #5)
> If I understand correctly this will make the items in 'Transit' status show
> up in the list. But if they really are in transit - they can't be pulled...
> so should they show up? I think the logic of the SQL might be right and we
> need to take care to fix the holds, if the state of found = "T" and
> itemnumber empty is unwanted.

No the transit does not come up. Since it is in a itemnumber NOT IN ... construction. The only thing we exclude here is getting a NULL and then saying itemnumber NOT IN (NULL) making all biblio level holds disappear.
Comment 7 Marcel de Rooy 2020-07-03 09:38:27 UTC
Created attachment 106493 [details] [review]
Bug 25726: make holds to pull ignore holds where found is not null and itemnumber is null

To test:
1- place 2 bib-level holds on available items
2- confirm they both show on Holds to Pull
3- edit one hold from the database to set found='T'
4- reload Holds to Pull, confirm it is now empty
5- apply patch
6- Reload Holds to Pull
7- confirm it now shows the hold you did not edit

Signed-off-by: donnab <donna@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Using capitals for SQL reserved words; added a FIXME
Tested with:
    update reserves set found='T', itemnumber=NULL where reserve_id=...
This should be a workaround while we fix the underlying problem.
Comment 8 Alex Arnaud 2020-07-03 10:22:42 UTC
(In reply to Katrin Fischer from comment #5)
>
> If I understand correctly this will make the items in 'Transit' status show
> up in the list. But if they really are in transit - they can't be pulled...
> so should they show up? 

That doesn't seem to make the items in transit show up in Holds to pull list. However, i'm not sure of any other side effects.

> I think the logic of the SQL might be right and we
> need to take care to fix the holds, if the state of found = "T" and
> itemnumber empty is unwanted.
Agreed. Maybe we should understand why we get this pathological holds in the first place.
> 
> I've tried several things to get a 'pathological' hold, but haven't succeded
> other than in turning up more oddities (see bug 25748).
Unable to recreate neither...
Comment 9 Andrew Fuerste-Henry 2020-07-06 12:18:25 UTC
Thanks, all! I should add that I've only run into this on one library, so whatever got their holds into this state seems to be pretty vanishingly rare, but I'm keeping and eye out for more. If I can figure out how to reproduce it, I'll absolutely file a bug for it.
Comment 10 Jonathan Druart 2020-07-20 12:08:05 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Lucas Gass 2020-07-24 21:20:14 UTC
backported to 20.05.x for 20.05.03
Comment 12 Aleisha Amohia 2020-08-03 21:56:04 UTC
backported to 19.11.x for 19.11.09
Comment 13 Fridolin SOMERS 2020-08-13 11:08:35 UTC
Super I was looking for this exact fix.
Looks like its the behavior of NOT IN () with NULL :

I've found some explanations in :
https://stackoverflow.com/questions/1001144/mysql-select-x-from-a-where-not-in-select-x-from-b-unexpected-result#1001197
.
Comment 14 Victor Grousset/tuxayo 2020-09-18 17:36:38 UTC
Backported to 19.05.x branch for 19.05.15