From 4f85fa334a0f4d518e0f37578f56afe8f22dcb43 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 --- 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 9c43f36..6a9fa1e 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -2725,7 +2725,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 85ae9e8..fa3c303 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 2b39f11..1a9e3c4 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 276658c..b7daa9c 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1717,11 +1717,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; } @@ -1737,9 +1733,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, @@ -1749,11 +1744,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, @@ -1763,8 +1760,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, @@ -1774,7 +1773,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 dc360c2..12ee77a 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 de00846..e89b8ae 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, -- 1.7.2.5