gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This commit is contained in:
Jonathan Druart 2017-12-20 17:01:08 -03:00
parent 0bd1f30c8c
commit 5c7ff786d5
12 changed files with 132 additions and 163 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

81
C4/Letters.pm
View File

@ -39,6 +39,7 @@ use Koha::Email;
use Koha::Notice::Messages;
use Koha::DateUtils qw( format_sqldatetime dt_from_string );
use Koha::Patrons;
use Koha::Subscriptions;
use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS);
@ -46,7 +47,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
);
}
@ -266,71 +267,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);
@ -379,22 +315,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,

46
Koha/Subscription.pm
View File

@ -60,6 +60,52 @@ sub vendor {
return scalar Koha::Acquisition::Booksellers->find($self->aqbooksellerid);
}
=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

7
koha-tmpl/intranet-tmpl/prog/en/modules/serials/viewalerts.tt
View File

@ -19,14 +19,15 @@
<span class="label">Subscription:</span> <a href="subscription-detail.pl?subscriptionid=[% subscriptionid %]">[% bibliotitle %] #[% subscriptionid %]</a>
</p>
[% IF ( alertloop ) %]
[% IF subscribers.count %]
<table>
<tr>
<th>Patron name</th>
</tr>
[% FOREACH alertloo IN alertloop %]
[% FOREACH subscriber IN subscribers %]
<tr>
<td><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% alertloo.borrowernumber %]">[% alertloo.name %]</a></td>
[%# FIXME use patron-title when 18403 will be pushed %]
<td><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% subscriber.borrowernumber %]">[% subscriber.firstname %] [% subscriber.surname %]</a></td>
</tr>
[% END %]
</table>

4
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-alert-subscribe.tt
View File

@ -24,7 +24,7 @@
<p>Do you want to receive an email when a new issue for this subscription arrives?</p>
<h4>[% bibliotitle %]</h4>
[% IF ( notes ) %]<p>[% notes %]</p>[% END %]
<input type="hidden" name="externalid" value="[% externalid %]">
<input type="hidden" name="subscriptionid" value="[% subscriptionid %]">
<input type="hidden" name="referer" value="[% referer %]">
<input type="hidden" name="biblionumber" value="[% biblionumber | html %]">
<input type="hidden" name="op" value="alert_confirmed">
@ -38,7 +38,7 @@
<p>Please confirm that you do not want to receive email when a new issue arrives for this subscription.</p>
<h4>[% bibliotitle %]</h4>
[% IF ( notes ) %]<p>[% notes %]</p>[% END %]
<input type="hidden" name="externalid" value="[% externalid %]">
<input type="hidden" name="subscriptionid" value="[% subscriptionid %]">
<input type="hidden" name="referer" value="[% referer %]">
<input type="hidden" name="biblionumber" value="[% biblionumber | html %]">
<input type="hidden" name="op" value="cancel_confirmed">

4
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
View File

@ -834,9 +834,9 @@
[% IF ( subscription.letter ) %]<span class="email_notifications">
[% IF ( loggedinusername ) %]
[% IF ( subscription.hasalert ) %]
<span>You have subscribed to email notification on new issues. </span><a style="color:#000;" class="btn" title="Cancel email notification" href="/cgi-bin/koha/opac-alert-subscribe.pl?op=cancel&amp;externalid=[% subscription.subscriptionid %]&amp;biblionumber=[% subscription.biblionumber %]">Cancel email notification</a>
<span>You have subscribed to email notification on new issues. </span><a style="color:#000;" class="btn" title="Cancel email notification" href="/cgi-bin/koha/opac-alert-subscribe.pl?op=cancel&amp;subscriptionid=[% subscription.subscriptionid %]&amp;biblionumber=[% subscription.biblionumber %]">Cancel email notification</a>
[% ELSE %]
<a style="color:#000;" class="btn" title="Subscribe to email notification on new issues" href="/cgi-bin/koha/opac-alert-subscribe.pl?externalid=[% subscription.subscriptionid %]&amp;biblionumber=[% subscription.biblionumber %]">Subscribe to email notification on new issues</a>
<a style="color:#000;" class="btn" title="Subscribe to email notification on new issues" href="/cgi-bin/koha/opac-alert-subscribe.pl?subscriptionid=[% subscription.subscriptionid %]&amp;biblionumber=[% subscription.biblionumber %]">Subscribe to email notification on new issues</a>
[% END %]
[% ELSE %]
<span>You must log in if you want to subscribe to email notification on new issues</span>

4
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-serial-issues.tt
View File

@ -137,11 +137,11 @@
[% IF ( subscription_LOO.letter ) %]
[% IF ( loggedinusername ) %]
[% IF ( subscription_LOO.hasalert ) %]
You have subscribed to email notification on new issues <a href="opac-alert-subscribe.pl?op=cancel&amp;externalid=[% subscription_LOO.subscriptionid %]&amp;referer=serial&amp;biblionumber=[% subscription_LOO.biblionumber %]" class="btn" title="Cancel email notification">
You have subscribed to email notification on new issues <a href="opac-alert-subscribe.pl?op=cancel&amp;subscriptionid=[% subscription_LOO.subscriptionid %]&amp;referer=serial&amp;biblionumber=[% subscription_LOO.biblionumber %]" class="btn" title="Cancel email notification">
Cancel email notification
</a>
[% ELSE %]
<a href="opac-alert-subscribe.pl?externalid=[% subscription_LOO.subscriptionid %]&amp;referer=serial&amp;biblionumber=[% subscription_LOO.biblionumber %]" class="btn" title="Subscribe to email notification on new issues">
<a href="opac-alert-subscribe.pl?subscriptionid=[% subscription_LOO.subscriptionid %]&amp;referer=serial&amp;biblionumber=[% subscription_LOO.biblionumber %]" class="btn" title="Subscribe to email notification on new issues">
Subscribe to email notification on new issues
</a>
[% END %]

19
opac/opac-alert-subscribe.pl
View File

@ -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,
);
}

7
opac/opac-detail.pl
View File

@ -589,10 +589,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;
}

16
opac/opac-serial-issues.pl
View File

@ -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;
}
}

29
serials/viewalerts.pl
View File

@ -23,14 +23,10 @@ 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',
@ -41,20 +37,17 @@ my ($template, $loggedinuser, $cookie)
debug => 1,
});
my $subscriptionid=$input->param('subscriptionid');
my $subscriptionid = $input->param('subscriptionid');
my $borrowers = getalert('', $subscriptionid);
my $subscription = GetSubscription($subscriptionid);
my $subscription = Koha::Subscriptions->find( $subscriptionid );
# FIXME raise a message if subscription does not exist (easy with 18403)
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 $subscribers = $subscription->subscribers;
$template->param(
subscribers => $subscribers,
bibliotitle => $subscription->biblio->title,
subscriptionid => $subscriptionid,
);
output_html_with_http_headers $input, $cookie, $template->output;

35
t/db_dependent/Koha/Subscription.t
View File

@ -19,7 +19,7 @@
use Modern::Perl;
use Test::More tests => 2;
use Test::More tests => 3;
use Koha::Subscriptions;
use Koha::Biblio;
@ -40,7 +40,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' );
};
subtest 'Koha::Subscription->vendor' => sub {

43
t/db_dependent/Letters.t
View File

@ -18,7 +18,7 @@
# along with Koha; if not, see <http://www.gnu.org/licenses>.
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();
@ -236,41 +238,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');
@ -509,8 +476,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 {