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
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...
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
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
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
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.
Could you rebase please?
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...
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
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
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
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.
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>
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>
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>
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>
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>
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.
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>
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>
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>
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>
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>
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 ;)
Created attachment 74750 [details] [review] Bug 19855: Use patron-title to display patron's info
Pushed to master for 18.05, thanks to everybody involved!