From bcffcaffac1cbd51ec4e1ff3783643973b6ca365 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Fri, 6 Apr 2018 09:35:26 +0200 Subject: [PATCH] Bug 18725: (QA follow-up) Restore status in Letters, adjust test Content-Type: text/plain; charset=utf-8 We need to restore the pending status. Otherwise we will loose some messages. (Just look at the code.) Correct number of tests, do not rely on number processed. The return of SendQueuedMessage is unreliable (just look again at the code). Viewing this as the number of processed/sent messages seems a bit useless. It merely serves as an indication now if we started processing. Test plan: Run Letters.t again. Signed-off-by: Marcel de Rooy --- C4/Letters.pm | 4 +++- t/db_dependent/Letters.t | 25 +++++++++++++++++-------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index 6ec0608..f67a303 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -1061,9 +1061,11 @@ sub SendQueuedMessages { my $unsent_messages = _get_unsent_messages( $which_unsent_messages ); MESSAGE: foreach my $message ( @$unsent_messages ) { my $message_object = Koha::Notice::Messages->find( $message->{message_id} ); - $message_object->status('processing'); + # If this fails the database is unwritable and we won't manage to send a message that continues to be marked 'pending' + $message_object->status('processing'); # enum: same as empty string return unless $message_object->store(); + $message_object->status('pending')->store; # We need to restore the original status in order to prevent possible loss, but this implicitly assumes that the second store worked too. # warn Data::Dumper->Dump( [ $message ], [ 'message' ] ); warn sprintf( 'sending %s message to patron: %s', diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index 8ceddfb..1215c8a 100644 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -18,7 +18,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 77; +use Test::More tests => 78; use Test::MockModule; use Test::Warn; @@ -48,6 +48,8 @@ use Koha::Acquisition::Bookseller::Contacts; use Koha::Acquisition::Orders; use Koha::Libraries; use Koha::Notice::Templates; +use Koha::Notice::Messages; + my $schema = Koha::Database->schema; $schema->storage->txn_begin(); @@ -753,7 +755,7 @@ subtest 'Test limit parameter for SendQueuedMessages' => sub { }; subtest 'Test canary call for SendQueuedMessages' => sub { - plan tests => 2; + plan tests => 4; my $letter = { 'letter' => { @@ -768,14 +770,21 @@ subtest 'Test canary call for SendQueuedMessages' => sub { 'message_transport_type' => 'email', 'from_address' => 'from@example.com' }; - C4::Letters::EnqueueLetter($letter); + my $queued_msg = Koha::Notice::Messages->find( + C4::Letters::EnqueueLetter($letter), + ); + is( $queued_msg->status, 'pending', 'Status should be pending now' ); my $mocked_storing = Test::MockModule->new('Koha::Object'); $mocked_storing->mock('store' => sub { return; }); - $messages_processed = C4::Letters::SendQueuedMessages(); - is( $messages_processed, undef, 'Bad store should prevent sending any.'); + is( C4::Letters::SendQueuedMessages(), undef, 'Bad store should prevent handling any messages.' ); + $queued_msg->discard_changes; # refresh + is( $queued_msg->status, 'pending', 'Status still pending' ); + $mocked_storing->unmock('store'); - $messages_processed = C4::Letters::SendQueuedMessages(); - is( $messages_processed, 1, 'Good store should send.'); + C4::Letters::SendQueuedMessages(); + $queued_msg->discard_changes; # refresh again + is( $queued_msg->status, 'failed', 'Good store and mocked sendmail should trigger status change.' ); +}; -} +$schema->storage->txn_rollback; -- 2.1.4