Bug 24350 - Can't place holds
Summary: Can't place holds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22284
Blocks: 24368
  Show dependency treegraph
 
Reported: 2020-01-06 11:48 UTC by Katrin Fischer
Modified: 2020-11-30 21:44 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Circulation function:


Attachments
Bug 24350: Fix Branches.pickup_locations (1.78 KB, patch)
2020-01-06 21:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24350: Fix Branches.pickup_locations (1.85 KB, patch)
2020-01-07 16:50 UTC, Caroline Cyr La Rose
Details | Diff | Splinter Review
Bug 24350: Fix Branches.pickup_locations (1.92 KB, patch)
2020-01-07 17:01 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 24350: follow-up - unbless each library, not the array (1.02 KB, patch)
2020-01-10 15:38 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations(). (4.57 KB, patch)
2020-01-12 04:16 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations(). (5.18 KB, patch)
2020-01-12 05:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24350: (follow-up) Clear Koha::Libraries->pickup_locations() and move logic to Koha::Template::Plugin::Branches->pickup_locations(). (5.49 KB, patch)
2020-01-12 05:55 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24350: (follow-up) Fix pickup_location test in t/db_dependent/Template/Plugin/Branches.t (848 bytes, patch)
2020-01-12 06:13 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24350: ->pickup_locations returns unblessed objects (7.64 KB, patch)
2020-01-14 13:30 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 (tcohen) 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 (tcohen) 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 (ashimema) 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 (ashimema) 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 (ashimema) 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 (kidclamp) 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 (ashimema) 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 (ashimema) 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 (tcohen) 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 (ashimema) 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 (tcohen) 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 (ashimema) 2020-01-14 13:38:16 UTC
Followup pushed, thanks Tomas.