Bug 15774: (follow-up) Address QA issues

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm
index 9200318..1ad7a23 100644 (file)
--- 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 b4452a3..4b702c9 100644 (file)
--- 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 48c399f..69515cd 100644 (file)
--- 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 2b89014..cd0b284 100644 (file)
--- 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 e297058..6dcfdf7 100644 (file)
--- 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 cf3db56..3ef6931 100644 (file)
--- 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 0419636..90e8043 100644 (file)
--- 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 5ccb9ee..88b4325 100644 (file)
--- 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 52d3ef8..618c4e3 100755 (executable)
--- 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 7b5b160..202f6b9 100755 (executable)
--- 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 5b1a447..68b80c6 100755 (executable)
--- 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 78322ef..bbc136d 100755 (executable)
--- 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 08bf4e9..e06182e 100755 (executable)
--- 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',