Bug 24350

Summary: Can't place holds
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P5 - low CC: 1joynelson, andrewfh, gmcharlt, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 22284    
Bug Blocks: 24368    
Attachments: Bug 24350: Fix Branches.pickup_locations
Bug 24350: Fix Branches.pickup_locations
Bug 24350: Fix Branches.pickup_locations
Bug 24350: follow-up - unbless each library, not the array
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations().
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations().
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations().
Bug 24350: (follow-up) Fix pickup_location test in t/db_dependent/Template/Plugin/Branches.t
Bug 24350: ->pickup_locations returns unblessed objects

Description Katrin Fischer 2020-01-06 11:48:53 UTC
When trying to place a hold in staff on master, I have the following error:

Template process failed: undef error - Not a HASH reference at /home/vagrant/kohaclone/Koha/Template/Plugin/Branches.pm line 96.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122


It works pre cd4dfc6e121a8f867fee5fcf8ccfd594bf163ba7

I haven't done a git bisect, but as this is the most recent holds work, I am going to add a link suspecting it to be the cause.
Comment 1 Tomás Cohen Arazi 2020-01-06 11:59:21 UTC
It makes sense to blame that dev as it reengineers the pickup locations code. I will take a look.
Comment 2 Jonathan Druart 2020-01-06 21:20:24 UTC
Created attachment 96860 [details] [review]
Bug 24350: Fix Branches.pickup_locations

This will fix the following error:
Template process failed: undef error - Not a HASH reference at
/home/vagrant/kohaclone/Koha/Template/Plugin/Branches.pm line 96.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

Koha::Libraries->pickup_locations does not always return an array, but
arrayref if $biblio is not a Koha::Biblio object.

