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@libriotech.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

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<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 {

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

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;

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 );
 }
 

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' ) {

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.

koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt

@@ -328,7 +328,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' %]