Bug 25421 - Make Koha::Item and Koha::Biblio ->pickup_locations return an arrayref
Summary: Make Koha::Item and Koha::Biblio ->pickup_locations return an arrayref
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 22284
Blocks: 24368
  Show dependency treegraph
 
Reported: 2020-05-08 09:27 UTC by Jonathan Druart
Modified: 2020-05-13 16:11 UTC (History)
5 users (show)

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


Attachments
Bug 25421: Make ->pickup_locations only return an arrayref (4.29 KB, patch)
2020-05-08 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25421: Remove use of Koha::Libraries->pickup_locations (2.67 KB, patch)
2020-05-08 14:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25421: Make ->pickup_locations only return an arrayref (4.36 KB, patch)
2020-05-11 11:27 UTC, didier
Details | Diff | Splinter Review
Bug 25421: Remove use of Koha::Libraries->pickup_locations (2.73 KB, patch)
2020-05-11 11:27 UTC, didier
Details | Diff | Splinter Review
Bug 25421: Make ->pickup_locations only return an arrayref (4.43 KB, patch)
2020-05-11 13:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25421: Remove use of Koha::Libraries->pickup_locations (2.80 KB, patch)
2020-05-11 13:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25421: Update POD (907 bytes, patch)
2020-05-11 13:07 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 Jonathan Druart 2020-05-08 09:27:55 UTC
In list context we sometimes return an array, or an arrayref:

 84     return $libraries->unblessed unless $item or $biblio;                                            

110     return wantarray ? @pickup_locations : \@pickup_locations;

It causes the following error:
ARRAY(0x55cb36339ec0) at /usr/share/perl5/CGI/Compile.pm line 137.                                                                                                                                                 Template process failed: undef error - Not a HASH reference at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 129.                               
 at /kohadevbox/koha/C4/Templates.pm line 122                                                                                                                                                                              C4::Templates::output('C4::Templates=HASH(0x55cb355b7a40)') called at /kohadevbox/koha/reserve/request.pl line 764    

When doing:
Have 1 bibliographic record with 1 item on it
Place 1 biblio level hold
Delete the item (from moredetail.pl)
Go to the hold list
Comment 1 Tomás Cohen Arazi 2020-05-08 14:45:03 UTC
Created attachment 104601 [details] [review]
Bug 25421: Make ->pickup_locations only return an arrayref

This patch makes Koha::Item->pickup_locations and
Koha::Biblio->pickup_locations always return an arrayref.

Test are adjusted to reflect this:

1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
2. Apply this patch and repeat 1
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-05-08 14:45:09 UTC
Created attachment 104602 [details] [review]
Bug 25421: Remove use of Koha::Libraries->pickup_locations

The current implementation uses Koha::Libraries->pickup_locations which
is problematic and due to be removed by bug 24368. This patch makes the
trivial change of just searching for libraries that are marked with
pickup_location => 1.

Calls to Koha::Item->pickup_locations and Koha::Biblio->pickup_locations
are as well adapted to the new arrayref return value.

To test:
1. Pick a record with only one item
2. Place a biblio-level hold on it
3. Edit the items: remove the item
4. Go to the Holds tab
=> FAIL: It explodes
5. Apply this patch and restart:
   $ sudo koha-plack --restart kohadev
6. Go back and go to the holds tab again
=> SUCCESS: No failure!
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 didier 2020-05-11 11:27:09 UTC
Created attachment 104687 [details] [review]
Bug 25421: Make ->pickup_locations only return an arrayref

This patch makes Koha::Item->pickup_locations and
Koha::Biblio->pickup_locations always return an arrayref.

Test are adjusted to reflect this:

1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
2. Apply this patch and repeat 1
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 4 didier 2020-05-11 11:27:13 UTC
Created attachment 104688 [details] [review]
Bug 25421: Remove use of Koha::Libraries->pickup_locations

The current implementation uses Koha::Libraries->pickup_locations which
is problematic and due to be removed by bug 24368. This patch makes the
trivial change of just searching for libraries that are marked with
pickup_location => 1.

Calls to Koha::Item->pickup_locations and Koha::Biblio->pickup_locations
are as well adapted to the new arrayref return value.

