View | Details | Raw Unified | Return to bug 34223
Collapse All | Expand All

(-)a/0001-Bug-34223-Work-around-ONLY_FULL_GROUP_BY-in-Illbacke.patch (+76 lines)
Line 0 Link Here
1
From 0b459fb5608222abd5ec7c77eaf946eb496b0af3 Mon Sep 17 00:00:00 2001
2
From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
3
Date: Mon, 10 Jul 2023 17:03:34 +0000
4
Subject: [PATCH 1/3] Bug 34223: Work around ONLY_FULL_GROUP_BY in Illbackend
5
6
This patch re-works the query in the existing_statuses method to remove
7
the FIXME and improve performance.  We pass an SQL literal into the
8
query to make it explicit which illrequest_id we're looking for (which
9
dampens the SQL warning) even though we really don't mind which request
10
is returned here.
11
12
Test plan:
13
14
The following command will (hopefully) reset your ILL data and create 10k fake ILL requests (run this in DEV KTD only).
15
16
1) On an empty k-t-d, run:
17
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev-data.sh)
18
2) Pet a cat
19
3) Visit /cgi-bin/koha/ill/ill-requests.pl and select a backend on the left side filters
20
4) Notice how the status filter takes a while (3-5 secs) to load
21
5) Apply patch and koha-plack --restart kohadev
22
6) Repeat 3, notice how the status filter now loads fast
23
24
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
25
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
26
---
27
 Koha/Illbackend.pm | 25 ++++++++++++-------------
28
 1 file changed, 12 insertions(+), 13 deletions(-)
29
30
diff --git a/Koha/Illbackend.pm b/Koha/Illbackend.pm
31
index 8a6be8a7a6..cb27e21154 100644
32
--- a/Koha/Illbackend.pm
33
+++ b/Koha/Illbackend.pm
34
@@ -48,16 +48,19 @@ Return a list of existing ILL statuses
35
 sub existing_statuses {
36
     my ( $self, $backend_id ) = @_;
37
 
38
-    #FIXME: Currently fetching all requests, it'd be great if we could fetch distinct(status).
39
-    # Even doing it with distinct status, we need the ILL request object, so that strings_map works and
40
-    # the ILL request returns the correct status and info respective to its backend.
41
+    # NOTE: This query fetches all distinct status's found in the database for this backend.
42
+    # We need the 'status' field for obvious reasons, the 'backend' field is required to not
43
+    # throw 'Koha::Exceptions::Ill::InvalidBackendId' when we're converting to a Koha object.
44
+    # Finally, to get around 'ONLY_FULL_GROUP_BY', we have to be explicit about which
45
+    # 'request_id' we want to return, hense the 'MAX' call.
46
     my $ill_requests = Koha::Illrequests->search(
47
-            {backend => $backend_id},
48
-            # {
49
-            #     columns => [ qw/status/ ],
50
-            #     group_by => [ qw/status/ ],
51
-            # }
52
-        );
53
+        { backend => $backend_id },
54
+        {
55
+            select   => [ 'status', \'MAX(illrequest_id)', 'backend' ],
56
+            as       => [qw/ status illrequest_id backend /],
57
+            group_by => [qw/status backend/],
58
+        }
59
+    );
60
 
61
     my @data;
62
     while (my $request = $ill_requests->next) {
63
@@ -74,10 +77,6 @@ sub existing_statuses {
64
         }
65
     }
66
 
67
-    # Remove duplicate statuses
68
-    my %seen;
69
-    @data =  grep { my $e = $_; my $key = join '___', map { $e->{$_}; } sort keys %$_;!$seen{$key}++ } @data;
70
-
71
     return \@data;
72
 }
73
 
