From 56028a0fce61b1959170c5cd7872c5fed2815748 Mon Sep 17 00:00:00 2001 From: Pedro Amorim Date: Wed, 24 Jul 2024 18:14:09 +0000 Subject: [PATCH] Bug 35044: (QA follow-up): DRY code before set_additional_fields --- Koha/Object/Mixin/AdditionalFields.pm | 28 ++++++++ acqui/addorder.pl | 14 +--- acqui/basketheader.pl | 13 +--- acqui/invoice.pl | 13 +--- acqui/parcels.pl | 13 +--- members/mancredit.pl | 12 +--- members/maninvoice.pl | 12 +--- members/paycollect.pl | 28 ++------ serials/subscription-add.pl | 28 ++------ .../Koha/Object/Mixin/AdditionalFields.t | 64 ++++++++++++++++++- 10 files changed, 108 insertions(+), 117 deletions(-) diff --git a/Koha/Object/Mixin/AdditionalFields.pm b/Koha/Object/Mixin/AdditionalFields.pm index 9f7a39728b..00718e99ad 100644 --- a/Koha/Object/Mixin/AdditionalFields.pm +++ b/Koha/Object/Mixin/AdditionalFields.pm @@ -91,6 +91,34 @@ sub set_additional_fields { } } +=head3 prepare_cgi_additional_field_values + +Prepares additional field values from CGI input for use in set_additional_fields + + Usage example for aqorders: + my @additional_fields = $order->prepare_cgi_additional_field_values( $input, 'aqorders' ); + +=cut + +sub prepare_cgi_additional_field_values { + my ( $self, $cgi, $tablename ) = @_; + + my @additional_fields; + my $table_fields = Koha::AdditionalFields->search( { tablename => $tablename } ); + + while ( my $field = $table_fields->next ) { + my @field_values = $cgi->multi_param( 'additional_field_' . $field->id ); + foreach my $value (@field_values) { + push @additional_fields, { + id => $field->id, + value => $value, + } if $value; + } + } + + return @additional_fields; +} + =head3 add_additional_fields Similar to set_additional_fields, but instead of overwriting existing fields, only adds new ones diff --git a/acqui/addorder.pl b/acqui/addorder.pl index 45bfa93b40..ef759925c5 100755 --- a/acqui/addorder.pl +++ b/acqui/addorder.pl @@ -386,19 +386,7 @@ if ( $op eq 'cud-order' ) { my @order_users = split( /:/, $order_users_ids ); ModOrderUsers( $order->ordernumber, @order_users ); - # Retrieve and save additional fields values - my @additional_fields; - my $order_fields = Koha::AdditionalFields->search( { tablename => 'aqorders' } ); - while ( my $field = $order_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values){ - push @additional_fields, - { - id => $field->id, - value => $value, - } if $value; - }; - } + my @additional_fields = $order->prepare_cgi_additional_field_values( $input, 'aqorders' ); $order->set_additional_fields( \@additional_fields ); # now, add items if applicable diff --git a/acqui/basketheader.pl b/acqui/basketheader.pl index 96a7e95e26..c45376103f 100755 --- a/acqui/basketheader.pl +++ b/acqui/basketheader.pl @@ -162,17 +162,8 @@ if ( $op eq 'add_form' ) { ); } - my @additional_fields; - my $basket_fields = Koha::AdditionalFields->search({ tablename => 'aqbasket' }); - while ( my $field = $basket_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my @additional_fields = + Koha::Acquisition::Baskets->find($basketno)->prepare_cgi_additional_field_values( $input, 'aqbasket' ); Koha::Acquisition::Baskets->find($basketno)->set_additional_fields(\@additional_fields); print $input->redirect('basket.pl?basketno='.$basketno); diff --git a/acqui/invoice.pl b/acqui/invoice.pl index 8627944617..bed7a6440e 100755 --- a/acqui/invoice.pl +++ b/acqui/invoice.pl @@ -128,17 +128,8 @@ elsif ( $op && $op eq 'cud-mod' ) { defined($invoice_files) && $invoice_files->MergeFileRecIds(@sources); } - my @additional_fields; - my $invoice_fields = Koha::AdditionalFields->search({ tablename => 'aqinvoices' }); - while ( my $field = $invoice_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my @additional_fields = + Koha::Acquisition::Invoices->find($invoiceid)->prepare_cgi_additional_field_values( $input, 'aqinvoices' ); Koha::Acquisition::Invoices->find($invoiceid)->set_additional_fields(\@additional_fields); $template->param( modified => 1 ); diff --git a/acqui/parcels.pl b/acqui/parcels.pl index 5bead818a9..13f9b3726e 100755 --- a/acqui/parcels.pl +++ b/acqui/parcels.pl @@ -131,17 +131,8 @@ if ($op and $op eq 'cud-confirm') { ); if (defined $invoiceid) { - my @additional_fields; - my $invoice_fields = Koha::AdditionalFields->search({ tablename => 'aqinvoices' }); - while ( my $field = $invoice_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my @additional_fields = + Koha::Acquisition::Invoices->find($invoiceid)->prepare_cgi_additional_field_values( $input, 'aqinvoices' ); if (@additional_fields) { my $invoice = Koha::Acquisition::Invoices->find( $invoiceid ); $invoice->set_additional_fields(\@additional_fields); diff --git a/members/mancredit.pl b/members/mancredit.pl index 84c1f6eea5..04aa0d88fb 100755 --- a/members/mancredit.pl +++ b/members/mancredit.pl @@ -99,17 +99,7 @@ if ( $op eq 'cud-add' ) { } ); - my @additional_fields; - my $accountline_fields = Koha::AdditionalFields->search({ tablename => 'accountlines:credit' }); - while ( my $field = $accountline_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my @additional_fields = $line->prepare_cgi_additional_field_values( $input, 'accountlines:credit' ); if (@additional_fields) { $line->set_additional_fields(\@additional_fields); } diff --git a/members/maninvoice.pl b/members/maninvoice.pl index 9aa8cc1542..6557db8ccb 100755 --- a/members/maninvoice.pl +++ b/members/maninvoice.pl @@ -161,17 +161,7 @@ if ( $op eq 'cud-add' ) { } ); - my @additional_fields; - my $accountline_fields = Koha::AdditionalFields->search({ tablename => 'accountlines:debit' }); - while ( my $field = $accountline_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my @additional_fields = $line->prepare_cgi_additional_field_values( $input, 'accountlines:debit' ); if (@additional_fields) { $line->set_additional_fields(\@additional_fields); } diff --git a/members/paycollect.pl b/members/paycollect.pl index 89cb9e1d32..09e0b351ea 100755 --- a/members/paycollect.pl +++ b/members/paycollect.pl @@ -170,19 +170,9 @@ if ( $total_paid and $total_paid ne '0.00' ) { ); $payment_id = $pay_result->{payment_id}; - my @additional_fields; - my $accountline_fields = Koha::AdditionalFields->search({ tablename => 'accountlines:credit' }); - while ( my $field = $accountline_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my $payment = Koha::Account::Lines->find($payment_id); + my @additional_fields = $payment->prepare_cgi_additional_field_values( $input, 'accountlines:credit' ); if (@additional_fields) { - my $payment = Koha::Account::Lines->find($payment_id); $payment->set_additional_fields(\@additional_fields); } @@ -248,19 +238,9 @@ if ( $total_paid and $total_paid ne '0.00' ) { } $payment_id = $pay_result->{payment_id}; - my @additional_fields; - my $accountline_fields = Koha::AdditionalFields->search({ tablename => 'accountlines:credit' }); - while ( my $field = $accountline_fields->next ) { - my @field_values = $input->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my $payment = Koha::Account::Lines->find($payment_id); + my @additional_fields = $payment->prepare_cgi_additional_field_values( $input, 'accountlines:credit' ); if (@additional_fields) { - my $payment = Koha::Account::Lines->find($payment_id); $payment->set_additional_fields(\@additional_fields); } diff --git a/serials/subscription-add.pl b/serials/subscription-add.pl index b9c66eae22..35ba10e05f 100755 --- a/serials/subscription-add.pl +++ b/serials/subscription-add.pl @@ -359,18 +359,8 @@ sub redirect_add_subscription { $template->param( mana_msg => $result->{msg} ); } - my @additional_fields; - my $biblio = Koha::Biblios->find($biblionumber); - my $subscription_fields = Koha::AdditionalFields->search({ tablename => 'subscription' }); - while ( my $field = $subscription_fields->next ) { - my @field_values = $query->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my @additional_fields = Koha::Subscriptions->find($subscriptionid) + ->prepare_cgi_additional_field_values( $query, 'subscription' ); Koha::Subscriptions->find($subscriptionid)->set_additional_fields(\@additional_fields); print $query->redirect("/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=$subscriptionid"); @@ -475,18 +465,8 @@ sub redirect_mod_subscription { $skip_serialseq, $itemtype, $previousitemtype, $mana_id, $ccode, $published_on_template ); - my @additional_fields; - my $biblio = Koha::Biblios->find($biblionumber); - my $subscription_fields = Koha::AdditionalFields->search({ tablename => 'subscription' }); - while ( my $field = $subscription_fields->next ) { - my @field_values = $query->param( 'additional_field_' . $field->id ); - foreach my $value (@field_values) { - push @additional_fields, { - id => $field->id, - value => $value, - } if $value; - } - } + my @additional_fields = + Koha::Subscriptions->find($subscriptionid)->prepare_cgi_additional_field_values( $query, 'subscription' ); Koha::Subscriptions->find($subscriptionid)->set_additional_fields(\@additional_fields); print $query->redirect("/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=$subscriptionid"); diff --git a/t/db_dependent/Koha/Object/Mixin/AdditionalFields.t b/t/db_dependent/Koha/Object/Mixin/AdditionalFields.t index 051ea13736..e49c6b48b7 100755 --- a/t/db_dependent/Koha/Object/Mixin/AdditionalFields.t +++ b/t/db_dependent/Koha/Object/Mixin/AdditionalFields.t @@ -1,13 +1,14 @@ #!/usr/bin/perl use Modern::Perl; -use Test::More tests => 4; +use Test::More tests => 5; use t::lib::TestBuilder; use String::Random qw(random_string); use Koha::Database; use Koha::Subscription; use Koha::AdditionalField; use C4::Context; +use CGI; my $builder = t::lib::TestBuilder->new; my $schema = Koha::Database->schema; @@ -218,3 +219,64 @@ subtest 'add_additional_fields' => sub { $schema->txn_rollback; }; + +subtest 'prepare_cgi_additional_field_values' => sub { + plan tests => 1; + + $schema->txn_begin; + + my $field = Koha::AdditionalField->new( + { + tablename => 'subscription', + name => random_string( 'c' x 100 ) + } + ); + $field->store()->discard_changes(); + + my $field2 = Koha::AdditionalField->new( + { + tablename => 'subscription', + name => random_string( 'c' x 100 ) + } + ); + $field2->store()->discard_changes(); + + my $q = CGI->new; + $q->param( + -name => 'additional_field_' . $field->id, + -value => [qw/value1 value2/], + ); + $q->param( + -name => 'additional_field_' . $field2->id, + -value => '0', + ); + $q->param( + -name => 'irrelevant_param', + -value => 'foo', + ); + + my @additional_fields = + Koha::Object::Mixin::AdditionalFields->prepare_cgi_additional_field_values( $q, 'subscription' ); + + is_deeply( + \@additional_fields, + [ + { + 'value' => 'value1', + 'id' => $field->id + }, + { + 'value' => 'value2', + 'id' => $field->id + }, + { + 'value' => '0', + 'id' => $field2->id + } + ], + 'Return of prepare_cgi_additional_field_values should be correct' + ); + + $schema->txn_rollback; + +}; -- 2.39.2