From 214291eb341f59b4f9a76560c6054c5855d8db02 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 27 Jul 2023 13:20:13 +0100 Subject: [PATCH] Bug 34223: Add unit test for existing_statuses This patch adds a unit test for the 'existing_statuses' method. Signed-off-by: Martin Renvoize Signed-off-by: Laura Escamilla Signed-off-by: Pedro Amorim --- ...round-ONLY_FULL_GROUP_BY-in-Illbacke.patch | 75 ++++++++++ ...low-up-Restore-status_alias-handling.patch | 80 +++++++++++ ...-Add-unit-test-for-existing_statuses.patch | 133 ++++++++++++++++++ t/db_dependent/Koha/Illbackend.t | 111 +++++++++++++++ 4 files changed, 399 insertions(+) create mode 100644 0001-Bug-34223-Work-around-ONLY_FULL_GROUP_BY-in-Illbacke.patch create mode 100644 0002-Bug-34223-follow-up-Restore-status_alias-handling.patch create mode 100644 0003-Bug-34223-Add-unit-test-for-existing_statuses.patch create mode 100644 t/db_dependent/Koha/Illbackend.t diff --git a/0001-Bug-34223-Work-around-ONLY_FULL_GROUP_BY-in-Illbacke.patch b/0001-Bug-34223-Work-around-ONLY_FULL_GROUP_BY-in-Illbacke.patch new file mode 100644 index 0000000000..3d8ac16f73 --- /dev/null +++ b/0001-Bug-34223-Work-around-ONLY_FULL_GROUP_BY-in-Illbacke.patch @@ -0,0 +1,75 @@ +From 0b459fb5608222abd5ec7c77eaf946eb496b0af3 Mon Sep 17 00:00:00 2001 +From: Martin Renvoize +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 +Signed-off-by: Laura Escamilla +--- + 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 diff --git a/0002-Bug-34223-follow-up-Restore-status_alias-handling.patch b/0002-Bug-34223-follow-up-Restore-status_alias-handling.patch new file mode 100644 index 0000000000..ef79c9c9ea --- /dev/null +++ b/0002-Bug-34223-follow-up-Restore-status_alias-handling.patch @@ -0,0 +1,80 @@ +From 10129003cb09092a1f6ba8039ad3a7f8ee2ba4aa Mon Sep 17 00:00:00 2001 +From: Pedro Amorim +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 +Signed-off-by: Laura Escamilla +--- + 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 diff --git a/0003-Bug-34223-Add-unit-test-for-existing_statuses.patch b/0003-Bug-34223-Add-unit-test-for-existing_statuses.patch new file mode 100644 index 0000000000..bb7071dc60 --- /dev/null +++ b/0003-Bug-34223-Add-unit-test-for-existing_statuses.patch @@ -0,0 +1,133 @@ +From 4ed0166e2d0cbb63055a769ed038a063e4443e3f Mon Sep 17 00:00:00 2001 +From: Martin Renvoize +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 +Signed-off-by: Laura Escamilla +--- + 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 . ++ ++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 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 . + +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