From 8035151e41fc2e97e8a0c32cfa0e8c252c393bf2 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 1 Nov 2018 18:32:05 +0000 Subject: [PATCH] Bug 15774: (follow-up) Address QA issues Signed-off-by: Jonathan Druart Signed-off-by: Josef Moravec Signed-off-by: Nick Clemens --- C4/Acquisition.pm | 3 +- C4/Serials.pm | 2 +- Koha/AdditionalFieldValue.pm | 26 ++++++++++++- Koha/AdditionalFieldValues.pm | 1 + Koha/Object/Mixin/AdditionalFields.pm | 18 ++++----- Koha/Objects/Mixin/AdditionalFields.pm | 8 ++-- Koha/Schema/Result/Aqbasket.pm | 1 - Koha/Schema/Result/Subscription.pm | 1 - acqui/basket.pl | 2 +- acqui/basketheader.pl | 5 ++- serials/subscription-add.pl | 8 ++-- t/db_dependent/Acquisition.t | 38 ++++++++++++++++++- .../Koha/Objects/Mixin/AdditionalFields.t | 12 +++--- 13 files changed, 93 insertions(+), 32 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index 9200318ce3..1ad7a2361c 100644 --- a/C4/Acquisition.pm +++ b/C4/Acquisition.pm @@ -2445,8 +2445,9 @@ sub GetHistory { if ( @$ordernumbers ) { $query .= ' AND (aqorders.ordernumber IN ( ' . join (',', ('?') x @$ordernumbers ) . '))'; push @query_params, @$ordernumbers; + } if ( @$additional_fields ) { - my @baskets = Koha::Acquisition::Baskets->search_additional_fields($additional_fields); + my @baskets = Koha::Acquisition::Baskets->filter_by_additional_fields($additional_fields); return [] unless @baskets; diff --git a/C4/Serials.pm b/C4/Serials.pm index b4452a31dc..4b702c98e5 100644 --- a/C4/Serials.pm +++ b/C4/Serials.pm @@ -528,7 +528,7 @@ sub SearchSubscriptions { my $additional_fields = $args->{additional_fields} // []; my $matching_record_ids_for_additional_fields = []; if ( @$additional_fields ) { - my @subscriptions = Koha::Subscriptions->search_additional_fields($additional_fields); + my @subscriptions = Koha::Subscriptions->filter_by_additional_fields($additional_fields); return () unless @subscriptions; diff --git a/Koha/AdditionalFieldValue.pm b/Koha/AdditionalFieldValue.pm index 48c399f147..69515cd1af 100644 --- a/Koha/AdditionalFieldValue.pm +++ b/Koha/AdditionalFieldValue.pm @@ -1,5 +1,9 @@ package Koha::AdditionalFieldValue; +use Modern::Perl; + +use base 'Koha::Object'; + =head1 NAME Koha::AdditionalFieldValue - Koha::Object derived class for additional field @@ -7,9 +11,27 @@ values =cut -use Modern::Perl; +=head2 Class methods -use base 'Koha::Object'; +=cut + +=head3 field + +Return the Koha:AdditionalField object for this AdditionalFeidlValue + +=cut + +sub field { + my ( $self ) = @_; + + return Koha::AdditionalField->_new_from_dbic( $self->_result()->field() ); +} + +=head2 Internal methods + +=head3 _type + +=cut sub _type { 'AdditionalFieldValue' } diff --git a/Koha/AdditionalFieldValues.pm b/Koha/AdditionalFieldValues.pm index 2b890140e7..cd0b284ece 100644 --- a/Koha/AdditionalFieldValues.pm +++ b/Koha/AdditionalFieldValues.pm @@ -8,6 +8,7 @@ values =cut use Modern::Perl; +use Koha::AdditionalFieldValue; use base 'Koha::Objects'; diff --git a/Koha/Object/Mixin/AdditionalFields.pm b/Koha/Object/Mixin/AdditionalFields.pm index e297058adf..6dcfdf7353 100644 --- a/Koha/Object/Mixin/AdditionalFields.pm +++ b/Koha/Object/Mixin/AdditionalFields.pm @@ -1,6 +1,7 @@ package Koha::Object::Mixin::AdditionalFields; use Modern::Perl; +use Koha::AdditionalFieldValues; =head1 NAME @@ -43,18 +44,16 @@ Koha::Object::Mixin::AdditionalFields sub set_additional_fields { my ($self, $additional_fields) = @_; - my $rs = Koha::Database->new->schema->resultset('AdditionalFieldValue'); + my @additional_field_values = $self->additional_field_values->delete; foreach my $additional_field (@$additional_fields) { - my $field_value = $rs->find_or_new({ - field_id => $additional_field->{id}, - record_id => $self->id, - }); my $value = $additional_field->{value}; if (defined $value) { - $field_value->set_columns({ value => $value })->update_or_insert; - } elsif ($field_value->in_storage) { - $field_value->delete; + my $field_value = Koha::AdditionalFieldValue->new({ + field_id => $additional_field->{id}, + record_id => $self->id, + value => $value, + })->store; } } } @@ -70,7 +69,8 @@ Returns additional field values sub additional_field_values { my ($self) = @_; - return $self->_result->additional_field_values; + my $afv_rs = $self->_result->additional_field_values; + return Koha::AdditionalFieldValues->_new_from_dbic( $afv_rs ); } =head1 AUTHOR diff --git a/Koha/Objects/Mixin/AdditionalFields.pm b/Koha/Objects/Mixin/AdditionalFields.pm index cf3db56132..3ef6931ace 100644 --- a/Koha/Objects/Mixin/AdditionalFields.pm +++ b/Koha/Objects/Mixin/AdditionalFields.pm @@ -20,15 +20,15 @@ Koha::Objects::Mixin::AdditionalFields use Koha::Foos; - Koha::Foos->search_additional_fields(...) + Koha::Foos->filter_by_additional_fields(...) =head1 API =head2 Public methods -=head3 search_additional_fields +=head3 filter_by_additional_fields - my @objects = Koha::Foos->search_additional_fields([ + my @objects = Koha::Foos->filter_by_additional_fields([ { id => 1, value => 'foo', @@ -41,7 +41,7 @@ Koha::Objects::Mixin::AdditionalFields =cut -sub search_additional_fields { +sub filter_by_additional_fields { my ($class, $additional_fields) = @_; my %conditions; diff --git a/Koha/Schema/Result/Aqbasket.pm b/Koha/Schema/Result/Aqbasket.pm index 0419636955..90e8043773 100644 --- a/Koha/Schema/Result/Aqbasket.pm +++ b/Koha/Schema/Result/Aqbasket.pm @@ -321,7 +321,6 @@ __PACKAGE__->has_many( return { "$args->{foreign_alias}.record_id" => { -ident => "$args->{self_alias}.basketno" }, - # TODO Add column additional_field_values.tablename to avoid subquery ? "$args->{foreign_alias}.field_id" => { -in => \'(SELECT id FROM additional_fields WHERE tablename = "aqbasket")' }, }; diff --git a/Koha/Schema/Result/Subscription.pm b/Koha/Schema/Result/Subscription.pm index 5ccb9ee44f..88b43251c1 100644 --- a/Koha/Schema/Result/Subscription.pm +++ b/Koha/Schema/Result/Subscription.pm @@ -464,7 +464,6 @@ __PACKAGE__->has_many( return { "$args->{foreign_alias}.record_id" => { -ident => "$args->{self_alias}.subscriptionid" }, - # TODO Add column additional_field_values.tablename to avoid subquery ? "$args->{foreign_alias}.field_id" => { -in => \'(SELECT id FROM additional_fields WHERE tablename = "subscription")' }, }; diff --git a/acqui/basket.pl b/acqui/basket.pl index 52d3ef8a2e..618c4e383d 100755 --- a/acqui/basket.pl +++ b/acqui/basket.pl @@ -436,7 +436,7 @@ if ( $op eq 'list' ) { available_additional_fields => [ Koha::AdditionalFields->search( { tablename => 'aqbasket' } ) ], additional_field_values => { map { $_->field->name => $_->value - } Koha::Acquisition::Baskets->find($basketno)->additional_field_values }, + } Koha::Acquisition::Baskets->find($basketno)->additional_field_values->as_list }, ); } diff --git a/acqui/basketheader.pl b/acqui/basketheader.pl index 7b5b16041e..202f6b9079 100755 --- a/acqui/basketheader.pl +++ b/acqui/basketheader.pl @@ -101,7 +101,7 @@ if ( $op eq 'add_form' ) { $template->param( additional_field_values => { map { $_->field->id => $_->value - } Koha::Acquisition::Baskets->find($basketno)->additional_field_values }, + } Koha::Acquisition::Baskets->find($basketno)->additional_field_values->as_list }, ); } else { #new basket @@ -171,7 +171,8 @@ if ( $op eq 'add_form' ) { } my @additional_fields; - for my $field (Koha::AdditionalFields->search({ tablename => 'aqbasket' })) { + my $basket_fields = Koha::AdditionalFields->search({ tablename => 'aqbasket' }); + while ( my $field = $basket_fields->next ) { my $value = $input->param('additional_field_' . $field->id); push @additional_fields, { id => $field->id, diff --git a/serials/subscription-add.pl b/serials/subscription-add.pl index 5b1a447d5f..68b80c65f3 100755 --- a/serials/subscription-add.pl +++ b/serials/subscription-add.pl @@ -148,7 +148,7 @@ my @additional_fields = Koha::AdditionalFields->search({ tablename => 'subscript my %additional_field_values; if ($subscriptionid) { my $subscription = Koha::Subscriptions->find($subscriptionid); - foreach my $value ($subscription->additional_field_values) { + foreach my $value ($subscription->additional_field_values->as_list) { $additional_field_values{$value->field_id} = $value->value; } } @@ -384,7 +384,8 @@ sub redirect_add_subscription { my @additional_fields; my $record = GetMarcBiblio({ biblionumber => $biblionumber, embed_items => 1 }); - for my $field (Koha::AdditionalFields->search({ tablename => 'subscription' })) { + my $basket_fields = Koha::AdditionalFields->search({ tablename => 'aqbasket' }); + while ( my $field = $basket_fields->next ) { my $value = $query->param('additional_field_' . $field->id); if ($field->marcfield) { my ($field, $subfield) = split /\$/, $field->marcfield; @@ -502,7 +503,8 @@ sub redirect_mod_subscription { my @additional_fields; my $record = GetMarcBiblio({ biblionumber => $biblionumber, embed_items => 1 }); - for my $field (Koha::AdditionalFields->search({ tablename => 'subscription' })) { + my $basket_fields = Koha::AdditionalFields->search({ tablename => 'aqbasket' }); + while ( my $field = $basket_fields->next ) { my $value = $query->param('additional_field_' . $field->id); if ($field->marcfield) { my ($field, $subfield) = split /\$/, $field->marcfield; diff --git a/t/db_dependent/Acquisition.t b/t/db_dependent/Acquisition.t index 78322ef79f..bbc136d5ce 100755 --- a/t/db_dependent/Acquisition.t +++ b/t/db_dependent/Acquisition.t @@ -19,9 +19,10 @@ use Modern::Perl; use POSIX qw(strftime); -use Test::More tests => 73; +use Test::More tests => 74; use t::lib::Mocks; use Koha::Database; +use Koha::Acquisition::Basket; use MARC::File::XML ( BinaryEncoding => 'utf8', RecordFormat => 'MARC21' ); @@ -754,4 +755,39 @@ subtest 'ModReceiveOrder and subscription' => sub { is( $order->get_from_storage->order_internalnote, $first_note ); }; +subtest 'GetHistory with additional fields' => sub { + plan tests => 3; + my $builder = t::lib::TestBuilder->new; + my $order_basket = $builder->build({ source => 'Aqbasket', value => { is_standing => 0 } }); + my $orderinfo ={ + basketno => $order_basket->{basketno}, + rrp => 19.99, + replacementprice => undef, + quantity => 1, + quantityreceived => 0, + datereceived => undef, + datecancellationprinted => undef, + }; + my $order = $builder->build({ source => 'Aqorder', value => $orderinfo }); + my $history = GetHistory(ordernumber => $order->{ordernumber}); + is( scalar( @$history ), 1, 'GetHistory returns the one order'); + + my $additional_field = $builder->build({source => 'AdditionalField', value => { + tablename => 'aqbasket', + name => 'snakeoil', + authorised_value_category => "", + } + }); + $history = GetHistory( ordernumber => $order->{ordernumber}, additional_fields => [{ id => $additional_field->{id}, value=>'delicious'}]); + is( scalar ( @$history ), 0, 'GetHistory returns no order for an unused additional field'); + my $basket = Koha::Acquisition::Baskets->find({ basketno => $order_basket->{basketno} }); + $basket->set_additional_fields([{ + id => $additional_field->{id}, + value => 'delicious', + }]); + + $history = GetHistory( ordernumber => $order->{ordernumber}, additional_fields => [{ id => $additional_field->{id}, value=>'delicious'}]); + is( scalar( @$history ), 1, 'GetHistory returns the order when additional field is set'); +}; + $schema->storage->txn_rollback(); diff --git a/t/db_dependent/Koha/Objects/Mixin/AdditionalFields.t b/t/db_dependent/Koha/Objects/Mixin/AdditionalFields.t index 08bf4e9fad..e06182e4d6 100755 --- a/t/db_dependent/Koha/Objects/Mixin/AdditionalFields.t +++ b/t/db_dependent/Koha/Objects/Mixin/AdditionalFields.t @@ -53,7 +53,7 @@ Koha::AdditionalFieldValue->new({ value => 'bar value for basket2', })->store; -my @baskets = Koha::Acquisition::Baskets->search_additional_fields([ +my @baskets = Koha::Acquisition::Baskets->filter_by_additional_fields([ { id => $foo->id, value => 'foo value for basket1', @@ -63,7 +63,7 @@ my @baskets = Koha::Acquisition::Baskets->search_additional_fields([ is(scalar @baskets, 1, 'search returns only one result'); is($baskets[0]->basketno, $basket1->basketno, 'result is basket1'); -@baskets = Koha::Acquisition::Baskets->search_additional_fields([ +@baskets = Koha::Acquisition::Baskets->filter_by_additional_fields([ { id => $foo->id, value => 'foo value for basket2', @@ -73,7 +73,7 @@ is($baskets[0]->basketno, $basket1->basketno, 'result is basket1'); is(scalar @baskets, 1, 'search returns only one result'); is($baskets[0]->basketno, $basket2->basketno, 'result is basket2'); -@baskets = Koha::Acquisition::Baskets->search_additional_fields([ +@baskets = Koha::Acquisition::Baskets->filter_by_additional_fields([ { id => $foo->id, value => 'foo value for basket1', @@ -87,7 +87,7 @@ is($baskets[0]->basketno, $basket2->basketno, 'result is basket2'); is(scalar @baskets, 1, 'search returns only one result'); is($baskets[0]->basketno, $basket1->basketno, 'result is basket1'); -@baskets = Koha::Acquisition::Baskets->search_additional_fields([ +@baskets = Koha::Acquisition::Baskets->filter_by_additional_fields([ { id => $foo->id, value => 'foo value for basket1', @@ -100,7 +100,7 @@ is($baskets[0]->basketno, $basket1->basketno, 'result is basket1'); is(scalar @baskets, 0, 'search returns no result'); -@baskets = Koha::Acquisition::Baskets->search_additional_fields([ +@baskets = Koha::Acquisition::Baskets->filter_by_additional_fields([ { id => $foo->id, value => 'foo', @@ -109,7 +109,7 @@ is(scalar @baskets, 0, 'search returns no result'); is(scalar @baskets, 2, 'search returns two results'); -@baskets = Koha::Acquisition::Baskets->search_additional_fields([ +@baskets = Koha::Acquisition::Baskets->filter_by_additional_fields([ { id => $foo->id, value => 'foo', -- 2.39.5