This enhancement will add a messaging preference for patron expiration notices, and adjust the cronjob that sends these notices to obey the patrons preference
Created attachment 169860 [details] [review] Bug 30300: Add a patron expiry messaging preference
Created attachment 169861 [details] [review] Bug 30300: Update the cronjob to respect patron messaging preferences Test plan: 1) Apply patch and run updatedatabase 2) In system preferences set MembershipExpiryDaysNotice to 5 3) Choose a patron and edit their expiry date to today's date 4) Ensure that patron has a primary email address 5) Run perl misc/cronjobs/membership_expiry.pl -c -v -before 10 -after 10 6) Check the patron's messages, there should be no expiry notice queued 7) Edit the patron's messaging preferences and select the checkbox for email in the row for 'Patron expiry' 8) Repeat step 5 9) There should be a patron expiry message in the patron's message queue 10) Sign off!
Created attachment 169870 [details] [review] Bug 30300: Add a patron expiry messaging preference Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 169871 [details] [review] Bug 30300: Update the cronjob to respect patron messaging preferences Test plan: 1) Apply patch and run updatedatabase 2) In system preferences set MembershipExpiryDaysNotice to 5 3) Choose a patron and edit their expiry date to today's date 4) Ensure that patron has a primary email address 5) Run perl misc/cronjobs/membership_expiry.pl -c -v -before 10 -after 10 6) Check the patron's messages, there should be no expiry notice queued 7) Edit the patron's messaging preferences and select the checkbox for email in the row for 'Patron expiry' 8) Repeat step 5 9) There should be a patron expiry message in the patron's message queue 10) Sign off! Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
I think a lot of libraries currently use the "where" parameter of this script to limit to certain patron categories. I think it's a good idea to add to messaging preferences, but will this still work with the patch?
It would do, the script fetches patrons based on the "where" option and then loops through them which is where the patron messaging preferences stuff is handled
(In reply to Matt Blenkinsop from comment #6) > It would do, the script fetches patrons based on the "where" option and then > loops through them which is where the patron messaging preferences stuff is > handled +1 Thanks!
If I read this correctly, before this patch all expiring patrons selected by the job would receive a notice. Afterwards, no one will receive a notice until they set messaging preferences, so libraries will need to add a preference for all patrons, should we do this in the update? or simply note in the release notes? We get the messaging preferences here, but queue_notice also does - do we need to do this? Or can we simply let queue_notice decide what/if to send?
Also, the user won't get a membership renewal notice if they don't want expiry notices? Is that correct?
Created attachment 173810 [details] [review] Bug 30300: Update the cronjob to respect patron messaging preferences Test plan: 1) Apply patch and run updatedatabase 2) In system preferences set MembershipExpiryDaysNotice to 5 3) Choose a patron and edit their expiry date to today's date 4) Ensure that patron has a primary email address 5) Run perl misc/cronjobs/membership_expiry.pl -c -v -before 10 -after 10 6) Check the patron's messages, there should be no expiry notice queued 7) Edit the patron's messaging preferences and select the checkbox for email in the row for 'Patron expiry' 8) Repeat step 5 9) There should be a patron expiry message in the patron's message queue 10) Sign off!
Created attachment 173811 [details] [review] Bug 30300: (QA follow-up) Move logic to queue_notice
Created attachment 173812 [details] [review] Bug 30300: (QA follow-up) Move logic to queue_notice
(In reply to Nick Clemens (kidclamp) from comment #8) > If I read this correctly, before this patch all expiring patrons selected by > the job would receive a notice. Afterwards, no one will receive a notice > until they set messaging preferences, so libraries will need to add a > preference for all patrons, should we do this in the update? or simply note > in the release notes? I'm not sure what the preference would be for libraries? Having it set by default or wanting it to be set explicitly
Created attachment 173820 [details] [review] Bug 30300: Add a patron expiry messaging preference Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 173821 [details] [review] Bug 30300: Update the cronjob to respect patron messaging preferences Test plan: 1) Apply patch and run updatedatabase 2) In system preferences set MembershipExpiryDaysNotice to 5 3) Choose a patron and edit their expiry date to today's date 4) Ensure that patron has a primary email address 5) Run perl misc/cronjobs/membership_expiry.pl -c -v -before 10 -after 10 6) Check the patron's messages, there should be no expiry notice queued 7) Edit the patron's messaging preferences and select the checkbox for email in the row for 'Patron expiry' 8) Repeat step 5 9) There should be a patron expiry message in the patron's message queue 10) Sign off! Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 173822 [details] [review] Bug 30300: (QA follow-up) Move logic to queue_notice Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 173823 [details] [review] Bug 30300: (QA follow-up) Remove extra return The routine loops on the found transports, if there are none it will simply return, no need to short circuit Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Matt Blenkinsop from comment #13) > (In reply to Nick Clemens (kidclamp) from comment #8) > > If I read this correctly, before this patch all expiring patrons selected by > > the job would receive a notice. Afterwards, no one will receive a notice > > until they set messaging preferences, so libraries will need to add a > > preference for all patrons, should we do this in the update? or simply note > > in the release notes? > > I'm not sure what the preference would be for libraries? Having it set by > default or wanting it to be set explicitly We have the pref "AutoRenewlNotices" for automatic renewals - but it's a little crufty Leaving this for RM decisions
I struggle a bit with this one in terms of having no regressions/confusion for libraries and I think this needs more work. 1) Regression (blocker) After updating, no MEMBERSHIP_EXPIRY notices will be sent as no patrons will have the flag in their patron accounts, even if it worked before. I don't have a good solution here. Maybe the best we could do is globally add the flag if MembershipExpiryDaysNotice is set to a value >= 1? I believe together with the where it would keep the current behavior? 2) Documentation (not blocker, but easy) It's not clear from the GUI that a "where" option might overwrite the setting in the patron account. I could have a patron with checked patron expiry notice, that is then "removed" from the list by a where parameter. I think at least a note on the system preference could help here to fight off general confusion. 3) Visibility of configuration option (blocker) I feel the visiblity of the messaging preference should be dependent on the MembershipExpiryDaysNotice system preference so libraries have the option to easily turn this feature off or "keep it invisible" after update. I hope I was not missing something!
Created attachment 174713 [details] [review] Bug 30300: Add a patron expiry messaging preference Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 174714 [details] [review] Bug 30300: Update the cronjob to respect patron messaging preferences Test plan: 1) Apply patch and run updatedatabase 2) In system preferences set MembershipExpiryDaysNotice to 5 3) Choose a patron and edit their expiry date to today's date 4) Ensure that patron has a primary email address 5) Run perl misc/cronjobs/membership_expiry.pl -c -v -before 10 -after 10 6) Check the patron's messages, there should be no expiry notice queued 7) Edit the patron's messaging preferences and select the checkbox for email in the row for 'Patron expiry' 8) Repeat step 5 9) There should be a patron expiry message in the patron's message queue 10) Sign off! Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 174715 [details] [review] Bug 30300: (QA follow-up) Move logic to queue_notice Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 174716 [details] [review] Bug 30300: (QA follow-up) Remove extra return The routine loops on the found transports, if there are none it will simply return, no need to short circuit Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 174717 [details] [review] Bug 30300: (QA follow-up) Prevent regression and improve clarity This patch amends the DB update to check the MembershipExpiryDaysNotice syspref and sets the flag for all patrons if the syspref is set It also improves the UI visiblity by hiding the messaging preference if the syspref isn't set and adding more clarity to the syspref definition
(In reply to Katrin Fischer from comment #19) > 1) Regression (blocker) > > After updating, no MEMBERSHIP_EXPIRY notices will be sent as no patrons will > have the flag in their patron accounts, even if it worked before. > > I don't have a good solution here. Maybe the best we could do is globally > add the flag if MembershipExpiryDaysNotice is set to a value >= 1? I > believe together with the where it would keep the current behavior? I've implemented the above suggestion in the database update file and I've set the bug back to SO so that this can be properly QA checked. This could be quite an intensive update on systems with large numbers of patrons > 2) Documentation (not blocker, but easy) > > It's not clear from the GUI that a "where" option might overwrite the > setting in the patron account. > > I could have a patron with checked patron expiry notice, that is then > "removed" from the list by a where parameter. I think at least a note on the > system preference could help here to fight off general confusion. Added a note to the syspref > 3) Visibility of configuration option (blocker) > > I feel the visiblity of the messaging preference should be dependent on the > MembershipExpiryDaysNotice system preference so libraries have the option to > easily turn this feature off or "keep it invisible" after update. The line in the table is now hidden if the syspref isn't switched on
Created attachment 174888 [details] [review] Bug 30300: (QA follow-up) Prevent regression and improve clarity This patch amends the DB update to check the MembershipExpiryDaysNotice syspref and sets the flag for all patrons if the syspref is set It also improves the UI visiblity by hiding the messaging preference if the syspref isn't set and adding more clarity to the syspref definition