74
-- 
75
2.30.2
76
(-)a/0002-Bug-34223-follow-up-Restore-status_alias-handling.patch (+81 lines)
Line 0 Link Here
1
From 10129003cb09092a1f6ba8039ad3a7f8ee2ba4aa Mon Sep 17 00:00:00 2001
2
From: Pedro Amorim <pedro.amorim@ptfs-europe.com>
3
Date: Thu, 27 Jul 2023 12:25:20 +0100
4
Subject: [PATCH 2/3] Bug 34223: (follow-up) Restore status_alias handling
5
6
By reducing the original call to all distinct 'status' we also removed
7
the status_alias combinations. This patch adds an additional distinct
8
query to fetch all 'status_alias' and add them to the returned data
9
structure.
10
11
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
12
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
13
---
14
 Koha/Illbackend.pm | 41 +++++++++++++++++++++++++++++++----------
15
 1 file changed, 31 insertions(+), 10 deletions(-)
16
17
diff --git a/Koha/Illbackend.pm b/Koha/Illbackend.pm
18
index cb27e21154..df29e970a5 100644
19
--- a/Koha/Illbackend.pm
20
+++ b/Koha/Illbackend.pm
21
@@ -48,6 +48,8 @@ Return a list of existing ILL statuses
22
 sub existing_statuses {
23
     my ( $self, $backend_id ) = @_;
24
 
25
+    my @data;
26
+
27
     # NOTE: This query fetches all distinct status's found in the database for this backend.
28
     # We need the 'status' field for obvious reasons, the 'backend' field is required to not
29
     # throw 'Koha::Exceptions::Ill::InvalidBackendId' when we're converting to a Koha object.
30
@@ -61,19 +63,38 @@ sub existing_statuses {
31
             group_by => [qw/status backend/],
32
         }
33
     );
