From 47e4f3ed84c819f61c4325c94316372cb27fa437 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Mon, 26 Mar 2012 11:43:02 -0400 Subject: [PATCH] Talking Tech Support - Phase I - Followup - Fix Messaging Preferences There is a flaw in C4::Members::Messaging::GetMessagingPreferences where the system assumes that every transport will use the same letter. This is not necessarily true. Even with the default preferences of just 'email' and 'sms', we should be able to have different letters for each, as one has a maximum character length ( sms ) and one does not. GetMessagingPreferences currently uses the letter code of the last result of its query as the letter code for every transport type. The returned data is a hashref with a key 'transport_types' that is an array of transport_types this borrower has selected for the given alert. This commit modifies GetMessagingPreferences such that the the 'transport_types' array is now a hash where the name of the transport type is now a key to the value of the letter code set for that transport type. It also modifies code calling GetMessagingPreferences where necessary, and as a side benefit will correctly get the letter codes for email and sms correctly, if they are defined differently. http://bugs.koha-community.org/show_bug.cgi?id=4246 Signed-off-by: Nicole C. Engard In use in production by two libraries: Middletown and Washoe who give their sign off but don't have git to do so. Signed-off-by: Paul Poulain --- C4/Circulation.pm | 2 +- C4/Form/MessagingPreferences.pm | 2 +- C4/Members/Messaging.pm | 6 +++--- C4/Reserves.pm | 24 +++++++++++++----------- admin/categorie.pl | 2 +- misc/cronjobs/advance_notices.pl | 6 +++--- 6 files changed, 22 insertions(+), 20 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 9b523e3a15..d4329a72a5 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2756,7 +2756,7 @@ sub SendCirculationAlert { } ) or return; - my @transports = @{ $borrower_preferences->{transports} }; + my @transports = keys %{ $borrower_preferences->{transports} }; # warn "no transports" unless @transports; for (@transports) { # warn "transport: $_"; diff --git a/C4/Form/MessagingPreferences.pm b/C4/Form/MessagingPreferences.pm index 85ae9e8ceb..fa3c303483 100644 --- a/C4/Form/MessagingPreferences.pm +++ b/C4/Form/MessagingPreferences.pm @@ -139,7 +139,7 @@ sub set_form_values { selected => $_ == $days_in_advance ? 'selected="selected"' :'' } } ( 0..30 ); # FIXME: 30 is a magic number. } - foreach my $transport ( @{$pref->{'transports'}} ) { + foreach my $transport ( keys %{$pref->{'transports'}} ) { $option->{'transports_'.$transport} = 1; } $option->{'digest'} = 1 if $pref->{'wants_digest'}; diff --git a/C4/Members/Messaging.pm b/C4/Members/Messaging.pm index 2b39f115d2..1a9e3c47f9 100644 --- a/C4/Members/Messaging.pm +++ b/C4/Members/Messaging.pm @@ -98,9 +98,8 @@ END_SQL $return->{'days_in_advance'} = $row->{'days_in_advance'} if defined $row->{'days_in_advance'}; $return->{'wants_digest'} = $row->{'wants_digest'} if defined $row->{'wants_digest'}; $return->{'letter_code'} = $row->{'letter_code'}; - $transports{$row->{'message_transport_type'}} = 1; + $return->{'transports'}->{ $row->{'message_transport_type'} } = $row->{'letter_code'}; } - @{$return->{'transports'}} = keys %transports; return $return; } @@ -248,8 +247,9 @@ sub SetMessagingPreferencesFromDefaults { # FIXME - except for setting the borrowernumber, it really ought to be possible # to have the output of GetMessagingPreferences be able to be the input # to SetMessagingPreference + my @message_transport_types = keys %{ $default_pref->{transports} }; $default_pref->{message_attribute_id} = $option->{'message_attribute_id'}; - $default_pref->{message_transport_types} = $default_pref->{transports}; + $default_pref->{message_transport_types} = \@message_transport_types; $default_pref->{borrowernumber} = $params->{borrowernumber}; SetMessagingPreference( $default_pref ); } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index a64abdeec2..32f966de73 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1849,11 +1849,7 @@ sub _koha_notify_reserve { my $messagingprefs; if ( $to_address || $borrower->{'smsalertnumber'} ) { $messagingprefs = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $borrowernumber, message_name => 'Hold_Filled' } ); - - return if ( !defined( $messagingprefs->{'letter_code'} ) ); - $letter_code = $messagingprefs->{'letter_code'}; } else { - $letter_code = 'HOLD_PRINT'; $print_mode = 1; } @@ -1869,9 +1865,8 @@ sub _koha_notify_reserve { my $admin_email_address = $branch_details->{'branchemail'} || C4::Context->preference('KohaAdminEmailAddress'); - my $letter = C4::Letters::GetPreparedLetter ( + my %letter_params = ( module => 'reserves', - letter_code => $letter_code, branchcode => $reserve->{branchcode}, tables => { 'branches' => $branch_details, @@ -1881,11 +1876,13 @@ sub _koha_notify_reserve { 'items', $reserve->{'itemnumber'}, }, substitute => { today => C4::Dates->new()->output() }, - ) or die "Could not find a letter called '$letter_code' in the 'reserves' module"; - + ); if ( $print_mode ) { + $letter_params{ 'letter_code' } = 'HOLD_PRINT'; + my $letter = C4::Letters::GetPreparedLetter ( %letter_params ) or die "Could not find a letter called '$letter_params{'letter_code'}' in the 'reserves' module"; + C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $borrowernumber, @@ -1895,8 +1892,10 @@ sub _koha_notify_reserve { return; } - if ( grep { $_ eq 'email' } @{$messagingprefs->{transports}} ) { - # aka, 'email' in ->{'transports'} + if ( $to_address && defined $messagingprefs->{transports}->{'email'} ) { + $letter_params{ 'letter_code' } = $messagingprefs->{transports}->{'email'}; + my $letter = C4::Letters::GetPreparedLetter ( %letter_params ) or die "Could not find a letter called '$letter_params{'letter_code'}' in the 'reserves' module"; + C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $borrowernumber, @@ -1906,7 +1905,10 @@ sub _koha_notify_reserve { ); } - if ( grep { $_ eq 'sms' } @{$messagingprefs->{transports}} ) { + if ( $borrower->{'smsalertnumber'} && defined $messagingprefs->{transports}->{'sms'} ) { + $letter_params{ 'letter_code' } = $messagingprefs->{transports}->{'sms'}; + my $letter = C4::Letters::GetPreparedLetter ( %letter_params ) or die "Could not find a letter called '$letter_params{'letter_code'}' in the 'reserves' module"; + C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $borrowernumber, diff --git a/admin/categorie.pl b/admin/categorie.pl index d11b100ce7..7a02a5d5fb 100755 --- a/admin/categorie.pl +++ b/admin/categorie.pl @@ -250,7 +250,7 @@ sub _get_brief_messaging_prefs { message_name => $option->{'message_name'}, $option->{'message_name'} => 1 }; - foreach my $transport ( @{$pref->{'transports'}} ) { + foreach my $transport ( keys %{$pref->{'transports'}} ) { push @{ $brief_pref->{'transports'} }, { transport => $transport }; } push @$results, $brief_pref; diff --git a/misc/cronjobs/advance_notices.pl b/misc/cronjobs/advance_notices.pl index de00846039..e89b8ae830 100755 --- a/misc/cronjobs/advance_notices.pl +++ b/misc/cronjobs/advance_notices.pl @@ -210,7 +210,7 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { print $letter->{'content'}; } else { - foreach my $transport ( @{$borrower_preferences->{'transports'}} ) { + foreach my $transport ( keys %{$borrower_preferences->{'transports'}} ) { C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $upcoming->{'borrowernumber'}, from_address => $from_address, @@ -264,7 +264,7 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$upcoming_digest ) { print $letter->{'content'}; } else { - foreach my $transport ( @{$borrower_preferences->{'transports'}} ) { + foreach my $transport ( keys %{$borrower_preferences->{'transports'}} ) { C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $borrowernumber, from_address => $from_address, @@ -303,7 +303,7 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$due_digest ) { print $letter->{'content'}; } else { - foreach my $transport ( @{$borrower_preferences->{'transports'}} ) { + foreach my $transport ( keys %{$borrower_preferences->{'transports'}} ) { C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $borrowernumber, from_address => $from_address, -- 2.39.5