From 1c8952a36b3b810b7bc290a7eb7cc85cc766ed61 Mon Sep 17 00:00:00 2001 From: Andrew Isherwood 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 block in the 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 Signed-off-by: Josef Moravec Signed-off-by: Nick Clemens --- Koha/Illrequest.pm | 21 ++-- Koha/Illrequest/Config.pm | 22 ++-- Koha/REST/V1/Illrequests.pm | 103 +++++++++++++++--- about.pl | 7 ++ ill/ill-requests.pl | 9 +- .../intranet-tmpl/prog/en/modules/about.tt | 5 + .../prog/en/modules/ill/ill-requests.tt | 2 +- 7 files changed, 130 insertions(+), 39 deletions(-) diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm index 4dc64aefe5..4754b435a7 100644 --- a/Koha/Illrequest.pm +++ b/Koha/Illrequest.pm @@ -717,8 +717,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, @@ -728,13 +727,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" } =head3 get_type @@ -982,12 +976,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 ); @@ -1019,6 +1008,10 @@ sub _censor { Overloaded I 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 { 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 3a97969152..28e8cc634f 100755 --- a/ill/ill-requests.pl +++ b/ill/ill-requests.pl @@ -55,9 +55,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 ce570b254b..348b925d8f 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'. [%END %] + [% IF ill_branch_not_defined %] + Warning + The ILL module is enabled, but no 'branch' block is defined in koha-conf.xml. You must define this block before use. + + [%END %] [% IF ill_partner_code_doesnt_exist %] Warning 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 fccb257376..f962f991e9 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 @@ -328,7 +328,7 @@
- [% IF !backends_available %] + [% IF !backends_available || !has_branch %]
ILL module configuration problem. Take a look at the about page
[% ELSE %] [% INCLUDE 'ill-toolbar.inc' %] -- 2.39.5