https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20644 --- Comment #13 from Nick Clemens (kidclamp) <nick@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.