From 7cfae03c28ec0b00386f8ca9af3b92a9b671e2fe 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. ---- 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 9397cbc..19153ee 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -49,6 +49,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; @@ -3337,14 +3338,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 a6c938a..77d9ef5 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 71bbfba..9924132 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -47,6 +47,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 ); @@ -1648,10 +1649,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; @@ -1691,7 +1692,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 $patron->smsalertnumber ) # No SMS number diff --git a/Koha/Patron/Category.pm b/Koha/Patron/Category.pm index 7632593..1790e71 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 e2c815b..7ae5456 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -57,6 +57,7 @@ use Koha::Checkouts; use Koha::Holds; use Koha::Items; use Koha::Patrons; +use Koha::Patron::Message::Preferences; my $query = new CGI; @@ -439,7 +440,10 @@ if ( $messages->{'WrongTransfer'} and not $messages->{'WasTransfered'}) { if ( $messages->{'ResFound'}) { my $reserve = $messages->{'ResFound'}; my $patron = Koha::Patrons->find( $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( @@ -481,7 +485,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 73697b8..943b57d 100755 --- a/misc/cronjobs/advance_notices.pl +++ b/misc/cronjobs/advance_notices.pl @@ -60,6 +60,7 @@ use C4::Log; use Koha::Items; use Koha::Libraries; use Koha::Patrons; +use Koha::Patron::Message::Preferences; =head1 NAME @@ -244,11 +245,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}++; @@ -262,7 +265,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'}, @@ -276,26 +279,28 @@ 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 $item = Koha::Items->find( $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()) { $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); } ## 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()) { $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); @@ -362,7 +369,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, @@ -404,8 +411,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'; @@ -418,7 +427,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 f1ac0c1..11b4e77 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.7.4