From 2eb4ab77d7fee34c59a2bc66ca4a93c47080edda Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Thu, 27 Oct 2016 16:08:50 +0300 Subject: [PATCH 5/9] 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 Signed-off-by: David Bourgault Rebased-by: Emmi Takkinen --- C4/Circulation.pm | 6 ++- C4/Form/MessagingPreferences.pm | 9 ++-- C4/Members/Messaging.pm | 62 ------------------------- C4/Reserves.pm | 13 ++++-- Koha/Patron/Category.pm | 14 +++--- circ/returns.pl | 9 +++- misc/cronjobs/advance_notices.pl | 44 +++++++++--------- t/db_dependent/Koha/Patron/Categories.t | 6 +-- 8 files changed, 56 insertions(+), 107 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 29b3347666..a2770002a8 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -48,6 +48,7 @@ use Koha::Illrequests; use Koha::Items; use Koha::Patrons; use Koha::Patron::Debarments; +use Koha::Patron::Message::Preferences; use Koha::Database; use Koha::Libraries; use Koha::Account::Lines; @@ -3466,14 +3467,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 a6c938a64a..77d9ef5c9c 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 92ea6515da..d28de2ccd7 100644 --- a/C4/Members/Messaging.pm +++ b/C4/Members/Messaging.pm @@ -35,68 +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; - 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 d971324f1b..3622186a4f 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -44,6 +44,7 @@ use Koha::ItemTypes; use Koha::Items; use Koha::Libraries; use Koha::Old::Hold; +use Koha::Patron::Message::Preferences; use Koha::Patrons; use Koha::Plugins; @@ -1794,10 +1795,10 @@ sub _koha_notify_reserve { # Try to get the borrower's email address my $to_address = $patron->notice_email_address; - 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; @@ -1837,7 +1838,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 7fb6105831..a416c6734e 100644 --- a/Koha/Patron/Category.pm +++ b/Koha/Patron/Category.pm @@ -84,19 +84,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 bd59f6c357..3331626b52 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -56,6 +56,7 @@ use Koha::DateUtils; use Koha::Holds; use Koha::Items; use Koha::Patrons; +use Koha::Patron::Message::Preferences; my $query = CGI->new; @@ -415,7 +416,11 @@ 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', + }); + my $mtts = $holdmsgpreferences->message_transport_types if defined $holdmsgpreferences; my $branchCheck = ( $userenv_branch eq $reserve->{branchcode} ); if ( $reserve->{'ResFound'} eq "Reserved" && C4::Context->preference('HoldsAutoFill') ) { my $item = Koha::Items->find( $itemnumber ); @@ -460,7 +465,7 @@ if ( $messages->{'ResFound'}) { destbranch => $reserve->{'branchcode'}, reservenotes => $reserve->{'reservenotes'}, reserve_id => $reserve->{reserve_id}, - bormessagepref => $holdmsgpreferences->{'transports'}, + bormessagepref => $mtts, ); } diff --git a/misc/cronjobs/advance_notices.pl b/misc/cronjobs/advance_notices.pl index d8dcf73a51..bd60635511 100755 --- a/misc/cronjobs/advance_notices.pl +++ b/misc/cronjobs/advance_notices.pl @@ -61,6 +61,7 @@ use C4::Log; use Koha::Items; use Koha::Libraries; use Koha::Patrons; +use Koha::Patron::Message::Preferences; =head1 NAME @@ -261,11 +262,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. if ($digest_per_branch) { $due_digest->{ $upcoming->{branchcode} }->{ $upcoming->{borrowernumber} }->{email} = $from_address; @@ -284,7 +287,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'}, @@ -298,12 +301,14 @@ 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'}; + $borrower_preferences = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $upcoming->{'borrowernumber'}, + message_name => 'advance_notice', + }); + next UPCOMINGITEM unless $borrower_preferences && defined $borrower_preferences->days_in_advance; + next UPCOMINGITEM unless $borrower_preferences->days_in_advance == $upcoming->{'days_until_due'}; - if ( $borrower_preferences->{'wants_digest'} ) { + if ( $borrower_preferences->wants_digest ) { # cache this one to process after we've run through all of the items. if ($digest_per_branch) { $upcoming_digest->{ $upcoming->{branchcode} }->{ $upcoming->{borrowernumber} }->{email} = $from_address; @@ -315,14 +320,14 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { } 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'}, @@ -380,7 +385,7 @@ if ($digest_per_branch) { get_item_info => sub { my $params = shift; $params->{sth}->execute($params->{borrowernumber}, - $params->{borrower_preferences}->{'days_in_advance'}); + $params->{borrower_preferences}->days_in_advance); return sub { $params->{sth}->fetchrow_hashref; }; @@ -413,7 +418,7 @@ if ($digest_per_branch) { get_item_info => sub { my $params = shift; $params->{sth}->execute($params->{borrowernumber}, - $params->{borrower_preferences}->{'days_in_advance'}); + $params->{borrower_preferences}->days_in_advance); return sub { $params->{sth}->fetchrow_hashref; }; @@ -549,13 +554,10 @@ sub send_digests { $branchcode = $branch_info{'branches.branchcode'}; } - my $borrower_preferences = - C4::Members::Messaging::GetMessagingPreferences( - { - borrowernumber => $borrowernumber, - message_name => $params->{message_name} - } - ); + my $borrower_preferences = Koha::Patron::Message::Preferences->find_with_message_name({ + borrowernumber => $borrowernumber, + message_name => $params->{message_name}, + }); next PATRON unless $borrower_preferences; # how could this happen? @@ -569,7 +571,7 @@ sub send_digests { $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); } - foreach my $transport ( keys %{ $borrower_preferences->{'transports'} } ) { + foreach my $transport ( keys %{ $borrower_preferences->message_transport_types } ) { my $letter = parse_letter( { letter_code => $params->{letter_code}, diff --git a/t/db_dependent/Koha/Patron/Categories.t b/t/db_dependent/Koha/Patron/Categories.t index eb6467db83..fe8c32a4db 100755 --- 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::Dates; use t::lib::TestBuilder; use t::lib::Mocks; @@ -123,13 +123,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.17.1