I do not think it's the correct fix, the pattern in
Koha::Libraries->pickup_locations is wrong: we should not expect 2
different types for a given parameter, biblio should always be a
Koha::Biblio (idem for item btw).
That could be fixed easily if the template had the Koha::Biblio object
sent.
Comment 3 Tomás Cohen Arazi 2020-01-07 13:46:51 UTC
(In reply to Jonathan Druart from comment #2)
> Created attachment 96860 [details] [review] [review]
> Bug 24350: Fix Branches.pickup_locations
> 
> This will fix the following error:
> Template process failed: undef error - Not a HASH reference at
> /home/vagrant/kohaclone/Koha/Template/Plugin/Branches.pm line 96.
>  at /home/vagrant/kohaclone/C4/Templates.pm line 122
> 
> Koha::Libraries->pickup_locations does not always return an array, but
> arrayref if $biblio is not a Koha::Biblio object.
> 
> I do not think it's the correct fix, the pattern in
> Koha::Libraries->pickup_locations is wrong: we should not expect 2
> different types for a given parameter, biblio should always be a
> Koha::Biblio (idem for item btw).
> That could be fixed easily if the template had the Koha::Biblio object
> sent.

I think we should fix this blocker with your patch, Jonathan, and file a separate bug for refactoring things. This way people can keep testing the feature and writing the manual pages, while we implement things in a different way.
Comment 4 Katrin Fischer 2020-01-07 13:47:13 UTC
*** Bug 24362 has been marked as a duplicate of this bug. ***
Comment 5 Caroline Cyr La Rose 2020-01-07 16:50:50 UTC
Created attachment 96912 [details] [review]
Bug 24350: Fix Branches.pickup_locations

This will fix the following error:
Template process failed: undef error - Not a HASH reference at
/home/vagrant/kohaclone/Koha/Template/Plugin/Branches.pm line 96.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

Koha::Libraries->pickup_locations does not always return an array, but
arrayref if $biblio is not a Koha::Biblio object.

I do not think it's the correct fix, the pattern in
Koha::Libraries->pickup_locations is wrong: we should not expect 2
different types for a given parameter, biblio should always be a
Koha::Biblio (idem for item btw).
That could be fixed easily if the template had the Koha::Biblio object
sent.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 6 Martin Renvoize 2020-01-07 16:53:48 UTC
Agreed that longer term we should do the refactoring fix.. but this is a pragmatic solution to get things working again so I'm happy to take it as is.. 


I'll add a dependant bug for the refactor now so we don't forget it.
Comment 7 Martin Renvoize 2020-01-07 17:01:59 UTC
Created attachment 96913 [details] [review]
Bug 24350: Fix Branches.pickup_locations

This will fix the following error:
Template process failed: undef error - Not a HASH reference at
/home/vagrant/kohaclone/Koha/Template/Plugin/Branches.pm line 96.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

Koha::Libraries->pickup_locations does not always return an array, but
arrayref if $biblio is not a Koha::Biblio object.

I do not think it's the correct fix, the pattern in
Koha::Libraries->pickup_locations is wrong: we should not expect 2
different types for a given parameter, biblio should always be a
Koha::Biblio (idem for item btw).
That could be fixed easily if the template had the Koha::Biblio object
sent.

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-01-07 17:04:02 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Andrew Fuerste-Henry 2020-01-10 14:59:14 UTC
I'm getting a slightly different error when trying to place a hold now:
Template process failed: undef error - Can't call method "unblessed" on unblessed reference at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 93.
Comment 10 Nick Clemens 2020-01-10 15:38:08 UTC
Created attachment 97231 [details] [review]
Bug 24350: follow-up - unbless each library, not the array
Comment 11 Martin Renvoize 2020-01-10 15:55:48 UTC
Followup pushed, though I'm slightly sceptical.. it'll be interesting to see if anything falls out in the Jenkins run.
Comment 12 Owen Leonard 2020-01-11 16:23:21 UTC
Still getting this error when placing a hold:

Template process failed: undef error - Not an ARRAY reference at /home/vagrant/kohaclone/Koha/Template/Plugin/Branches.pm line 95.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

A new bug, or this one is still broken?
Comment 13 Martin Renvoize 2020-01-11 17:38:00 UTC
Still this one.. Agustin is working of fixes I believe... Jonathan and Nick just flip flopped the issue with their patches unfortunately.. it needs fixing further up the stack.
Comment 14 Tomás Cohen Arazi 2020-01-11 17:41:31 UTC
On Friday I worked on refactoring the template methods to work in a different way.

I think we should 'do it right' and think a bit about this.

Agustín is back from vacation and will step in
Comment 15 Agustín Moyano 2020-01-12 04:16:32 UTC
Created attachment 97258 [details] [review]
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations().
Comment 16 Agustín Moyano 2020-01-12 05:44:58 UTC
Created attachment 97259 [details] [review]
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations().
Comment 17 Agustín Moyano 2020-01-12 05:55:43 UTC
Created attachment 97260 [details] [review]
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations().
Comment 18 Agustín Moyano 2020-01-12 06:13:24 UTC
Created attachment 97261 [details] [review]
Bug 24350: (follow-up) Fix pickup_location test in t/db_dependent/Template/Plugin/Branches.t
Comment 19 Martin Renvoize 2020-01-13 10:05:46 UTC
Followups pushed, thanks Agustin :)
Comment 20 Jonathan Druart 2020-01-13 11:43:55 UTC
Tests are failing:

t/db_dependent/Koha/Libraries.t .. 4/9             # Looks like you planned 11 tests but ran 2.

        #   Failed test 'Given BranchTransferLimitsType = itemtype and item-level_itypes = ON'
        #   at t/db_dependent/Koha/Libraries.t line 258.
        # Looks like you planned 4 tests but ran 2.
        # Looks like you failed 1 test of 2 run.

    #   Failed test 'UseBranchTransferLimits = ON'
    #   at t/db_dependent/Koha/Libraries.t line 422.
    # Looks like you failed 1 test of 2.

#   Failed test 'pickup_locations'
#   at t/db_dependent/Koha/Libraries.t line 423.
Can't call method "branchcode" on unblessed reference at t/db_dependent/Koha/Libraries.t line 195.
Comment 21 Joy Nelson 2020-01-14 00:16:19 UTC
Not backported to 19.11.x due to dependencies
Comment 22 Tomás Cohen Arazi 2020-01-14 13:30:07 UTC
Created attachment 97352 [details] [review]
Bug 24350: ->pickup_locations returns unblessed objects

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Martin Renvoize 2020-01-14 13:38:16 UTC
Followup pushed, thanks Tomas.