|
Description
Kyle M Hall (khall)
2022-09-26 16:16:24 UTC
Created attachment 140998 [details] [review] Bug 31627: Add ability to embed the letter id in outgoing email notices It can be useful to know exactly what template was used to generate a notice. To further this end, it would be useful be able to send the letter id as part of any emails sent out if there is a letter template associated with the message. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Enable SendLetterIdInEmailNotices 4) Generate an email notice like a checkout notice 5) Note your email has the letter id at the bottom! Either this isn't working or I'm not understanding the intent. Should I be seeing the letter_code appended to the content of the notice? In the database you've called the system preference 'SendLetterIdInEmailNotices'. But when you check in in C4/Letters.pm it is looking for 'SendLetterIdNotices'. (In reply to Lucas Gass from comment #3) > In the database you've called the system preference > 'SendLetterIdInEmailNotices'. > > But when you check in in C4/Letters.pm it is looking for > 'SendLetterIdNotices'. Good catch Lucas! I'll get that fixed asap. Created attachment 141025 [details] [review] Bug 31627: Add ability to embed the letter id in outgoing email notices It can be useful to know exactly what template was used to generate a notice. To further this end, it would be useful be able to send the letter id as part of any emails sent out if there is a letter template associated with the message. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Enable SendLetterIdInEmailNotices 4) Generate an email notice like a checkout notice 5) Note your email has the letter id at the bottom! Please don't forget to set Assignee, thx! Can't apply the patch: ... Applying: Bug 31627: Add ability to embed the letter id in outgoing email notices error: sha1 information is lacking or useless (C4/Letters.pm). error: could not build fake ancestor Patch failed at 0001 Bug 31627: Add ability to embed the letter id in outgoing email notices .. Created attachment 142826 [details] [review] Bug 31627: Add ability to embed the letter id in outgoing email notices It can be useful to know exactly what template was used to generate a notice. To further this end, it would be useful be able to send the letter id as part of any emails sent out if there is a letter template associated with the message. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Enable SendLetterIdInEmailNotices 4) Generate an email notice like a checkout notice 5) Note your email has the letter id at the bottom! I'm not seeing the letter id in the message - either in the preview of the notice (Patron account > Notices) or in the message_queue.
Am I missing something from my setup?
Initially, I thought the letter id would show up in the preview for patron's notices.
Here is what I did when testing using koha-testing-docker (KTD):
1. Applied the patch
2. Ran updatedatabase
3. Ran flush_memcached and restart_all
4. Enable SendLetterIdInEmailNotices
5. Setup so email could be sent in KTD using Gmail [1]
6. Added Gmail address to patron
7. Set up the patron messaging preferences so that the email option is selected for item check-in and item checkout
8. Checked out an item to the patron
9. Preview of notice is available under Patron account > Notices - no letter id/or code
10. Checked what was in the database: select * from message_queue;
11. Processed the message queue: misc/cronjobs/process_message_queue.pl -v
12. No letter id in message received in mny inbox
[1] To setup KTD so that an email message can be sent using Gmail, add this to /etc/koha/sites/kohadev/koha-conf.xml (where an app password is created for a Gmail account and GMAILEMAILADDRESS and APPPASSWORD replaced as appropriate):
<smtp_server>
<host>smtp.gmail.com</host>
<port>587</port>
<timeout>5</timeout>
<ssl_mode>STARTTLS</ssl_mode>
<user_name>GMAILEMAILADDRESS</user_name>
<password>APPPASSWORD</password>
<debug>1</debug>
</smtp_server>
I wonder if it would be nice to have a way to make this invisible at least in HTML notices. Something like <div style="display:none">ID</div> ?
This line looks like it would allow to do just that:
if ($is_html) {
+ $content .= "<br/>$message->{letter_id}" if $message->{id} && C4::Context->preference('SendLetterIdInEmailNotices');
At the moment there is no way libraries could change how this appears in the notices and it might raise questions about the 'number' appearing at the end of the message.
(In reply to Katrin Fischer from comment #10) > I wonder if it would be nice to have a way to make this invisible at least > in HTML notices. Something like <div style="display:none">ID</div> ? > > This line looks like it would allow to do just that: > if ($is_html) { > + $content .= "<br/>$message->{letter_id}" if $message->{id} > && C4::Context->preference('SendLetterIdInEmailNotices'); > > At the moment there is no way libraries could change how this appears in the > notices and it might raise questions about the 'number' appearing at the end > of the message. Sounds good! I'll post a followup for that! Created attachment 143022 [details] [review] Bug 31627: (followup) Hide id for HTML messages I've tried this in KTD and on a sandbox and am not seeing a letter id added to notices as they're generated. Should it be present as soon as the notice is generated? Or does it get added when the notice is sent? (In reply to Andrew Fuerste-Henry from comment #13) > I've tried this in KTD and on a sandbox and am not seeing a letter id added > to notices as they're generated. Should it be present as soon as the notice > is generated? Or does it get added when the notice is sent? It is not present in the generated notice, only in the outgoing email. This is necessary due to the way some notices are built. To test this you'll need to send an actual email and look the email contents. If the email is HTML based, you'll need to look at the HTML source. Created attachment 151536 [details] [review] Bug 31627: Add ability to embed the letter id in outgoing email notices It can be useful to know exactly what template was used to generate a notice. To further this end, it would be useful be able to send the letter id as part of any emails sent out if there is a letter template associated with the message. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Enable SendLetterIdInEmailNotices 4) Generate an email notice like a checkout notice 5) Note your email has the letter id at the bottom! Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 151537 [details] [review] Bug 31627: (followup) Hide id for HTML messages Signed-off-by: Sam Lau <samalau@gmail.com> WARN C4/Letters.pm
OK critic
OK forbidden patterns
OK git manipulation
OK pod
OK pod coverage
SKIP spelling
WARN tidiness
The file is less tidy than before (bad/messy lines before: 394, now: 396)
OK valid
WARN installer/data/mysql/atomicupdate/bug_31627.pl
OK critic
OK file permissions
OK forbidden patterns
OK git manipulation
OK pod
SKIP spelling
WARN tidiness
The file is less tidy than before (bad/messy lines before: 0, now: 5)
OK valid
OK installer/data/mysql/mandatory/sysprefs.sql
OK git manipulation
OK semicolon
OK sysprefs_order
Processing additional checks
* Commit title does not contain 'follow-up' correctly spelt - 17c2012171
Will have a look, dont worry
The HTML solution is fine. But we do not have a counterpart for the plain text notice. It is a bit weird to do this. And referring to 'it is useful to know' from the description, it might be useful for QA to know what the use case is here exactly before getting this further. Please clarify a bit more. Changing status for feedback. Created attachment 155364 [details] [review] Bug 31627: (follow-up) Hide id for HTML messages Signed-off-by: Sam Lau <samalau@gmail.com> I'd actually be tempted to put this in the email headers somehow instead. Created attachment 166374 [details] [review] Bug 31627: Add ability to embed the letter id in outgoing email notices It can be useful to know exactly what template was used to generate a notice. To further this end, it would be useful be able to send the letter id as part of any emails sent out if there is a letter template associated with the message. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Enable SendLetterIdInEmailNotices 4) Generate an email notice like a checkout notice 5) Note your email has the letter id at the bottom! Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 166375 [details] [review] Bug 31627: (follow-up) Hide id for HTML messages Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 166376 [details] [review] Bug 31627: Send id in an email header (In reply to Martin Renvoize from comment #20) > I'd actually be tempted to put this in the email headers somehow instead. Done! Should we remove the syspref altogether with this change? Created attachment 166377 [details] [review] Bug 31627: Send message id in email header as well Created attachment 166378 [details] [review] Bug 31627: Remove new syspref SendLetterIdInEmailNotices Created attachment 166379 [details] [review] Bug 31627: Add ability to embed the letter id in outgoing email notices It can be useful to know exactly what template was used to generate a notice. To further this end, it would be useful be able to send the letter id as part of any emails sent out if there is a letter template associated with the message. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Generate an email notice like a checkout notice 4) Inspect the email headers, note the new X-Koha-Letter-Id and X-Koha-Message-Id headers! Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 166399 [details] [review] Bug 31627: Add ability to embed the letter id in outgoing email notices It can be useful to know exactly what template was used to generate a notice. To further this end, it would be useful be able to send the letter id as part of any emails sent out if there is a letter template associated with the message. Test Plan: 1) Apply this patch 2) Restart all the things! 3) Generate an email notice like a checkout notice 4) Inspect the email headers, note the new X-Koha-Template-Id and X-Koha-Message-Id headers! Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 166400 [details] [review] Bug 31627: (QA follow-up) Catch more locations This patch moves the header set into Koha::Email->create and passes the template_id in a few additional locations to ensure we catch more cases. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 166401 [details] [review] Bug 31627: (QA follow-up) Tidy test block The use case is that it makes it significantly simpler to trace mail output issues for staff.. you can now inspect the message headers and find the template used to generate the notice or the message in the message queue. Updated -Id to -ID in a follow-up! Pushed for 24.05! Well done everyone, thank you! Not backported to 23.11.x |