From f12be5771ae8affd42bded82555d0349b99f9e67 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 --- Koha/Illrequest.pm | 51 ++-------- 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(+), 69 deletions(-) diff --git a/Koha/Illrequest.pm b/Koha/Illrequest.pm index 4dc64ae..0b4d71c 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 { @@ -1027,36 +1020,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; - } - # Augment the request response with the number of comments if appropriate - if ( $embed->{comments} ) { - $object->{comments} = $self->illcomments->count; - } - } - return $object; } diff --git a/Koha/Illrequest/Config.pm b/Koha/Illrequest/Config.pm index 28e80ce..617de7d 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 6e3eace..59e3f82 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 0e88d8a..349dfc9 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 3a97969..28e8cc6 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 ce570b2..348b925 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 dea0a48..9059b7c 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 @@ -325,7 +325,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.1.4