Bug 20351: Shortcut serials scripts if a blocking error appeared

The idea of output_and_exit_if_error (added by bug 18403) is to make sure
parameters are valid before executing the script.
If not (old or broken URLs), we shortcut everything coming next to display a
generic error ("object does not exist", "you do not have permission to do that", etc.)

This bug report fixes the scripts under serials/*.

Test plan:
Hit the script under the serials directory with an invalid subscriptionid parameter
and confirm you get an error instead of the normal view with empty values.

The goal is not to be exhaustive during the first iteration, but at least to fix
the most common views.

For instance:
/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=XXX
/cgi-bin/koha/serials/serials-collection.pl?subscriptionid=XXX
/cgi-bin/koha/serials/routing.pl?subscriptionid=XXX&op=new
/cgi-bin/koha/serials/subscription-add.pl?op=modify&subscriptionid=XXx
/cgi-bin/koha/serials/subscription-add.pl?op=dup&subscriptionid=XXX

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

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

diff --git a/C4/Serials.pm b/C4/Serials.pm
index 0eb699f..91498ee 100644 (file)
--- a/C4/Serials.pm
+++ b/C4/Serials.pm
@@ -269,6 +269,8 @@ sub GetSubscription {
     $sth->execute($subscriptionid);
     my $subscription = $sth->fetchrow_hashref;
 
+    return unless $subscription;
+
     $subscription->{cannotedit} = not can_edit_subscription( $subscription );
 
     # Add additional fields to the subscription into a new key "additional_fields"

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/blocking_errors.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/blocking_errors.inc
index c91c252..e803427 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/blocking_errors.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/blocking_errors.inc
@@ -7,6 +7,8 @@
         <div class="dialog message">This bibliographic record does not exist.</div>
     [% CASE 'unknown_item' %]
         <div class="dialog message">This item does not exist.</div>
+    [% CASE 'unknown_subscription' %]
+        <div class="dialog message">This subscription does not exist.</div>
     [% CASE %][% blocking_error | html %]
     [% END %]
 

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
index 90af448..e28b63d 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/routing.tt
@@ -7,7 +7,14 @@
 [% INCLUDE 'header.inc' %]
 [% INCLUDE 'serials-search.inc' %]
 
-<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/serials/serials-home.pl">Serials</a> &rsaquo; <a href="/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=[% subscriptionid | html %]"><i>[% title | html %]</i></a> &rsaquo; [% IF ( op ) %]Create routing list[% ELSE %]Edit routing list[% END %]</div>
+<div id="breadcrumbs">
+    <a href="/cgi-bin/koha/mainpage.pl">Home</a>
+    &rsaquo; <a href="/cgi-bin/koha/serials/serials-home.pl">Serials</a>
+    [% UNLESS blocking_error %]
+        &rsaquo; <a href="/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=[% subscriptionid | uri %]"><i>[% title | html %]</i></a>
+        &rsaquo; [% IF ( op ) %]Create routing list[% ELSE %]Edit routing list[% END %]
+    [% END %]
+</div>
 
 <div id="doc3" class="yui-t2">
    
@@ -15,6 +22,7 @@
        <div id="yui-main">
        <div class="yui-b">
 
+[% INCLUDE 'blocking_errors.inc' %]
 
 [% IF ( op ) %]
 <h1>Create routing list for <i>[% title | html %]</i></h1>

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
index 7f1586c..edab9b3 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
@@ -18,7 +18,14 @@ fieldset.rows li.radio { width: 100%; } /* override staff-global.css */
 [% INCLUDE 'header.inc' %]
 [% INCLUDE 'serials-search.inc' %]
 
-<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/serials/serials-home.pl">Serials</a> &rsaquo; [% IF ( modify ) %]<a href="/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=[% subscriptionid | html %]"><i>[% bibliotitle | html %]</i></a> &rsaquo; Modify subscription[% ELSE %]New subscription[% END %]</div>
+<div id="breadcrumbs">
+    <a href="/cgi-bin/koha/mainpage.pl">Home</a>
+    &rsaquo; <a href="/cgi-bin/koha/serials/serials-home.pl">Serials</a>
+    [% UNLESS blocking_error %]
+        &rsaquo; [% IF ( modify ) %]<a href="/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=[% subscriptionid | uri %]"><i>[% bibliotitle | html %]</i></a> &rsaquo; Modify subscription[% ELSE %]New subscription[% END %]
+    [% END %]
+</div>
+[% INCLUDE 'blocking_errors.inc' %]
 
 <div class="main container-fluid">
     <div class="row">

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
index 5818cf1..d9f1732 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt
@@ -17,7 +17,11 @@
 [% INCLUDE 'header.inc' %]
 [% INCLUDE 'serials-search.inc' %]
 
-<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/serials/serials-home.pl">Serials</a> &rsaquo; Details for subscription #[% subscriptionid | html %]</div>
+<div id="breadcrumbs">
+    <a href="/cgi-bin/koha/mainpage.pl">Home</a>
+    &rsaquo; <a href="/cgi-bin/koha/serials/serials-home.pl">Serials</a>
+    [% UNLESS blocking_error %]&rsaquo; Details for subscription #[% subscriptionid | html %][% END %]
+</div>
 
 <div id="doc3" class="yui-t2">
    

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt
index a976dac..fcfbd26 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-renew.tt
@@ -11,6 +11,8 @@
 <body id="ser_subscription-renew" class="ser">
     <div class="container-fluid">
 
+[% INCLUDE 'blocking_errors.inc' %]
+
 [% IF op == 'renew' OR op =='multi_renew' %]
     [% IF op == 'renew' %]
         <span>Subscription renewed.<span>

