From 0b459fb5608222abd5ec7c77eaf946eb496b0af3 Mon Sep 17 00:00:00 2001

From: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Date: Mon, 10 Jul 2023 17:03:34 +0000

Subject: [PATCH 1/3] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend

This patch re-works the query in the existing_statuses method to remove

the FIXME and improve performance.  We pass an SQL literal into the

query to make it explicit which illrequest_id we're looking for (which

dampens the SQL warning) even though we really don't mind which request

is returned here.

Test plan:

The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only).

1) On an empty k-t-d, run:

bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh)

2) Pet a cat

3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters

4) Notice how the status filter takes a while (3-5 secs) to load

5) Apply patch and koha-plack --restart kohadev

6) Repeat 3, notice how the status filter now loads fast

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>

---

 Koha/Illbackend.pm | 25 ++++++++++++-------------

 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/Koha/Illbackend.pm b/Koha/Illbackend.pm

index 8a6be8a7a6..cb27e21154 100644

--- a/Koha/Illbackend.pm

+++ b/Koha/Illbackend.pm

@@ -48,16 +48,19 @@ Return a list of existing ILL statuses

 sub existing_statuses {

     my ( $self, $backend_id ) = @_;

-    #FIXME: Currently fetching all requests, it'd be great if we could fetch distinct(status).

-    # Even doing it with distinct status, we need the ILL request object, so that strings_map works and

-    # the ILL request returns the correct status and info respective to its backend.

+    # NOTE: This query fetches all distinct status's found in the database for this backend.

+    # We need the 'status' field for obvious reasons, the 'backend' field is required to not

+    # throw 'Koha::Exceptions::Ill::InvalidBackendId' when we're converting to a Koha object.

+    # Finally, to get around 'ONLY_FULL_GROUP_BY', we have to be explicit about which

+    # 'request_id' we want to return, hense the 'MAX' call.

     my $ill_requests = Koha::Illrequests->search(

-            {backend => $backend_id},

-            # {

-            #     columns => [ qw/status/ ],

-            #     group_by => [ qw/status/ ],

-            # }

-        );

+        { backend => $backend_id },

+        {

+            select   => [ 'status', \'MAX(illrequest_id)', 'backend' ],

+            as       => [qw/ status illrequest_id backend /],

+            group_by => [qw/status backend/],

+        }

+    );

     my @data;

     while (my $request = $ill_requests->next) {

@@ -74,10 +77,6 @@ sub existing_statuses {

         }

     }

-    # Remove duplicate statuses

-    my %seen;

-    @data =  grep { my $e = $_; my $key = join '___', map { $e->{$_}; } sort keys %$_;!$seen{$key}++ } @data;

-

     return \@data;

 }

--

2.30.2

From 10129003cb09092a1f6ba8039ad3a7f8ee2ba4aa Mon Sep 17 00:00:00 2001

From: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Date: Thu, 27 Jul 2023 12:25:20 +0100

Subject: [PATCH 2/3] Bug 34223: (follow-up) Restore status_alias handling

By reducing the original call to all distinct 'status' we also removed

the status_alias combinations. This patch adds an additional distinct

query to fetch all 'status_alias' and add them to the returned data

structure.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>

---

 Koha/Illbackend.pm | 41 +++++++++++++++++++++++++++++++----------

 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/Koha/Illbackend.pm b/Koha/Illbackend.pm

index cb27e21154..df29e970a5 100644

--- a/Koha/Illbackend.pm

+++ b/Koha/Illbackend.pm

@@ -48,6 +48,8 @@ Return a list of existing ILL statuses

 sub existing_statuses {

     my ( $self, $backend_id ) = @_;

+    my @data;

+

     # NOTE: This query fetches all distinct status's found in the database for this backend.

     # We need the 'status' field for obvious reasons, the 'backend' field is required to not

     # throw 'Koha::Exceptions::Ill::InvalidBackendId' when we're converting to a Koha object.

@@ -61,19 +63,38 @@ sub existing_statuses {

             group_by => [qw/status backend/],

         }

     );

