Bug 27921: Log correct timestamp for HOLD MODIFY when set waiting

The HOLD MODIFY log at the end of ModReserveAffect is not using an
up-to-date $hold object.

$hold is modified at
1201         $hold->set_waiting($desk_id);
But not refreshed before logged (and so the timestamp is not logged
correctly).

Test plan:
Turn on HoldsLog
Place an item on hold
Check it in to mark it waiting

Confirm that the timestamp logged is the one from the check in, not when
you created the hold

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(cherry picked from commit cb03909af6)
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

C4/Reserves.pm

@@ -1230,7 +1230,7 @@ sub ModReserveAffect {
     });
     $std->execute($hold->reserve_id);
 
-    logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, Dumper($hold->unblessed) )
+    logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, Dumper($hold->get_from_storage->unblessed) )
         if C4::Context->preference('HoldsLog');
 
     return;

C4/Search.pm

@@ -118,7 +118,8 @@ sub FindDuplicate {
         }
     }
 
-    my ( $error, $searchresults, undef ) = SimpleSearch($query); # FIXME :: hardcoded !
+    my $searcher = Koha::SearchEngine::Search->new({index => $Koha::SearchEngine::BIBLIOS_INDEX});
+    my ( $error, $searchresults, undef ) = $searcher->simple_search_compat($query,0,50);
     my @results;
     if (!defined $error) {
         foreach my $possible_duplicate_record (@{$searchresults}) {

Koha/Email.pm

@@ -78,7 +78,7 @@ sub create {
     my $args = {};
     $args->{from} = $params->{from} || C4::Context->preference('KohaAdminEmailAddress');
     Koha::Exceptions::BadParameter->throw("Invalid 'from' parameter: ".$args->{from})
-        unless Email::Valid->address($args->{from}); # from is mandatory
+        unless Email::Valid->address( -address => $args->{from}, -fqdn => 0 ); # from is mandatory
 
     $args->{subject} = $params->{subject} // '';
 
@@ -90,7 +90,7 @@ sub create {
     }
 
     Koha::Exceptions::BadParameter->throw("Invalid 'to' parameter: ".$args->{to})
-        unless Email::Valid->address($args->{to}); # to is mandatory
+        unless Email::Valid->address( -address => $args->{to}, -fqdn => 0 ); # to is mandatory
 
     my $addresses = {};
     $addresses->{reply_to} = $params->{reply_to};
@@ -108,9 +108,13 @@ sub create {
             if exists $params->{bcc};
     }
 
-    foreach my $address ( keys %{ $addresses } ) {
-        Koha::Exceptions::BadParameter->throw("Invalid '$address' parameter: ".$addresses->{$address})
-            if $addresses->{$address} and !Email::Valid->address($addresses->{$address});
+    foreach my $address ( keys %{$addresses} ) {
+        Koha::Exceptions::BadParameter->throw(
+            "Invalid '$address' parameter: " . $addresses->{$address} )
+          if $addresses->{$address} and !Email::Valid->address(
+            -address => $addresses->{$address},
+            -fqdn    => 0
+          );
     }
 
     $args->{cc} = $addresses->{cc}

koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt

@@ -205,7 +205,11 @@
                                                   [% ELSE %]
                                                       <option value="[% budget.b_id | html %]">
                                                   [% END %]
-                                                  [% budget.b_txt | html %]
+                                                  [% IF budget.b_active %]
+                                                      [% budget.b_txt | html %]
+                                                  [% ELSE %]
+                                                      [% budget.b_txt | html %] (inactive)
+                                                  [% END %]
                                                   </option>
                                               [% END %]
                                           </select>

koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt

@@ -147,7 +147,7 @@
 
 [% IF ( searchfield ) %]
     <h1>Authorized values for category [% category.category_name | html %]</h1>
-    [% PROCESS category_descriptions code_category = category %]
+    [% PROCESS category_descriptions code_category = category.category_name %]
 [% ELSE %]
     <h1>Authorized values</h1>
 [% END %]

koha-tmpl/intranet-tmpl/prog/en/modules/clubs/clubs.tt

@@ -160,21 +160,21 @@
                 ]
             } ));
 
