Bug 14846 - Items with no holdingbranch causes user's holds display to freeze
Summary: Items with no holdingbranch causes user's holds display to freeze
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Bouzid Fergani
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-17 15:32 UTC by Blou
Modified: 2017-06-14 22:10 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:


Attachments
Image of freezing Holds tab display (78.91 KB, image/jpeg)
2015-09-17 15:32 UTC, Blou
Details
Bug 14846 - Items with no holdingbranch causes svc/holds to crash (1.41 KB, patch)
2015-09-17 15:35 UTC, Blou
Details | Diff | Splinter Review
Bug 14846 - Items with no holdingbranch causes svc/holds to crash (1.55 KB, patch)
2015-11-13 13:57 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 14846 - Items with no holdingbranch causes svc/holds to crash (1.56 KB, patch)
2015-12-08 13:24 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 14846 - Items with no holdingbranch causes svc/holds to crash (1.63 KB, patch)
2015-12-09 08:41 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2015-09-17 15:32:07 UTC
Created attachment 42690 [details]
Image of freezing Holds tab display

When trying to display the holds for a user, if an item has no holdingbranch, it causes the svc/holds service to crash and the display to freeze.

Tentative steps to reproduce on any DB:
1) put 1 or more items on hold for a given user.
2) go into the database and set the item.holdingbranch to NULL
3) In the user's page, in checkouts (circulation.pl), click the tab that should be written 1 Hold(s)
4) It will be "Processing" indefinitely.

See attached image.

The cause is the svc/holds code that crashes.  The fix will validate that there's an holdingbranch before proceding with the code.
Comment 1 Blou 2015-09-17 15:35:12 UTC Comment hidden (obsolete)
Comment 2 Blou 2015-09-17 15:57:02 UTC
This was done differently (fixed) by Kyle recently in master.

The issue is just with 3.20, then.  I suppose this makes this little patch useless.
Comment 3 Liz Rea 2015-11-06 02:15:15 UTC
I get this when applying on 3.20.05:



Bug 14846 - Items with no holdingbranch causes user's holds display to freeze

42691 - Bug 14846 - Items with no holdingbranch causes svc/holds to crash

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14846 - Items with no holdingbranch causes svc/holds to crash
Using index info to reconstruct a base tree...
M	svc/holds
Falling back to patching base and 3-way merge...
Auto-merging svc/holds
CONFLICT (content): Merge conflict in svc/holds
Failed to merge in the changes.
Patch failed at 0001 Bug 14846 - Items with no holdingbranch causes svc/holds to crash
Comment 4 Bouzid Fergani 2015-11-13 13:57:56 UTC Comment hidden (obsolete)
Comment 5 Frédéric Demians 2015-12-08 12:15:38 UTC
(In reply to Blou from comment #2)
> This was done differently (fixed) by Kyle recently in master.
> 
> The issue is just with 3.20, then.  I suppose this makes this little patch
> useless.

How was it fixed in master? Couldn't it be backported?
Comment 6 Blou 2015-12-08 13:04:08 UTC
I tried it, but it didn't apply.  Before that patch (13853), there are at least two others that modify the structure of svc/holds.

So maybe it would be the right solution.  But it would certainly require more work, if it's only to patch on an older release.  

I know what we did locally. :)
Comment 7 Frédéric Demians 2015-12-08 13:19:49 UTC
(In reply to Blou from comment #6)

> I tried it, but it didn't apply.  Before that patch (13853), there are at
> least two others that modify the structure of svc/holds.

Thanks for the explanation. Since #13853 is an enhancement, it mustn't be backported, and so your 3.20 & 3.18 patch is valid, imho.
Comment 8 Frédéric Demians 2015-12-08 13:24:07 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-12-08 14:39:41 UTC
The bug exists on master, and the patch fixes the described issue.
Could you please explain how you can have a holdingbranch value set to NULL?
Comment 10 Blou 2015-12-08 21:16:57 UTC
I asked Francois, and he has no idea how we got to that.  But we had TWO clients with the situation, one of which it couldn't be blamed on a migration issue.

The clerk put the item on Reserve to put them aside when they would come back.  We thought he had no branch associated, but then he probably could not do much, and anyway we confirmed it was not the case.
Comment 11 Jonathan Druart 2015-12-09 08:41:56 UTC
Created attachment 45517 [details] [review]
Bug 14846 - Items with no holdingbranch causes svc/holds to crash

When trying to display the holds for a user, if an item has no
holdingbranch, it causes the svc/holds service to crash and the display
to freeze.

Tentative steps to reproduce on any DB:
1) put 1 or more items on hold for a given user.
2) go into the database and set the item.holdingbranch to NULL
3) In the user's page, in checkouts (circulation.pl), click the tab that
   should be written 1 Hold(s)
4) It will be "Processing" indefinitely.

The cause is the svc/holds code that crashes.  The fix will validate
that there's an holdingbranch before proceding with the code.

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2015-12-09 08:44:28 UTC
I don't know if we should allow NULL in items.holdingbranch and items.homebranch.
But since it's possible at the moment, we should take it into account.
Comment 13 Kyle M Hall 2015-12-11 16:22:08 UTC
Pushed to master!

Thanks Philippe!
Comment 14 Julian Maurice 2015-12-14 07:21:52 UTC
Patch pushed to 3.22.x, will be in 3.22.1
Comment 15 Frédéric Demians 2015-12-14 17:58:10 UTC
This patch has been pushed to 3.20.x, will be in 3.20.7.