Bug 19855 - Move the "alert" code to Koha::Subscription
Summary: Move the "alert" code to Koha::Subscription
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15449
  Show dependency treegraph
 
Reported: 2017-12-20 20:04 UTC by Jonathan Druart
Modified: 2018-04-23 17:29 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19855: Remove getalert from moremember.pl (1.15 KB, patch)
2017-12-20 20:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove getalert from opac-user.pl (1009 bytes, patch)
2017-12-20 20:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove C4::Letters::findrelatedto (3.20 KB, patch)
2017-12-20 20:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove $type from the alerts (16.85 KB, patch)
2017-12-20 20:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Move getalert, addalert and delalert to Koha::Subscription (26.03 KB, patch)
2017-12-20 20:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove getalert from moremember.pl (1.15 KB, patch)
2018-04-04 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove getalert from opac-user.pl (1009 bytes, patch)
2018-04-04 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove C4::Letters::findrelatedto (3.20 KB, patch)
2018-04-04 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove $type from the alerts (16.85 KB, patch)
2018-04-04 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Move getalert, addalert and delalert to Koha::Subscription (25.85 KB, patch)
2018-04-04 13:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19855: Remove getalert from moremember.pl (1.22 KB, patch)
2018-04-20 10:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19855: Remove getalert from opac-user.pl (1.06 KB, patch)
2018-04-20 10:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19855: Remove C4::Letters::findrelatedto (3.27 KB, patch)
2018-04-20 10:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19855: Remove $type from the alerts (16.92 KB, patch)
2018-04-20 10:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19855: Move getalert, addalert and delalert to Koha::Subscription (25.65 KB, patch)
2018-04-20 10:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19855: Remove getalert from moremember.pl (1.25 KB, patch)
2018-04-22 09:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19855: Remove getalert from opac-user.pl (1.09 KB, patch)
2018-04-22 09:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19855: Remove C4::Letters::findrelatedto (3.30 KB, patch)
2018-04-22 09:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19855: Remove $type from the alerts (16.90 KB, patch)
2018-04-22 09:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19855: Move getalert, addalert and delalert to Koha::Subscription (25.62 KB, patch)
2018-04-22 09:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19855: Use patron-title to display patron's info (1.25 KB, patch)
2018-04-23 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-12-20 20:04:04 UTC
There are some weird subroutines in C4::Letters: addalert, getalert, delalert, findrelatedto.
They have been designed to support different kind of alerts, but only emails notification for new subscription issues is working correctly.

I suggest to clean, simplify and move this code to Koha::Subscription
Comment 1 Jonathan Druart 2017-12-20 20:20:27 UTC
Created attachment 69970 [details] [review]
Bug 19855: Remove getalert from moremember.pl

This is not used later and seems unused since:
commit b38997925f603dc9784c25d3db3501644c19e068
Date:   Wed Oct 26 09:11:02 2005 +0000
    big commit, still breaking things...
Comment 2 Jonathan Druart 2017-12-20 20:20:32 UTC
Created attachment 69971 [details] [review]
Bug 19855: Remove getalert from opac-user.pl

This is not used later and seems unused since:
commit e61173aa8eb23c111a707eba2cfbd2003df709bf
Date:   Sun May 21 02:29:09 2006 +0000
    syncing dev-week and HEAD
Comment 3 Jonathan Druart 2017-12-20 20:20:35 UTC
Created attachment 69972 [details] [review]
Bug 19855: Remove C4::Letters::findrelatedto

This subroutine is called only once. It only concat firstname and
surname for subscribers.
It can be easily replaced with Koha::Patron
Comment 4 Jonathan Druart 2017-12-20 20:20:39 UTC
Created attachment 69973 [details] [review]
Bug 19855: Remove $type from the alerts

It looks like this feature has never been finished. It has been
developed with more flexibility in mind, but only 'issue' is used for
this parameter. Apparently it could have been 'virtual', for virtual shelves.

Let remove this parameter and clean the code a bit.
TODO: Remove the DB column
Comment 5 Jonathan Druart 2017-12-20 20:20:43 UTC
Created attachment 69974 [details] [review]
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.
Comment 6 Josef Moravec 2018-04-04 10:15:53 UTC
Could you rebase please?
Comment 7 Jonathan Druart 2018-04-04 13:01:38 UTC
Created attachment 73621 [details] [review]
Bug 19855: Remove getalert from moremember.pl

This is not used later and seems unused since:
commit b38997925f603dc9784c25d3db3501644c19e068
Date:   Wed Oct 26 09:11:02 2005 +0000
    big commit, still breaking things...
Comment 8 Jonathan Druart 2018-04-04 13:01:41 UTC
Created attachment 73622 [details] [review]
Bug 19855: Remove getalert from opac-user.pl

