From 9de3872b907c6771cf8b70ebc303247d8dddb87c Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Thu, 10 Mar 2016 14:42:08 +0100
Subject: [PATCH] Bug 16155: Adjust TestBuilder.t

The changes in TestBuilder.pm require some changes in this test.

[1] Tests have been organized under subtests. A few superfluous tests have
    been removed. (There is still some overlap between the sections
    of overduerules_transport_type and userpermission.)
[2] The results in the build all sources-test are checked one step further.
[3] Tests are added for field length, null values and delete method.
[4] The former defaults from TestBuilder are incorporated in the tests
    for userpermission.

Test plan:
Run t/db_dependent/TestBuilder.t

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 t/db_dependent/TestBuilder.t | 481 +++++++++++++++++++----------------
 1 file changed, 262 insertions(+), 219 deletions(-)

diff --git a/t/db_dependent/TestBuilder.t b/t/db_dependent/TestBuilder.t
index a3cb813be6..0baec3884d 100644
--- a/t/db_dependent/TestBuilder.t
+++ b/t/db_dependent/TestBuilder.t
@@ -19,7 +19,9 @@
 
 use Modern::Perl;
 
-use Test::More tests => 41;
+use Test::More tests => 9;
+use Test::Warn;
+use Data::Dumper qw(Dumper);
 
 use Koha::Database;
 
@@ -27,234 +29,269 @@ BEGIN {
     use_ok('t::lib::TestBuilder');
 }
 
-my $schema  = Koha::Database->new->schema;
+my $schema = Koha::Database->new->schema;
 $schema->storage->txn_begin;
+my $builder;
 
-my $builder = t::lib::TestBuilder->new();
 