34
+    while ( my $request = $ill_requests->next ) {
35
+        my $status_data = $request->strings_map;
36
 
37
-    my @data;
38
-    while (my $request = $ill_requests->next) {
39
+        if ( $status_data->{status} ) {
40
+            push @data, {
41
+                  $status_data->{status}->{str}  ? ( str => $status_data->{status}->{str} )
42
+                : $status_data->{status}->{code} ? ( str => $status_data->{status}->{code} )
43
+                : (),
44
+                $status_data->{status}->{code} ? ( code => $status_data->{status}->{code} ) : (),
45
+            };
46
+        }
47
+    }
48
+
49
+    # Now do the same to get all status_aliases
50
+    $ill_requests = Koha::Illrequests->search(
51
+        { backend => $backend_id },
52
+        {
53
+            select   => [ 'status_alias', \'MAX(illrequest_id)', 'backend' ],
54
+            as       => [qw/ status_alias illrequest_id backend /],
55
+            group_by => [qw/status_alias backend/],
56
+        }
57
+    );
58
+    while ( my $request = $ill_requests->next ) {
59
         my $status_data = $request->strings_map;
60
 
61
-        foreach my $status_class ( qw(status_alias status) ){
62
-            if ($status_data->{$status_class}){
63
-                push @data, {
64
-                    $status_data->{$status_class}->{str} ? (str => $status_data->{$status_class}->{str}) :
65
-                        $status_data->{$status_class}->{code} ? (str => $status_data->{$status_class}->{code}) : (),
66
-                    $status_data->{$status_class}->{code} ? (code => $status_data->{$status_class}->{code}) : (),
67
-                }
68
-            }
69
+        if ( $status_data->{status_alias} ) {
70
+            push @data, {
71
+                  $status_data->{status_alias}->{str}  ? ( str => $status_data->{status_alias}->{str} )
72
+                : $status_data->{status_alias}->{code} ? ( str => $status_data->{status_alias}->{code} )
73
+                : (),
74
+                $status_data->{status_alias}->{code} ? ( code => $status_data->{status_alias}->{code} ) : (),
75
+            };
76
         }
77
     }
78
 
79
-- 
80
2.30.2
81
(-)a/0003-Bug-34223-Add-unit-test-for-existing_statuses.patch (+134 lines)
Line 0 Link Here
1
From 4ed0166e2d0cbb63055a769ed038a063e4443e3f Mon Sep 17 00:00:00 2001
2
From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
3
Date: Thu, 27 Jul 2023 13:20:13 +0100
4
Subject: [PATCH 3/3] Bug 34223: Add unit test for existing_statuses
5
6
This patch adds a unit test for the 'existing_statuses' method.
7
8
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
9
Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
10
---
11
 t/db_dependent/Koha/Illbackend.t | 111 +++++++++++++++++++++++++++++++
12
 1 file changed, 111 insertions(+)
13
 create mode 100644 t/db_dependent/Koha/Illbackend.t
14
15
diff --git a/t/db_dependent/Koha/Illbackend.t b/t/db_dependent/Koha/Illbackend.t
16
new file mode 100644
17
index 0000000000..d0ae26a4ea
18
--- /dev/null
19
+++ b/t/db_dependent/Koha/Illbackend.t
20
@@ -0,0 +1,111 @@
21
+#!/usr/bin/perl
22
+
23
+# Copyright 2023 Koha Development team
24
+#
25
+# This file is part of Koha
26
+#
27
+# Koha is free software; you can redistribute it and/or modify it
28
+# under the terms of the GNU General Public License as published by
29
+# the Free Software Foundation; either version 3 of the License, or
30
+# (at your option) any later version.
31
+#
32
+# Koha is distributed in the hope that it will be useful, but
33
+# WITHOUT ANY WARRANTY; without even the implied warranty of
34
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
35
+# GNU General Public License for more details.
36
+#
37
+# You should have received a copy of the GNU General Public License
38
+# along with Koha; if not, see <http://www.gnu.org/licenses>.
39
+
40
+use Modern::Perl;
41
+
42
+use Test::More tests => 1;
43
+
44
+use Koha::Illbackend;
45
+
46
+use t::lib::TestBuilder;
47
+use t::lib::Mocks;
48
+
49
+my $builder = t::lib::TestBuilder->new;
50
+my $schema  = Koha::Database->new->schema;
51
+
52
+subtest 'existing_statuses() tests' => sub {
53
+
54
+    plan tests => 2;
55
+
56
+    $schema->storage->txn_begin;
57
+    Koha::Illrequests->search->delete;
58
+
59
+    my $alias = $builder->build_object(
60
+        {
61
+            class => 'Koha::AuthorisedValues',
62
+            value => {
63
+                category         => 'ILL_STATUS_ALIAS',
64
+                authorised_value => 'BOB',
65
+                lib              => "Bob is the best status"
66
+            }
67
+        }
68
+    );
69
+
70
+    my $req = $builder->build_object(
71
+        {
72
+            class => 'Koha::Illrequests',
73
+            value => {
74
+                status       => 'REQ',
75
+                status_alias => undef,
76
+                biblio_id    => undef,
77
+                backend      => 'FreeForm'
78
+            }
79
+        }
80
+    );
81
+    my $chk = $builder->build_object(
82
+        {
83
+            class => 'Koha::Illrequests',
84
+            value => {
85
+                status       => 'CHK',
86
+                status_alias => undef,
87
+                biblio_id    => undef,
88
+                backend      => 'FreeForm'
89
+            }
90
+        }
91
+    );
92
+    my $bob = $builder->build_object(
93
+        {
94
+            class => 'Koha::Illrequests',
95
+            value => {
96
+                status       => 'REQ',
97
+                status_alias => 'BOB',
98
+                biblio_id    => undef,
99
+                backend      => 'FreeForm'
100
+            }
101
+        }
102
+    );
103
+    my $req2 = $builder->build_object(
104
+        {
105
+            class => 'Koha::Illrequests',
106
+            value => {
107
+                status       => 'REQ',
108
+                status_alias => undef,
109
+                biblio_id    => undef,
110
+                backend      => 'FreeForm'
111
+            }
112
+        }
113
+    );
114
+
115
+    my $backend_module = Koha::Illbackend->new;
116
+
117
+    my $existing_statuses = $backend_module->existing_statuses('FreeForm');
118
+
119
+    is( @{$existing_statuses}, 3, "Return 3 unique existing statuses" );
120
+
121
+    # FIXME: Add tests to check content and order of return
122
+    my $expected_statuses = [
123
+        { code => 'CHK', str => 'Checked out' },
124
+        { code => 'REQ', str => 'Requested' },
125
+        { code => 'BOB', str => 'Bob is the best status' }
126
+    ];
127
+
128
+    is_deeply( $existing_statuses, $expected_statuses, 'Deep match on return' );
129
+
130
+    $schema->storage->txn_rollback;
131
+};
132
-- 
133
2.30.2
134
(-)a/t/db_dependent/Koha/Illbackend.t (-1 / +111 lines)
Line 0 Link Here
0
- 
1
#!/usr/bin/perl
2
3
# Copyright 2023 Koha Development team
4
#
5
# This file is part of Koha
6
#
7
# Koha is free software; you can redistribute it and/or modify it
8
# under the terms of the GNU General Public License as published by
9
# the Free Software Foundation; either version 3 of the License, or
10
# (at your option) any later version.
11
#
12
# Koha is distributed in the hope that it will be useful, but
13
# WITHOUT ANY WARRANTY; without even the implied warranty of
14
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15
# GNU General Public License for more details.
16
#
17
# You should have received a copy of the GNU General Public License
18
# along with Koha; if not, see <http://www.gnu.org/licenses>.
19
20
use Modern::Perl;
21
22
use Test::More tests => 1;
23
24
use Koha::Illbackend;
25
26
use t::lib::TestBuilder;
27
use t::lib::Mocks;
28
29
my $builder = t::lib::TestBuilder->new;
30
my $schema  = Koha::Database->new->schema;
31
32
subtest 'existing_statuses() tests' => sub {
33
34
    plan tests => 2;
35
36
    $schema->storage->txn_begin;
37
    Koha::Illrequests->search->delete;
38
39
    my $alias = $builder->build_object(
40
        {
41
            class => 'Koha::AuthorisedValues',
42
            value => {
43
                category         => 'ILL_STATUS_ALIAS',
44
                authorised_value => 'BOB',
45
                lib              => "Bob is the best status"
46
            }
47
        }
48
    );
49
50
    my $req = $builder->build_object(
51
        {
52
            class => 'Koha::Illrequests',
53
            value => {
54
                status       => 'REQ',
55
                status_alias => undef,
56
                biblio_id    => undef,
57
                backend      => 'FreeForm'
58
            }
59
        }
60
    );
61
    my $chk = $builder->build_object(
62
        {
63
            class => 'Koha::Illrequests',
64
            value => {
65
                status       => 'CHK',
66
                status_alias => undef,
67
                biblio_id    => undef,
68
                backend      => 'FreeForm'
69
            }
70
        }
71
    );
72
    my $bob = $builder->build_object(
73
        {
74
            class => 'Koha::Illrequests',
75
            value => {
76
                status       => 'REQ',
77
                status_alias => 'BOB',
78
                biblio_id    => undef,
79
                backend      => 'FreeForm'
80
            }
81
        }
82
    );
83
    my $req2 = $builder->build_object(
84
        {
85
            class => 'Koha::Illrequests',
86
            value => {
87
                status       => 'REQ',
88
                status_alias => undef,
89
                biblio_id    => undef,
90
                backend      => 'FreeForm'
91
            }
92
        }
93
    );
94
95
    my $backend_module = Koha::Illbackend->new;
96
97
    my $existing_statuses = $backend_module->existing_statuses('FreeForm');
98
99
    is( @{$existing_statuses}, 3, "Return 3 unique existing statuses" );
100
101
    # FIXME: Add tests to check content and order of return
102
    my $expected_statuses = [
103
        { code => 'CHK', str => 'Checked out' },
104
        { code => 'REQ', str => 'Requested' },
105
        { code => 'BOB', str => 'Bob is the best status' }
106
    ];
107
108
    is_deeply( $existing_statuses, $expected_statuses, 'Deep match on return' );
109
110
    $schema->storage->txn_rollback;
111
};

Return to bug 34223