https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20644
--- Comment #13 from Nick Clemens (kidclamp) <n...@bywatersolutions.com> --- (In reply to Matthias Le Gac from comment #12) > > But for the point where in the tests it's marked NULL and not "inherit" I > don't know what you're talking about, because for me it's already marked > "inherit" as requested. t/db_dependent/Koha/ItemTypes.t: 180 'Two item types with \'checkprevcheckout\' set to \'undef\' have been added' It's just the comment there > > For the "should fix" point, could you be more precise? I don't know which > part you're talking about. When editing an itemtype, it the system preference is 'soft' the options are: Yes and try to override system preference No and try to override system preference Inherit from system preference From what I understand it does not 'try' to override, it always does I think we should also display the system preference when set to 'Hard' otherwise this can be a somewhat hidden feature > The same goes for the "would fix" part. Could you be more specific about how > to improve test comprehension? For me, 4 nested 'map' calls is a bit obfuscated. Clearer would be something like: foreach my $syspref_setting ('hardno','hardyes','softno','softyes'){ foreach my $itemtype_setting( 'inherit','no','yes'){ ... The output from your tests is clear, the matrix in the comments is clear, if large, it's a matter of preference but not a blocker > > For the "could fix" part, I've tried to change using the builder, but with > the builder you can't create an "ItemType" with the value > "checkprevcheckout" at "undef" because this field can't be empty, so the > builder throws an error. > I tried this: > my $itemType1 = $builder->build_object({ > class => 'Koha::ItemTypes', > value => { > checkprevcheckout => undef, > } > }); > and I get this error: > "Null value for checkprevcheckout in Itemtype not allowed at > /inlibro/git/koha-master-dev-matthias/t/lib/TestBuilder.pm line 430." > Tell me if there is another solution with the builder or if we leave it as > it is now? t/db_dependent/Patron/Borrower_PrevCheckout.t 49 itemtype => 'YESIT', 59 itemtype => 'NOIT', 69 itemtype => 'INHERITIT' I just meant you don't need to specify the codes here, you use them later, but could save the generated codes as variables in the hash and reference them - this just ensures the tests won't have a conflict with existing data - rare, but slightly possible if tests die and rollback fails, etc. Again, not a blocker > Finally, for the last point about the script that adds the column, there's > already something that tests whether the column exists, isn't there? with : > "ALTER TABLE itemtypes ADD IF NOT EXISTS checkprevcheckout" > Does "ADD IF NOT EXISTS" do this? It does, but it is inconsistent with how we handle this in Koha: if( !column_exists( 'borrowers', 'middle_name' ) ) { I am not sure if your construction was blocked for being a MySQL-ism, but I on second glance it seems we should stick with the established pattern git grep column_exists git grep "ADD IF NOT EXISTS" -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/