-is( $builder->build(), undef, 'build without arguments returns undef' );
+subtest 'Start with some trivial tests' => sub {
+    plan tests => 6;
 
-my @sources    = $builder->schema->sources;
-my @source_in_failure;
-for my $source (@sources) {
-    eval { $builder->build( { source => $source } ); };
-    push @source_in_failure, $source if $@;
-}
-is( @source_in_failure, 0, 'TestBuilder should be able to create an object for every sources' );
-if ( @source_in_failure ) {
-    diag ("The following sources have not been generated correctly: " . join ', ', @source_in_failure)
-}
+    $builder = t::lib::TestBuilder->new;
+    isnt( $builder, undef, 'We got a builder' );
+
+    is( $builder->build, undef, 'build without arguments returns undef' );
+    is( ref( $builder->schema ), 'Koha::Schema', 'check schema' );
+    is( ref( $builder->can('delete') ), 'CODE', 'found delete method' );
 
-my $my_overduerules_transport_type = {
-    message_transport_type => {
-        message_transport_type => 'my msg_t_t',
-    },
-    overduerules_id => {
-        branchcode   => 'codeB',
-        categorycode => 'codeC',
-    },
-    categorycode => undef,
+    # invalid argument
+    warning_like { $builder->build({
+            source => 'Borrower',
+            value  => { surname => { invalid_hash => 1 } },
+        }) } qr/^Hash not allowed for surname/,
+        'Build should not accept a hash for this column';
+
+    # return undef if a record exists
+    my $param = { source => 'Branch', value => { branchcode => 'MPL' } };
+    $builder->build( $param ); # at least it should exist now
+    is( $builder->build( $param ), undef, 'Return undef when exists' );
 };
-$my_overduerules_transport_type->{categorycode} = $my_overduerules_transport_type->{branchcode};
-my $overduerules_transport_type = $builder->build({
-    source => 'OverduerulesTransportType',
-    value  => $my_overduerules_transport_type,
-});
-is(
-    $overduerules_transport_type->{message_transport_type},
-    $my_overduerules_transport_type->{message_transport_type}->{message_transport_type},
-    'build stores the message_transport_type correctly'
-);
-is(
-    $overduerules_transport_type->{branchcode},
-    $my_overduerules_transport_type->{branchcode}->{branchcode},
-    'build stores the branchcode correctly'
-);
-is(
-    $overduerules_transport_type->{categorycode},
-    $my_overduerules_transport_type->{categorycode}->{categorycode},
-    'build stores the categorycode correctly'
-);
-is(
-    $overduerules_transport_type->{_fk}->{message_transport_type}->{message_transport_type},
-    $my_overduerules_transport_type->{message_transport_type}->{message_transport_type},
-    'build stores the foreign key message_transport_type correctly'
-);
-is(
-    $overduerules_transport_type->{_fk}->{branchcode}->{branchcode},
-    $my_overduerules_transport_type->{branchcode}->{branchcode},
-    'build stores the foreign key branchcode correctly'
-);
-is(
-    $overduerules_transport_type->{_fk}->{categorycode}->{categorycode},
-    $my_overduerules_transport_type->{categorycode}->{categorycode},
-    'build stores the foreign key categorycode correctly'
-);
-is_deeply(
-    $overduerules_transport_type->{_fk}->{branchcode},
-    $overduerules_transport_type->{_fk}->{categorycode},
-    'build links the branchcode and the categorycode correctly'
-);
-isnt(
-    $overduerules_transport_type->{_fk}->{overduerules_id}->{letter2},
-    undef,
-    'build generates values if they are not given'
-);
-
-my $my_user_permission = $t::lib::TestBuilder::default_value->{UserPermission};
-my $user_permission = $builder->build({
-    source => 'UserPermission',
-});
-isnt(
-    $user_permission->{borrowernumber},
-    undef,
-    'build generates a borrowernumber correctly'
-);
-is(
-    $user_permission->{module_bit},
-    $my_user_permission->{module_bit}->{module_bit}->{bit},
-    'build stores the default value correctly'
-);
-is(
-    $user_permission->{code},
-    $my_user_permission->{module_bit}->{code},
-    'build stores the default value correctly'
-);
-is(
-    $user_permission->{borrowernumber},
-    $user_permission->{_fk}->{borrowernumber}->{borrowernumber},
-    'build links the foreign key correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{surname},
-    $my_user_permission->{borrowernumber}->{surname},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{address},
-    $my_user_permission->{borrowernumber}->{address},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{city},
-    $my_user_permission->{borrowernumber}->{city},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{branchcode}->{branchcode},
-    $my_user_permission->{borrowernumber}->{branchcode}->{branchcode},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{branchcode}->{branchname},
-    $my_user_permission->{borrowernumber}->{branchcode}->{branchname},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{categorycode}->{categorycode},
-    $my_user_permission->{borrowernumber}->{categorycode}->{categorycode},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{categorycode}->{hidelostitems},
-    $my_user_permission->{borrowernumber}->{categorycode}->{hidelostitems},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{categorycode}->{category_type},
-    $my_user_permission->{borrowernumber}->{categorycode}->{category_type},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{categorycode}->{defaultprivacy},
-    $my_user_permission->{borrowernumber}->{categorycode}->{defaultprivacy},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{borrowernumber}->{privacy},
-    $my_user_permission->{borrowernumber}->{privacy},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{module_bit}->{_fk}->{module_bit}->{bit},
-    $my_user_permission->{module_bit}->{module_bit}->{bit},
-    'build stores the foreign key value correctly'
-);
-is(
-    $user_permission->{_fk}->{module_bit}->{code},
-    $my_user_permission->{module_bit}->{code},
-    'build stores the foreign key value correctly'
-);
-is_deeply(
-    $user_permission->{_fk}->{module_bit},
-    $user_permission->{_fk}->{code},
-    'build links the codes correctly'
-);
-isnt(
-    $user_permission->{_fk}->{borrowernumber}->{cardnumber},
-    undef,
-    'build generates values if they are not given'
-);
-isnt(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{branchcode}->{branchaddress1},
-    undef,
-    'build generates values if they are not given'
-);
-isnt(
-    $user_permission->{_fk}->{borrowernumber}->{_fk}->{categorycode}->{description},
-    undef,
-    'build generates values if they are not given'
-);
-isnt(
-    $user_permission->{_fk}->{module_bit}->{description},
-    undef,
-    'build generates values if they are not given'
-);
-isnt(
-    $user_permission->{_fk}->{module_bit}->{_fk}->{module_bit}->{flag},
-    undef,
-    'build generates values if they are not given'
-);
-
-
-my $nb_basket = $builder->schema->resultset('Aqbasket')->search();
-isnt( $nb_basket, 0, 'add stores the generated entries correctly' );
-$builder->clear( { source => 'Aqbasket' } );
-$nb_basket = $builder->schema->resultset('Aqbasket')->search();
-is( $nb_basket, 0, 'clear removes all the entries correctly' );
-
-
-my $rs_aqbookseller = $builder->schema->resultset('Aqbookseller');
-my $bookseller = $builder->build({
-    source  => 'Aqbookseller',
-    only_fk => 1,
-});
-delete $bookseller->{_fk};
-my $bookseller_from_db = $rs_aqbookseller->find($bookseller);
-is( $bookseller_from_db, undef, 'build with only_fk = 1 does not store the entry' );
-my $bookseller_result = $rs_aqbookseller->create($bookseller);
-is( $bookseller_result->in_storage, 1, 'build with only_fk = 1 creates the foreign keys correctly' );
-
-$bookseller = $builder->build({
-    source  => 'Aqbookseller',
-});
-ok( length( $bookseller->{phone} ) <= 30, 'The length for a generated string should not be longer than the size of the DB field' );
-delete $bookseller->{_fk};
-$bookseller_from_db = $rs_aqbookseller->find($bookseller);
-is( $bookseller_from_db->in_storage, 1, 'build without the parameter only_sk stores the entry correctly' );
-
-$bookseller = $builder->build({
-    source  => 'Aqbookseller',
-    only_fk => 0,
-});
-delete $bookseller->{_fk};
-$bookseller_from_db = $rs_aqbookseller->find($bookseller);
-is( $bookseller_from_db->in_storage, 1, 'build with only_fk = 0 stores the entry correctly' );
 
-subtest 'Auto-increment values tests' => sub {
 
+subtest 'Build all sources' => sub {
+    plan tests => 1;
+
+    my @sources = $builder->schema->sources;
+    my @source_in_failure;
+    for my $source ( @sources ) {
+        my $res;
+        eval { $res = $builder->build( { source => $source } ); };
+        push @source_in_failure, $source if $@ || !defined( $res );
+    }
+    is( @source_in_failure, 0,
+        'TestBuilder should be able to create an object for every source' );
+    if ( @source_in_failure ) {
+        diag( "The following sources have not been generated correctly: " .
+        join ', ', @source_in_failure );
+    }
+};
+
+
+subtest 'Test length of some generated fields' => sub {
     plan tests => 2;
 
+    # Test the length of a returned character field
+    my $bookseller = $builder->build({ source  => 'Aqbookseller' });
+    my $max = $schema->source('Aqbookseller')->column_info('phone')->{size};
+    is( length( $bookseller->{phone} ) > 0, 1,
+        'The length for a generated string (phone) should not be zero' );
+    is( length( $bookseller->{phone} ) <= $max, 1,
+        'Check maximum length for a generated string (phone)' );
+};
+
+
+subtest 'Test FKs in overduerules_transport_type' => sub {
+    plan tests => 5;
+
+    my $my_overduerules_transport_type = {
+        message_transport_type => {
+            message_transport_type => 'my msg_t_t',
+        },
+        overduerules_id => {
+            branchcode   => 'codeB',
+            categorycode => 'codeC',
+        },
+    };
+
+    my $overduerules_transport_type = $builder->build({
+        source => 'OverduerulesTransportType',
+        value  => $my_overduerules_transport_type,
+    });
+    is(
+        $overduerules_transport_type->{message_transport_type},
+        $my_overduerules_transport_type->{message_transport_type}->{message_transport_type},
+        'build stores the message_transport_type correctly'
+    );
+    is(
+        $schema->resultset('Overduerule')->find( $overduerules_transport_type->{overduerules_id} )->branchcode,
+        $my_overduerules_transport_type->{overduerules_id}->{branchcode},
+        'build stores the branchcode correctly'
+    );
+    is(
+        $schema->resultset('Overduerule')->find( $overduerules_transport_type->{overduerules_id} )->categorycode,
+        $my_overduerules_transport_type->{overduerules_id}->{categorycode},
+        'build stores the categorycode correctly'
+    );
+    is(
+        $schema->resultset('MessageTransportType')->find( $overduerules_transport_type->{message_transport_type} )->message_transport_type,
+        $overduerules_transport_type->{message_transport_type},
+        'build stores the foreign key message_transport_type correctly'
+    );
+    isnt(
+        $schema->resultset('Overduerule')->find( $my_overduerules_transport_type->{overduerules_id} )->letter2,
+        undef,
+        'build generates values if they are not given'
+    );
+};
+
+
+subtest 'Tests with composite FK in userpermission' => sub {
+    plan tests => 9;
+
+    my $my_user_permission = default_userpermission();
+    my $user_permission = $builder->build({
+        source => 'UserPermission',
+        value  => $my_user_permission,
+    });
+
+    # Checks on top level of userpermission
+    isnt(
+        $user_permission->{borrowernumber},
+        undef,
+        'build generates a borrowernumber correctly'
+    );
+    is(
+        $user_permission->{code},
+        $my_user_permission->{code}->{code},
+        'build stores code correctly'
+    );
+
+    # Checks one level deeper userpermission -> borrower
+    my $patron = $schema->resultset('Borrower')->find({ borrowernumber => $user_permission->{borrowernumber} });
+    is(
+        $patron->surname,
+        $my_user_permission->{borrowernumber}->{surname},
+        'build stores surname correctly'
+    );
+    isnt(
+        $patron->cardnumber,
+        undef,
+        'build generated cardnumber'
+    );
+
+    # Checks two levels deeper userpermission -> borrower -> branch
+    my $branch = $schema->resultset('Branch')->find({ branchcode => $patron->branchcode->branchcode });
+    is(
+        $branch->branchname,
+        $my_user_permission->{borrowernumber}->{branchcode}->{branchname},
+        'build stores branchname correctly'
+    );
+    isnt(
+        $branch->branchaddress1,
+        undef,
+        'build generated branch address'
+    );
+
+    # Checks with composite FK: userpermission -> permission
+    my $perm = $schema->resultset('Permission')->find({ module_bit => $user_permission->{module_bit}, code => $my_user_permission->{code}->{code} });
+    isnt( $perm, undef, 'build generated record for composite FK' );
+    is(
+        $perm->code,
+        $my_user_permission->{code}->{code},
+        'build stored code correctly'
+    );
+    is(
+        $perm->description,
+        $my_user_permission->{code}->{description},
+        'build stored description correctly'
+    );
+};
+
+sub default_userpermission {
+    return {
+        borrowernumber => {
+            surname => 'my surname',
+            address => 'my adress',
+            city    => 'my city',
+            branchcode => {
+                branchname => 'my branchname',
+            },
+            categorycode => {
+                hidelostitems   => 0,
+                category_type   => 'A',
+                default_privacy => 'default',
+            },
+            privacy => 1,
+        },
+        module_bit => {
+            flag        => 'my flag',
+        },
+        code => {
+            code        => 'my code',
+            description => 'my desc',
+        },
+    };
+}
+
+
+subtest 'Test build with NULL values' => sub {
+    plan tests => 3;
+
+    # PK should not be null
+    my $params = { source => 'Branch', value => { branchcode => undef }};
+    is( $builder->build( $params ), undef, 'PK should not be null' );
+    # Nullable column
+    my $info = $schema->source( 'Item' )->column_info( 'barcode' );
+    $params = { source => 'Item', value  => { barcode => undef }};
+    my $item = $builder->build( $params );
+    is( $info->{is_nullable} && $item && !defined( $item->{barcode} ), 1,
+        'Barcode can be NULL' );
+    # Nullable FK
+    $params = { source => 'Reserve', value  => { itemnumber => undef }};
+    my $reserve = $builder->build( $params );
+    $info = $schema->source( 'Reserve' )->column_info( 'itemnumber' );
+    is( $reserve && $info->{is_nullable} && $info->{is_foreign_key} &&
+        !defined( $reserve->{itemnumber} ), 1, 'Nullable FK' );
+};
+
+
+subtest 'Tests for delete method' => sub {
+    plan tests => 12;
+
+    # Test delete with single and multiple records
+    my $basket1 = $builder->build({ source => 'Aqbasket' });
+    my $basket2 = $builder->build({ source => 'Aqbasket' });
+    my $basket3 = $builder->build({ source => 'Aqbasket' });
+    my ( $id1, $id2 ) = ( $basket1->{basketno}, $basket2->{basketno} );
+    $builder->delete({ source => 'Aqbasket', records => $basket1 });
+    isnt( exists $basket1->{basketno}, 1, 'Delete cleared PK hash value' );
+
+    is( $builder->schema->resultset('Aqbasket')->search({ basketno => $id1 })->count, 0, 'Basket1 is no longer found' );
+    is( $builder->schema->resultset('Aqbasket')->search({ basketno => $id2 })->count, 1, 'Basket2 is still found' );
+    is( $builder->delete({ source => 'Aqbasket', records => [ $basket2, $basket3 ] }), 2, "Returned two delete attempts" );
+    is( $builder->schema->resultset('Aqbasket')->search({ basketno => $id2 })->count, 0, 'Basket2 is no longer found' );
+
+
+    # Test delete in table without primary key (..)
+    is( $schema->source('TmpHoldsqueue')->primary_columns, 0,
+        'Table without primary key detected' );
+    my $bibno = 123; # just a number
+    my $cnt1 = $schema->resultset('TmpHoldsqueue')->count;
+    # Insert a new record in TmpHoldsqueue with that biblionumber
+    my $val = { biblionumber => $bibno };
+    my $rec = $builder->build({ source => 'TmpHoldsqueue', value => $val });
+    my $cnt2 = $schema->resultset('TmpHoldsqueue')->count;
+    is( defined($rec) && $cnt2 == $cnt1 + 1 , 1, 'Created a record' );
+    is( $builder->delete({ source => 'TmpHoldsqueue', records => $rec }),
+        undef, 'delete returns undef' );
+    is( $rec->{biblionumber}, $bibno, 'Hash value untouched' );
+    is( $schema->resultset('TmpHoldsqueue')->count, $cnt2,
+        "Method did not delete record in table without PK" );
+
+    # Test delete with NULL values
+    $val = { branchcode => undef };
+    is( $builder->delete({ source => 'Branch', records => $val }), 0,
+        'delete returns zero for an undef search with one key' );
+    $val = { module_bit => 1, #catalogue
+             code       => undef };
+    is( $builder->delete({ source => 'Permission', records => $val }), 0,
+        'delete returns zero for an undef search with a composite PK' );
+};
+
+
+subtest 'Auto-increment values tests' => sub {
+    plan tests => 3;
+
     # Pick a table with AI PK
     my $source  = 'Biblio'; # table
     my $column  = 'biblionumber'; # ai column
@@ -272,6 +309,12 @@ subtest 'Auto-increment values tests' => sub {
     my $next_ai_value = $biblio_2->{ biblionumber };
     is( $ai_value + 1, $next_ai_value, "AI values are consecutive");
 
+    # respect autoincr column
+    warning_like { $builder->build({
+            source => $source,
+            value  => { biblionumber => 123 },
+        }) } qr/^Value not allowed for auto_incr/,
+        'Build should not overwrite an auto_incr column';
 };
 
 $schema->storage->txn_rollback;
-- 
2.39.5