+    while ( my $request = $ill_requests->next ) {

+        my $status_data = $request->strings_map;

-    my @data;

-    while (my $request = $ill_requests->next) {

+        if ( $status_data->{status} ) {

+            push @data, {

+                  $status_data->{status}->{str}  ? ( str => $status_data->{status}->{str} )

+                : $status_data->{status}->{code} ? ( str => $status_data->{status}->{code} )

+                : (),

+                $status_data->{status}->{code} ? ( code => $status_data->{status}->{code} ) : (),

+            };

+        }

+    }

+

+    # Now do the same to get all status_aliases

+    $ill_requests = Koha::Illrequests->search(

+        { backend => $backend_id },

+        {

+            select   => [ 'status_alias', \'MAX(illrequest_id)', 'backend' ],

+            as       => [qw/ status_alias illrequest_id backend /],

+            group_by => [qw/status_alias backend/],

+        }

+    );

+    while ( my $request = $ill_requests->next ) {

         my $status_data = $request->strings_map;

-        foreach my $status_class ( qw(status_alias status) ){

-            if ($status_data->{$status_class}){

-                push @data, {

-                    $status_data->{$status_class}->{str} ? (str => $status_data->{$status_class}->{str}) :

-                        $status_data->{$status_class}->{code} ? (str => $status_data->{$status_class}->{code}) : (),

-                    $status_data->{$status_class}->{code} ? (code => $status_data->{$status_class}->{code}) : (),

-                }

-            }

+        if ( $status_data->{status_alias} ) {

+            push @data, {

+                  $status_data->{status_alias}->{str}  ? ( str => $status_data->{status_alias}->{str} )

+                : $status_data->{status_alias}->{code} ? ( str => $status_data->{status_alias}->{code} )

+                : (),

+                $status_data->{status_alias}->{code} ? ( code => $status_data->{status_alias}->{code} ) : (),

+            };

         }

     }

--

2.30.2

From 4ed0166e2d0cbb63055a769ed038a063e4443e3f Mon Sep 17 00:00:00 2001

From: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Date: Thu, 27 Jul 2023 13:20:13 +0100

Subject: [PATCH 3/3] Bug 34223: Add unit test for existing_statuses

This patch adds a unit test for the 'existing_statuses' method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>

---

 t/db_dependent/Koha/Illbackend.t | 111 +++++++++++++++++++++++++++++++

 1 file changed, 111 insertions(+)

 create mode 100644 t/db_dependent/Koha/Illbackend.t

diff --git a/t/db_dependent/Koha/Illbackend.t b/t/db_dependent/Koha/Illbackend.t

new file mode 100644

index 0000000000..d0ae26a4ea

--- /dev/null

+++ b/t/db_dependent/Koha/Illbackend.t

@@ -0,0 +1,111 @@

+#!/usr/bin/perl

+

+# Copyright 2023 Koha Development team

+#

+# This file is part of Koha

+#

+# Koha is free software; you can redistribute it and/or modify it

+# under the terms of the GNU General Public License as published by

+# the Free Software Foundation; either version 3 of the License, or

+# (at your option) any later version.

+#

+# Koha is distributed in the hope that it will be useful, but

+# WITHOUT ANY WARRANTY; without even the implied warranty of

+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

+# GNU General Public License for more details.

+#

+# You should have received a copy of the GNU General Public License

+# along with Koha; if not, see <http://www.gnu.org/licenses>.

+

+use Modern::Perl;

+

+use Test::More tests => 1;

+

+use Koha::Illbackend;

+

+use t::lib::TestBuilder;

+use t::lib::Mocks;

+

+my $builder = t::lib::TestBuilder->new;

+my $schema  = Koha::Database->new->schema;

+

+subtest 'existing_statuses() tests' => sub {

+

+    plan tests => 2;

+

+    $schema->storage->txn_begin;

+    Koha::Illrequests->search->delete;

+

+    my $alias = $builder->build_object(

+        {

+            class => 'Koha::AuthorisedValues',

+            value => {

+                category         => 'ILL_STATUS_ALIAS',

+                authorised_value => 'BOB',

+                lib              => "Bob is the best status"

+            }

+        }

+    );

+

+    my $req = $builder->build_object(

+        {

+            class => 'Koha::Illrequests',

+            value => {

+                status       => 'REQ',

+                status_alias => undef,

+                biblio_id    => undef,

+                backend      => 'FreeForm'

+            }

+        }

+    );

+    my $chk = $builder->build_object(

+        {

+            class => 'Koha::Illrequests',

+            value => {

+                status       => 'CHK',

+                status_alias => undef,

+                biblio_id    => undef,

+                backend      => 'FreeForm'

+            }

+        }

+    );

+    my $bob = $builder->build_object(

+        {

+            class => 'Koha::Illrequests',

+            value => {

+                status       => 'REQ',

+                status_alias => 'BOB',

+                biblio_id    => undef,

+                backend      => 'FreeForm'

+            }

+        }

+    );

+    my $req2 = $builder->build_object(

+        {

+            class => 'Koha::Illrequests',

+            value => {

+                status       => 'REQ',

+                status_alias => undef,

+                biblio_id    => undef,

+                backend      => 'FreeForm'

+            }

+        }

+    );

+

+    my $backend_module = Koha::Illbackend->new;

+

+    my $existing_statuses = $backend_module->existing_statuses('FreeForm');

+

+    is( @{$existing_statuses}, 3, "Return 3 unique existing statuses" );

+

+    # FIXME: Add tests to check content and order of return

+    my $expected_statuses = [

+        { code => 'CHK', str => 'Checked out' },

+        { code => 'REQ', str => 'Requested' },

+        { code => 'BOB', str => 'Bob is the best status' }

+    ];

+

+    is_deeply( $existing_statuses, $expected_statuses, 'Deep match on return' );

+

+    $schema->storage->txn_rollback;

+};

--

2.30.2