-            $(".delete_club").on("click", function(e){
+            $("body").on("click", ".delete_club", function(e){
                 e.preventDefault();
                 var club_id = $(this).data("id");
                 var club_name = $(this).data("name");
                 ConfirmDeleteClub( club_id, club_name );
             });
 
-            $(".delete_template").on("click", function(e){
+            $("body").on("click", ".delete_template", function(e){
                 e.preventDefault();
                 var template_id = $(this).data("id");
                 var template_name = $(this).data("name");
                 ConfirmDeleteTemplate( template_id, template_name );
             });
 
-            $(".club_hold_search").on("click", function(e){
+            $("body").on("click", ".club_hold_search", function(e){
                 e.preventDefault;
                 var club_id = $(this).data("id");
                 SearchToHold( club_id );

koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt

@@ -684,7 +684,11 @@
                                                         [% END %]
                                                     </td>
                                                     <td>
+                                                    [% IF itemloo.any_pickup_location %]
+                                                        Any library
+                                                    [% ELSE %]
                                                         [% itemloo.pickup_locations | html %]
+                                                    [% END %]
                                                     </td>
                                                 </tr>
                                             [% END # / UNLESS itemloo.hide %]

koha-tmpl/intranet-tmpl/prog/js/calendar.js

@@ -192,6 +192,8 @@ $(document).ready(function () {
     }).on("change", function () {
         if (!is_valid_date($(this).val())) {
             $(this).val("");
+        } else {
+            $(this).datepicker("setDate",$(this).val());
         }
     });
     // http://jqueryui.com/demos/datepicker/#date-range

koha-tmpl/intranet-tmpl/prog/js/datatables.js

@@ -590,13 +590,15 @@ jQuery.fn.dataTable.ext.errMode = function(settings, note, message) {
 
                                 if(options.columns) {
                                     var order = data.order;
+                                    var orderArray = new Array();
                                     order.forEach(function (e,i) {
                                         var order_col      = e.column;
                                         var order_by       = options.columns[order_col].data;
                                         order_by           = order_by.split(':');
                                         var order_dir      = e.dir == 'asc' ? '+' : '-';
-                                        dataSet._order_by = order_by.map(x => order_dir + (!x.includes('.')?'me.'+x:x)).join(',');
+                                        Array.prototype.push.apply(orderArray,order_by.map(x => order_dir + (!x.includes('.')?'me.'+x:x)));
                                     });
+                                    dataSet._order_by = orderArray.filter((v, i, a) => a.indexOf(v) === i).join(',');
                                 }
 
                                 return dataSet;

reserve/request.pl

@@ -568,17 +568,15 @@ foreach my $biblionumber (@biblionumbers) {
                 {
                     $item->{available} = 1;
                     $num_available++;
-                    if($branchitemrule->{'hold_fulfillment_policy'} eq 'any' ) {
-                        $item->{pickup_locations} = 'Any library';
-                        $item->{pickup_locations_code} = 'all';
-                    } else {
-                        my $arr_locations = Koha::Items->find($itemnumber)
-                                    ->pickup_locations( { patron => $patron } )->as_list();
 
-                        $item->{pickup_locations} = join( ', ',
-                            map { $_->unblessed->{branchname} } @$arr_locations);
-                        $item->{pickup_locations_code} = join( ',',
-                            map { $_->unblessed->{branchcode} } @$arr_locations);
+                    if ( $branchitemrule->{'hold_fulfillment_policy'} eq 'any' )
+                    {
+                        $item->{any_pickup_location} = 1;
+                    }
+                    else {
+                        my @pickup_locations = $item_object->pickup_locations({ patron => $patron });
+
+                        $item->{pickup_locations} = join( ', ', map { $_->branchname } @pickup_locations );
                     }
 
                     push( @available_itemtypes, $item->{itype} );

t/Koha/Email.t

@@ -28,7 +28,7 @@ use_ok('Koha::Email');
 
 subtest 'create() tests' => sub {
 
-    plan tests => 23;
+    plan tests => 25;
 
     t::lib::Mocks::mock_preference( 'SendAllEmailsTo', undef );
 
@@ -62,6 +62,17 @@ subtest 'create() tests' => sub {
     like( $email->email->content_type, qr|charset="?iso-8859-1"?|, "Charset set correctly");
     like( $email->email->header('Message-ID'), qr/\<.*@.*\>/, 'Value set correctly' );
 
+    $email = Koha::Email->create(
+        {
+            from        => 'from@8.8.8.8',
+            to          => 'to@example.com',
+            bcc         => 'root@localhost',
+        }
+    );
+
+    is( $email->email->header('Bcc'), 'root@localhost', 'Non-FQDN (@localhost) supported' );
+    is( $email->email->header('From'), 'from@8.8.8.8', 'IPs supported' );
+
     t::lib::Mocks::mock_preference( 'SendAllEmailsTo', 'catchall@example.com' );
     t::lib::Mocks::mock_preference( 'ReplytoDefault', 'replytodefault@example.com' );
     t::lib::Mocks::mock_preference( 'ReturnpathDefault', 'returnpathdefault@example.com' );

t/db_dependent/Letters.t

@@ -962,7 +962,7 @@ subtest 'Test limit parameter for SendQueuedMessages' => sub {
 
 subtest 'Test message_id parameter for SendQueuedMessages' => sub {
 
-    plan tests => 5;
+    plan tests => 6;
 
     my $dbh = C4::Context->dbh;
 
@@ -987,7 +987,7 @@ subtest 'Test message_id parameter for SendQueuedMessages' => sub {
         'borrowernumber'         => $borrowernumber,
         'to_address'             => 'to@example.org',
         'message_transport_type' => 'email',
-        'from_address'           => 'root@localhost' # invalid KohaAdminEmailAddress
+        'from_address'           => 'root@localhost.' # invalid KohaAdminEmailAddress
     };
     my $message_id = C4::Letters::EnqueueLetter($my_message);
     throws_ok {
@@ -1000,7 +1000,9 @@ subtest 'Test message_id parameter for SendQueuedMessages' => sub {
 
     $my_message->{from_address} = 'root@example.org'; # valid KohaAdminEmailAddress
     $message_id = C4::Letters::EnqueueLetter($my_message);
-    C4::Letters::SendQueuedMessages( { message_id => $message_id } );
+    warning_like { C4::Letters::SendQueuedMessages( { message_id => $message_id } ); }
+        qr|Fake send_or_die|,
+        "SendQueuedMessages is using the mocked send_or_die routine";
     $message_1 = C4::Letters::GetMessage($message_1->{message_id});
     my $message_2 = C4::Letters::GetMessage($message_id);
     is( $message_1->{status}, 'pending', 'Message 1 status is unchanged' ); # Must be 'failed'

t/db_dependent/Reserves.t

@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 66;
+use Test::More tests => 67;
 use Test::MockModule;
 use Test::Warn;
 
@@ -32,6 +32,7 @@ use C4::Items;
 use C4::Biblio;
 use C4::Members;
 use C4::Reserves;
+use Koha::ActionLogs;
 use Koha::Caches;
 use Koha::DateUtils;
 use Koha::Holds;
@@ -1217,6 +1218,50 @@ subtest 'MergeHolds' => sub {
     is( $biblio_2->holds->count, 1, 'Hold has been transferred' );
 };
 
+subtest 'ModReserveAffect logging' => sub {
+
+    plan tests => 4;
+
+    my $item = $builder->build_sample_item;
+    my $patron = $builder->build_object(
+        {
+            class => "Koha::Patrons",
+            value => { branchcode => $item->homebranch }
+        }
+    );
+
+    t::lib::Mocks::mock_userenv({ patron => $patron });
+    t::lib::Mocks::mock_preference('HoldsLog', 1);
+
+    my $reserve_id = AddReserve(
+        {
+            branchcode     => $item->homebranch,
+            borrowernumber => $patron->borrowernumber,
+            biblionumber   => $item->biblionumber,
+            priority       => 1,
+            itemnumber     => $item->itemnumber,
+        }
+    );
+
+    my $hold = Koha::Holds->find($reserve_id);
+    my $previous_timestamp = '1970-01-01 12:34:56';
+    $hold->timestamp($previous_timestamp)->store;
+
+    $hold = Koha::Holds->find($reserve_id);
+    is( $hold->timestamp, $previous_timestamp, 'Make sure the previous timestamp has been used' );
+
+    # Mark it waiting
+    ModReserveAffect( $item->itemnumber, $patron->borrowernumber );
+
+    $hold = Koha::Holds->find($reserve_id);
+    is( $hold->found, 'W', 'Hold has been set waiting' );
+    isnt( $hold->timestamp, $previous_timestamp, 'The timestamp has been modified' );
+
+    my $log = Koha::ActionLogs->search({ module => 'HOLDS', action => 'MODIFY', object => $hold->reserve_id })->next;
+    my $expected = sprintf q{'timestamp' => '%s'}, $hold->timestamp;
+    like( $log->info, qr{$expected}, 'Timestamp logged is the current one' );
+};
+
 sub count_hold_print_messages {
     my $message_count = $dbh->selectall_arrayref(q{
         SELECT COUNT(*)

t/db_dependent/Search.t

@@ -925,37 +925,48 @@ subtest 'UNIMARC + DOM' => sub {
 
 
 subtest 'FindDuplicate' => sub {
-    plan tests => 3;
+    plan tests => 6;
     Koha::Caches->get_instance('config')->flush_all;
     t::lib::Mocks::mock_preference('marcflavour', 'marc21' );
     mock_GetMarcSubfieldStructure('marc21');
-    my $searcher = Test::MockModule->new('C4::Search');
-    $searcher->mock('SimpleSearch', sub {
+    my $z_searcher = Test::MockModule->new('C4::Search');
+    $z_searcher->mock('SimpleSearch', sub {
+        warn shift @_;
+        return 1;
+    });
+    my $e_searcher = Test::MockModule->new('Koha::SearchEngine::Elasticsearch::Search');
+    $e_searcher->mock('simple_search_compat', sub {
+        shift @_;
         warn shift @_;
         return 1;
     });
 
-    my $record = MARC::Record->new;
-    $record->add_fields(
+    my $record_1 = MARC::Record->new;
+    $record_1 ->add_fields(
             [ '100', '0', '0', a => 'Morgenstern, Erin' ],
             [ '245', '0', '0', a => 'The night circus /' ]
     );
-    warning_is { C4::Search::FindDuplicate($record);}
-        q/ti,ext:"The night circus \/" and au,ext:"Morgenstern, Erin"/,"Term correctly formed";
-
-    $record = MARC::Record->new;
-    $record->add_fields(
+    my $record_2 = MARC::Record->new;
+    $record_2 ->add_fields(
             [ '245', '0', '0', a => 'The book of nothing /' ]
     );
-    warning_is { C4::Search::FindDuplicate($record);}
-        q/ti,ext:"The book of nothing \/"/,"Term correctly formed";
-
-    $record = MARC::Record->new;
-    $record->add_fields(
+    my $record_3 = MARC::Record->new;
+    $record_3->add_fields(
             [ '245', '0', '0', a => 'Frog and toad all year /' ]
     );
-    warning_is { C4::Search::FindDuplicate($record);}
-        q/ti,ext:"Frog and toad all year \/"/,"Term correctly formed";
+
+    foreach my $engine ('Zebra','Elasticsearch'){
+        t::lib::Mocks::mock_preference('searchEngine', $engine );
+
+        warning_is { C4::Search::FindDuplicate($record_1);}
+            q/ti,ext:"The night circus \/" and au,ext:"Morgenstern, Erin"/,"Term correctly formed and passed to $engine";
+
+        warning_is { C4::Search::FindDuplicate($record_2);}
+            q/ti,ext:"The book of nothing \/"/,"Term correctly formed and passed to $engine";
+
+        warning_is { C4::Search::FindDuplicate($record_3);}
+            q/ti,ext:"Frog and toad all year \/"/,"Term correctly formed and passed to $engine";
+    }
 
 };