Attachment 23600 Details for Bug 10855 – Bug 10855: Squash several fixes

[patch] Bug 10855: Squash several fixes

Bug-10855-Squash-several-fixes.patch (text/plain), 15.96 KB, created by Jonathan Druart on 2013-12-17 12:50:07 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2013-12-17 12:50:07 UTC

Size: 15.96 KB

patch

obsolete

>From 705fffa539ca2d732c08075c1eb7732dd9bfa0f7 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/subscription-add.tt    |    8 +--
> serials/claims.pl                                  |    2 +-
> serials/serials-search.pl                          |    5 +-
> serials/subscription-add.pl                        |   12 ++---
> t/db_dependent/AdditionalField.t                   |   51 ++++++++++++++------
> 8 files changed, 61 insertions(+), 35 deletions(-)
>
>diff --git a/C4/Serials.pm b/C4/Serials.pm
>index 31f1cdc..1ad72e0 100644
>--- a/C4/Serials.pm
>+++ b/C4/Serials.pm
>@@ -669,8 +669,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;
>     }
>@@ -772,7 +773,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/subscription-add.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
>index 83a7edf..153ec66 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
>@@ -843,9 +843,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>
>@@ -856,10 +856,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 199eee2..b612c70 100755
>--- a/serials/serials-search.pl
>+++ b/serials/serials-search.pl
>@@ -78,10 +78,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

Actions: View | Diff | Splinter Review

Attachments on bug 10855: 20937 | 20938 | 20939 | 20940 | 20941 | 20942 | 20943 | 20944 | 21184 | 21185 | 21186 | 21187 | 21188 | 21189 | 21190 | 21191 | 21663 | 22049 | 22050 | 22051 | 22052 | 22053 | 22054 | 22055 | 22056 | 22057 | 22359 | 22360 | 22600 | 22601 | 22602 | 22603 | 22604 | 22605 | 22606 | 22607 | 22608 | 22616 | 22698 | 22699 | 22700 | 22701 | 22702 | 22703 | 22722 | 22768 | 22769 | 22869 | 23052 | 23053 | 23142 | 23143 | 23301 | 23302 | 23594 | 23595 | 23596 | 23597 | 23598 | 23599 | 23600 | 23601 | 23602 | 23774 | 23775 | 23961 | 23962 | 23963 | 23964 | 24452 | 24453 | 24454 | 24455 | 24456 | 24457 | 24569 | 24570 | 26091 | 26092 | 26093 | 26094 | 26095 | 26096 | 26097 | 26254 | 26255 | 26256 | 26257 | 26258 | 26259 | 26260 | 26276 | 26651 | 26655 | 28929 | 28930 | 28931 | 28932 | 28933 | 28934 | 28935 | 28936 | 28937 | 28938 | 28939 | 28940 | 28941 | 29095 | 29161 | 35211 | 35212 | 35213 | 35214 | 35215 | 35216 | 35217 | 35218 | 35219 | 35220 | 35221 | 35309 | 35310 | 35369 | 35556 | 36359 | 36362 | 36363 | 36364 | 36365 | 36366 | 36367 | 36368 | 36369 | 36370 | 36371 | 36372 | 36373 | 36374 | 38549 | 38550 | 38551 | 38552 | 38553 | 38554 | 38555 | 38556 | 38557 | 38558 | 38559 | 38560 | 38561 | 42296 | 42297 | 42298 | 42299 | 42300 | 42301 | 42302 | 42303 | 42304 | 42305 | 42306 | 42307 | 42308 | 42309 | 42310 | 42332 | 42333 | 43076