From 7af422940051e176562b2fe3941f7ec4fa509419 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@biblibre.com>
Date: Wed, 4 Dec 2013 16:26:59 +0100
Subject: [PATCH] Bug 10855: Squash several fixes

Bug 10855: FIX: Add additional fields for closed subscriptions

Bug 10855: Fix instance vs static method

This patch fixes the error message.
The 3 modified routines *are* static methods.

Bug 10855: FIX conflicts with bug 7688

Bug 10855: Fix typo addition_fields -> additional_fields

Bug 10855: A partial search should return the subscriptions

If a search on an additional fields is done using a partial string
("foo" and the defined value is "foobar"), the subscription should
appear in the result list.

Test plan:
Try to search a part of the string for an additional field.

Bug 10855: Filtering on additional fields don't work if value is equal to 0

If you tried to filter on an additional field linked to an authorised
value, that did not work if the value was 0.

Bug 10855: Remove the advanced serial search box on the serial home page

Bug 10855: FIX an add field should not be created if the marc field does not exist.

This patch fixes the following:
Create an add fields linked to a nonexistent marc field (does not exist
in serials).
Edit a subscription and save.
Without this patch, an error occured:
Software error:
DBD::mysql::db do failed: Column 'value' cannot be null at
/var/root-koha/bug-10855/Koha/AdditionalField.pm line 107.

Bug 10855: Fix Type table vs tablename

Koha::AdditionalField->all method take "tablename" not "table" in
parameter".
---
 C4/Serials.pm                                      |    5 +-
 Koha/AdditionalField.pm                            |   12 +++--
 .../prog/en/modules/serials/serials-home.tt        |    1 -
 .../prog/en/modules/serials/serials-search.tt      |   29 ++++++++---
 .../prog/en/modules/serials/subscription-add.tt    |    8 +--
 serials/claims.pl                                  |    2 +-
 serials/serials-search.pl                          |    5 +-
 serials/subscription-add.pl                        |   12 ++---
 t/db_dependent/AdditionalField.t                   |   51 ++++++++++++++------
 9 files changed, 82 insertions(+), 43 deletions(-)

diff --git a/C4/Serials.pm b/C4/Serials.pm
index 409fa10..da49a2e 100644
--- a/C4/Serials.pm
+++ b/C4/Serials.pm
@@ -655,8 +655,9 @@ sub SearchSubscriptions {
     my $matching_record_ids_for_additional_fields = [];
     if ( @$additional_fields ) {
         $matching_record_ids_for_additional_fields = Koha::AdditionalField->get_matching_record_ids({
-                fields => $args->{additional_fields},
+                fields => $additional_fields,
                 tablename => 'subscription',
+                exact_match => 0,
         });
         return () unless @$matching_record_ids_for_additional_fields;
     }
@@ -746,7 +747,7 @@ sub SearchSubscriptions {
             record_id => $subscription->{subscriptionid},
             tablename => 'subscription'
         });
-        $subscription->{addition_fields} = $additional_field_values->{$subscription->{subscriptionid}};
+        $subscription->{additional_fields} = $additional_field_values->{$subscription->{subscriptionid}};
     }
 
     return @$results;
