[Koha-bugs] [Bug 13146] Improve GetRecordValue function by caching field mapping

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13146

--- Comment #8 from M. Tompsett <mtompset at hotmail.com> ---
Comment on attachment 44132
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=44132
Bug 13146 UT

Review of attachment 44132:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=13146&attachment=44132)
-----------------------------------------------------------------

Test::DBIx::Class isn't required to run Koha, just the tests, correct? Probably
should skip tests we know we can't run. This will run fine on kohadevbox, but
not necessarily on another users koha git box.

::: t/db_dependent/FieldMapping.t
@@ +21,5 @@
> +use File::Basename;
> +use File::Path;
> +use DateTime;
> +use Test::MockModule;
> +use Test::More tests => 18;

Just use Test::More; because of suggested change below.

@@ +25,5 @@
> +use Test::More tests => 18;
> +use C4::Biblio;
> +use Koha::Schema;
> +
> +

use Module::Load::Conditional qw/check_install/;

BEGIN {
    if ( check_install( module => 'Test::DBIx::Class' ) ) {
        plan tests => 18; # or is it 19... whatever works.
    } else {
        plan skip_all => "Need Test::DBIx::Class"
    }
}

@@ +43,5 @@
> +    _new_schema => sub { return Schema(); }
> +);
> +
> +
> +if (0) {

What is the point of keeping the code, if it isn't run?

@@ +55,5 @@
> +}
> +
> +
> +SetFieldMapping('', 'maintitle', '245', 'a');
> +ok my $fm = Fieldmapping->find( {field => 'maintitle'} )

Multiple side-effects on the same line of code is more difficult to read.

@@ +56,5 @@
> +
> +
> +SetFieldMapping('', 'maintitle', '245', 'a');
> +ok my $fm = Fieldmapping->find( {field => 'maintitle'} )
> +   => 'maintitle field mapping properly set for default framework';

While this is valid, given that you did the simpler version of is() below,
could this not be:
my $fm = Fieldmapping->find( {field => 'maintitle'} );
ok( defined $fm, 'maintitle field mapping properly set for default framework');
instead? This is more readable.

@@ +60,5 @@
> +   => 'maintitle field mapping properly set for default framework';
> +
> +SetFieldMapping('', 'subtitle', '245', 'b');
> +ok $fm = Fieldmapping->find( {field => 'subtitle'} )
> +   => 'subtitle field mapping properly set for default framework';

same here.

@@ +70,5 @@
> +}, 'expected fields are there';
> +
> +SetFieldMapping('BOOK', 'subtitle', '245', 'b');
> +ok $fm = Fieldmapping->find( {frameworkcode => 'BOOK', field => 'subtitle'} )
> +   => 'subtitle field mapping properly set for BOOK framework';

same here.

-- 
You are receiving this mail because:
You are watching all bug changes.