Bugzilla – Attachment 81845 Details for
Bug 15774
Additional fields for baskets
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 15774: (follow-up) Address QA issues
Bug-15774-follow-up-Address-QA-issues.patch (text/plain), 14.69 KB, created by
Jonathan Druart
on 2018-11-01 21:09:34 UTC
(
hide
)
Description:
Bug 15774: (follow-up) Address QA issues
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2018-11-01 21:09:34 UTC
Size:
14.69 KB
patch
obsolete
>From 25abfdf09230a698dd0f7780352e72d6ea6dbf27 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Thu, 1 Nov 2018 18:32:05 +0000 >Subject: [PATCH] Bug 15774: (follow-up) Address QA issues > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >--- > C4/Acquisition.pm | 2 +- > 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 | 39 +++++++++++++++++++++- > .../Koha/Objects/Mixin/AdditionalFields.t | 12 +++---- > 13 files changed, 93 insertions(+), 32 deletions(-) > >diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm >index c5d3776..81ac448 100644 >--- a/C4/Acquisition.pm >+++ b/C4/Acquisition.pm >@@ -2456,7 +2456,7 @@ sub GetHistory { > } > > 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 9cd5a6d..d54151f 100644 >--- a/C4/Serials.pm >+++ b/C4/Serials.pm >@@ -521,7 +521,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 >--- 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 >--- 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 >--- 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 >--- 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 >--- 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 2726775..35365a2 100644 >--- a/Koha/Schema/Result/Subscription.pm >+++ b/Koha/Schema/Result/Subscription.pm >@@ -457,7 +457,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 b108c02..7cd877e 100755 >--- a/acqui/basket.pl >+++ b/acqui/basket.pl >@@ -435,7 +435,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 >--- 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 3a8534b..16ef537 100755 >--- a/serials/subscription-add.pl >+++ b/serials/subscription-add.pl >@@ -142,7 +142,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; > } > } >@@ -357,7 +357,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; >@@ -455,7 +456,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 04687eb..4fb165b 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 => 70; >+use Test::More tests => 71; > use t::lib::Mocks; > use Koha::Database; >+use Koha::Acquisition::Basket; > > use MARC::File::XML ( BinaryEncoding => 'utf8', RecordFormat => 'MARC21' ); > >@@ -708,4 +709,40 @@ subtest 'ModReceiveOrder replacementprice tests' => sub { > is ($received_order->{replacementprice},'16.120000',"Replacement price set if none passed in"); > }; > >+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 >--- 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.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 15774
:
48224
|
53608
|
53609
|
72078
|
72079
|
72085
|
72086
|
75083
|
75084
|
75085
|
75086
|
75336
|
75354
|
75361
|
79976
|
79977
|
79978
|
79979
|
79980
|
79981
|
79982
|
80060
|
80061
|
80062
|
80063
|
80064
|
80065
|
80066
|
80680
|
81490
|
81491
|
81492
|
81493
|
81494
|
81495
|
81496
|
81497
|
81833
|
81837
|
81838
|
81839
|
81840
|
81841
|
81842
|
81843
|
81844
|
81845
|
81846
|
81847
|
81848
|
81849
|
81850
|
81851
|
81852
|
83349
|
83350
|
83351
|
83352
|
83353
|
83354
|
83355
|
83356
|
83357
|
83358
|
83359
|
83360
|
83361
|
83362
|
83363
|
83364
|
83444
|
83445
|
83446
|
83447
|
83448
|
83449
|
83450
|
83451
|
83452
|
83453
|
83454
|
83455
|
83456
|
83457
|
83458
|
83459
|
85333
|
85334
|
85335
|
85336
|
85337
|
85338
|
85339
|
85340
|
85341
|
85342
|
85343
|
85344
|
85345
|
85346
|
85347
|
85348
|
85792
|
85806
|
85807
|
85808
|
85809
|
85810
|
85811
|
85812
|
85813
|
85814
|
85815
|
85816
|
85817
|
85818
|
85819
|
85820
|
85821
|
85822
|
85823
|
85824
|
86411