Created attachment 127237 [details] [review] Bug 29393 - Ability to send emails from patron details page Test plan - apply Bug 12802 - go to a patron's details page - click on "Add message" in tools bar - choose "Email" in "Add a message for" - Select a notice (module members) or manualy fill the message
Created attachment 127238 [details] [review] Bug 29393 - Ability to send emails from patron details page Test plan - apply Bug 12802 - go to a patron's details page - click on "Add message" in tools bar - choose "Email" in "Add a message for" - Select a notice (module members) or manualy fill the message - Save - Check the message is saved in message_queue table
This adds the ability to send email via patron messages (/cgi-bin/koha/circ/add_message.pl). This feature depends on bug 12802. Recipients are defined by AutoEmailPrimaryAddress.
I worry that too many of the available notices depend on logic or circumstances which aren't taken into account for here. - ACCTDETAILS Account Details Template - DEFAULT -- This one is useful, although it's not good that our sample notice includes the encrypted password. - MEMBERSHIP_EXPIRY Account expiration -- I can see this being useful as well. - OPAC_REG_VERIFY Opac Self-Registration Verification Email -- This would be useful if it could be done, but can it? If you require email verification for self-registrations the patron record isn't created until verified. Meaning you couldn't find the patron record to re-send the verification. - PASSWORD_RESET Online password reset -- Useful, but doesn't work. There is no link in the message. - DISCHARGE Discharge -- This notice is usually generated as a PDF, not sent via email. Does an email work for the discharge process? These notices don't make sense because there's not enough context for the message: PROBLEM_REPORT OPAC Problem Report SHARE_ACCEPT Notification about an accepted share SHARE_INVITE Invitation for sharing a list
I feel this would benefit from having a permission attached to it. Many libraries will not want all staff to have this ability and will want an easy way to disable it, without blocking staff from being able to use the Add message functionality.
(In reply to Owen Leonard from comment #4) > I worry that too many of the available notices depend on logic or > circumstances which aren't taken into account for here. > Hello Owen, I would say that it is the responsibility of the user/librarian to use the appropriate letter. Is there any security/system/GDPR risk according to you ? The goal here is to allow librarian to have some predefined letters to be used in the "Add message" feature. I choose "members" as letters category for this. Would a new module be better ? And thank you for testing.
Created attachment 127452 [details] [review] Bug 29393 - Use patron language in letters
Created attachment 127506 [details] [review] Bug 29393 - add a dedicated letters module for add message
Created attachment 127511 [details] [review] Bug 29393 - Add send_messages_to_borrowers permission
Created attachment 127847 [details] [review] Bug 29393 - Don't show notice templates for each language
Created attachment 128215 [details] [review] Bug 29393 - Ability to send emails from patron details page Test plan - apply Bug 12802 - go to a patron's details page - click on "Add message" in tools bar - choose "Email" in "Add a message for" - Select a notice (module members) or manualy fill the message - Save - Check the message is saved in message_queue table Sponsored-by: Aix-Marseille University
Created attachment 128216 [details] [review] Bug 29393 - Use patron language in letters Sponsored-by: Aix-Marseille University
Created attachment 128217 [details] [review] Bug 29393 - add a dedicated letters module for add message Sponsored-by: Aix-Marseille University
Created attachment 128218 [details] [review] Bug 29393 - Add send_messages_to_borrowers permission Sponsored-by: Aix-Marseille University
Created attachment 128219 [details] [review] Bug 29393 - Don't show notice templates for each language Sponsored-by: Aix-Marseille University
Created attachment 128363 [details] [review] Bug 29393 - Sort notice templates by name Sponsored-by: Aix-Marseille University
patch doesn't apply
Created attachment 148187 [details] [review] Bug 29393: (follow-up) Explicite use of methods from C4
Created attachment 148188 [details] [review] Bug 29393: Ability to send emails from patron details page Test plan - apply Bug 12802 - go to a patron's details page - click on "Add message" in tools bar - choose "Email" in "Add a message for" - Select a notice (module members) or manualy fill the message - Save - Check the message is saved in message_queue table
Created attachment 148189 [details] [review] Bug 29393: Use patron language in letters
Created attachment 148190 [details] [review] Bug 29393: Add a dedicated letters module for add message
Created attachment 148191 [details] [review] Bug 29393: Add send_messages_to_borrowers permission
Created attachment 148192 [details] [review] Bug 29393: Don't show notice templates for each language
Created attachment 148193 [details] [review] Bug 29393: Sort notice templates by name
Created attachment 148194 [details] [review] Bug 29393: (follow-up) Explicite use of methods from C4
rebased on current master + a small follow-up
Created attachment 149812 [details] [review] Bug 29393: Prevent SQL errors when SQL strict mode is set
Merge conflicts in circ/add_message.pl.
Created attachment 160107 [details] [review] Bug 29393: Ability to send emails from patron details page Test plan - apply Bug 12802 - go to a patron's details page - click on "Add message" in tools bar - choose "Email" in "Add a message for" - Select a notice (module members) or manualy fill the message - Save - Check the message is saved in message_queue table
Created attachment 160108 [details] [review] Bug 29393: Use patron language in letters
Created attachment 160109 [details] [review] Bug 29393: Add a dedicated letters module for add message
Created attachment 160110 [details] [review] Bug 29393: Add send_messages_to_borrowers permission
Created attachment 160111 [details] [review] Bug 29393: Don't show notice templates for each language
Created attachment 160112 [details] [review] Bug 29393: Sort notice templates by name
Created attachment 160113 [details] [review] Bug 29393: (follow-up) Explicite use of methods from C4
Created attachment 160114 [details] [review] Bug 29393: Prevent SQL errors when SQL strict mode is set
Just a rebase for now.. Reviewing the code a bit next.
I'm interested in the idea's here.. but I'd love to see a specification or set or original requirements attached. I fear we're ending up with a series of overlapping features.. we need someone to take a step back and look at the big picture. We now have 'OPAC Problem reports', 'Catalog concerns (misnamed due to original requirements.. should really be 'request tracking'), Notices, 'OPAC Note' and checkout notes.
I feel like we could improve the notice template functionality, displaying a preview (maybe even editable) of the notice prior to sending.
Some notes: - If you add a new notice to Patrons (MEMBERS) category, they do not show up in Add message email drop down. Test plan does not apply in this case. - If you add a new notice to "Add message" category, they show up in Add message email drop down. There is no mention of this category in the test plan. - When you choose a notice to send, you cannot see it's content. You can see it only after it is saved to message queue.
Created attachment 160633 [details] [review] Bug 29393: Prevent SQL errors when SQL strict mode is set Signed-off-by: matthias <matthias.le-gac@inlibro.net>
Created attachment 160638 [details] [review] Bug 29393: Prevent SQL errors when SQL strict mode is set Signed-off-by: matthias <matthias.le-gac@inlibro.net>
Created attachment 160639 [details] [review] Bug 29393: Prevent SQL errors when SQL strict mode is set Signed-off-by: Phan Tung Bui <phan-tung.bui@inlibro.com>
Created attachment 165498 [details] [review] Bug 29393: Ability to send emails from patron details page Test plan - apply Bug 12802 - go to a patron's details page - click on "Add message" in tools bar - choose "Email" in "Add a message for" - Select a notice (module members) or manualy fill the message - Save - Check the message is saved in message_queue table
Created attachment 165499 [details] [review] Bug 29393: Use patron language in letters
Created attachment 165500 [details] [review] Bug 29393: Add a dedicated letters module for add message
Created attachment 165501 [details] [review] Bug 29393: Add send_messages_to_borrowers permission
Created attachment 165502 [details] [review] Bug 29393: Don't show notice templates for each language
Created attachment 165503 [details] [review] Bug 29393: Sort notice templates by name
Created attachment 165504 [details] [review] Bug 29393: (follow-up) Explicite use of methods from C4
Created attachment 165505 [details] [review] Bug 29393: Prevent SQL errors when SQL strict mode is set Signed-off-by: Phan Tung Bui <phan-tung.bui@inlibro.com>
Another rebase done.. still haven't had a chance to properly review and I still wonder if we have a bunch of conflicting coding going on.
I'm going to experiment a bit on bug 32436. I'm looking at introducing API routes to allow fetching the list of available notices in a particular 'module' and also to fetch the result of a GetPreparedLetter. I think that would allow us to have a considerably nicer interface implementation here. I'd also be interested in some refinements to enhance the modal to appear more like the 'Display ticket' or 'Add catalog concern' modals from that area as I think they're more appealing than the current proposal here. That all said, I don't want to block this one, so it would be nice to find someone to QA it... I'll ask around.
Created attachment 165511 [details] [review] Bug 29393: Ability to send emails from patron details page Test plan - apply Bug 12802 - go to a patron's details page - click on "Add message" in tools bar - choose "Email" in "Add a message for" - Select a notice (module members) or manualy fill the message - Save - Check the message is saved in message_queue table Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165512 [details] [review] Bug 29393: Use patron language in letters Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165513 [details] [review] Bug 29393: Add a dedicated letters module for add message Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165514 [details] [review] Bug 29393: Add send_messages_to_borrowers permission Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165515 [details] [review] Bug 29393: Don't show notice templates for each language Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165516 [details] [review] Bug 29393: Sort notice templates by name Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165517 [details] [review] Bug 29393: (follow-up) Explicite use of methods from C4 Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165518 [details] [review] Bug 29393: Prevent SQL errors when SQL strict mode is set Signed-off-by: Matthias Le Gac <matthias.le-gac@inlibro.com> Signed-off-by: Phan Tung Bui <phan-tung.bui@inlibro.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165519 [details] [review] Bug 29393: (QA follow-up) Make database update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165520 [details] [review] Bug 29393: (QA follow-up) Tidy add_message.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165521 [details] [review] Bug 29393: (QA follow-up) Tidy and make executable the new test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 165522 [details] [review] Bug 29393: (QA follow-up) Improve modal styling The modal still used a lot of older styles, this patch updates the classes to make it more consistent with other message type modals. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
So, whilst I think there's lots of improvements we can make here, I think this is good enough as is for a first push. I'm going to submit bugs for the follow-ups I'd like to see later. QA scripts are happy and the feature works reasonably well. Tests passing Passing QA
Please also look out for the small stuff ;) 1) Database update success message should be specific + say $out "Update is going well so far"; Fixed inline when moving the database update. 2) Name of new notice module Updated from "Add message" to read "Patrons (custom message)". We also discussed merging with "Patrons (custom slips)" making use of the different transport methods. NOTE: Please file bug :)
Pushed for 24.05! Well done everyone, thank you!
I need a follow-up here to fix the tests: t_Koha_Auth_Permissions_t.superlibrarian tests (11)
Created attachment 165942 [details] [review] Bug 29393: (QA follow-up) Add key to Permissions test We missed adding the new superlibrarian key to the Permissions test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Follow-up pushed to make Jenkins happy (it's a public holiday for the RM so I'm going in as RM backup today)
(In reply to Martin Renvoize from comment #71) > Follow-up pushed to make Jenkins happy (it's a public holiday for the RM so > I'm going in as RM backup today) Switching back to 'Pushed to main' :)
Any sponsors for this one that need recognition? Please add them to the release notes if there were. **Sponsored by** *Your sponsor name here* That format will show up nicely in the notes.
OK.. I see the sponsorship notes got lost at some point during rebases.
Not backported to 23.11.x