Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl

To avoid bug like bug 16518 and to ease the readability/maintainability
of these scripts, this patch replaces the use of 'our' with 'my' to
avoid the use of global variables.

Basically the code has been moved from subroutines to the appropriate places.

Test plan:
At the intranet and OPAC sides
1/ Add items to a list
2/ Add items to a list using an existing name
3/ Add items to a list you don't have right on it (by modifying the
biblionumber in the url)
4/ At the OPAC, use the opac-addbybiblionumber.pl without being logged
in to add items to a list

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/opac/opac-addbybiblionumber.pl b/opac/opac-addbybiblionumber.pl
index a5decf7..9227fa0 100755 (executable)
--- a/opac/opac-addbybiblionumber.pl
+++ b/opac/opac-addbybiblionumber.pl
@@ -1,8 +1,7 @@
 #!/usr/bin/perl
 
-#script to provide virtualshelf management
-#
 # Copyright 2000-2002 Katipo Communications
+# Copyright 2016 Koha Development Team
 #
 # This file is part of Koha.
 #
@@ -19,8 +18,7 @@
 # You should have received a copy of the GNU General Public License
 # along with Koha; if not, see <http://www.gnu.org/licenses>.
 
-use strict;
-use warnings;
+use Modern::Perl;
 
 use CGI qw ( -utf8 );
 use C4::Biblio;
@@ -29,180 +27,139 @@ use C4::Auth;
 
 use Koha::Virtualshelves;
 
-our $query             = new CGI;
-our @biblionumber      = $query->param('biblionumber');
-our $selectedshelf     = $query->param('selectedshelf');
-our $newshelf          = $query->param('newshelf');
-our $shelfnumber       = $query->param('shelfnumber');
-our $newvirtualshelf   = $query->param('newvirtualshelf');
-our $category          = $query->param('category');
-our $authorized          = 1;
-our $errcode           = 0;
-our @biblios = ();
+my $query           = new CGI;
+my @biblionumbers   = $query->multi_param('biblionumber');
+my $selectedshelf   = $query->param('selectedshelf');
+my $newshelf        = $query->param('newshelf');
+my $shelfnumber     = $query->param('shelfnumber');
+my $newvirtualshelf = $query->param('newvirtualshelf');
+my $category        = $query->param('category');
+my ( $errcode, $authorized ) = ( 0, 1 );
+my @biblios;
 
 # if virtualshelves is disabled, leave immediately