diff --git a/serials/routing.pl b/serials/routing.pl
index c14205a..8d893f6 100755 (executable)
--- a/serials/routing.pl
+++ b/serials/routing.pl
@@ -61,6 +61,11 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
     }
 );
 
+my $subs = GetSubscription($subscriptionid);
+
+output_and_exit( $query, $cookie, $template, 'unknown_subscription')
+    unless $subs;
+
 if($op eq 'delete'){
     delroutingmember($routingid,$subscriptionid);
 }
@@ -76,7 +81,7 @@ if($op eq 'save'){
 }
 
 my @routinglist = getroutinglist($subscriptionid);
-my $subs = GetSubscription($subscriptionid);
+
 my ($count,@serials) = GetSerials($subscriptionid);
 my $serialdates = GetLatestSerials($subscriptionid,$count);
 

diff --git a/serials/serials-collection.pl b/serials/serials-collection.pl
index a4fd91d..e8735b3 100755 (executable)
--- a/serials/serials-collection.pl
+++ b/serials/serials-collection.pl
@@ -104,6 +104,7 @@ if($op eq 'gennext' && @subscriptionid){
         last if HasSubscriptionExpired($subscriptionid) > 0;
     }
     print $query->redirect('/cgi-bin/koha/serials/serials-collection.pl?subscriptionid='.$subscriptionid);
+    exit;
 }
 
 my $subscriptioncount;
@@ -113,6 +114,7 @@ if (@subscriptionid){
    my $closed = 0;
    foreach my $subscriptionid (@subscriptionid){
     my $subs= GetSubscription($subscriptionid);
+    next unless $subs;
     $closed = 1 if $subs->{closed};
 
     $subs->{opacnote}     =~ s/\n/\<br\/\>/g;
@@ -134,6 +136,9 @@ if (@subscriptionid){
     my $tmpsubscription= GetFullSubscription($subscriptionid);
     @subscriptioninformation=(@$tmpsubscription,@subscriptioninformation);
   }
+
+  output_and_exit( $query, $cookie, $template, 'unknown_subscription') unless @subscriptioninformation;
+
   $template->param(closed => $closed);
   $subscriptions=PrepareSerialsData(\@subscriptioninformation);
   $subscriptioncount = CountSubscriptionFromBiblionumber($subscriptiondescs->[0]{'biblionumber'});

diff --git a/serials/serials-edit.pl b/serials/serials-edit.pl
index 928ff47..33e11ad 100755 (executable)
--- a/serials/serials-edit.pl
+++ b/serials/serials-edit.pl
@@ -114,6 +114,7 @@ unless ( @serialids ) {
     $string =~ s/,$//;
 
     print $query->redirect($string);
+    exit;
 }
 
 my ( $template, $loggedinuser, $cookie ) = get_template_and_user(

diff --git a/serials/subscription-add.pl b/serials/subscription-add.pl
index bb9e8cf..fb7e797 100755 (executable)
--- a/serials/subscription-add.pl
+++ b/serials/subscription-add.pl
@@ -69,6 +69,9 @@ if ($op eq 'modify' || $op eq 'dup' || $op eq 'modsubscription') {
     my $subscriptionid = $query->param('subscriptionid');
     $subs = GetSubscription($subscriptionid);
 
+    output_and_exit( $query, $cookie, $template, 'unknown_subscription')
+        unless $subs;
+
     ## FIXME : Check rights to edit if mod. Could/Should display an error message.
     if ($subs->{'cannotedit'} && $op eq 'modify'){
       carp "Attempt to modify subscription $subscriptionid by ".C4::Context->userenv->{'id'}." not allowed";

diff --git a/serials/subscription-detail.pl b/serials/subscription-detail.pl
index cc7a5d2..4eefe18 100755 (executable)
--- a/serials/subscription-detail.pl
+++ b/serials/subscription-detail.pl
@@ -62,6 +62,10 @@ my ($template, $loggedinuser, $cookie)
 
 
 my $subs = GetSubscription($subscriptionid);
+
+output_and_exit( $query, $cookie, $template, 'unknown_subscription')
+    unless $subs;
+
 $subs->{enddate} ||= GetExpirationDate($subscriptionid);
 
 my ($totalissues,@serialslist) = GetSerials($subscriptionid);

diff --git a/serials/subscription-renew.pl b/serials/subscription-renew.pl
index 836e612..ccfbad2 100755 (executable)
--- a/serials/subscription-renew.pl
+++ b/serials/subscription-renew.pl
@@ -75,6 +75,9 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
 if ( $op eq "renew" ) {
     # Do not use this script with op=renew and @subscriptionids > 1!
     my $subscriptionid = $subscriptionids[0];
+    # Make sure the subscription exists
+    my $subscription = GetSubscription( $subscriptionid );
+    output_and_exit( $query, $cookie, $template, 'unknown_subscription') unless $subscription;
     my $startdate = output_pref( { str => scalar $query->param('startdate'), dateonly => 1, dateformat => 'iso' } );
     ReNewSubscription(
         $subscriptionid, $loggedinuser,
@@ -95,6 +98,7 @@ if ( $op eq "renew" ) {
 } else {
     my $subscriptionid = $subscriptionids[0];
     my $subscription = GetSubscription($subscriptionid);
+    output_and_exit( $query, $cookie, $template, 'unknown_subscription') unless $subscription;
     if ($subscription->{'cannotedit'}){
       carp "Attempt to renew subscription $subscriptionid by ".C4::Context->userenv->{'id'}." not allowed";
       print $query->redirect("/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=$subscriptionid");