Bug 10386: improvements to VirtualShelves.t
authorMarcel de Rooy <m.de.rooy@rijksmuseum.nl>
Mon, 3 Jun 2013 13:27:50 +0000 (15:27 +0200)
committerGalen Charlton <gmc@esilibrary.com>
Fri, 28 Jun 2013 12:19:49 +0000 (05:19 -0700)
Most important: Does no longer delete all shelves!
Checks if there are ten borrowers for testing. But even works without them :)
When creating or modifying lists, takes name clashes into consideration.

Small change to _CheckShelfName in VirtualShelves module. Making it possible to
check a name for a list whose owner has been set to NULL. Note that a test
like field=? with undef for placeholder will not work in MySql.

Test plan:
How do you test a test? Well, you could run it on various databases..
But for real hacking, you could also add some debug lines.
I tested this by forcing 10 undefs in @borrowernumbers.
And by overwriting the return value of randomname with an existing name.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
C4/VirtualShelves.pm
t/db_dependent/VirtualShelves.t

index ed489f2..38fa0e8 100644 (file)
@@ -681,19 +681,26 @@ sub _CheckShelfName {
     my ($name, $cat, $owner, $number)= @_;
 
     my $dbh = C4::Context->dbh;
+    my @pars;
     my $query = qq(
         SELECT DISTINCT shelfnumber
         FROM   virtualshelves
         LEFT JOIN virtualshelfshares sh USING (shelfnumber)
         WHERE  shelfname=? AND shelfnumber<>?);
-    if($cat==1) {
+    if($cat==1 && defined($owner)) {
         $query.= ' AND (sh.borrowernumber=? OR owner=?) AND category=1';
+        @pars=($name, $number, $owner, $owner);
     }
-    else {
+    elsif($cat==1 && !defined($owner)) { #owner is null (exceptional)
+        $query.= ' AND owner IS NULL AND category=1';
+        @pars=($name, $number);
+    }
+    else { #public list
         $query.= ' AND category=2';
+        @pars=($name, $number);
     }
     my $sth = $dbh->prepare($query);
-    $sth->execute($cat==1? ($name, $number, $owner, $owner): ($name, $number));
+    $sth->execute(@pars);
     return $sth->rows>0? 0: 1;
 }
 
index 323cc2c..563eeca 100755 (executable)
@@ -1,12 +1,11 @@
 #!/usr/bin/perl
 
-#
 # This file is a test script for C4::VirtualShelves.pm
 # Author : Antoine Farnault, antoine@koha-fr.org
-#
+# Larger modifications by Jonathan Druart and Marcel de Rooy
 
 use Modern::Perl;
-use Test::More tests => 82;
+use Test::More tests => 81;
 use MARC::Record;
 
 use C4::Biblio qw( AddBiblio DelBiblio );
@@ -14,16 +13,16 @@ use C4::Context;
 
 # Getting some borrowers from database.
 my $dbh = C4::Context->dbh;
-my $query = q{
-    SELECT borrowernumber
-    FROM   borrowers
-    LIMIT  10
-};
-my $sth = $dbh->prepare($query);
-$sth->execute;
+my $query = q{SELECT borrowernumber FROM borrowers LIMIT 10};
+my $borr_ref=$dbh->selectall_arrayref($query);
+if(@$borr_ref==0) { #no borrowers? should not occur of course
+    $borr_ref->[0][0]=undef;
+        #but even then, we can run this robust test :)
+}
 my @borrowers;
-while(my $borrower = $sth->fetchrow){
-    push @borrowers, $borrower;
+foreach(1..10) {
+    my $t= $_> @$borr_ref ? int(rand()*@$borr_ref): $_-1; #repeat if not enough
+    push @borrowers, $borr_ref->[$t][0];
 }
 
 # Creating some biblios
@@ -34,20 +33,6 @@ foreach(0..9) {
     push @biblionumbers, $biblionumber;
 }
 
-# FIXME Why are you deleting my shelves? See bug 10386.
-my $delete_virtualshelf = q{
-    DELETE FROM  virtualshelves WHERE 1
-};
-my $delete_virtualshelfcontent = q{
-    DELETE  FROM  virtualshelfcontents WHERE 1
-};
-
-$sth = $dbh->prepare($delete_virtualshelf);
-$sth->execute;
-$sth = $dbh->prepare($delete_virtualshelfcontent);
-$sth->execute;
-# ---
-
 #----------------------------------------------------------------------#
 #
 #           TESTS START HERE
@@ -59,33 +44,61 @@ use_ok('C4::VirtualShelves');
 #-----------------------TEST AddShelf function------------------------#
 # usage : $shelfnumber = &AddShelf( $shelfname, $owner, $category);
 
-# creating 10 good shelves.
+# creating shelves (could be <10 when names are not unique)
 my @shelves;
-for(my $i=0; $i<10;$i++){
-     my $ShelfNumber = AddShelf(
-    {shelfname=>"Shelf_".$i, category=>int(rand(2))+1 }, $borrowers[$i] );
-     die "test Not ok, remove some shelves before" if ($ShelfNumber == -1);
-     ok($ShelfNumber > -1, "created shelf");   # Shelf creation successful;
-     push @shelves, $ShelfNumber if $ShelfNumber > -1;
+for(my $i=0; $i<10;$i++) {
+    my $name= randomname();
+    my $catg= int(rand(2))+1;
+    my $ShelfNumber= AddShelf(
+        {
+            shelfname => $name,
+            category  => $catg,
+        },
+        $borrowers[$i]);
+
+    if($ShelfNumber>-1) {
+        ok($ShelfNumber > -1, "created shelf");   # Shelf creation successful;
+    }
+    else {
+        my $t= C4::VirtualShelves::_CheckShelfName(
+            $name, $catg, $borrowers[$i], 0);
+        ok($t==0, "Name clash expected on shelf creation");
+    }
+    push @shelves, {
+        number => $ShelfNumber,
+        name   => $name,
+        catg   => $catg,
+        owner  => $borrowers[$i],
+    }; #also push the errors
 }
 
-ok(10 == scalar @shelves, 'created 10 lists'); # 10 shelves in @shelves;
-
-# try to create some shelf which already exists.
+# try to create shelves with duplicate names
 for(my $i=0;$i<10;$i++){
-    my @shlf=GetShelf($shelves[$i]);
-
-    # FIXME This test still needs some attention
-    # A shelf name is not per se unique ! See report 10386
-    #temporary hack: Original test only for public list
-    if( $shlf[3]==2 ) {
-        my $badNumShelf= AddShelf(
-            {shelfname=>"Shelf_".$i, category => 2}, $borrowers[$i]);
+    if($shelves[$i]->{number}<0) {
+        ok(1, 'skip duplicate test for earlier name clash');
+        next;
+    }
+    my @shlf=GetShelf($shelves[$i]->{number}); #number, name, owner, catg, ...
+
+    # A shelf name is not per se unique!
+    if( $shlf[3]==2 ) { #public list: try to create with same name
+        my $badNumShelf= AddShelf( {
+            shelfname=> $shelves[$i]->{name},
+            category => 2
+        }, $borrowers[$i]);
         ok(-1==$badNumShelf, 'do not create public lists with duplicate names');
             #AddShelf returns -1 if name already exist.
+        DelShelf($badNumShelf) if $badNumShelf>-1; #delete if went wrong..
     }
-    else {
-        ok(1==1, "This trivial test cannot fail :)"); #leave count intact
+    else { #private list, try to add another one for SAME user (owner)
+        my $badNumShelf= defined($shlf[2])? AddShelf(
+            {
+                shelfname=> $shelves[$i]->{name},
+                category => 1,
+            },
+            $shlf[2]): -1;
+        ok(-1==$badNumShelf, 'do not create private lists with duplicate name for same user');
+        DelShelf($badNumShelf) if $badNumShelf>-1; #delete if went wrong..
     }
 }
 
@@ -97,10 +110,17 @@ for(my $i=0;$i<10;$i++){
 my %used = ();
 for(my $i=0; $i<10;$i++){
     my $bib = $biblionumbers[int(rand(9))];
-    my $shelfnumber = $shelves[int(rand(9))];
+    my $shelfnumber = $shelves[int(rand(9))]->{number};
+    if($shelfnumber<0) {
+        ok(1, 'skip add to list-test for shelf -1');
+        ok(1, 'skip counting list entries for shelf -1');
+        next;
+    }
 
     my $key = "$bib\t$shelfnumber";
     my $should_fail = exists($used{$key}) ? 1 : 0;
+    #FIXME We assume here that we have permission to add..
+    #The different permissions could be tested too.
 
     my ($biblistBefore,$countbefore) = GetShelfContents($shelfnumber);
     my $status = AddToShelf($bib,$shelfnumber,$borrowers[$i]);
@@ -115,11 +135,10 @@ for(my $i=0; $i<10;$i++){
     if (defined $status) {
         ok($countbefore == $countafter - 1, 'added bib to list');  # the bib has been successfuly added.
     } else {
-        ok($countbefore == $countafter,     'did not add duplicate bib to list');  # the bib has been successfuly added.
+        ok($countbefore == $countafter, 'did not add duplicate bib to list');
     }
 
     $used{$key}++;
-
 }
 
 #-----------------------TEST ModShelf & GetShelf functions------------------------#
@@ -128,26 +147,55 @@ for(my $i=0; $i<10;$i++){
 
 for(my $i=0; $i<10;$i++){
     my $rand = int(rand(9));
-    my $numA = $shelves[$rand];
-    my $shelf = { shelfname => "NewName_".$rand,
-    category =>  int(rand(2))+1 };
-
-    ModShelf($numA,$shelf);
-    my ($numB,$nameB,$ownerB,$categoryB) = GetShelf($numA);
-
-    ok($numA == $numB, 'modified shelf');
-    ok($shelf->{shelfname} eq $nameB,     '... and name change took');
-    ok($shelf->{category}  eq $categoryB, '... and category change took');
+    my $numA = $shelves[$rand]->{number};
+    if($numA<0) {
+        ok(1, 'Skip ModShelf test for shelf -1');
+        ok(1, 'Skip ModShelf test for shelf -1');
+        ok(1, 'Skip ModShelf test for shelf -1');
+        next;
+    }
+    my $newname= randomname();
+    my $shelf = {
+        shelfname => $newname,
+        category =>  3-$shelves[$rand]->{catg}, # tric: 1->2 and 2->1
+    };
+    #check name change (with category change)
+    if(C4::VirtualShelves::_CheckShelfName($newname,$shelf->{category},
+            $shelves[$rand]->{owner}, $numA)) {
+        ModShelf($numA,$shelf);
+        my ($numB,$nameB,$ownerB,$categoryB) = GetShelf($numA);
+        ok($numA == $numB, 'modified shelf');
+        ok($shelf->{shelfname} eq $nameB,     '... and name change took');
+        ok($shelf->{category}  eq $categoryB, '... and category change took');
+    }
+    else {
+        ok(1, "No ModShelf for $newname") for 1..3;
+    }
 }
 
 #-----------------------TEST DelShelf & DelFromShelf functions------------------------#
 # usage : ($status) = &DelShelf($shelfnumber);
 
 for(my $i=0; $i<10;$i++){
-    my $shelfnumber = $shelves[$i];
+    my $shelfnumber = $shelves[$i]->{number};
+    if($shelfnumber<0) {
+        ok(1, 'Skip DelShelf for shelf -1');
+        next;
+    }
     my $status = DelShelf($shelfnumber);
     ok(1 == $status, "deleted shelf $shelfnumber and its contents");
 }
 
 #----------------------- CLEANUP ----------------------------------------------#
+
 DelBiblio($_) for @biblionumbers;
+
+#----------------------- SOME SUBS --------------------------------------------#
+
+sub randomname {
+    my $rv='';
+    for(0..19) {
+        $rv.= ('a'..'z','A'..'Z',0..9) [int(rand()*62)];
+    }
+    return $rv;
+}