-if ( ! C4::Context->preference('virtualshelves') ) {
+if ( !C4::Context->preference('virtualshelves') ) {
     print $query->redirect("/cgi-bin/koha/errors/404.pl");
     exit;
 }
 
-if (scalar(@biblionumber) == 1) {
-    @biblionumber = (split /\//,$biblionumber[0]);
+if ( scalar(@biblionumbers) == 1 ) {
+    @biblionumbers = ( split /\//, $biblionumbers[0] );
 }
 
-our ( $template, $loggedinuser, $cookie ) = get_template_and_user(
-    {
-        template_name   => "opac-addbybiblionumber.tt",
+my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
+    {   template_name   => "opac-addbybiblionumber.tt",
         query           => $query,
         type            => "opac",
         authnotrequired => 0,
     }
 );
 
-if( $newvirtualshelf) {
-    HandleNewVirtualShelf();
-    exit if $authorized;
-    ShowTemplate(); #error message
-}
-elsif($shelfnumber) {
-    HandleShelfNumber();
-    exit if $authorized;
-    ShowTemplate(); #error message
-}
-elsif($selectedshelf) {
-    HandleSelectedShelf();
-    LoadBib() if $authorized;
-    ShowTemplate();
-}
-else {
-    HandleSelect();
-    LoadBib() if $authorized;
-    ShowTemplate();
-}
-#end
-
-sub HandleNewVirtualShelf {
-    if ( $loggedinuser > 0 and
-        (
-            $category == 1
-                or $category == 2 and $loggedinuser>0 && C4::Context->preference('OpacAllowPublicListCreation')
-        )
-    ) {
-        my $shelf = eval {
-            Koha::Virtualshelf->new(
-                {
-                    shelfname => $newvirtualshelf,
-                    category => $category,
-                    owner => $loggedinuser,
-                }
-            )->store;
-        };
+if ($newvirtualshelf) {
+    if ($loggedinuser > 0
+        and (  $category == 1
+            or $category == 2 and $loggedinuser > 0 && C4::Context->preference('OpacAllowPublicListCreation') )
+      ) {
+        my $shelf = eval { Koha::Virtualshelf->new( { shelfname => $newvirtualshelf, category => $category, owner => $loggedinuser, } )->store; };
         if ( $@ or not $shelf ) {
+            $errcode    = 1;
             $authorized = 0;
-            $errcode = 1;
-            return;
-        }
+        } else {
+            for my $biblionumber (@biblionumbers) {
+                $shelf->add_biblio( $biblionumber, $loggedinuser );
+            }
 
-        for my $bib (@biblionumber) {
-            $shelf->add_biblio( $bib, $loggedinuser );
+            #Reload the page where you came from
+            print $query->header;
+            print "<html><meta http-equiv=\"refresh\" content=\"0\" /><body onload=\"window.opener.location.reload(true);self.close();\"></body></html>";
+            exit;
         }
-
-        #Reload the page where you came from
-        print $query->header;
-        print "<html><meta http-equiv=\"refresh\" content=\"0\" /><body onload=\"window.opener.location.reload(true);self.close();\"></body></html>";
     }
-}
-
-sub HandleShelfNumber {
+} elsif ($shelfnumber) {
     my $shelfnumber = $query->param('shelfnumber');
-    my $shelf = Koha::Virtualshelves->find( $shelfnumber );
-    if ( $shelf->can_biblios_be_added( $loggedinuser ) ) {
-        for my $bib (@biblionumber) {
-            $shelf->add_biblio( $bib, $loggedinuser );
+    my $shelf       = Koha::Virtualshelves->find($shelfnumber);
+    if ( $shelf->can_biblios_be_added($loggedinuser) ) {
+        for my $biblionumber (@biblionumbers) {
+            $shelf->add_biblio( $biblionumber, $loggedinuser );
         }
+
         #Close this page and return
         print $query->header;
         print "<html><meta http-equiv=\"refresh\" content=\"0\" /><body onload=\"self.close();\"></body></html>";
+        exit;
     } else {
-        # TODO
+        $authorized = 0;
     }
-}
-
-sub HandleSelectedShelf {
+} elsif ($selectedshelf) {
     my $shelfnumber = $query->param('selectedshelf');
-    my $shelf = Koha::Virtualshelves->find( $shelfnumber );
-    if ( $shelf->can_biblios_be_added( $loggedinuser ) ) {
+    my $shelf       = Koha::Virtualshelves->find($shelfnumber);
+    if ( $shelf->can_biblios_be_added($loggedinuser) ) {
+        $template->param(
+            singleshelf => 1,
+            shelfnumber => $shelf->shelfnumber,
+            shelfname   => $shelf->shelfname,
+        );
+    } else {
+        $authorized = 0;
+    }
+} else {
+    if ( $loggedinuser > 0 ) {
+        my $private_shelves = Koha::Virtualshelves->search(
+            {   category => 1,
+                owner    => $loggedinuser,
+            },
+            { order_by => 'shelfname' }
+        );
+        my $shelves_shared_with_me = Koha::Virtualshelves->search(
+            {   category                            => 1,
+                'virtualshelfshares.borrowernumber' => $loggedinuser,
+                -or                                 => {
+                    allow_add => 1,
+                    owner     => $loggedinuser,
+                }
+            },
+            { join => 'virtualshelfshares', }
+        );
+        my $public_shelves = Koha::Virtualshelves->search(
+            {   category => 2,
+                -or      => {
+                    allow_add => 1,
+                    owner     => $loggedinuser,
+                }
+            },
+            { order_by => 'shelfname' }
+        );
         $template->param(
-            singleshelf               => 1,
-            shelfnumber               => $shelf->shelfnumber,
-            shelfname                 => $shelf->shelfname,
+            private_shelves                => $private_shelves,
+            private_shelves_shared_with_me => $shelves_shared_with_me,
+            public_shelves                 => $public_shelves,
         );
     } else {
-        # TODO
+        $authorized = 0;
     }
 }
 
-sub HandleSelect {
-    return unless $authorized= $loggedinuser>0;
-    my $private_shelves = Koha::Virtualshelves->search(
-        {
-            category => 1,
-            owner => $loggedinuser,
-        },
-        { order_by => 'shelfname' }
-    );
-    my $shelves_shared_with_me = Koha::Virtualshelves->search(
-        {
-            category => 1,
-            'virtualshelfshares.borrowernumber' => $loggedinuser,
-            -or => {
-                allow_add => 1,
-                owner => $loggedinuser,
-            }
-        },
-        {
-            join => 'virtualshelfshares',
-        }
-    );
-    my $public_shelves= Koha::Virtualshelves->search(
-        {
-            category => 2,
-            -or => {
-                allow_add => 1,
-                owner => $loggedinuser,
+if ($authorized) {
+    for my $biblionumber (@biblionumbers) {
+        my $data = GetBiblioData($biblionumber);
+        push(
+            @biblios,
+            {   biblionumber => $biblionumber,
+                title        => $data->{'title'},
+                author       => $data->{'author'},
             }
-        },
-        { order_by => 'shelfname' }
-    );
-    $template->param (
-        private_shelves => $private_shelves,
-        private_shelves_shared_with_me => $shelves_shared_with_me,
-        public_shelves  => $public_shelves,
-    );
-}
-
-sub LoadBib {
-    for my $bib (@biblionumber) {
-        my $data = GetBiblioData( $bib );
-        push(@biblios,
-            { biblionumber => $bib,
-              title        => $data->{'title'},
-              author       => $data->{'author'},
-        } );
+        );
     }
     $template->param(
-        multiple => (scalar(@biblios) > 1),
-    total    => scalar @biblios,
-    biblios  => \@biblios,
+        multiple => ( scalar(@biblios) > 1 ),
+        total    => scalar @biblios,
+        biblios  => \@biblios,
     );
-}
 
-sub ShowTemplate {
-    $template->param (
-    newshelf => $newshelf||0,
-    authorized => $authorized,
-    errcode            => $errcode,
-    OpacAllowPublicListCreation => C4::Context->preference('OpacAllowPublicListCreation'),
+    $template->param(
+        newshelf => $newshelf || 0,
+        OpacAllowPublicListCreation => C4::Context->preference('OpacAllowPublicListCreation'),
     );
-    output_html_with_http_headers $query, $cookie, $template->output;
 }
+$template->param( authorized => $authorized, errcode => $errcode, );
+output_html_with_http_headers $query, $cookie, $template->output;

diff --git a/virtualshelves/addbybiblionumber.pl b/virtualshelves/addbybiblionumber.pl
index 71270b7..6e058e9 100755 (executable)
--- a/virtualshelves/addbybiblionumber.pl
+++ b/virtualshelves/addbybiblionumber.pl
@@ -1,9 +1,7 @@
 #!/usr/bin/perl
 
-#script to provide virtual shelf management
-#
-#
 # Copyright 2000-2002 Katipo Communications
+# Copyright 2016 Koha Development Team
 #
 # This file is part of Koha.
 #
@@ -58,8 +56,7 @@ addbybiblionumber.pl
 
 =cut
 
-use strict;
-use warnings;
+use Modern::Perl;
 
 use CGI qw ( -utf8 );
 use C4::Biblio;
@@ -68,20 +65,25 @@ use C4::Auth;
 
 use Koha::Virtualshelves;
 
-our $query           = new CGI;
-our @biblionumber    = HandleBiblioPars();
-our $shelfnumber     = $query->param('shelfnumber');
-our $newvirtualshelf = $query->param('newvirtualshelf');
-our $newshelf        = $query->param('newshelf');
-our $category        = $query->param('category');
-our $sortfield     = $query->param('sortfield');
+my $query           = new CGI;
+my $shelfnumber     = $query->param('shelfnumber');
+my $newvirtualshelf = $query->param('newvirtualshelf');
+my $newshelf        = $query->param('newshelf');
+my $category        = $query->param('category');
+my $sortfield       = $query->param('sortfield');
 my $confirmed       = $query->param('confirmed') || 0;
-our $authorized      = 1;
-our $errcode       = 0;
+my ( $errcode, $authorized ) = ( 0, 1 );
+my @biblionumbers = $query->multi_param('biblionumber');
+
+if ( @biblionumbers == 0 && $query->param('biblionumbers') ) {
+    my $str = $query->param('biblionumbers');
+    @biblionumbers = split '/', $str;
+} elsif ( @biblionumbers == 1 && $biblionumbers[0] =~ /\// ) {
+    @biblionumbers = split '/', $biblionumbers[0];
+}
 
-our ( $template, $loggedinuser, $cookie ) = get_template_and_user(
-    {
-        template_name   => "virtualshelves/addbybiblionumber.tt",
+my ( $template, $loggedinuser, $cookie ) = get_template_and_user(
+    {   template_name   => "virtualshelves/addbybiblionumber.tt",
         query           => $query,
         type            => "intranet",
         authnotrequired => 0,
@@ -89,155 +91,117 @@ our ( $template, $loggedinuser, $cookie ) = get_template_and_user(
     }
 );
 
-if( $newvirtualshelf) {
-    HandleNewVirtualShelf();
-    exit if $authorized;
-    ShowTemplate(); #error message
-}
-elsif($shelfnumber && $confirmed) {
-    HandleShelfNumber();
-    exit if $authorized;
-    ShowTemplate(); #error message
-}
-elsif($shelfnumber) { #still needs confirmation
-    HandleSelectedShelf();
-    LoadBib() if $authorized;
-    ShowTemplate();
-}
-else {
-    HandleSelect();
-    LoadBib();
-    ShowTemplate();
-}
-#end
-
-sub HandleBiblioPars {
-    my @bib= $query->multi_param('biblionumber');
-    if(@bib==0 && $query->param('biblionumbers')) {
-        my $str= $query->param('biblionumbers');
-        @bib= split '/', $str;
-    }
-    elsif(@bib==1 && $bib[0]=~/\//) {
-        @bib= split '/', $bib[0];
-    }
-    return @bib;
-}
-
-sub HandleNewVirtualShelf {
+if ($newvirtualshelf) {
     my $shelf = eval {
         Koha::Virtualshelf->new(
             {
                 shelfname => $newvirtualshelf,
-                category => $category,
+                category  => $category,
                 sortfield => $sortfield,
-                owner => $loggedinuser,
+                owner     => $loggedinuser,
             }
         )->store;
     };
     if ( $@ or not $shelf ) {
-        $authorized = 0;
         $errcode    = 1;
-        return;
-    }
+        $authorized = 0;
+    } else {
+
+        for my $biblionumber (@biblionumbers) {
+            $shelf->add_biblio( $biblionumber, $loggedinuser );
+        }
 
-    for my $bib (@biblionumber){
-        $shelf->add_biblio( $bib, $loggedinuser );
+        #Reload the page where you came from
+        print $query->header;
+        print "<html><meta http-equiv=\"refresh\" content=\"0\" /><body onload=\"window.opener.location.reload(true);self.close();\"></body></html>";
+        exit;
     }
-    #Reload the page where you came from
-    print $query->header;
-    print "<html><meta http-equiv=\"refresh\" content=\"0\" /><body onload=\"window.opener.location.reload(true);self.close();\"></body></html>";
-}
 
-sub HandleShelfNumber {
-    my $shelf = Koha::Virtualshelves->find( $shelfnumber );
-    if($authorized = $shelf->can_biblios_be_added( $loggedinuser ) ) {
-        for my $bib (@biblionumber){
-            $shelf->add_biblio( $bib, $loggedinuser );
+} elsif ( $shelfnumber && $confirmed ) {
+    my $shelf = Koha::Virtualshelves->find($shelfnumber);
+    if ( $shelf->can_biblios_be_added($loggedinuser) ) {
+        for my $biblionumber (@biblionumbers) {
+            $shelf->add_biblio( $biblionumber, $loggedinuser );
         }
+
         #Close this page and return
         print $query->header;
         print "<html><meta http-equiv=\"refresh\" content=\"0\" /><body onload=\"self.close();\"></body></html>";
+        exit;
+    } else {
+        $errcode    = 2;    #no perm
+        $authorized = 0;
     }
-    else {
-        $errcode=2; #no perm
-    }
-}
 
-sub HandleSelectedShelf {
-    my $shelf = Koha::Virtualshelves->find( $shelfnumber );
-    if($authorized = $shelf->can_biblios_be_added( $loggedinuser ) ) {
+} elsif ($shelfnumber) {    #still needs confirmation
+    my $shelf = Koha::Virtualshelves->find($shelfnumber);
+    if ( $shelf->can_biblios_be_added($loggedinuser) ) {
+
         #confirm adding to specific shelf
         $template->param(
-        singleshelf               => 1,
-        shelfnumber               => $shelf->shelfnumber,
-        shelfname                 => $shelf->shelfname,
+            singleshelf => 1,
+            shelfnumber => $shelf->shelfnumber,
+            shelfname   => $shelf->shelfname,
         );
+    } else {
+        $authorized = 0;
+        $errcode    = 2;    #no perm
     }
-    else {
-    $errcode=2; #no perm
-    }
-}
 
-sub HandleSelect {
+} else {
     my $private_shelves = Koha::Virtualshelves->search(
-        {
-            category => 1,
-            owner => $loggedinuser,
+        {   category => 1,
+            owner    => $loggedinuser,
         },
         { order_by => 'shelfname' }
     );
     my $shelves_shared_with_me = Koha::Virtualshelves->search(
-        {
-            category => 1,
+        {   category                            => 1,
             'virtualshelfshares.borrowernumber' => $loggedinuser,
-            -or => {
+            -or                                 => {
                 allow_add => 1,
-                owner => $loggedinuser,
+                owner     => $loggedinuser,
             }
         },
-        {
-            join => 'virtualshelfshares',
-        }
+        { join => 'virtualshelfshares', }
     );
-    my $public_shelves= Koha::Virtualshelves->search(
-        {
-            category => 2,
-            -or => {
+    my $public_shelves = Koha::Virtualshelves->search(
+        {   category => 2,
+            -or      => {
                 allow_add => 1,
-                owner => $loggedinuser,
+                owner     => $loggedinuser,
             }
         },
         { order_by => 'shelfname' }
     );
-    $template->param (
-        private_shelves => $private_shelves,
+    $template->param(
+        private_shelves                => $private_shelves,
         private_shelves_shared_with_me => $shelves_shared_with_me,
-        public_shelves  => $public_shelves,
+        public_shelves                 => $public_shelves,
     );
-}
 
-sub LoadBib {
-    my @biblios;
-    for my $bib (@biblionumber) {
-        my $data = GetBiblioData($bib);
-    push(@biblios,
-        { biblionumber => $bib,
-          title        => $data->{'title'},
-          author       => $data->{'author'},
-    } );
-    }
-    $template->param(
-        multiple => (scalar(@biblios) > 1),
-    total    => scalar @biblios,
-    biblios  => \@biblios,
-    );
 }
 
-sub ShowTemplate {
-    $template->param (
-    newshelf => $newshelf||0,
-    authorized => $authorized,
-    errcode            => $errcode,
+my @biblios;
+for my $biblionumber (@biblionumbers) {
+    my $data = GetBiblioData($biblionumber);
+    push(
+        @biblios,
+        {   biblionumber => $biblionumber,
+            title        => $data->{'title'},
+            author       => $data->{'author'},
+        }
     );
-    output_html_with_http_headers $query, $cookie, $template->output;
 }
+$template->param(
+    multiple => ( scalar(@biblios) > 1 ),
+    total    => scalar @biblios,
+    biblios  => \@biblios,
+);
+
+$template->param(
+    newshelf => $newshelf || 0,
+    authorized => $authorized,
+    errcode    => $errcode,
+);
+output_html_with_http_headers $query, $cookie, $template->output;