From 5955a1c7bf84a53980cbc3b32271821c1b040805 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 19 Jan 2017 20:11:21 +0100 Subject: [PATCH] Bug 17969: Refactor the way <> is generated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit <> is generated 4x in advance_notices.pl and once in overdue_notices.pl It would be better to have it in C4::Letters. It will enforce the fact that it already has the same behavior, make it testable and reusable. Test plan: Use the <> tag for advance and overdue notices. The generated notices must be the same as before this patch. Followed test plan, works as expected. Signed-off-by: Marc VĂ©ron Signed-off-by: Tomas Cohen Arazi --- C4/Letters.pm | 19 +++++++++++++++++++ misc/cronjobs/advance_notices.pl | 18 ++++-------------- misc/cronjobs/overdue_notices.pl | 7 ++----- t/db_dependent/Letters.t | 36 +++++++++++++++++++++++++++++++++++- 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index de8253b..a3821ea 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -1632,6 +1632,25 @@ sub _get_tt_params { return $params; } +sub get_item_content { + my ( $params ) = @_; + my $item = $params->{item}; + my $dateonly = $params->{dateonly} || 0; + my $item_content_fields = $params->{item_content_fields} || []; + + return unless $item; + + my @item_info = map { + $_ =~ /^date|date$/ + ? eval { + output_pref( + { dt => dt_from_string( $item->{$_} ), dateonly => $dateonly } ); + } + : $item->{$_} + || '' + } @$item_content_fields; + return join( "\t", @item_info ) . "\n"; +} 1; __END__ diff --git a/misc/cronjobs/advance_notices.pl b/misc/cronjobs/advance_notices.pl index 0475ee0..73697b8 100755 --- a/misc/cronjobs/advance_notices.pl +++ b/misc/cronjobs/advance_notices.pl @@ -258,8 +258,7 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { $sth->execute($upcoming->{'borrowernumber'},$upcoming->{'itemnumber'},'0'); my $titles = ""; while ( my $item_info = $sth->fetchrow_hashref()) { - my @item_info = map { $_ =~ /^date|date$/ ? format_date($item_info->{$_}) : $item_info->{$_} || '' } @item_content_fields; - $titles .= join("\t",@item_info) . "\n"; + $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); } ## Get branch info for borrowers home library. @@ -292,8 +291,7 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { $sth->execute($upcoming->{'borrowernumber'},$upcoming->{'itemnumber'},$borrower_preferences->{'days_in_advance'}); my $titles = ""; while ( my $item_info = $sth->fetchrow_hashref()) { - my @item_info = map { $_ =~ /^date|date$/ ? format_date($item_info->{$_}) : $item_info->{$_} || '' } @item_content_fields; - $titles .= join("\t",@item_info) . "\n"; + $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); } ## Get branch info for borrowers home library. @@ -358,8 +356,7 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$upcoming_digest ) { $sth->execute($borrowernumber,$borrower_preferences->{'days_in_advance'}); my $titles = ""; while ( my $item_info = $sth->fetchrow_hashref()) { - my @item_info = map { $_ =~ /^date|date$/ ? format_date($item_info->{$_}) : $item_info->{$_} || '' } @item_content_fields; - $titles .= join("\t",@item_info) . "\n"; + $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); } ## Get branch info for borrowers home library. @@ -415,8 +412,7 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$due_digest ) { $sth->execute($borrowernumber,'0'); my $titles = ""; while ( my $item_info = $sth->fetchrow_hashref()) { - my @item_info = map { $_ =~ /^date|date$/ ? format_date($item_info->{$_}) : $item_info->{$_} || '' } @item_content_fields; - $titles .= join("\t",@item_info) . "\n"; + $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); } ## Get branch info for borrowers home library. @@ -497,12 +493,6 @@ sub parse_letter { ); } -sub format_date { - my $date_string = shift; - my $dt=dt_from_string($date_string); - return output_pref($dt); -} - =head2 get_branch_info =cut diff --git a/misc/cronjobs/overdue_notices.pl b/misc/cronjobs/overdue_notices.pl index 7497381..a1b477c 100755 --- a/misc/cronjobs/overdue_notices.pl +++ b/misc/cronjobs/overdue_notices.pl @@ -663,11 +663,8 @@ END_SQL last; } $j++; - my @item_info = map { $_ =~ /^date|date$/ ? - eval { output_pref( { dt => dt_from_string( $item_info->{$_} ), dateonly => 1 } ); } - : - $item_info->{$_} || '' } @item_content_fields; - $titles .= join("\t", @item_info) . "\n"; + + $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields, dateonly => 1 } ); $itemcount++; push @items, $item_info; } diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index cbb2bec..2c96e23 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 => 75; +use Test::More tests => 76; use Test::MockModule; use Test::Warn; @@ -649,5 +649,39 @@ subtest 'SendQueuedMessages' => sub { status => 'sent' })->next()->to_address(); is( $sms_message_address, '5555555555@kidclamp.rocks', 'SendQueuedMessages populates the to address correctly for SMS by email' ); +}; + +subtest 'get_item_content' => sub { + plan tests => 2; + + t::lib::Mocks::mock_preference('dateformat', 'metric'); + t::lib::Mocks::mock_preference('timeformat', '24hr'); + my @items = ( + {date_due => '2041-01-01 12:34', title => 'a first title', barcode => 'a_first_barcode', author => 'a_first_author', itemnumber => 1 }, + {date_due => '2042-01-02 23:45', title => 'a second title', barcode => 'a_second_barcode', author => 'a_second_author', itemnumber => 2 }, + ); + my @item_content_fields = qw( date_due title barcode author itemnumber ); + + my $items_content; + for my $item ( @items ) { + $items_content .= C4::Letters::get_item_content( { item => $item, item_content_fields => \@item_content_fields } ); + } + + my $expected_items_content = < $item, item_content_fields => \@item_content_fields, dateonly => 1, } ); + } + $expected_items_content = < 1)' ); }; -- 2.7.4