From 5d707729bdf6723cf9de4a42646383ca963a54c2 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 20 Dec 2017 17:01:08 -0300 Subject: [PATCH] Bug 19855: Move getalert, addalert and delalert to Koha::Subscription This patch removes 3 subroutines from C4::Letters: - getalert - addalert - delalert And add 3 methods to Koha::Subscription: - subscribers - add_subscriber - remove_subscriber It makes the code cleaner for future cleanup. TODO - we should remove alert.alertid and alert.type, and rename alert.externalid with alert.subscriptionid That way alert will be renamed borrowers_subscriptions (or similar) and will become a simple join table between borrowers and subscriptions. We will need to deal with FK that could not be satisfied. Let's do that after this patch is pushed. Test plan: Subscribe and unsubscribe to email notifications sent when a new issues is available. Make sure everything works as before and you receive the emails. --- C4/Letters.pm | 81 +++------------------- Koha/Subscription.pm | 46 ++++++++++++ .../prog/en/modules/serials/viewalerts.tt | 7 +- .../bootstrap/en/modules/opac-alert-subscribe.tt | 4 +- .../opac-tmpl/bootstrap/en/modules/opac-detail.tt | 4 +- .../bootstrap/en/modules/opac-serial-issues.tt | 4 +- opac/opac-alert-subscribe.pl | 19 +++-- opac/opac-detail.pl | 7 +- opac/opac-serial-issues.pl | 16 ++--- serials/viewalerts.pl | 36 ++++------ t/db_dependent/Koha/Subscription.t | 39 ++++++++++- t/db_dependent/Letters.t | 43 ++---------- 12 files changed, 138 insertions(+), 168 deletions(-) diff --git a/C4/Letters.pm b/C4/Letters.pm index 0c5ad9b7c5..abf932e39b 100644 --- a/C4/Letters.pm +++ b/C4/Letters.pm @@ -38,6 +38,7 @@ use Koha::SMS::Providers; use Koha::Email; use Koha::DateUtils qw( format_sqldatetime dt_from_string ); use Koha::Patrons; +use Koha::Subscriptions; use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); @@ -45,7 +46,7 @@ BEGIN { require Exporter; @ISA = qw(Exporter); @EXPORT = qw( - &GetLetters &GetLettersAvailableForALibrary &GetLetterTemplates &DelLetter &GetPreparedLetter &GetWrappedLetter &addalert &getalert &delalert &SendAlerts &GetPrintMessages &GetMessageTransportTypes + &GetLetters &GetLettersAvailableForALibrary &GetLetterTemplates &DelLetter &GetPreparedLetter &GetWrappedLetter &SendAlerts &GetPrintMessages &GetMessageTransportTypes ); } @@ -262,71 +263,6 @@ sub DelLetter { , undef, $branchcode, $module, $code, ( $mtt ? $mtt : () ), ( $lang ? $lang : () ) ); } -=head2 addalert ($borrowernumber, $subscriptionid) - - parameters : - - $borrowernumber : the number of the borrower subscribing to the alert - - $subscriptionid - - create an alert and return the alertid (primary key) - -=cut - -sub addalert { - my ( $borrowernumber, $subscriptionid) = @_; - my $dbh = C4::Context->dbh; - my $sth = - $dbh->prepare( - "insert into alert (borrowernumber, externalid) values (?,?)"); - $sth->execute( $borrowernumber, $subscriptionid ); - - # get the alert number newly created and return it - my $alertid = $dbh->{'mysql_insertid'}; - return $alertid; -} - -=head2 delalert ($alertid) - - parameters : - - alertid : the alert id - deletes the alert - -=cut - -sub delalert { - my $alertid = shift or die "delalert() called without valid argument (alertid)"; # it's gonna die anyway. - $debug and warn "delalert: deleting alertid $alertid"; - my $sth = C4::Context->dbh->prepare("delete from alert where alertid=?"); - $sth->execute($alertid); -} - -=head2 getalert ([$borrowernumber], [$subscriptionid]) - - parameters : - - $borrowernumber : the number of the borrower subscribing to the alert - - $subscriptionid - all parameters NON mandatory. If a parameter is omitted, the query is done without the corresponding parameter. For example, without $subscriptionid, returns all alerts for a borrower on a topic. - -=cut - -sub getalert { - my ( $borrowernumber, $subscriptionid ) = @_; - my $dbh = C4::Context->dbh; - my $query = "SELECT a.*, b.branchcode FROM alert a JOIN borrowers b USING(borrowernumber) WHERE 1"; - my @bind; - if ($borrowernumber and $borrowernumber =~ /^\d+$/) { - $query .= " AND borrowernumber=?"; - push @bind, $borrowernumber; - } - if ($subscriptionid) { - $query .= " AND externalid=?"; - push @bind, $subscriptionid; - } - my $sth = $dbh->prepare($query); - $sth->execute(@bind); - return $sth->fetchall_arrayref({}); -} - =head2 SendAlerts my $err = &SendAlerts($type, $externalid, $letter_code); @@ -375,22 +311,21 @@ sub SendAlerts { return; my %letter; - # find the list of borrowers to alert - my $alerts = getalert( '', $subscriptionid ); - foreach (@$alerts) { - my $patron = Koha::Patrons->find( $_->{borrowernumber} ); - next unless $patron; # Just in case + # find the list of subscribers to notify + my $subscription = Koha::Subscriptions->find( $subscriptionid ); + my $subscribers = $subscription->subscribers; + while ( my $patron = $subscribers->next ) { my $email = $patron->email or next; # warn "sending issues..."; my $userenv = C4::Context->userenv; - my $library = Koha::Libraries->find( $_->{branchcode} ); + my $library = $patron->library; my $letter = GetPreparedLetter ( module => 'serial', letter_code => $letter_code, branchcode => $userenv->{branch}, tables => { - 'branches' => $_->{branchcode}, + 'branches' => $library->branchcode, 'biblio' => $biblionumber, 'biblioitems' => $biblionumber, 'borrowers' => $patron->unblessed, diff --git a/Koha/Subscription.pm b/Koha/Subscription.pm index 2a793e5946..d09219f518 100644 --- a/Koha/Subscription.pm +++ b/Koha/Subscription.pm @@ -48,6 +48,52 @@ sub biblio { return scalar Koha::Biblios->find($self->biblionumber); } +=head3 subscribers + +my $subscribers = $subscription->subscribers; + +return a Koha::Patrons object + +=cut + +sub subscribers { + my ($self) = @_; + my $schema = Koha::Database->new->schema; + my @borrowernumbers = $schema->resultset('Alert')->search({ externalid => $self->subscriptionid })->get_column( 'borrowernumber' )->all; + return Koha::Patrons->search({ borrowernumber => {-in => \@borrowernumbers } }); +} + +=head3 add_subscriber + +$subscription->add_subscriber( $patron ); + +Add a new subscriber (Koha::Patron) to this subscription + +=cut + +sub add_subscriber { + my ( $self, $patron ) = @_; + my $schema = Koha::Database->new->schema; + my $rs = $schema->resultset('Alert'); + $rs->create({ externalid => $self->subscriptionid, borrowernumber => $patron->borrowernumber }); +} + +=head3 remove_subscriber + +$subscription->remove_subscriber( $subscriber ); + +Remove a subscriber (Koha::Patron) from this subscription + +=cut + +sub remove_subscriber { + my ($self, $patron) = @_; + my $schema = Koha::Database->new->schema; + my $rs = $schema->resultset('Alert'); + my $subscriber = $rs->find({ externalid => $self->subscriptionid, borrowernumber => $patron->borrowernumber }); + $subscriber->delete if $subscriber; +} + =head3 type =cut diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/viewalerts.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/viewalerts.tt index 5ad1866eca..00a3f5be4a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/serials/viewalerts.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/serials/viewalerts.tt @@ -19,14 +19,15 @@ Subscription: [% bibliotitle %] #[% subscriptionid %]

-[% IF ( alertloop ) %] +[% IF subscribers.count %] - [% FOREACH alertloo IN alertloop %] + [% FOREACH subscriber IN subscribers %] - + [%# FIXME use patron-title when 18403 will be pushed %] + [% END %]
Patron name
[% alertloo.name %][% subscriber.firstname %] [% subscriber.surname %]
diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-alert-subscribe.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-alert-subscribe.tt index adba9f3306..e616888bfc 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-alert-subscribe.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-alert-subscribe.tt @@ -24,7 +24,7 @@

Do you want to receive an email when a new issue for this subscription arrives?

[% bibliotitle %]

[% IF ( notes ) %]

[% notes %]

[% END %] - + @@ -38,7 +38,7 @@

Please confirm that you do not want to receive email when a new issue arrives for this subscription.

[% bibliotitle %]

[% IF ( notes ) %]

[% notes %]

[% END %] - + diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt index fd97133967..0896bb33b2 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt @@ -828,9 +828,9 @@ [% IF ( subscription.letter ) %] [% IF ( loggedinusername ) %] [% IF ( subscription.hasalert ) %] - You have subscribed to email notification on new issues. Cancel email notification + You have subscribed to email notification on new issues. Cancel email notification [% ELSE %] - Subscribe to email notification on new issues + Subscribe to email notification on new issues [% END %] [% ELSE %] You must log in if you want to subscribe to email notification on new issues diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-serial-issues.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-serial-issues.tt index 45104bfb8e..7cee2fdb37 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-serial-issues.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-serial-issues.tt @@ -137,11 +137,11 @@ [% IF ( subscription_LOO.letter ) %] [% IF ( loggedinusername ) %] [% IF ( subscription_LOO.hasalert ) %] - You have subscribed to email notification on new issues + You have subscribed to email notification on new issues Cancel email notification [% ELSE %] - + Subscribe to email notification on new issues [% END %] diff --git a/opac/opac-alert-subscribe.pl b/opac/opac-alert-subscribe.pl index 7733c36c26..e8809e4f03 100755 --- a/opac/opac-alert-subscribe.pl +++ b/opac/opac-alert-subscribe.pl @@ -36,7 +36,7 @@ my $dbh = C4::Context->dbh; my $sth; my ( $template, $loggedinuser, $cookie ); -my $externalid = $query->param('externalid'); +my $subscriptionid = $query->param('subscriptionid'); my $referer = $query->param('referer') || 'detail'; my $biblionumber = $query->param('biblionumber'); @@ -51,8 +51,11 @@ my $biblionumber = $query->param('biblionumber'); } ); +my $subscription = Koha::Subscriptions->find( $subscriptionid ); +my $logged_in_patron = Koha::Patrons->find( $loggedinuser ); + if ( $op eq 'alert_confirmed' ) { - addalert( $loggedinuser, $externalid ); + $subscription->add_subscriber( $logged_in_patron ); if ( $referer eq 'serial' ) { print $query->redirect( "opac-serial-issues.pl?biblionumber=$biblionumber"); @@ -64,12 +67,8 @@ if ( $op eq 'alert_confirmed' ) { } } elsif ( $op eq 'cancel_confirmed' ) { - my $alerts = getalert( $loggedinuser, $externalid ); - warn "CANCEL confirmed : $loggedinuser, $externalid".Data::Dumper::Dumper( $alerts ); - foreach (@$alerts) - { # we are supposed to have only 1 result, but just in case... - delalert( $_->{alertid} ); - } + $subscription->remove_subscriber( $logged_in_patron ); + warn "CANCEL confirmed : $loggedinuser, $subscriptionid"; if ( $referer eq 'serial' ) { print $query->redirect( "opac-serial-issues.pl?biblionumber=$biblionumber"); @@ -83,13 +82,13 @@ elsif ( $op eq 'cancel_confirmed' ) { } else { - my $subscription = &GetSubscription($externalid); + my $subscription = &GetSubscription($subscriptionid); $template->param( referer => $referer, "typeissue$op" => 1, bibliotitle => $subscription->{bibliotitle}, notes => $subscription->{notes}, - externalid => $externalid, + subscriptionid => $subscriptionid, biblionumber => $biblionumber, ); } diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index 5c4e2bb96d..c07fbb7a50 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -584,10 +584,9 @@ foreach my $subscription (@subscriptions) { $cell{latestserials} = GetLatestSerials( $subscription->{subscriptionid}, $serials_to_display ); if ( $borrowernumber ) { - my $sub = getalert($borrowernumber, $subscription->{subscriptionid}); - if (@$sub[0]) { - $cell{hasalert} = 1; - } + my $subscription_object = Koha::Subscriptions->find( $subscription->{subscriptionid} ); + my $subscriber = $subscription_object->subscribers->find( $borrowernumber ); + $cell{hasalert} = 1 if $subscriber; } push @subs, \%cell; } diff --git a/opac/opac-serial-issues.pl b/opac/opac-serial-issues.pl index b74a886698..9fe6fc386a 100755 --- a/opac/opac-serial-issues.pl +++ b/opac/opac-serial-issues.pl @@ -62,10 +62,9 @@ if ( $selectview eq "full" ) { # now, check is there is an alert subscription for one of the subscriptions if ($loggedinuser) { foreach (@$subscriptions) { - my $sub = getalert($loggedinuser, $_->{subscriptionid}); - if ($sub) { - $_->{hasalert} = 1; - } + my $subscription = Koha::Subscriptions->find( $_->{subscriptionid} ); + my $subscriber = $subscription->subscribers->find( $loggedinuser ); + $_->{hasalert} = 1 if $subscriber; } } @@ -100,12 +99,11 @@ else { my $subscriptions = GetSubscriptionsFromBiblionumber($biblionumber); # now, check is there is an alert subscription for one of the subscriptions - if ($loggedinuser){ + if ($loggedinuser) { foreach (@$subscriptions) { - my $subscription = getalert($loggedinuser, $_->{subscriptionid}); - if (@$subscription[0]) { - $_->{hasalert} = 1; - } + my $subscription = Koha::Subscriptions->find( $_->{subscriptionid} ); + my $subscriber = $subscription->subscribers->find( $loggedinuser ); + $_->{hasalert} = 1 if $subscriber; } } diff --git a/serials/viewalerts.pl b/serials/viewalerts.pl index fe7242f901..39539e1172 100755 --- a/serials/viewalerts.pl +++ b/serials/viewalerts.pl @@ -18,20 +18,15 @@ # You should have received a copy of the GNU General Public License # along with Koha; if not, see . -use strict; -use warnings; +use Modern::Perl; use CGI qw ( -utf8 ); use C4::Auth; use C4::Context; use C4::Output; -use C4::Koha; -use C4::Letters; -use C4::Serials; -my $dbh = C4::Context->dbh; +use Koha::Subscriptions; my $input = new CGI; -my $print = $input->param('print'); my ($template, $loggedinuser, $cookie) = get_template_and_user({template_name => 'serials/viewalerts.tt', @@ -42,20 +37,17 @@ my ($template, $loggedinuser, $cookie) debug => 1, }); -my $subscriptionid=$input->param('subscriptionid'); - -my $borrowers = getalert('', $subscriptionid); -my $subscription = GetSubscription($subscriptionid); - -for my $borrowernumber (@$borrowers) { - my $patron = Koha::Patrons->find( $borrowernumber ); - next unless $borrowernumber; # Just in case... - $borrowers->{name} = join( ' ', $patron->firstname, $patron->surname ); -} -$template->param(alertloop => $borrowers, - bibliotitle => $subscription->{bibliotitle}, - subscriptionid => $subscriptionid, - (uc(C4::Context->preference("marcflavour"))) => 1 - ); +my $subscriptionid = $input->param('subscriptionid'); + +my $subscription = Koha::Subscriptions->find( $subscriptionid ); +# FIXME raise a message if subscription does not exist (easy with 18403) + +my $subscribers = $subscription->subscribers; + +$template->param( + subscribers => $subscribers, + bibliotitle => $subscription->biblio->title, + subscriptionid => $subscriptionid, +); output_html_with_http_headers $input, $cookie, $template->output; diff --git a/t/db_dependent/Koha/Subscription.t b/t/db_dependent/Koha/Subscription.t index 46256d3b41..cf68f99b66 100644 --- a/t/db_dependent/Koha/Subscription.t +++ b/t/db_dependent/Koha/Subscription.t @@ -19,15 +19,17 @@ use Modern::Perl; -use Test::More tests => 1; +use Test::More tests => 2; use Koha::Subscription; use Koha::Biblio; +use t::lib::TestBuilder; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; +my $builder = t::lib::TestBuilder->new; -subtest 'Koha::Subscription::biblio' => sub { +subtest 'Koha::Subscription->biblio' => sub { plan tests => 1; my $biblio = Koha::Biblio->new()->store(); @@ -36,7 +38,38 @@ subtest 'Koha::Subscription::biblio' => sub { })->store(); my $b = $subscription->biblio; - is($b->biblionumber, $biblio->biblionumber, 'Koha::Subscription::biblio returns the correct biblio'); + is($b->biblionumber, $biblio->biblionumber, 'Koha::Subscription->biblio returns the correct biblio'); +}; + +subtest 'Notifications on new issues - add_subscriber|remove_subscriber|subscribers' => sub { + plan tests => 5; + my $subscriber_1 = $builder->build_object( { class => 'Koha::Patrons' }); + my $subscriber_2 = $builder->build_object( { class => 'Koha::Patrons' }); + + my $subscription = Koha::Subscription->new({ + biblionumber => Koha::Biblio->new->store->biblionumber, + })->store(); + + my $subscribers = $subscription->subscribers; + is( $subscribers->count, 0, '->subscribers should return 0 if there are no subscribers'); + is( ref($subscribers), 'Koha::Patrons', '->subscribers should return a Koha::Patrons object'); + + $subscription->add_subscriber( $subscriber_1 ); + $subscription->add_subscriber( $subscriber_2 ); + + $subscribers = $subscription->subscribers; + is( $subscribers->count, 2, '->subscribers should return 2 if there are 2 subscribers' ); + + $subscription->remove_subscriber( $subscriber_1 ); + + $subscribers = $subscription->subscribers; + is( $subscribers->count, 1, '->remove_subscriber should have remove the subscriber' ); + + $subscription->remove_subscriber( $subscriber_1 ); # We do not explode if the patron is not a subscriber + + my $is_subscriber = $subscribers->find( $subscriber_2->borrowernumber ); + ok( $is_subscriber, 'This structure is used in the code and should work as expected' ); + }; $schema->storage->txn_rollback; diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t index da3fc21433..eb76191fe8 100644 --- a/t/db_dependent/Letters.t +++ b/t/db_dependent/Letters.t @@ -18,7 +18,7 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 75; +use Test::More tests => 64; use Test::MockModule; use Test::Warn; @@ -48,6 +48,8 @@ use Koha::Acquisition::Bookseller::Contacts; use Koha::Acquisition::Orders; use Koha::Libraries; use Koha::Notice::Templates; +use Koha::Patrons; +use Koha::Subscriptions; my $schema = Koha::Database->schema; $schema->storage->txn_begin(); @@ -235,41 +237,6 @@ my $branchcode = 'FFL'; my $letter14206_c = C4::Letters::getletter('my module', $overdue_rules->{"letter$i"}, $branchcode); is( $letter14206_c->{message_transport_type}, 'print', 'Bug 14206 - correct mtt detected for call from overdue_notices.pl' ); -# addalert -my $externalid = 'my external id'; -my $alert_id = C4::Letters::addalert($borrowernumber, $externalid); -isnt( $alert_id, undef, 'addalert does not return undef' ); - - -# getalert -my $alerts = C4::Letters::getalert(); -is( @$alerts, 1, 'getalert should not fail without parameter' ); -$alerts = C4::Letters::getalert($borrowernumber); -is( @$alerts, 1, 'addalert adds an alert' ); -is( $alerts->[0]->{alertid}, $alert_id, 'addalert returns the alert id correctly' ); -is( $alerts->[0]->{externalid}, $externalid, 'addalert stores the externalid correctly' ); - -$alerts = C4::Letters::getalert($borrowernumber); -is( @$alerts, 1, 'getalert returns the correct number of alerts' ); -$alerts = C4::Letters::getalert($borrowernumber, $externalid); -is( @$alerts, 1, 'getalert returns the correct number of alerts' ); -$alerts = C4::Letters::getalert($borrowernumber, 'another external id'); -is( @$alerts, 0, 'getalert returns the correct number of alerts' ); - - -# delalert -eval { - C4::Letters::delalert(); -}; -isnt( $@, undef, 'delalert without argument returns an error' ); -$alerts = C4::Letters::getalert($borrowernumber); -is( @$alerts, 1, 'delalert without argument does not remove an alert' ); - -C4::Letters::delalert($alert_id); -$alerts = C4::Letters::getalert($borrowernumber); -is( @$alerts, 0, 'delalert removes an alert' ); - - # GetPreparedLetter t::lib::Mocks::mock_preference('OPACBaseURL', 'http://thisisatest.com'); @@ -508,8 +475,8 @@ my $borrowernumber = AddMember( dateofbirth => $date, email => 'john.smith@test.de', ); -my $alert_id = C4::Letters::addalert($borrowernumber, $subscriptionid); - +my $subscription = Koha::Subscriptions->find( $subscriptionid ); +$subscription->add_subscriber( scalar Koha::Patrons->find( $borrowernumber ) ); my $err2; warning_is { -- 2.11.0