From 2677da8f17fdf5a7f505770a2dc9935e55a3e73c Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 7 Aug 2019 10:44:24 -0500 Subject: [PATCH] Bug 23463: Remove no longer needed subs related to default values Signed-off-by: Tomas Cohen Arazi Signed-off-by: Nick Clemens Signed-off-by: Martin Renvoize --- C4/Items.pm | 318 ++--------------------------------------- t/db_dependent/Items.t | 134 +---------------- 2 files changed, 9 insertions(+), 443 deletions(-) diff --git a/C4/Items.pm b/C4/Items.pm index 8df39046cd..5769b9ce0b 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -246,15 +246,12 @@ sub AddItemBatchFromMarc { next ITEMFIELD; } - _set_derived_columns_for_add($item); - my ( $itemnumber, $error ) = _koha_new_item( $item, $item->{barcode} ); - warn $error if $error; - push @itemnumbers, $itemnumber; # FIXME not checking error - $item->{'itemnumber'} = $itemnumber; + my $item_object = Koha::Item->new($item)->store; + push @itemnumbers, $item_object->itemnumber; # FIXME not checking error - logaction("CATALOGUING", "ADD", $itemnumber, "item") if C4::Context->preference("CataloguingLog"); + logaction("CATALOGUING", "ADD", $item->itemnumber, "item") if C4::Context->preference("CataloguingLog"); - my $new_item_marc = _marc_from_item_hash($item, $frameworkcode, $unlinked_item_subfields); + my $new_item_marc = _marc_from_item_hash($item->unblessed, $frameworkcode, $unlinked_item_subfields); $item_field->replace_with($new_item_marc->field($itemtag)); } @@ -269,83 +266,6 @@ sub AddItemBatchFromMarc { return (\@itemnumbers, \@errors); } -=head2 ModItemFromMarc - - ModItemFromMarc($item_marc, $biblionumber, $itemnumber); - -This function updates an item record based on a supplied -C object containing an embedded item field. -This API is meant for the use of C; for -other purposes, C should be used. - -This function uses the hash %default_values_for_mod_from_marc, -which contains default values for item fields to -apply when modifying an item. This is needed because -if an item field's value is cleared, TransformMarcToKoha -does not include the column in the -hash that's passed to ModItem, which without -use of this hash makes it impossible to clear -an item field's value. See bug 2466. - -Note that only columns that can be directly -changed from the cataloging and serials -item editors are included in this hash. - -Returns item record - -=cut - -sub _build_default_values_for_mod_marc { - # Has no framework parameter anymore, since Default is authoritative - # for Koha to MARC mappings. - - my $cache = Koha::Caches->get_instance(); - my $cache_key = "default_value_for_mod_marc-"; - my $cached = $cache->get_from_cache($cache_key); - return $cached if $cached; - - my $default_values = { - barcode => undef, - booksellerid => undef, - ccode => undef, - 'items.cn_source' => undef, - coded_location_qualifier => undef, - copynumber => undef, - damaged => 0, - enumchron => undef, - holdingbranch => undef, - homebranch => undef, - itemcallnumber => undef, - itemlost => 0, - itemnotes => undef, - itemnotes_nonpublic => undef, - itype => undef, - location => undef, - permanent_location => undef, - materials => undef, - new_status => undef, - notforloan => 0, - price => undef, - replacementprice => undef, - replacementpricedate => undef, - restricted => undef, - stack => undef, - stocknumber => undef, - uri => undef, - withdrawn => 0, - }; - my %default_values_for_mod_from_marc; - while ( my ( $field, $default_value ) = each %$default_values ) { - my $kohafield = $field; - $kohafield =~ s|^([^\.]+)$|items.$1|; - $default_values_for_mod_from_marc{$field} = $default_value - if C4::Biblio::GetMarcFromKohaField( $kohafield ); - } - - $cache->set_in_cache($cache_key, \%default_values_for_mod_from_marc); - return \%default_values_for_mod_from_marc; -} - sub ModItemFromMarc { my $item_marc = shift; my $biblionumber = shift; @@ -358,16 +278,17 @@ sub ModItemFromMarc { $localitemmarc->append_fields( $item_marc->field($itemtag) ); my $item = TransformMarcToKoha( $localitemmarc, $frameworkcode, 'items' ); my $default_values = _build_default_values_for_mod_marc(); + my $item_object = Koha::Items->find($itemnumber); foreach my $item_field ( keys %$default_values ) { - $item->{$item_field} = $default_values->{$item_field} + $item_object->$item_field($default_values->{$item_field}) unless exists $item->{$item_field}; } my $unlinked_item_subfields = _get_unlinked_item_subfields( $localitemmarc, $frameworkcode ); - my $item_object = Koha::Items->find($itemnumber); $item_object->more_subfields_xml(_get_unlinked_subfields_xml($unlinked_item_subfields))->store; + $item_object->store; - return $item_object->get_from_storage->unblessed; + return $item_object->unblessed; } =head2 ModItemTransfer @@ -1120,201 +1041,6 @@ the inner workings of C. =cut -=head2 %derived_columns - -This hash keeps track of item columns that -are strictly derived from other columns in -the item record and are not meant to be set -independently. - -Each key in the hash should be the name of a -column (as named by TransformMarcToKoha). Each -value should be hashref whose keys are the -columns on which the derived column depends. The -hashref should also contain a 'BUILDER' key -that is a reference to a sub that calculates -the derived value. - -=cut - -my %derived_columns = ( - 'items.cn_sort' => { - 'itemcallnumber' => 1, - 'items.cn_source' => 1, - 'BUILDER' => \&_calc_items_cn_sort, - } -); - -=head2 _set_derived_columns_for_add - - _set_derived_column_for_add($item); - -Given an item hash representing a new item to be added, -calculate any derived columns. Currently the only -such column is C. - -=cut - -sub _set_derived_columns_for_add { - my $item = shift; - - foreach my $column (keys %derived_columns) { - my $builder = $derived_columns{$column}->{'BUILDER'}; - my $source_values = {}; - foreach my $source_column (keys %{ $derived_columns{$column} }) { - next if $source_column eq 'BUILDER'; - $source_values->{$source_column} = $item->{$source_column}; - } - $builder->($item, $source_values); - } -} - -=head2 _get_single_item_column - - _get_single_item_column($column, $itemnumber); - -Retrieves the value of a single column from an C -row specified by C<$itemnumber>. - -=cut - -sub _get_single_item_column { - my $column = shift; - my $itemnumber = shift; - - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT $column FROM items WHERE itemnumber = ?"); - $sth->execute($itemnumber); - my ($value) = $sth->fetchrow(); - return $value; -} - -=head2 _calc_items_cn_sort - - _calc_items_cn_sort($item, $source_values); - -Helper routine to calculate C. - -=cut - -sub _calc_items_cn_sort { - my $item = shift; - my $source_values = shift; - - $item->{'items.cn_sort'} = GetClassSort($source_values->{'items.cn_source'}, $source_values->{'itemcallnumber'}, ""); -} - -=head2 _koha_new_item - - my ($itemnumber,$error) = _koha_new_item( $item, $barcode ); - -Perform the actual insert into the C table. - -=cut - -sub _koha_new_item { - my ( $item, $barcode ) = @_; - my $dbh=C4::Context->dbh; - my $error; - $item->{permanent_location} //= $item->{location}; - _mod_item_dates( $item ); - my $query = - "INSERT INTO items SET - biblionumber = ?, - biblioitemnumber = ?, - barcode = ?, - dateaccessioned = ?, - booksellerid = ?, - homebranch = ?, - price = ?, - replacementprice = ?, - replacementpricedate = ?, - datelastborrowed = ?, - datelastseen = ?, - stack = ?, - notforloan = ?, - damaged = ?, - itemlost = ?, - withdrawn = ?, - itemcallnumber = ?, - coded_location_qualifier = ?, - restricted = ?, - itemnotes = ?, - itemnotes_nonpublic = ?, - holdingbranch = ?, - location = ?, - permanent_location = ?, - onloan = ?, - issues = ?, - renewals = ?, - reserves = ?, - cn_source = ?, - cn_sort = ?, - ccode = ?, - itype = ?, - materials = ?, - uri = ?, - enumchron = ?, - more_subfields_xml = ?, - copynumber = ?, - stocknumber = ?, - new_status = ? - "; - my $sth = $dbh->prepare($query); - my $today = output_pref({ dt => dt_from_string, dateformat => 'iso', dateonly => 1 }); - $sth->execute( - $item->{'biblionumber'}, - $item->{'biblioitemnumber'}, - $barcode, - $item->{'dateaccessioned'}, - $item->{'booksellerid'}, - $item->{'homebranch'}, - $item->{'price'}, - $item->{'replacementprice'}, - $item->{'replacementpricedate'} || $today, - $item->{datelastborrowed}, - $item->{datelastseen} || $today, - $item->{stack}, - $item->{'notforloan'}, - $item->{'damaged'}, - $item->{'itemlost'}, - $item->{'withdrawn'}, - $item->{'itemcallnumber'}, - $item->{'coded_location_qualifier'}, - $item->{'restricted'}, - $item->{'itemnotes'}, - $item->{'itemnotes_nonpublic'}, - $item->{'holdingbranch'}, - $item->{'location'}, - $item->{'permanent_location'}, - $item->{'onloan'}, - $item->{'issues'}, - $item->{'renewals'}, - $item->{'reserves'}, - $item->{'items.cn_source'}, - $item->{'items.cn_sort'}, - $item->{'ccode'}, - $item->{'itype'}, - $item->{'materials'}, - $item->{'uri'}, - $item->{'enumchron'}, - $item->{'more_subfields_xml'}, - $item->{'copynumber'}, - $item->{'stocknumber'}, - $item->{'new_status'}, - ); - - my $itemnumber; - if ( defined $sth->errstr ) { - $error.="ERROR in _koha_new_item $query".$sth->errstr; - } - else { - $itemnumber = $dbh->{'mysql_insertid'}; - } - - return ( $itemnumber, $error ); -} - =head2 MoveItemFromBiblio MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio); @@ -1365,34 +1091,6 @@ sub MoveItemFromBiblio { return; } -sub _mod_item_dates { # date formatting for date fields in item hash - my ( $item ) = @_; - return if !$item || ref($item) ne 'HASH'; - - my @keys = grep - { $_ =~ /^onloan$|^date|date$|datetime$/ } - keys %$item; - # Incl. dateaccessioned,replacementpricedate,datelastborrowed,datelastseen - # NOTE: We do not (yet) have items fields ending with datetime - # Fields with _on$ have been handled already - - foreach my $key ( @keys ) { - next if !defined $item->{$key}; # skip undefs - my $dt = eval { dt_from_string( $item->{$key} ) }; - # eval: dt_from_string will die on us if we pass illegal dates - - my $newstr; - if( defined $dt && ref($dt) eq 'DateTime' ) { - if( $key =~ /datetime/ ) { - $newstr = DateTime::Format::MySQL->format_datetime($dt); - } else { - $newstr = DateTime::Format::MySQL->format_date($dt); - } - } - $item->{$key} = $newstr; # might be undef to clear garbage - } -} - =head2 _marc_from_item_hash my $item_marc = _marc_from_item_hash($item, $frameworkcode[, $unlinked_item_subfields]); diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index c4bee0f700..0c1c2f85e2 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -33,7 +33,7 @@ use Koha::AuthorisedValues; use t::lib::Mocks; use t::lib::TestBuilder; -use Test::More tests => 16; +use Test::More tests => 14; use Test::Warn; @@ -803,138 +803,6 @@ subtest 'C4::Biblio::EmbedItemsInMarcBiblio' => sub { }; -subtest 'C4::Items::_build_default_values_for_mod_marc' => sub { - plan tests => 4; - - $schema->storage->txn_begin(); - - my $builder = t::lib::TestBuilder->new; - my $framework = $builder->build({ source => 'BiblioFramework' }); - - # Link biblio.biblionumber and biblioitems.biblioitemnumber to avoid _koha_marc_update_bib_ids to fail with 'no biblio[item]number tag for framework" - Koha::MarcSubfieldStructures->search({ frameworkcode => '', tagfield => '999', tagsubfield => [ 'c', 'd' ] })->delete; - Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '999', tagsubfield => 'c', kohafield => 'biblio.biblionumber' })->store; - Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '999', tagsubfield => 'd', kohafield => 'biblioitems.biblioitemnumber' })->store; - - # Same for item fields: itemnumber, barcode, itype - Koha::MarcSubfieldStructures->search({ frameworkcode => '', tagfield => '952', tagsubfield => [ '9', 'p', 'y' ] })->delete; - Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => '9', kohafield => 'items.itemnumber' })->store; - Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => 'p', kohafield => 'items.barcode' })->store; - Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => 'y', kohafield => 'items.itype' })->store; - Koha::Caches->get_instance->clear_from_cache( "MarcSubfieldStructure-" ); - - my $itemtype = $builder->build({ source => 'Itemtype' })->{itemtype}; - - # Create a record with a barcode - my $biblio = $builder->build_sample_biblio({ frameworkcode => $framework->{frameworkcode} }); - my $item_record = new MARC::Record; - my $a_barcode = 'a barcode'; - my $barcode_field = MARC::Field->new( - '952', ' ', ' ', - p => $a_barcode, - y => $itemtype - ); - my $itemtype_field = MARC::Field->new( - '952', ' ', ' ', - y => $itemtype - ); - $item_record->append_fields( $barcode_field ); - my (undef, undef, $item_itemnumber) = AddItemFromMarc($item_record, $biblio->biblionumber); - - # Make sure everything has been set up - my $item = Koha::Items->find($item_itemnumber); - is( $item->barcode, $a_barcode, 'Everything has been set up correctly, the barcode is defined as expected' ); - - # Delete the barcode field and save the record - $item_record->delete_fields( $barcode_field ); - $item_record->append_fields( $itemtype_field ); # itemtype is mandatory - ModItemFromMarc($item_record, $biblio->biblionumber, $item_itemnumber); - $item = Koha::Items->find($item_itemnumber); - is( $item->barcode, undef, 'The default value should have been set to the barcode, the field is mapped to a kohafield' ); - - # Re-add the barcode field and save the record - $item_record->append_fields( $barcode_field ); - ModItemFromMarc($item_record, $biblio->biblionumber, $item_itemnumber); - $item = Koha::Items->find($item_itemnumber); - is( $item->barcode, $a_barcode, 'Everything has been set up correctly, the barcode is defined as expected' ); - - # Remove the mapping for barcode - Koha::MarcSubfieldStructures->search({ frameworkcode => '', tagfield => '952', tagsubfield => 'p' })->delete; - - # And make sure the caches are cleared - my $cache = Koha::Caches->get_instance(); - $cache->clear_from_cache("default_value_for_mod_marc-"); - $cache->clear_from_cache("MarcSubfieldStructure-"); - - # Update the MARC field with another value - $item_record->delete_fields( $barcode_field ); - my $another_barcode = 'another_barcode'; - my $another_barcode_field = MARC::Field->new( - '952', ' ', ' ', - p => $another_barcode, - ); - $item_record->append_fields( $another_barcode_field ); - # The DB value should not have been updated - ModItemFromMarc($item_record, $biblio->biblionumber, $item_itemnumber); - $item = Koha::Items->find($item_itemnumber); - is ( $item->barcode, $a_barcode, 'items.barcode is not mapped anymore, so the DB column has not been updated' ); - - $cache->clear_from_cache("default_value_for_mod_marc-"); - $cache->clear_from_cache( "MarcSubfieldStructure-" ); - $schema->storage->txn_rollback; -}; - -subtest '_mod_item_dates' => sub { - plan tests => 11; - - is( C4::Items::_mod_item_dates(), undef, 'Call without parameters' ); - is( C4::Items::_mod_item_dates(1), undef, 'Call without hashref' ); - - my $orgitem; - my $item = { - itemcallnumber => 'V II 149 1963', - barcode => '109304', - }; - $orgitem = { %$item }; - C4::Items::_mod_item_dates($item); - is_deeply( $item, $orgitem, 'No dates passed to _mod_item_dates' ); - - # add two correct dates - t::lib::Mocks::mock_preference('dateformat', 'us'); - $item->{dateaccessioned} = '01/31/2016'; - $item->{onloan} = $item->{dateaccessioned}; - $orgitem = { %$item }; - C4::Items::_mod_item_dates($item); - is( $item->{dateaccessioned}, '2016-01-31', 'dateaccessioned is fine' ); - is( $item->{onloan}, '2016-01-31', 'onloan is fine too' ); - - - # add some invalid dates - $item->{notexistingcolumndate} = '13/1/2015'; # wrong format - $item->{anotherdate} = 'tralala'; # even worse - $item->{myzerodate} = '0000-00-00'; # wrong too - C4::Items::_mod_item_dates($item); - is( $item->{notexistingcolumndate}, undef, 'Invalid date became NULL' ); - is( $item->{anotherdate}, undef, 'Second invalid date became NULL too' ); - is( $item->{myzerodate}, undef, '0000-00-00 became NULL too' ); - - # check if itemlost_on was not touched - $item->{itemlost_on} = '12345678'; - $item->{withdrawn_on} = '12/31/2015 23:59:00'; - $item->{damaged_on} = '01/20/2017 09:00:00'; - $orgitem = { %$item }; - C4::Items::_mod_item_dates($item); - is_deeply( $item, $orgitem, 'Colums with _on are not touched' ); - - t::lib::Mocks::mock_preference('dateformat', 'metric'); - $item->{dateaccessioned} = '01/31/2016'; #wrong - $item->{yetanotherdatetime} = '20/01/2016 13:58:00'; #okay - C4::Items::_mod_item_dates($item); - is( $item->{dateaccessioned}, undef, 'dateaccessioned wrong format' ); - is( $item->{yetanotherdatetime}, '2016-01-20 13:58:00', - 'yetanotherdatetime is ok' ); -}; - subtest 'get_hostitemnumbers_of' => sub { plan tests => 3; -- 2.39.5