Attachment 79583 Details for Bug 20996 – Bug 20996: Remove prefix use of borrower category

[patch] Bug 20996: Remove prefix use of borrower category

Bug-20996-Remove-prefix-use-of-borrower-category.patch (text/plain), 13.21 KB, created by Magnus Enger on 2018-09-28 13:21:22 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Magnus Enger

Created: 2018-09-28 13:21:22 UTC

Size: 13.21 KB

patch

obsolete

>From 960d7e5c7b3b15646b8552181172b2287e643551 Mon Sep 17 00:00:00 2001
>From: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
>Date: Tue, 3 Jul 2018 11:36:58 +0100
>Subject: [PATCH] Bug 20996: Remove prefix use of borrower category
>
>This patch removes the potential use of borrower category as a ILL
>request ID prefix. It makes no sense. We provide the ability for a site
>to define a request prefix based on branch, there is no use case for
>using the borrower category. Add to this that the borrower for every
>request was being retrieved in order to get the category, it's a huge
>performance hit also.
>
>We also now require the <branch> block in the <interlibrary_loans> block
>and complain if it's not present. The request prefix should be defined
>in this block.
>
>This patch also improves the performance of the API request that returns all
>requests, optionally including additional data.
>
>It also deprecates the overloaded TO_JSON method and moves the request
>augmentation code into the API route's controller. It may be that we
>want to shift it out of there at some point, but it is fine where it is
>for now.
>
>Signed-off-by: Magnus Enger <magnus@enger.priv.no>
>
>https://bugs.koha-community.org/show_bug.cgi?id=20797
>---
> Koha/Illrequest.pm                                 |  47 ++--------
> Koha/Illrequest/Config.pm                          |  22 +++--
> Koha/REST/V1/Illrequests.pm                        | 103 ++++++++++++++++++---
> about.pl                                           |   7 ++
> ill/ill-requests.pl                                |   9 +-
> koha-tmpl/intranet-tmpl/prog/en/modules/about.tt   |   5 +
> .../prog/en/modules/ill/ill-requests.tt            |   2 +-
> 7 files changed, 130 insertions(+), 65 deletions(-)
>
>diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm
>index 99b97bdbe4..1f4501c135 100644
>--- a/Koha/Illrequest.pm
>+++ b/Koha/Illrequest.pm
>@@ -704,8 +704,7 @@ sub getLimits {
> =head3 getPrefix
> 
>     my $prefix = $abstract->getPrefix( {
>-        brw_cat => $brw_cat,
>-        branch  => $branch_code,
>+        branch  => $branch_code
>     } );
> 
> Return the ILL prefix as defined by our $params: either per borrower category,
>@@ -715,13 +714,8 @@ per branch or the default.
> 
> sub getPrefix {
>     my ( $self, $params ) = @_;
>-    my $brn_prefixes = $self->_config->getPrefixes('branch');
>-    my $brw_prefixes = $self->_config->getPrefixes('brw_cat');
>-
>-    return $brw_prefixes->{$params->{brw_cat}}
>-        || $brn_prefixes->{$params->{branch}}
>-        || $brw_prefixes->{default}
>-        || "";                  # "the empty prefix"
>+    my $brn_prefixes = $self->_config->getPrefixes();
>+    return $brn_prefixes->{$params->{branch}} || ""; # "the empty prefix"
> }
> 
> #### Illrequests Imports
>@@ -954,12 +948,7 @@ file.
> 
> sub id_prefix {
>     my ( $self ) = @_;
>-    my $brw = $self->patron;
>-    my $brw_cat = "dummy";
>-    $brw_cat = $brw->categorycode
>-        unless ( 'HASH' eq ref($brw) && $brw->{deleted} );
>     my $prefix = $self->getPrefix( {
>-        brw_cat => $brw_cat,
>         branch  => $self->branchcode,
>     } );
>     $prefix .= "-" if ( $prefix );
>@@ -991,6 +980,10 @@ sub _censor {
> Overloaded I<TO_JSON> method that takes care of inserting calculated values
> into the unblessed representation of the object.
> 
>+TODO: This method does nothing and is not called anywhere. However, bug 74325
>+touches it, so keeping this for now until both this and bug 74325 are merged,
>+at which point we can sort it out and remove it completely
>+
> =cut
> 
> sub TO_JSON {
>@@ -999,32 +992,6 @@ sub TO_JSON {
>     my $object = $self->SUPER::TO_JSON();
>     $object->{id_prefix} = $self->id_prefix;
> 
>-    if ( scalar (keys %$embed) ) {
>-        # Augment the request response with patron details if appropriate
>-        if ( $embed->{patron} ) {
>-            my $patron = $self->patron;
>-            $object->{patron} = {
>-                firstname  => $patron->firstname,
>-                surname    => $patron->surname,
>-                cardnumber => $patron->cardnumber
>-            };
>-        }
>-        # Augment the request response with metadata details if appropriate
>-        if ( $embed->{metadata} ) {
>-            $object->{metadata} = $self->metadata;
>-        }
>-        # Augment the request response with status details if appropriate
>-        if ( $embed->{capabilities} ) {
>-            $object->{capabilities} = $self->capabilities;
>-        }
>-        # Augment the request response with library details if appropriate
>-        if ( $embed->{library} ) {
>-            $object->{library} = Koha::Libraries->find(
>-                $self->branchcode
>-            )->TO_JSON;
>-        }
>-    }
>-
>     return $object;
> }
> 
>diff --git a/Koha/Illrequest/Config.pm b/Koha/Illrequest/Config.pm
>index 28e80ce0f6..617de7df0f 100644
>--- a/Koha/Illrequest/Config.pm
>+++ b/Koha/Illrequest/Config.pm
>@@ -117,6 +117,17 @@ sub available_backends {
>     return \@backends;
> }
> 
>+=head has_branch
>+
>+Return whether a 'branch' block is defined
>+
>+=cut
>+
>+sub has_branch {
>+    my ( $self ) = @_;
>+    return $self->{configuration}->{raw_config}->{branch};
>+}
>+
> =head3 partner_code
> 
>     $partner_code = $config->partner_code($new_code);
>@@ -150,18 +161,15 @@ sub limits {
> 
> =head3 getPrefixes
> 
>-    my $prefixes = $config->getPrefixes('brw_cat' | 'branch');
>+    my $prefixes = $config->getPrefixes();
> 
>-Return the prefix for ILLs defined by our config.
>+Return the branch prefix for ILLs defined by our config.
> 
> =cut
> 
> sub getPrefixes {
>-    my ( $self, $type ) = @_;
>-    die "Unexpected type." unless ( $type eq 'brw_cat' || $type eq 'branch' );
>-    my $values = $self->{configuration}->{prefixes}->{$type};
>-    $values->{default} = $self->{configuration}->{prefixes}->{default};
>-    return $values;
>+    my ( $self ) = @_;
>+    return $self->{configuration}->{prefixes}->{branch};
> }
> 
> =head3 getLimitRules
>diff --git a/Koha/REST/V1/Illrequests.pm b/Koha/REST/V1/Illrequests.pm
>index 6e3eace01d..59e3f82a44 100644
>--- a/Koha/REST/V1/Illrequests.pm
>+++ b/Koha/REST/V1/Illrequests.pm
>@@ -20,6 +20,9 @@ use Modern::Perl;
> use Mojo::Base 'Mojolicious::Controller';
> 
> use Koha::Illrequests;
>+use Koha::Illrequestattributes;
>+use Koha::Libraries;
>+use Koha::Patrons;
> use Koha::Libraries;
> 
> =head1 NAME
>@@ -38,34 +41,104 @@ sub list {
>     my $c = shift->openapi->valid_input or return;
> 
>     my $args = $c->req->params->to_hash // {};
>-    my $filter;
>-    my $output = [];
> 
>     # Create a hash where all keys are embedded values
>     # Enables easy checking
>     my %embed;
>+    my $args_arr = (ref $args->{embed} eq 'ARRAY') ? $args->{embed} : [ $args->{embed} ];
>     if (defined $args->{embed}) {
>-        %embed = map { $_ => 1 }  @{$args->{embed}};
>+        %embed = map { $_ => 1 }  @{$args_arr};
>         delete $args->{embed};
>     }
> 
>-    for my $filter_param ( keys %$args ) {
>-        my @values = split(/,/, $args->{$filter_param});
>-        $filter->{$filter_param} = \@values;
>+    my $requests = Koha::Illrequests->unblessed;
>+
>+    # Identify patrons & branches that
>+    # we're going to need and get them
>+    my $to_fetch = {};
>+    $to_fetch->{patrons} = {} if $embed{patron};
>+    $to_fetch->{branches} = {} if $embed{library};
>+    $to_fetch->{capabilities} = {} if $embed{capabilities};
>+    foreach my $req(@{$requests}) {
>+        $to_fetch->{patrons}->{$req->{borrowernumber}} = 1 if $embed{patron};
>+        $to_fetch->{branches}->{$req->{branchcode}} = 1 if $embed{library};
>+        $to_fetch->{capabilities}->{$req->{backend}} = 1 if $embed{capabilities};
>     }
> 
>-    my $requests = Koha::Illrequests->search($filter);
>+    # Fetch the patrons we need
>+    my $patron_arr = [];
>+    if ($embed{patron}) {
>+        my @patron_ids = keys %{$to_fetch->{patrons}};
>+        if (scalar @patron_ids > 0) {
>+            my $where = {
>+                borrowernumber => { -in => \@patron_ids }
>+            };
>+            $patron_arr = Koha::Patrons->search($where)->unblessed;
>+        }
>+    }
> 
>-    if ( scalar (keys %embed) )
>-    {
>-        # Need to embed stuff
>-        my @results = map { $_->TO_JSON(\%embed) } $requests->as_list;
>-        return $c->render( status => 200, openapi => \@results );
>+    # Fetch the branches we need
>+    my $branch_arr = [];
>+    if ($embed{library}) {
>+        my @branchcodes = keys %{$to_fetch->{branches}};
>+        if (scalar @branchcodes > 0) {
>+            my $where = {
>+                branchcode => { -in => \@branchcodes }
>+            };
>+            $branch_arr = Koha::Libraries->search($where)->unblessed;
>+        }
>     }
>-    else
>-    {
>-        return $c->render( status => 200, openapi => $requests );
>+
>+    # Fetch the capabilities we need
>+    if ($embed{capabilities}) {
>+        my @backends = keys %{$to_fetch->{capabilities}};
>+        if (scalar @backends > 0) {
>+            foreach my $bc(@backends) {
>+                my $backend = Koha::Illrequest->new->load_backend($bc);
>+                $to_fetch->{$bc} = $backend->capabilities;
>+            }
>+        }
>     }
>+
>+
>+    # Now we've got all associated users and branches,
>+    # we can augment the request objects
>+    foreach my $req(@{$requests}) {
>+        my $r = Koha::Illrequests->new->find($req->{illrequest_id});
>+        $req->{id_prefix} = $r->id_prefix;
>+        foreach my $p(@{$patron_arr}) {
>+            if ($p->{borrowernumber} == $req->{borrowernumber}) {
>+                $req->{patron} = {
>+                    firstname  => $p->{firstname},
>+                    surname    => $p->{surname},
>+                    cardnumber => $p->{cardnumber}
>+                };
>+                last;
>+            }
>+        }
>+        foreach my $b(@{$branch_arr}) {
>+            if ($b->{branchcode} eq $req->{branchcode}) {
>+                $req->{library} = $b;
>+                last;
>+            }
>+        }
>+        if ($embed{metadata}) {
>+            my $metadata = Koha::Illrequestattributes->search(
>+                { illrequest_id => $req->{illrequest_id} },
>+                { columns => [qw/type value/] }
>+            )->unblessed;
>+            my $meta_hash = {};
>+            foreach my $meta(@{$metadata}) {
>+                $meta_hash->{$meta->{type}} = $meta->{value};
>+            }
>+            $req->{metadata} = $meta_hash;
>+        }
>+        if ($embed{capabilities}) {
>+            $req->{capabilities} = $to_fetch->{$req->{backend}};
>+        }
>+    }
>+
>+    return $c->render( status => 200, openapi => $requests );
> }
> 
> 1;
>diff --git a/about.pl b/about.pl
>index 0e88d8a0d0..349dfc9668 100755
>--- a/about.pl
>+++ b/about.pl
>@@ -282,6 +282,13 @@ if ( C4::Context->preference('ILLModule') ) {
>         $warnILLConfiguration = 1;
>     }
> 
>+
>+    if ( !$ill_config_from_file->{branch} ) {
>+        # branch not defined
>+        $template->param( ill_branch_not_defined => 1 );
>+        $warnILLConfiguration = 1;
>+    }
>+
>     $template->param( warnILLConfiguration => $warnILLConfiguration );
> }
> 
>diff --git a/ill/ill-requests.pl b/ill/ill-requests.pl
>index 47cfc97050..6b7b76ace5 100755
>--- a/ill/ill-requests.pl
>+++ b/ill/ill-requests.pl
>@@ -53,9 +53,14 @@ my ( $template, $patronnumber, $cookie ) = get_template_and_user( {
> } );
> 
> # Are we able to actually work?
>-my $backends = Koha::Illrequest::Config->new->available_backends;
>+my $cfg = Koha::Illrequest::Config->new;
>+my $backends = $cfg->available_backends;
>+my $has_branch = $cfg->has_branch;
> my $backends_available = ( scalar @{$backends} > 0 );
>-$template->param( backends_available => $backends_available );
>+$template->param(
>+    backends_available => $backends_available,
>+    has_branch         => $has_branch
>+);
> 
> if ( $backends_available ) {
>     if ( $op eq 'illview' ) {
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/about.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
>index 49d521043f..8353e549f4 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/about.tt
>@@ -289,6 +289,11 @@
>                     The ILL module is enabled, but no 'partner_code' defined in koha-conf.xml. Falling back to the hardcoded 'ILLLIBS'.
>                     </td></tr>
>                   [%END %]
>+                  [% IF ill_branch_not_defined %]
>+                    <tr><th scope="row"><b>Warning</b> </th><td>
>+                    The ILL module is enabled, but no 'branch' block is defined in koha-conf.xml. You must define this block before use.
>+                    </td></tr>
>+                  [%END %]
>                   [% IF ill_partner_code_doesnt_exist %]
>                     <tr><th scope="row"><b>Warning</b> </th><td>
>                     The ILL module is enabled, but the configured 'partner_code' ([% ill_partner_code_doesnt_exist | html %]) is not defined on the system.
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
>index 8bc9268364..19f4289a95 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
>@@ -301,7 +301,7 @@
>     <div id="bd">
>         <div id="yui-main">
>             <div id="interlibraryloans" class="yui-b">
>-        [% IF !backends_available %]
>+        [% IF !backends_available || !has_branch %]
>             <div class="dialog message">ILL module configuration problem. Take a look at the <a href="/cgi-bin/koha/about.pl#sysinfo">about page</a></div>
>         [% ELSE %]
>                 [% INCLUDE 'ill-toolbar.inc' %]
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 20996: 76687 | 76688 | 76705 | 77100 | 79379 | 79576 | 79583 | 79584 | 79585 | 79586 | 79587 | 79588 | 79589 | 79590 | 79591 | 79592 | 81715 | 81716 | 81717 | 81718 | 81719 | 81720 | 81747 | 81748 | 81749 | 81750 | 81751 | 81752 | 81753 | 81755 | 81756 | 81757 | 81758 | 81759 | 81760 | 81761 | 81908 | 81909 | 81928 | 81967 | 81994 | 81996 | 81997 | 81998