diff --git a/Koha/AdditionalField.pm b/Koha/AdditionalField.pm
index de6f763..2f3f44b 100644
--- a/Koha/AdditionalField.pm
+++ b/Koha/AdditionalField.pm
@@ -97,6 +97,7 @@ sub insert_values {
     my $dbh = C4::Context->dbh;
     local $dbh->{RaiseError} = 1;
     while ( my ( $record_id, $value ) = each %{$self->{values}} ) {
+        next unless defined $value;
         my $updated = $dbh->do(q|
             UPDATE additional_field_values
             SET value = ?
@@ -135,7 +136,7 @@ sub fetch_values {
 
 sub all {
     my ( $class, $args ) = @_;
-    die "BAD CALL: Don't use fetch_all_values as a static method"
+    die "BAD CALL: Don't use fetch_all_values as an instance method"
         if ref $class and UNIVERSAL::can($class,'can');
     my $tablename = $args->{tablename};
     my $searchable = $args->{searchable};
@@ -172,7 +173,7 @@ sub all {
 
 sub fetch_all_values {
     my ( $class, $args ) = @_;
-    die "BAD CALL: Don't use fetch_all_values as a static method"
+    die "BAD CALL: Don't use fetch_all_values as an instance method"
         if ref $class and UNIVERSAL::can($class,'can');
 
     my $record_id = $args->{record_id};
@@ -199,11 +200,12 @@ sub fetch_all_values {
 
 sub get_matching_record_ids {
     my ( $class, $args ) = @_;
-    die "BAD CALL: Don't use fetch_all_values as a static method"
+    die "BAD CALL: Don't use fetch_all_values as an instance method"
         if ref $class and UNIVERSAL::can($class,'can');
 
     my $fields = $args->{fields} // [];
     my $tablename = $args->{tablename};
+    my $exact_match = $args->{exact_match} // 1;
     return [] unless @$fields;
 
     my $dbh = C4::Context->dbh;
@@ -222,13 +224,13 @@ sub get_matching_record_ids {
                     WHERE afv.field_id = af.id
                     AND af.name = ?
                     AND af.tablename = ?
-                    AND value = ?
+                    AND value LIKE ?
                 ) AS field$i USING (id)
             WHERE field$i.id IS NOT NULL
         ) AS values$i |;
         $subquery .= ' USING (record_id)' if $i > 1;
         push @subqueries, $subquery;
-        push @args, $field->{name}, $tablename, $field->{value};
+        push @args, $field->{name}, $tablename, ( $exact_match ? $field->{value} : "%$field->{value}%" );
     }
     $query .= join( ' LEFT JOIN ', @subqueries ) . ' WHERE 1';
     for my $j ( 1 .. $i ) {
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-home.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-home.tt
index 4037b45..eaafc94 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-home.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-home.tt
@@ -17,7 +17,6 @@
     </div>
   </div>
   <div class="yui-b">
-    [% INCLUDE 'subscriptions-search.inc' %]
     [% INCLUDE 'serials-menu.inc' %]
   </div>
 </div>
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
index 14ba7d2..089311a 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-search.tt
@@ -98,16 +98,16 @@
                 </li>
                 [% FOR field IN additional_fields_for_subscription %]
                   <li>
-                    <label for="additional_field_[% field.name %]"> [% field.name %]: </label>
+                    <label for="additional_field_[% field.id %]"> [% field.name %]: </label>
                     [% IF field.authorised_value_choices %]
-                      <select id="additional_field_[% field.name %]" name="additional_field_[% field.name %]_filter">
+                      <select id="additional_field_[% field.id %]" name="additional_field_[% field.id %]_filter">
                         <option value="">All</option>
                         [% FOREACH av IN field.authorised_value_choices %]
                           <option value="[% av.authorised_value %]">[% av.lib %]</option>
                         [% END %]
                       </select>
                     [% ELSE %]
-                      <input id="additional_field_[% field.name %]" type="text" value="[% additional_fields.${field.name} %]" name="additional_field_[% field.name %]_filter" />
+                      <input id="additional_field_[% field.id %]" type="text" value="[% additional_fields.${field.name} %]" name="additional_field_[% field.id %]_filter" />
                     [% END %]
                   </li>
                 [% END %]
@@ -186,8 +186,6 @@
                         <td>
                           [% IF ( subscription.enddate ) %][% subscription.enddate | $KohaDates %][% END %]
                         </td>
-                        [% IF ( routing && CAN_user_serials_routing ) %]
-                        [% END %]
                         [% FOR field IN additional_fields_for_subscription %]
                           [% IF field.authorised_value_category %]
                             <td>[% KohaAuthorisedValues.GetByCode( field.authorised_value_category, subscription.additional_fields.${field.name} ) %]</td>
@@ -197,6 +195,7 @@
                         [% END %]
                         <td><a href="/cgi-bin/koha/serials/serials-collection.pl?subscriptionid=[% subscription.subscriptionid %]">Issue history</a>
                         </td>
+                        [% IF ( routing && CAN_user_serials_routing ) %]
                         <td>
                           [% IF ( subscription.cannotedit ) %]
                             &nbsp;
@@ -209,6 +208,7 @@
                             [% END %]
                           [% END %]
                         </td>
+                        [% END %]
                         <td>
                         [% IF ( CAN_user_serials_receive_serials ) %]
                           <a href="/cgi-bin/koha/serials/serials-edit.pl?subscriptionid=[% subscription.subscriptionid %]&amp;serstatus=1,3,7">Serial receive</a>
@@ -236,6 +236,9 @@
                         <th> Notes </th>
                         <th>Library</th>
                         <th>Call number</th>
+                        [% FOR field IN additional_fields_for_subscription %]
+                          <th>[% field.name %]</th>
+                        [% END %]
                         <th>&nbsp;</th>
                         <th>&nbsp;</th>
                     </tr>
@@ -247,6 +250,9 @@
                       <td><input type="text" class="filter" data-column_num="2" placeholder="Search notes" /></td>
                       <td><input type="text" class="filter" data-column_num="3" placeholder="Search library" /></td>
                       <td><input type="text" class="filter" data-column_num="4" placeholder="Search callnumber" /></td>
+                      [% FOR field IN additional_fields_for_subscription %]
+                        <td><input type="text" class="filter" data-column_num="[% loop.count + 4 %]" placeholder="Search [% field.name %]" /></td>
+                      [% END %]
                       <td></td>
                       <td></td>
                     </tr>
@@ -273,6 +279,13 @@
                         <td>
                           [% IF ( subscription.callnumber ) %][% subscription.callnumber %][% END %]
                         </td>
+                        [% FOR field IN additional_fields_for_subscription %]
+                          [% IF field.authorised_value_category %]
+                            <td>[% KohaAuthorisedValues.GetByCode( field.authorised_value_category, subscription.additional_fields.${field.name} ) %]</td>
+                          [% ELSE %]
+                            <td>[% subscription.additional_fields.${field.name} %]</td>
+                          [% END %]
+                        [% END %]
                         <td>
                           [% UNLESS subscription.cannotedit %]
                             <a href="/cgi-bin/koha/serials/serials-search.pl?subscriptionid=[% subscription.subscriptionid %]&amp;op=reopen&amp;routing=[% subscription.routing %]&amp;searched=1&amp;title_filter=[% title_filter %]&amp;ISSN_filter=[% ISSN_filter %]&amp;EAN_filter=[% EAN_filter %]&amp;published_filter=[% publisher_filter %]&amp;bookseller_filter=[% bookseller_filter %]&amp;branch_filter=[% branch_filter %]" id="reopensub">Reopen</a>
@@ -349,9 +362,9 @@
                 </li>
                 [% FOR field IN additional_fields_for_subscription %]
                   <li>
-                    <label for="additional_field_[% field.name %]"> [% field.name %]: </label>
+                    <label for="additional_field_[% field.id %]ID"> [% field.name %]: </label>
                     [% IF field.authorised_value_choices %]
-                      <select id="additional_field_[% field.name %]" name="additional_field_[% field.name %]_filter">
+                      <select id="additional_field_[% field.id %]" name="additional_field_[% field.id %]_filter">
                         <option value="">All</option>
                         [% FOREACH av IN field.authorised_value_choices %]
                           [% IF av.authorised_value == additional_field_filters.${field.name} %]
@@ -362,7 +375,7 @@
                         [% END %]
                       </select>
                     [% ELSE %]
-                      <input id="additional_field_[% field.name %]" type="text" value="[% additional_field_filters.${field.name} %]" name="additional_field_[% field.name %]_filter" />
+                      <input id="additional_field_[% field.id %]" type="text" value="[% additional_field_filters.${field.name} %]" name="additional_field_[% field.id %]_filter" />
                     [% END %]
                   </li>
                 [% END %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
index 2a963bf..f52cc72 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
@@ -841,9 +841,9 @@ $(document).ready(function() {
                                   <ol>
                                     [% FOR field IN additional_fields_for_subscription %]
                                       <li>
-                                        <label for="additional_fields_[% field.name %]"> [% field.name %]: </label>
+                                        <label for="additional_field_[% field.id %]"> [% field.name %]: </label>
                                         [% IF field.authorised_value_choices %]
-                                          <select name="additional_fields_[% field.name %]">
+                                          <select name="additional_field_[% field.id %]" id="additional_field_[% field.id %]">
                                             [% FOREACH av IN field.authorised_value_choices %]
                                               [% IF av.authorised_value == additional_fields.${field.name} %]
                                                 <option value="[% av.authorised_value %]" selected="selected">[% av.lib %]</option>
@@ -854,10 +854,10 @@ $(document).ready(function() {
                                           </select> (Authorised values for [% field.authorised_value_category %])
                                         [% ELSE %]
                                           [% IF field.marcfield %]
-                                            <input type="text" value="[% additional_fields.${field.name} %]" name="additional_fields_[% field.name %]" readonly="readonly" />
+                                            <input type="text" value="[% additional_fields.${field.name} %]" id="additional_field_[% field.id %]" name="additional_field_[% field.id %]" readonly="readonly" />
                                             This value will be filled with the [% field.marcfield %] subfield of the selected biblio.
                                           [% ELSE %]
-                                            <input type="text" value="[% additional_fields.${field.name} %]" name="additional_fields_[% field.name %]" />
+                                            <input type="text" value="[% additional_fields.${field.name} %]" id="additional_field_[% field.id %]" name="additional_field_[% field.id %]" />
                                           [% END %]
                                         [% END %]
                                       </li>
diff --git a/serials/claims.pl b/serials/claims.pl
index d1af723..443b3b3 100755
--- a/serials/claims.pl
+++ b/serials/claims.pl
@@ -60,7 +60,7 @@ for my $s (@{$supplierlist} ) {
     }
 }
 
-my $additional_fields = Koha::AdditionalField->all( { table => 'subscription', searchable => 1 } );
+my $additional_fields = Koha::AdditionalField->all( { tablename => 'subscription', searchable => 1 } );
 for my $field ( @$additional_fields ) {
     if ( $field->{authorised_value_category} ) {
         $field->{authorised_value_choices} = GetAuthorisedValues( $field->{authorised_value_category} );
diff --git a/serials/serials-search.pl b/serials/serials-search.pl
index 881ef8c..f1b48f8 100755
--- a/serials/serials-search.pl
+++ b/serials/serials-search.pl
@@ -73,10 +73,11 @@ if ( $op and $op eq "close" ) {
 }
 
 
-my $additional_fields = Koha::AdditionalField->all( { table => 'subscription', searchable => 1 } );
+my $additional_fields = Koha::AdditionalField->all( { tablename => 'subscription', searchable => 1 } );
 my $additional_field_filters;
 for my $field ( @$additional_fields ) {
-    if ( my $filter_value = $query->param('additional_field_' . $field->{name} . '_filter') ) {
+    my $filter_value = $query->param('additional_field_' . $field->{id} . '_filter');
+    if ( defined ( $filter_value ) ) {
         $additional_field_filters->{ $field->{name} } = $filter_value;
     }
     if ( $field->{authorised_value_category} ) {
diff --git a/serials/subscription-add.pl b/serials/subscription-add.pl
index 5b8e352..ff7e006 100755
--- a/serials/subscription-add.pl
+++ b/serials/subscription-add.pl
@@ -155,7 +155,7 @@ $template->param(branchloop => $branchloop,
 );
 
 
-my $additional_fields = Koha::AdditionalField->all( { table => 'subscription' } );
+my $additional_fields = Koha::AdditionalField->all( { tablename => 'subscription' } );
 for my $field ( @$additional_fields ) {
     if ( $field->{authorised_value_category} ) {
         $field->{authorised_value_choices} = GetAuthorisedValues( $field->{authorised_value_category} );
@@ -349,13 +349,13 @@ sub redirect_add_subscription {
         $skip_serialseq
     );
 
-    my $additional_fields = Koha::AdditionalField->all( { table => 'subscription' } );
+    my $additional_fields = Koha::AdditionalField->all( { tablename => 'subscription' } );
     my @additional_field_values;
     for my $field ( @$additional_fields ) {
         my $af = Koha::AdditionalField->new({ id => $field->{id} });
         $af->{values} = {
-            $subscriptionid => $query->param('additional_fields_' . $field->{name})
-        };
+            $subscriptionid => $query->param('additional_field_' . $field->{id})
+        } if defined $query->param('additional_field_' . $field->{id});
         $af->insert_values;
     }
 
@@ -434,7 +434,7 @@ sub redirect_mod_subscription {
         $skip_serialseq
     );
 
-    my $additional_fields = Koha::AdditionalField->all( { table => 'subscription' } );
+    my $additional_fields = Koha::AdditionalField->all( { tablename => 'subscription' } );
     my @additional_field_values;
     for my $field ( @$additional_fields ) {
         my $af = Koha::AdditionalField->new({ id => $field->{id} })->fetch;
@@ -448,7 +448,7 @@ sub redirect_mod_subscription {
             };
         } else {
             $af->{values} = {
-                $subscriptionid => $query->param('additional_fields_' . $field->{name})
+                $subscriptionid => $query->param('additional_field_' . $field->{id})
             };
         }
         $af->insert_values;
diff --git a/t/db_dependent/AdditionalField.t b/t/db_dependent/AdditionalField.t
index f7f5227..df307a4 100644
--- a/t/db_dependent/AdditionalField.t
+++ b/t/db_dependent/AdditionalField.t
@@ -1,7 +1,7 @@
 #!/usr/bin/perl
 
 use Modern::Perl;
-use Test::More tests => 34;
+use Test::More tests => 37;
 
 use C4::Context;
 use Koha::AdditionalField;
@@ -104,6 +104,8 @@ use C4::Acquisition;
 use C4::Biblio;
 use C4::Budgets;
 use C4::Serials;
+use C4::Serials::Frequency;
+use C4::Serials::Numberpattern;
 
 my $booksellerid = C4::Bookseller::AddBookseller(
     {
@@ -118,7 +120,7 @@ my ($biblionumber, $biblioitemnumber) = AddBiblio(MARC::Record->new, '');
 my $budgetid;
 my $bpid = AddBudgetPeriod({
     budget_period_startdate => '01-01-2015',
-    budget_period_enddate   => '12-31-2015',
+    budget_period_enddate   => '01-01-2016',
     budget_description      => "budget desc"
 });
 
@@ -132,22 +134,28 @@ my $budget_id = AddBudget({
     budget_period_id   => $bpid
 });
 
+my $frequency_id = AddSubscriptionFrequency({ description => "Test frequency 1" });
+my $pattern_id = AddSubscriptionNumberpattern({
+    label => 'Test numberpattern 1',
+    numberingmethod => '{X}'
+});
+
 my $subscriptionid1 = NewSubscription(
-    undef,      "",     undef, undef, $budget_id, $biblionumber, '01-01-2013',undef,
-    undef,      undef,  undef, undef, undef,      undef,         undef,  undef,
-    undef,      undef,  undef, undef, undef,      undef,         undef,  undef,
-    undef,      undef,  undef, undef, undef,      undef,         undef,  1,
-    "notes",    undef,  undef, undef, undef,      undef,         undef,  0,
-    "intnotes", 0,      undef, undef, 0,          undef,         '31-12-2013',
+    undef,      "",     undef, undef, $budget_id, $biblionumber,
+    '2013-01-01', $frequency_id, undef, undef,  undef,
+    undef,      undef,  undef, undef, undef, undef,
+    1,          "notes",undef, '2013-01-01', undef, $pattern_id,
+    undef,       undef,  0,    "intnotes",  0,
+    undef, undef, 0,          undef,         '2013-01-01', 0
 );
 
 my $subscriptionid2 = NewSubscription(
-    undef,      "",     undef, undef, $budget_id, $biblionumber, '01-01-2013',undef,
-    undef,      undef,  undef, undef, undef,      undef,         undef,  undef,
-    undef,      undef,  undef, undef, undef,      undef,         undef,  undef,
-    undef,      undef,  undef, undef, undef,      undef,         undef,  1,
-    "notes",    undef,  undef, undef, undef,      undef,         undef,  0,
-    "intnotes", 0,      undef, undef, 0,          undef,         '31-12-2013',
+    undef,      "",     undef, undef, $budget_id, $biblionumber,
+    '2013-01-01', $frequency_id, undef, undef,  undef,
+    undef,      undef,  undef, undef, undef, undef,
+    1,          "notes",undef, '2013-01-01', undef, $pattern_id,
+    undef,       undef,  0,    "intnotes",  0,
+    undef, undef, 0,          undef,         '2013-01-01', 0
 );
 
 # insert
@@ -268,4 +276,19 @@ is ( $exists, 1, "get_matching_record_ids: field common: common_value matches su
 $exists = grep /not_existent_id/, @$matching_record_ids;
 is ( $exists, 0, "get_matching_record_ids: field common: common_value does not inexistent id" );
 
+$fields = [
+    {
+        name => 'common',
+        value => q|common|,
+    }
+];
+$matching_record_ids = Koha::AdditionalField->get_matching_record_ids({ tablename => 'subscription', fields => $fields, exact_match => 0 });
+$exists = grep /$subscriptionid1/, @$matching_record_ids;
+is ( $exists, 1, "get_matching_record_ids: field common: common% matches subscription1" );
+$exists = grep /$subscriptionid2/, @$matching_record_ids;
+is ( $exists, 1, "get_matching_record_ids: field common: common% matches subscription2 too" );
+$exists = grep /not_existent_id/, @$matching_record_ids;
+is ( $exists, 0, "get_matching_record_ids: field common: common% does not inexistent id" );
+
+
 $dbh->rollback;
-- 
1.7.10.4