This is not used later and seems unused since:
commit e61173aa8eb23c111a707eba2cfbd2003df709bf
Date:   Sun May 21 02:29:09 2006 +0000
    syncing dev-week and HEAD
Comment 9 Jonathan Druart 2018-04-04 13:01:44 UTC
Created attachment 73623 [details] [review]
Bug 19855: Remove C4::Letters::findrelatedto

This subroutine is called only once. It only concat firstname and
surname for subscribers.
It can be easily replaced with Koha::Patron
Comment 10 Jonathan Druart 2018-04-04 13:01:48 UTC
Created attachment 73624 [details] [review]
Bug 19855: Remove $type from the alerts

It looks like this feature has never been finished. It has been
developed with more flexibility in mind, but only 'issue' is used for
this parameter. Apparently it could have been 'virtual', for virtual shelves.

Let remove this parameter and clean the code a bit.
TODO: Remove the DB column
Comment 11 Jonathan Druart 2018-04-04 13:01:52 UTC
Created attachment 73625 [details] [review]
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.
Comment 12 Kyle M Hall 2018-04-20 10:38:57 UTC
Created attachment 74611 [details] [review]
Bug 19855: Remove getalert from moremember.pl

This is not used later and seems unused since:
commit b38997925f603dc9784c25d3db3501644c19e068
Date:   Wed Oct 26 09:11:02 2005 +0000
    big commit, still breaking things...

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2018-04-20 10:39:04 UTC
Created attachment 74612 [details] [review]
Bug 19855: Remove getalert from opac-user.pl

This is not used later and seems unused since:
commit e61173aa8eb23c111a707eba2cfbd2003df709bf
Date:   Sun May 21 02:29:09 2006 +0000
    syncing dev-week and HEAD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2018-04-20 10:39:06 UTC
Created attachment 74613 [details] [review]
Bug 19855: Remove C4::Letters::findrelatedto

This subroutine is called only once. It only concat firstname and
surname for subscribers.
It can be easily replaced with Koha::Patron

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2018-04-20 10:39:09 UTC
Created attachment 74614 [details] [review]
Bug 19855: Remove $type from the alerts

It looks like this feature has never been finished. It has been
developed with more flexibility in mind, but only 'issue' is used for
this parameter. Apparently it could have been 'virtual', for virtual shelves.

Let remove this parameter and clean the code a bit.
TODO: Remove the DB column

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2018-04-20 10:39:12 UTC
Created attachment 74615 [details] [review]
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>
Comment 17 Katrin Fischer 2018-04-22 09:45:48 UTC
QA script and other tests pass.

1) These FIXMEs appear fixable now:
[%# FIXME use patron-title when 18403 will be pushed %]
# FIXME raise a message if subscription does not exist (easy with 18403)

2) Do we want to keep this warn?
+    warn "CANCEL confirmed : $loggedinuser, $subscriptionid";

2) is 'preexisting' and 1) can be done in a follow-up as originalyl planned. Not Considering as a blocker.
Comment 18 Katrin Fischer 2018-04-22 09:46:28 UTC
Created attachment 74688 [details] [review]
Bug 19855: Remove getalert from moremember.pl

This is not used later and seems unused since:
commit b38997925f603dc9784c25d3db3501644c19e068
Date:   Wed Oct 26 09:11:02 2005 +0000
    big commit, still breaking things...

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2018-04-22 09:46:32 UTC
Created attachment 74689 [details] [review]
Bug 19855: Remove getalert from opac-user.pl

This is not used later and seems unused since:
commit e61173aa8eb23c111a707eba2cfbd2003df709bf
Date:   Sun May 21 02:29:09 2006 +0000
    syncing dev-week and HEAD

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2018-04-22 09:46:36 UTC
Created attachment 74690 [details] [review]
Bug 19855: Remove C4::Letters::findrelatedto

This subroutine is called only once. It only concat firstname and
surname for subscribers.
It can be easily replaced with Koha::Patron

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2018-04-22 09:46:39 UTC
Created attachment 74691 [details] [review]
Bug 19855: Remove $type from the alerts

It looks like this feature has never been finished. It has been
developed with more flexibility in mind, but only 'issue' is used for
this parameter. Apparently it could have been 'virtual', for virtual shelves.

Let remove this parameter and clean the code a bit.
TODO: Remove the DB column

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2018-04-22 09:46:43 UTC
Created attachment 74692 [details] [review]
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>
Comment 23 Katrin Fischer 2018-04-22 09:47:31 UTC
I couldn't verify that the email sent as it bypasses the message_queue, but did careful code review. Also trusting the sign-offer and writer to have this tested ;)
Comment 24 Jonathan Druart 2018-04-23 16:08:19 UTC
Created attachment 74750 [details] [review]
Bug 19855: Use patron-title to display patron's info
Comment 25 Jonathan Druart 2018-04-23 17:29:16 UTC
Pushed to master for 18.05, thanks to everybody involved!