From 960d7e5c7b3b15646b8552181172b2287e643551 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 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 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'. [%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 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 @@
- [% 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.11.0