Bug 19315

Summary: Routing preview may use wrong biblionumber
Product: Koha Reporter: Nick Clemens <nick>
Component: SerialsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: caroline.cyr-la-rose, colin.campbell, fridolin.somers, jonathan.druart, katrin.fischer, m.de.rooy
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 19315 - Use bibnum instead of biblionumber in routing-preview.pl and add item check
Bug 19315 - Use bibnum instead of biblionumber in routing-preview.pl and add item check
Bug 19315 - Use bibnum instead of biblionumber in routing-preview.pl and add item check
Bug 19315: [QA Follow-up] Trivial edits
Bug 19315: Prevent crash if no items and not library set for the subscription
Bug 19315: [16.11.x] Use bibnum instead of biblionumber in routing-preview.pl and add item check
Bug 19315: [16.11.x] [QA Follow-up] Trivial edits
Bug 19315: [16.11.x] Prevent crash if no items and not library set for the subscription
[SIGNED-OFF] Bug 19315: [16.11.x] Use bibnum instead of biblionumber in routing-preview.pl and add item check
[SIGNED-OFF] Bug 19315: [16.11.x] [QA Follow-up] Trivial edits
[SIGNED-OFF] Bug 19315: [16.11.x] Prevent crash if no items and not library set for the subscription

Description Nick Clemens 2017-09-14 12:22:46 UTC
When generating the routing preview from the serials collection screen:
/cgi-bin/koha/serials/serials-collection.pl?subscriptionid=##

the subscription information is obtained via a mysql call in C4::Serials::GetSubscription, however, the statement pulls subscription.* and subscriptionhistory.*

Both of these have a biblionumber field, so when routing-preview.pl pulls the biblionumber it may get bad information
Comment 1 Nick Clemens 2017-09-14 12:26:50 UTC
Created attachment 67134 [details] [review]
Bug 19315 - Use bibnum instead of biblionumber in routing-preview.pl and add item check

To test:
 1 - Create a serial subscription attached to a record
 2 - Receive some issues
 3 - Edit the subscription
 4 - Go to serails collection
 5 - Try to print a routing list
 6 - You may or may not get the right serial
 7 - Additionally create a subscription attached to a bib with no items
 8 - Try to print a routing list for this serial
 9 - You will get 'Internal server error'
10 - Apply patch
11 - Print routing list for first serial
12 - You will always get the correct bib
13 - Print routing list for second serial
14 - You should no longer get an error
Comment 2 Caroline Cyr La Rose 2017-09-15 19:43:13 UTC
At step 5, I'm getting the error message : "Can't call method "branchname" on an undefined value at /inlibro/git/koha-master-dev-inlibro/serials/routing-preview.pl line 72." Is it something I'm doing wrong?

I didn't apply the patch yet.
Comment 3 Caroline Cyr La Rose 2017-09-15 19:57:15 UTC
At step 9, I get a software error :"Can't call method "store" on an undefined value at /inlibro/git/koha-master-dev-inlibro/C4/Reserves.pm line 907."

After applying the patch, everything/both routing lists works! yay! 

It's hard to know if I would've gotten the right info on the first routing list (steps 1-6)
Comment 4 Caroline Cyr La Rose 2017-09-15 20:14:30 UTC
Created attachment 67164 [details] [review]
Bug 19315 - Use bibnum instead of biblionumber in routing-preview.pl and add item check

To test:
 1 - Create a serial subscription attached to a record
 2 - Receive some issues
 3 - Edit the subscription
 4 - Go to serails collection
 5 - Try to print a routing list
 6 - You may or may not get the right serial
 7 - Additionally create a subscription attached to a bib with no items
 8 - Try to print a routing list for this serial
 9 - You will get 'Internal server error'
10 - Apply patch
11 - Print routing list for first serial
12 - You will always get the correct bib
13 - Print routing list for second serial
14 - You should no longer get an error

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 5 Marcel de Rooy 2017-10-06 06:25:45 UTC
Created attachment 67668 [details] [review]
Bug 19315 - Use bibnum instead of biblionumber in routing-preview.pl and add item check

To test:
 1 - Create a serial subscription attached to a record
 2 - Receive some issues
 3 - Edit the subscription
 4 - Go to serails collection
 5 - Try to print a routing list
 6 - You may or may not get the right serial
 7 - Additionally create a subscription attached to a bib with no items
 8 - Try to print a routing list for this serial
 9 - You will get 'Internal server error'
10 - Apply patch
11 - Print routing list for first serial
12 - You will always get the correct bib
13 - Print routing list for second serial
14 - You should no longer get an error

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-10-06 06:25:49 UTC
Created attachment 67669 [details] [review]
Bug 19315: [QA Follow-up] Trivial edits

