Attachment 169676 Details for Bug 35044 – Bug 35044: (QA follow-up): DRY code before set_additional_fields

[patch] Bug 35044: (QA follow-up): DRY code before set_additional_fields

Bug-35044-QA-follow-up-DRY-code-before-setaddition.patch (text/plain), 14.60 KB, created by Julian Maurice on 2024-07-26 07:53:31 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Julian Maurice

Created: 2024-07-26 07:53:31 UTC

Size: 14.60 KB

patch

obsolete

>From 338fe0e0c4cd0a625f523e17b6e33842b2c1f6f1 Mon Sep 17 00:00:00 2001
>From: Pedro Amorim <pedro.amorim@ptfs-europe.com>
>Date: Wed, 24 Jul 2024 18:14:09 +0000
>Subject: [PATCH] Bug 35044: (QA follow-up): DRY code before
> set_additional_fields
>
>Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 35044: 157324 | 157325 | 157326 | 157327 | 157328 | 157329 | 157330 | 157331 | 157332 | 157333 | 157334 | 158113 | 158498 | 158499 | 158500 | 158501 | 158502 | 158503 | 158504 | 158505 | 158506 | 158507 | 158508 | 158857 | 158858 | 158859 | 158860 | 158861 | 158862 | 158863 | 158864 | 158865 | 158866 | 158867 | 158868 | 159268 | 160177 | 160178 | 160179 | 160180 | 160181 | 160182 | 160183 | 160184 | 160185 | 160186 | 160187 | 160188 | 160189 | 164162 | 164163 | 164164 | 164165 | 164166 | 164167 | 164168 | 164169 | 164170 | 164171 | 164172 | 164173 | 164174 | 167690 | 168537 | 168538 | 168539 | 168540 | 168541 | 168542 | 168543 | 168544 | 168545 | 168546 | 168547 | 168548 | 169545 | 169546 | 169547 | 169603 | 169604 | 169605 | 169606 | 169607 | 169643 | 169644 | 169645 | 169646 | 169647 | 169648 | 169649 | 169650 | 169651 | 169652 | 169653 | 169654 | 169655 | 169656 | 169657 | 169658 | 169659 | 169660 | 169661 | 169662 | 169663 | 169664 | 169665 | 169666 | 169667 | 169668 | 169669 | 169670 | 169671 | 169672 | 169673 | 169674 | 169675 | 169676 | 169677 | 169678 | 169679 | 169680 | 169681 | 169682 | 169683 | 169684 | 169697 | 170501