To test:
1. Pick a record with only one item
2. Place a biblio-level hold on it
3. Edit the items: remove the item
4. Go to the Holds tab
=> FAIL: It explodes
5. Apply this patch and restart:
   $ sudo koha-plack --restart kohadev
6. Go back and go to the holds tab again
=> SUCCESS: No failure!
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Comment 5 Jonathan Druart 2020-05-11 13:06:36 UTC
No longer critical as it's no longer possible to delete the last item of a bibliographic record that have a biblio-level hold on it.
Comment 6 Jonathan Druart 2020-05-11 13:06:55 UTC
Since bug 8132.
Comment 7 Jonathan Druart 2020-05-11 13:07:39 UTC
Created attachment 104690 [details] [review]
Bug 25421: Make ->pickup_locations only return an arrayref

This patch makes Koha::Item->pickup_locations and
Koha::Biblio->pickup_locations always return an arrayref.

Test are adjusted to reflect this:

1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t t/db_dependent/Koha/Item.t
=> SUCCESS: Tests pass!
2. Apply this patch and repeat 1
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Jonathan Druart 2020-05-11 13:07:43 UTC
Created attachment 104691 [details] [review]
Bug 25421: Remove use of Koha::Libraries->pickup_locations

The current implementation uses Koha::Libraries->pickup_locations which
is problematic and due to be removed by bug 24368. This patch makes the
trivial change of just searching for libraries that are marked with
pickup_location => 1.

Calls to Koha::Item->pickup_locations and Koha::Biblio->pickup_locations
are as well adapted to the new arrayref return value.

To test:
1. Pick a record with only one item
2. Place a biblio-level hold on it
3. Edit the items: remove the item
4. Go to the Holds tab
=> FAIL: It explodes
5. Apply this patch and restart:
   $ sudo koha-plack --restart kohadev
6. Go back and go to the holds tab again
=> SUCCESS: No failure!
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2020-05-11 13:07:48 UTC
Created attachment 104692 [details] [review]
Bug 25421: Update POD

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Martin Renvoize 2020-05-12 10:25:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 11 Jonathan Druart 2020-05-12 13:30:35 UTC
This was wrong:

-    @libraries = Koha::Libraries->pickup_locations() unless @libraries;
+    @libraries = Koha::Libraries->search( { pickup_location => 1 },
+        { order_by => ['branchname'] } )->as_list
+      unless @libraries;

->pickup_locations does much more than search for the pickup_location flag.

Tests from t/db_dependent/Template/Plugin/Branches.t are failing massively.
Comment 12 Tomás Cohen Arazi 2020-05-12 14:07:24 UTC
(In reply to Jonathan Druart from comment #11)
> This was wrong:
> 
> -    @libraries = Koha::Libraries->pickup_locations() unless @libraries;
> +    @libraries = Koha::Libraries->search( { pickup_location => 1 },
> +        { order_by => ['branchname'] } )->as_list
> +      unless @libraries;
> 
> ->pickup_locations does much more than search for the pickup_location flag.

Did that, but not from the call in the Branches plugin.

> Tests from t/db_dependent/Template/Plugin/Branches.t are failing massively.

Weird, I took care of them. I'll re-check
Comment 13 didier 2020-05-12 15:12:38 UTC
I thought I ran the Test plan and
prove xt $(find t -name "*.t")
Likely I screw it. 

BTW
Is there any way to find or add the master commit we test against?
Comment 14 Tomás Cohen Arazi 2020-05-12 17:27:01 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> (In reply to Jonathan Druart from comment #11)
> > This was wrong:
> > 
> > -    @libraries = Koha::Libraries->pickup_locations() unless @libraries;
> > +    @libraries = Koha::Libraries->search( { pickup_location => 1 },
> > +        { order_by => ['branchname'] } )->as_list
> > +      unless @libraries;
> > 
> > ->pickup_locations does much more than search for the pickup_location flag.
> 
> Did that, but not from the call in the Branches plugin.
> 
> > Tests from t/db_dependent/Template/Plugin/Branches.t are failing massively.
> 
> Weird, I took care of them. I'll re-check

I'm sorry for the failing tests. This patch was supposed to be pushed along with bug 24368 which basically rewrites all the tests as well.
Comment 15 Joy Nelson 2020-05-13 16:11:40 UTC
missing dependencies.  not backported to 19.11.x