Replace $subs->{bibnum} by $biblionumber on a following line.
The scalar in the boolean expression is not needed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-10-06 06:27:18 UTC
Nick: Isnt there anything to be done on the GetSubscription output? Whats the use of the 'wrong' biblionumber ?
Comment 8 Nick Clemens 2017-10-06 09:48:48 UTC
(In reply to Marcel de Rooy from comment #7)
> Nick: Isnt there anything to be done on the GetSubscription output? Whats
> the use of the 'wrong' biblionumber ?

Technically yes, the 'wrong' biblionumber can't reliably be used for anything, it's more convenience as you can't get 'all columns but one'

Right now we have:
SELECT  subscription.*,
        subscriptionhistory.*,

And to fix we would have to list every field individually:
SELECT  subscription.subscriptionid,subscription.biblionumber AS sbiblionumber,librarian, startdate...
        subscriptionhistory.biblionumber AS shbiblionumber,histstartdatem histenddate,missinglist...
Comment 9 Jonathan Druart 2017-10-06 15:18:47 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 10 Fridolin Somers 2017-10-27 10:58:25 UTC
I test for 17.05.x :

With patches applied.
I have create a subscription without branch nor items.
Printing a routing list for this serial returns an error :
    Can't call method "branchname" on an undefined value at /home/koha/src/serials/routing-preview.pl line 71.

Maybe $branch should be tested : 
  - my $branchname = Koha::Libraries->find($branch)->branchname;
  + my $branchname = Koha::Libraries->find($branch)->branchname if $branch;
Comment 11 Katrin Fischer 2017-10-29 18:20:46 UTC
Patches don't apply cleanly on 16.11.x, please rebase if you want this to be included.
Comment 12 Jonathan Druart 2017-12-21 13:50:02 UTC
Created attachment 70022 [details] [review]
Bug 19315: Prevent crash if no items and not library set for the subscription

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2017-12-21 13:55:50 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 70022 [details] [review] [review]
> Bug 19315: Prevent crash if no items and not library set for the subscription
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

This patch has been pushed to master for 18.05

The first 2 patches have been pushed already to 17.11 and 17.05, this one must be pushed as well.
Comment 14 Jonathan Druart 2017-12-21 14:03:18 UTC
Created attachment 70026 [details] [review]
Bug 19315: [16.11.x] Use bibnum instead of biblionumber in routing-preview.pl and add item check

To test:
 1 - Create a serial subscription attached to a record
 2 - Receive some issues
 3 - Edit the subscription
 4 - Go to serails collection
 5 - Try to print a routing list
 6 - You may or may not get the right serial
 7 - Additionally create a subscription attached to a bib with no items
 8 - Try to print a routing list for this serial
 9 - You will get 'Internal server error'
10 - Apply patch
11 - Print routing list for first serial
12 - You will always get the correct bib
13 - Print routing list for second serial
14 - You should no longer get an error

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Jonathan Druart 2017-12-21 14:03:22 UTC
Created attachment 70027 [details] [review]
Bug 19315: [16.11.x] [QA Follow-up] Trivial edits

Replace $subs->{bibnum} by $biblionumber on a following line.
The scalar in the boolean expression is not needed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Jonathan Druart 2017-12-21 14:03:26 UTC
Created attachment 70028 [details] [review]
Bug 19315: [16.11.x] Prevent crash if no items and not library set for the subscription

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2017-12-21 14:03:56 UTC
And the last 3 patches are the 16.11.x version
Comment 18 Jonathan Druart 2017-12-21 14:04:59 UTC
*** Bug 19858 has been marked as a duplicate of this bug. ***
Comment 19 Katrin Fischer 2017-12-21 15:06:31 UTC
Created attachment 70044 [details] [review]
[SIGNED-OFF] Bug 19315: [16.11.x] Use bibnum instead of biblionumber in routing-preview.pl and add item check

To test:
 1 - Create a serial subscription attached to a record
 2 - Receive some issues
 3 - Edit the subscription
 4 - Go to serails collection
 5 - Try to print a routing list
 6 - You may or may not get the right serial
 7 - Additionally create a subscription attached to a bib with no items
 8 - Try to print a routing list for this serial
 9 - You will get 'Internal server error'
10 - Apply patch
11 - Print routing list for first serial
12 - You will always get the correct bib
13 - Print routing list for second serial
14 - You should no longer get an error

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 20 Katrin Fischer 2017-12-21 15:06:51 UTC
Created attachment 70045 [details] [review]
[SIGNED-OFF] Bug 19315: [16.11.x] [QA Follow-up] Trivial edits

Replace $subs->{bibnum} by $biblionumber on a following line.
The scalar in the boolean expression is not needed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 21 Katrin Fischer 2017-12-21 15:06:55 UTC
Created attachment 70046 [details] [review]
[SIGNED-OFF] Bug 19315: [16.11.x] Prevent crash if no items and not library set for the subscription

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 22 Nick Clemens 2017-12-22 10:54:40 UTC
Patch in comment 12 has been pushed to stable for 17.11.01