From 13079390a01d887da4cb6bb1b7c4d82f7962fdf3 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Thu, 27 Oct 2016 16:08:50 +0300 Subject: [PATCH] Bug 18595: Move C4::Members::Messaging::GetMessagingPreferences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch moves C4::Members::Messaging::GetMessagingPreferences to Koha::Patron::Message::Preference(s) The patch makes a heavy touch on core messaging preference features, but should not introduce any visible changes for end user. Keep this in mind if testing. If you notice any changes in Koha, something is probably wrong! To test: Make sure you have enabled the following system preferences: - EnhancedMessagingPreferences ---- GENERAL ---- 1. Apply patch 2. Search Koha source code for references of "GetMessagingPreferences" (grep -r 'GetMessagingPreferences'). 3. Observe no results in source code. ---- INTRANET ---- 1. todo ---- UNIT TESTS ---- 1. prove t/db_dependent/Circulation.t 2. prove t/db_dependent/Reserves.t 3. prove t/db_dependent/Koha/Patron/Categories.t Signed-off-by: Marc VĂ©ron --- C4/Circulation.pm | 6 ++-- C4/Form/MessagingPreferences.pm | 9 ++--- C4/Members/Messaging.pm | 63 --------------------------------- C4/Reserves.pm | 13 ++++--- Koha/Patron/Category.pm | 14 ++++---- circ/returns.pl | 8 +++-- misc/cronjobs/advance_notices.pl | 47 ++++++++++++++---------- t/db_dependent/Koha/Patron/Categories.t | 6 ++-- 8 files changed, 60 insertions(+), 106 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index a439021..85014b4 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -48,6 +48,7 @@ use Koha::IssuingRules; use Koha::Items; use Koha::Patrons; use Koha::Patron::Debarments; +use Koha::Patron::Message::Preferences; use Koha::Database; use Koha::Libraries; use Koha::Holds; @@ -3376,14 +3377,15 @@ sub SendCirculationAlert { CHECKOUT => 'Item_Checkout', RENEWAL => 'Item_Checkout', ); - my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences({ + return unless my $borrower_preferences = Koha::Patron::Message::Preferences->find_with_message_name({ borrowernumber => $borrower->{borrowernumber}, message_name => $message_name{$type}, }); + my $issues_table = ( $type eq 'CHECKOUT' || $type eq 'RENEWAL' ) ? 'issues' : 'old_issues'; my $schema = Koha::Database->new->schema; - my @transports = keys %{ $borrower_preferences->{transports} }; + my @transports = keys %{ $borrower_preferences->message_transport_types }; # From the MySQL doc: # LOCK TABLES is not transaction-safe and implicitly commits any active transaction before attempting to lock the tables. diff --git a/C4/Form/MessagingPreferences.pm b/C4/Form/MessagingPreferences.pm index 888a47d..53885ed 100644 --- a/C4/Form/MessagingPreferences.pm +++ b/C4/Form/MessagingPreferences.pm @@ -133,21 +133,22 @@ sub set_form_values { # walk through the options and update them with these borrower_preferences my $messaging_options = Koha::Patron::Message::Preferences->get_options; PREF: foreach my $option ( @$messaging_options ) { - my $pref = C4::Members::Messaging::GetMessagingPreferences( { %{ $target_params }, message_name => $option->{'message_name'} } ); + my $pref = Koha::Patron::Message::Preferences->find( { %{ $target_params }, message_attribute_id => $option->{'message_attribute_id'} } ); $option->{ $option->{'message_name'} } = 1; # make a hashref of the days, selecting one. if ( $option->{'takes_days'} ) { - my $days_in_advance = $pref->{'days_in_advance'} ? $pref->{'days_in_advance'} : 0; + my $days_in_advance = $pref && $pref->days_in_advance ? $pref->days_in_advance : 0; $option->{days_in_advance} = $days_in_advance; @{$option->{'select_days'}} = map { { day => $_, selected => $_ == $days_in_advance } } ( 0..MAX_DAYS_IN_ADVANCE ); } - foreach my $transport ( keys %{$pref->{'transports'}} ) { + next unless $pref; + foreach my $transport ( keys %{$pref->message_transport_types} ) { $option->{'transports_'.$transport} = 1; } - $option->{'digest'} = 1 if $pref->{'wants_digest'}; + $option->{'digest'} = 1 if $pref->wants_digest; } $template->param(messaging_preferences => $messaging_options); } diff --git a/C4/Members/Messaging.pm b/C4/Members/Messaging.pm index 6019eed..d28de2c 100644 --- a/C4/Members/Messaging.pm +++ b/C4/Members/Messaging.pm @@ -35,69 +35,6 @@ This module lets you modify a patron's messaging preferences. =head1 FUNCTIONS -=head2 GetMessagingPreferences - - my $preferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $borrower->{'borrowernumber'}, - message_name => 'DUE' } ); - - my $preferences = C4::Members::Messaging::GetMessagingPreferences( { categorycode => 'LIBRARY', - message_name => 'DUE' } ); - -returns: a hashref of messaging preferences for a borrower or patron category for a particlar message_name - -Requires either a borrowernumber or a categorycode key, but not both. - -=cut - -sub GetMessagingPreferences { - my $params = shift; - - return unless exists $params->{message_name}; - return unless exists $params->{borrowernumber} xor exists $params->{categorycode}; # yes, xor - my $sql = <<'END_SQL'; -SELECT borrower_message_preferences.*, - borrower_message_transport_preferences.message_transport_type, - message_attributes.message_name, - message_attributes.takes_days, - message_transports.is_digest, - message_transports.letter_module, - message_transports.letter_code -FROM borrower_message_preferences -LEFT JOIN borrower_message_transport_preferences -ON borrower_message_transport_preferences.borrower_message_preference_id = borrower_message_preferences.borrower_message_preference_id -LEFT JOIN message_attributes -ON message_attributes.message_attribute_id = borrower_message_preferences.message_attribute_id -LEFT JOIN message_transports -ON message_transports.message_attribute_id = message_attributes.message_attribute_id -AND message_transports.message_transport_type = borrower_message_transport_preferences.message_transport_type -AND message_transports.is_digest = borrower_message_preferences.wants_digest -WHERE message_attributes.message_name = ? -END_SQL - - my @bind_params = ( $params->{'message_name'} ); - if ( exists $params->{'borrowernumber'} ) { - $sql .= " AND borrower_message_preferences.borrowernumber = ? "; - push @bind_params, $params->{borrowernumber}; - } else { - $sql .= " AND borrower_message_preferences.categorycode = ? "; - push @bind_params, $params->{categorycode}; - } - - my $sth = C4::Context->dbh->prepare($sql); - $sth->execute(@bind_params); - my $return; - my %transports; # helps build a list of unique message_transport_types - ROW: while ( my $row = $sth->fetchrow_hashref() ) { - next ROW unless $row->{'message_attribute_id'}; - $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'}; - next unless defined $row->{'message_transport_type'}; - $return->{'transports'}->{ $row->{'message_transport_type'} } = $row->{'letter_code'}; - } - return $return; -} - =head2 SetMessagingPreference This method defines how a user (or a default for a patron category) wants to get a certain diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 7e24ff4..3054e3e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -46,6 +46,7 @@ use Koha::Libraries; use Koha::IssuingRules; use Koha::Items; use Koha::ItemTypes; +use Koha::Patron::Message::Preferences; use Koha::Patrons; use List::MoreUtils qw( firstidx any ); @@ -1848,10 +1849,10 @@ sub _koha_notify_reserve { # Try to get the borrower's email address my $to_address = C4::Members::GetNoticeEmailAddress($borrowernumber); - my $messagingprefs = C4::Members::Messaging::GetMessagingPreferences( { - borrowernumber => $borrowernumber, - message_name => 'Hold_Filled' - } ); + my $messagingpref = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $borrowernumber, + message_name => 'Hold_Filled', + }); my $library = Koha::Libraries->find( $hold->branchcode )->unblessed; @@ -1892,7 +1893,9 @@ sub _koha_notify_reserve { } ); }; - while ( my ( $mtt, $letter_code ) = each %{ $messagingprefs->{transports} } ) { + my $transports = $messagingpref->message_transport_types if $messagingpref; + foreach my $mtt (keys %{$transports}) { + my $letter_code = $transports->{$mtt}; next if ( ( $mtt eq 'email' and not $to_address ) # No email address or ( $mtt eq 'sms' and not $borrower->{smsalertnumber} ) # No SMS number diff --git a/Koha/Patron/Category.pm b/Koha/Patron/Category.pm index a90123e..8e9b590 100644 --- a/Koha/Patron/Category.pm +++ b/Koha/Patron/Category.pm @@ -83,19 +83,17 @@ sub default_messaging { my $messaging_options = Koha::Patron::Message::Preferences->get_options; my @messaging; foreach my $option (@$messaging_options) { - my $pref = C4::Members::Messaging::GetMessagingPreferences( - { - categorycode => $self->categorycode, - message_name => $option->{message_name} - } - ); - next unless $pref->{transports}; + my $pref = Koha::Patron::Message::Preferences->find_with_message_name({ + categorycode => $self->categorycode, + message_name => $option->{message_name}, + }); + next if !$pref || $pref && !$pref->message_transport_types; my $brief_pref = { message_attribute_id => $option->{message_attribute_id}, message_name => $option->{message_name}, $option->{'message_name'} => 1, }; - foreach my $transport ( keys %{ $pref->{transports} } ) { + foreach my $transport ( keys %{ $pref->message_transport_types } ) { push @{ $brief_pref->{transports} }, { transport => $transport }; } push @messaging, $brief_pref; diff --git a/circ/returns.pl b/circ/returns.pl index d1b95e8..942dd11 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -52,6 +52,7 @@ use Koha::DateUtils; use Koha::Calendar; use Koha::Checkouts; use Koha::BiblioFrameworks; +use Koha::Patron::Message::Preferences; my $query = new CGI; @@ -436,7 +437,10 @@ if ( $messages->{'WrongTransfer'} and not $messages->{'WasTransfered'}) { if ( $messages->{'ResFound'}) { my $reserve = $messages->{'ResFound'}; my $borr = C4::Members::GetMember( borrowernumber => $reserve->{'borrowernumber'} ); - my $holdmsgpreferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $reserve->{'borrowernumber'}, message_name => 'Hold_Filled' } ); + my $holdmsgpreferences = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $reserve->{'borrowernumber'}, + message_name => 'Hold_Filled', + }); if ( $reserve->{'ResFound'} eq "Waiting" or $reserve->{'ResFound'} eq "Reserved" ) { if ( $reserve->{'ResFound'} eq "Waiting" ) { $template->param( @@ -478,7 +482,7 @@ if ( $messages->{'ResFound'}) { itemnumber => $reserve->{'itemnumber'}, reservenotes => $reserve->{'reservenotes'}, reserve_id => $reserve->{reserve_id}, - bormessagepref => $holdmsgpreferences->{'transports'}, + bormessagepref => $holdmsgpreferences->message_transport_types, ); } # else { ; } # error? } diff --git a/misc/cronjobs/advance_notices.pl b/misc/cronjobs/advance_notices.pl index 2274737..a753007 100755 --- a/misc/cronjobs/advance_notices.pl +++ b/misc/cronjobs/advance_notices.pl @@ -59,6 +59,7 @@ use Koha::DateUtils; use C4::Log; use Koha::Libraries; use Koha::Patrons; +use Koha::Patron::Message::Preferences; =head1 NAME @@ -242,11 +243,13 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { my $borrower_preferences; if ( 0 == $upcoming->{'days_until_due'} ) { # This item is due today. Send an 'item due' message. - $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $upcoming->{'borrowernumber'}, - message_name => 'item_due' } ); + $borrower_preferences = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $upcoming->{'borrowernumber'}, + message_name => 'item_due', + }); next unless $borrower_preferences; - if ( $borrower_preferences->{'wants_digest'} ) { + if ( $borrower_preferences->wants_digest ) { # cache this one to process after we've run through all of the items. $due_digest->{ $upcoming->{borrowernumber} }->{email} = $from_address; $due_digest->{ $upcoming->{borrowernumber} }->{count}++; @@ -261,7 +264,7 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { } ## Get branch info for borrowers home library. - foreach my $transport ( keys %{$borrower_preferences->{'transports'}} ) { + foreach my $transport ( keys %{$borrower_preferences->message_transport_types} ) { my $letter = parse_letter( { letter_code => $letter_type, borrowernumber => $upcoming->{'borrowernumber'}, branchcode => $upcoming->{'branchcode'}, @@ -275,19 +278,21 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { } } } else { - $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $upcoming->{'borrowernumber'}, - message_name => 'advance_notice' } ); - next UPCOMINGITEM unless $borrower_preferences && exists $borrower_preferences->{'days_in_advance'}; - next UPCOMINGITEM unless $borrower_preferences->{'days_in_advance'} == $upcoming->{'days_until_due'}; - - if ( $borrower_preferences->{'wants_digest'} ) { + $borrower_preferences = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $upcoming->{'borrowernumber'}, + message_name => 'advance_notice', + }); + next UPCOMINGITEM unless $borrower_preferences && $borrower_preferences->days_in_advance; + next UPCOMINGITEM unless $borrower_preferences->days_in_advance == $upcoming->{'days_until_due'}; + + if ( $borrower_preferences->wants_digest ) { # cache this one to process after we've run through all of the items. $upcoming_digest->{ $upcoming->{borrowernumber} }->{email} = $from_address; $upcoming_digest->{ $upcoming->{borrowernumber} }->{count}++; } else { my $biblio = C4::Biblio::GetBiblioFromItemNumber( $upcoming->{'itemnumber'} ); my $letter_type = 'PREDUE'; - $sth->execute($upcoming->{'borrowernumber'},$upcoming->{'itemnumber'},$borrower_preferences->{'days_in_advance'}); + $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; @@ -295,7 +300,7 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { } ## Get branch info for borrowers home library. - foreach my $transport ( keys %{$borrower_preferences->{'transports'}} ) { + foreach my $transport ( keys %{$borrower_preferences->message_transport_types} ) { my $letter = parse_letter( { letter_code => $letter_type, borrowernumber => $upcoming->{'borrowernumber'}, branchcode => $upcoming->{'branchcode'}, @@ -346,14 +351,16 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$upcoming_digest ) { my $count = $digest->{count}; my $from_address = $digest->{email}; - my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $borrowernumber, - message_name => 'advance_notice' } ); + my $borrower_preferences = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $borrowernumber, + message_name => 'advance_notice', + }); next PATRON unless $borrower_preferences; # how could this happen? my $letter_type = 'PREDUEDGST'; - $sth->execute($borrowernumber,$borrower_preferences->{'days_in_advance'}); + $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; @@ -363,7 +370,7 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$upcoming_digest ) { ## Get branch info for borrowers home library. my %branch_info = get_branch_info( $borrowernumber ); - foreach my $transport ( keys %{ $borrower_preferences->{'transports'} } ) { + foreach my $transport ( keys %{ $borrower_preferences->message_transport_types } ) { my $letter = parse_letter( { letter_code => $letter_type, @@ -405,8 +412,10 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$due_digest ) { my $count = $digest->{count}; my $from_address = $digest->{email}; - my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $borrowernumber, - message_name => 'item_due' } ); + my $borrower_preferences = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $borrowernumber, + message_name => 'item_due', + }); next PATRON unless $borrower_preferences; # how could this happen? my $letter_type = 'DUEDGST'; @@ -420,7 +429,7 @@ PATRON: while ( my ( $borrowernumber, $digest ) = each %$due_digest ) { ## Get branch info for borrowers home library. my %branch_info = get_branch_info( $borrowernumber ); - for my $transport ( keys %{ $borrower_preferences->{'transports'} } ) { + for my $transport ( keys %{ $borrower_preferences->message_transport_types } ) { my $letter = parse_letter( { letter_code => $letter_type, diff --git a/t/db_dependent/Koha/Patron/Categories.t b/t/db_dependent/Koha/Patron/Categories.t index 4d81768..e62a614 100644 --- a/t/db_dependent/Koha/Patron/Categories.t +++ b/t/db_dependent/Koha/Patron/Categories.t @@ -22,12 +22,12 @@ use Modern::Perl; use Test::More tests => 11; use C4::Context; -use C4::Members::Messaging; use Koha::Database; use Koha::DateUtils; use Koha::Patron; use Koha::Patron::Category; use Koha::Patron::Categories; +use Koha::Patron::Message::Preferences; use t::lib::TestBuilder; use t::lib::Mocks; @@ -122,13 +122,13 @@ subtest 'default_messaging tests' => sub { is_digest => 1, } }); - C4::Members::Messaging::SetMessagingPreference({ + Koha::Patron::Message::Preference->new({ categorycode => $category->categorycode, message_attribute_id => $attr->{'message_attribute_id'}, message_transport_types => [ qw( email ) ], days_in_advance => 5, wants_digest => 1 - }); + })->store; my $default_messaging = $category->default_messaging; is(@$default_messaging, 1, "One default messaging found."); $default_messaging = $default_messaging->[0]; -- 2.1.4