Attachment 154028 Details for Bug 34223 – Bug 34223: Add unit test for existing_statuses

[patch] Bug 34223: Add unit test for existing_statuses

Bug-34223-Add-unit-test-for-existingstatuses.patch (text/plain), 15.88 KB, created by Pedro Amorim on 2023-07-28 09:36:36 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Pedro Amorim

Created: 2023-07-28 09:36:36 UTC

Size: 15.88 KB

patch

obsolete

>From 214291eb341f59b4f9a76560c6054c5855d8db02 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] 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>
>Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
>---
> ...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 <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
>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 <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
>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 <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
>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

Actions: View | Diff | Splinter Review

Attachments on bug 34223: 153289 | 153953 | 153956 | 153957 | 153961 | 153962 | 153963 | 153968 | 153973 | 153978 | 153979 | 153980 | 153981 | 153989 | 153990 | 154026 | 154027 | 154028 | 154029 | 154030 | 154031 | 154032 | 154033 | 154068 | 154069 | 154070 | 154071 | 155280 | 155493