From 93e87ca0b61611fc35238bf13eb226f477cf9c4e Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 3 Jun 2013 15:27:50 +0200 Subject: [PATCH] Bug 10386: improvements to VirtualShelves.t 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 Signed-off-by: Chris Cormack Signed-off-by: Galen Charlton --- C4/VirtualShelves.pm | 13 ++- t/db_dependent/VirtualShelves.t | 168 ++++++++++++++++++++------------ 2 files changed, 118 insertions(+), 63 deletions(-) diff --git a/C4/VirtualShelves.pm b/C4/VirtualShelves.pm index ed489f2d3a..38fa0e82a8 100644 --- a/C4/VirtualShelves.pm +++ b/C4/VirtualShelves.pm @@ -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; } diff --git a/t/db_dependent/VirtualShelves.t b/t/db_dependent/VirtualShelves.t index 323cc2c1c0..563eeca8bb 100755 --- a/t/db_dependent/VirtualShelves.t +++ b/t/db_dependent/VirtualShelves.t @@ -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; +} -- 2.39.5