Bug 29330 - Koha cannot send emails with attachments using Koha::Email and message_queue table
Summary: Koha cannot send emails with attachments using Koha::Email and message_queue ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords: rel_20_11_candidate, rel_21_05_candidate
Depends on: 22343
Blocks: 29365
  Show dependency treegraph
 
Reported: 2021-10-27 03:39 UTC by David Cook
Modified: 2021-11-26 01:07 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05


Attachments
Bug 29330: Add Koha::Email->new_from_string (4.10 KB, patch)
2021-11-05 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29330: Restore handling of serialized MIME messages in message_queue (7.71 KB, patch)
2021-11-05 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29330: Add Koha::Email->new_from_string (4.14 KB, patch)
2021-11-06 22:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 29330: Restore handling of serialized MIME messages in message_queue (7.75 KB, patch)
2021-11-06 22:39 UTC, David Nind
Details | Diff | Splinter Review
Bug 29330: Add Koha::Email->new_from_string (4.20 KB, patch)
2021-11-08 05:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 29330: Restore handling of serialized MIME messages in message_queue (7.99 KB, patch)
2021-11-08 05:59 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 29330: Add Koha::Email->new_from_string (4.26 KB, patch)
2021-11-08 09:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29330: Restore handling of serialized MIME messages in message_queue (7.88 KB, patch)
2021-11-08 09:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29330: (QA follow-up) Use passed MIME type (1.11 KB, patch)
2021-11-08 11:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29330: (QA follow-up) Use passed MIME type (1.12 KB, patch)
2021-11-08 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29330: Add Koha::Email->new_from_string (4.36 KB, patch)
2021-11-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29330: Restore handling of serialized MIME messages in message_queue (7.85 KB, patch)
2021-11-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29330: (QA follow-up) Use passed MIME type (1.06 KB, patch)
2021-11-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29330: (QA follow-up) Change to message/rfc822 (2.03 KB, patch)
2021-11-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2021-10-27 03:39:07 UTC
It looks like bug 22343 introduced a regression where the content_type from the database isn't used when creating a Koha::Email object when processing the message_queue table.

Re-adding the content_type to the creation of the Koha::Email object should in theory fix this bug. 

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22343#c232
Comment 1 David Cook 2021-10-27 22:35:45 UTC
Actually, it looks like it might be more complicated than just losing the content type... 

Email::Stuffer has been such a pain...
Comment 2 David Cook 2021-10-28 00:33:06 UTC
I think we either need to redo how we handle attachments in the message_queue, or have a different email processing function for emails with a different content type from text/plain or text/html.

In either case, it's not going to be a quick fix I think.
Comment 3 David Cook 2021-10-28 00:41:24 UTC
I encountered the issue due to print notices in /usr/share/koha/bin/cronjobs/overdue_notices.pl

After some grepping around... it kind of looks like that's the only bit of Koha code that uses the "attachments" parameter for C4::Letters::EnqueueLetter().

I wonder if there are other parts of Koha that set other content types than text/html and text/plain though in the message_queue table...

A review of 1,000,000 entries in the message_queue shows either: 
- text/html; charset="UTF-8"
- multipart/mixed; boundary="<BLAHBLAH>"
- NULL

However, I suppose it's possible that Koha plugins have enqueued other content? I suppose that's not our concern as core developers though...
Comment 4 David Cook 2021-10-28 00:56:40 UTC
I suppose one option would be to add a 'message_attachments' table. 

But I'm probably not going to work on this one. Instead, I'm going to bypass the email with attachments in /usr/share/koha/bin/cronjobs/overdue_notices.pl
Comment 5 Katrin Fischer 2021-10-28 07:19:08 UTC
David, does this mean you have a solution for the overdues without email that are sent to the library? Other mails with attachements that come to mind are cart and list with their attached MARC files.
Comment 6 David Cook 2021-10-29 01:28:07 UTC
(In reply to Katrin Fischer from comment #5)
> David, does this mean you have a solution for the overdues without email
> that are sent to the library? Other mails with attachements that come to
> mind are cart and list with their attached MARC files.

It's still in progress but I think we'll just use one of the output options like -csv and send it as an email attachment outside of Koha. Then I'll just patch the overdues script not to send the broken email on a per branch basis. (Fixing it in core Koha seems like way too much work at the moment.)

It would be worth reviewing how this emails are sent, although they don't use the message_queue, so they're probably OK. They probably still use MIME::Lite instead of Koha::Email. I'll update the bug description to be more precise in any case.
Comment 7 Katrin Fischer 2021-10-29 07:00:59 UTC
Sorry, now I am confused. Can you describe what features are affected by the problem and thus broken?
Comment 8 Marcel de Rooy 2021-10-29 08:31:35 UTC
Looking at Koha::Email too on bug 28729.
Comment 9 Marcel de Rooy 2021-11-01 08:50:43 UTC
Will give it a try now
Comment 10 Marcel de Rooy 2021-11-01 10:09:58 UTC
(In reply to Marcel de Rooy from comment #9)
> Will give it a try now

Yes, it looks like David was right. This might not be a minimal fix. The add_attachments stuff in C4::Letters does not seem to work well with Email::Stuffer. Imo we should go back to Email::MIME in Koha::Email.

More comments on bug 29365
Comment 11 Marcel de Rooy 2021-11-01 10:14:21 UTC
The good thing is that we hardly use add_attachments only for overdue messages collected for patrons without email address or so.
Comment 12 Tomás Cohen Arazi 2021-11-02 02:33:06 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Marcel de Rooy from comment #9)
> > Will give it a try now
> 
> Yes, it looks like David was right. This might not be a minimal fix. The
> add_attachments stuff in C4::Letters does not seem to work well with
> Email::Stuffer. Imo we should go back to Email::MIME in Koha::Email.
> 
> More comments on bug 29365

Koha::Email used to be just a wrapper for a hashref used to build something suitable for passing to Mail::Sendmail. We didn't use Email::MIME. Multi-part emails with attachments were built by concatenating manually crafted strings...

That said, there's room for improvement. Using Email::MIME is hard, and error prone. That's why the author (of both libraries, and Email::Simple, Email::Sender, etc) suggests using Email::Stuffer.

I don't think it is urgent to replace Email::Stuffer because its API has design issues. In fact, if required we can add MIME content manually.

I hadn't payed attention to this bugs and will now. If the problem is how emails are serialized on the db, maybe we can use Koha::Email->to_string (inherited from Email::Stuffer). But as I said, I've just discovered this bugs.
Comment 13 Tomás Cohen Arazi 2021-11-04 12:03:42 UTC
(In reply to Katrin Fischer from comment #7)
> Sorry, now I am confused. Can you describe what features are affected by the
> problem and thus broken?

Sending attachments works, the cart, the list, etc. It all was tested, and works.
And Email::Stuffer can handle attachments correctly.

The problem is messages that go to the message queue, with attachments, and how the email is rebuilt to be sent.

Can anyone provide 'steps to reproduce' the problem? thanks.
Comment 14 Katrin Fischer 2021-11-04 18:17:14 UTC
Maybe a solution could then be to change the email to be send directly then instead of via the message queue?

The overdue_notices.pl script creates an email with attached overdue messages to be sent to the library when a user should receive an overdue notice, but has no email address.

How I would test:

* You'll need a patron without email address with overdues.
* Notice triggers where one or more of the delays match the overdue item.
* Run the overdue_notices.pl with -t (use triggers)
* You don't need to set up fines.

I believe the code is all in the (very long) overdue_notices.pl.
Comment 15 David Cook 2021-11-04 23:00:11 UTC
(In reply to Katrin Fischer from comment #14)
> Maybe a solution could then be to change the email to be send directly then
> instead of via the message queue?
> 
You're probably right about that being the best way, although it is nice to have an entry in the message queue for reviewing/logging.
Comment 16 Jonathan Druart 2021-11-05 07:52:47 UTC
(In reply to Katrin Fischer from comment #14)
> Maybe a solution could then be to change the email to be send directly then
> instead of via the message queue?

I always thought we were aiming to sent all the notices from the DB table.
Comment 17 Marcel de Rooy 2021-11-05 08:07:43 UTC
(In reply to Jonathan Druart from comment #16)
> (In reply to Katrin Fischer from comment #14)
> > Maybe a solution could then be to change the email to be send directly then
> > instead of via the message queue?
> 
> I always thought we were aiming to sent all the notices from the DB table.

I agree. Please note that this overdues case is really an edge case. We should resolve sending attachments in 22.05?
Comment 18 Tomás Cohen Arazi 2021-11-05 19:39:53 UTC
Created attachment 127404 [details] [review]
Bug 29330: Add Koha::Email->new_from_string

This patch introduces a new method to Koha::Email. This method allows us
to parse a MIME email to initialize the Koha::Email object. This is
particularly important when we are restoring emails from the DB. i.e.
from the *message_queue* table.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass! Koha::Email->new_from_string is the correct
counterpart for Koha::Email->as_string!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2021-11-05 19:39:57 UTC
Created attachment 127405 [details] [review]
Bug 29330: Restore handling of serialized MIME messages in message_queue

This patch changes how multipart MIME messages are handled on the
message_queue table.

The email, with the required attachments, is now generated using
Koha::Email, and serialized using Koha::Email->as_string.

This bug also adds Koha::Email->new_from_string which is used to read
that data from the DB, and produce a Koha::Email object, that can be
further augmented/modified using regular Koha::Email methods.

This implementation should be considered a middle ground, with
backportability in mind. higher-level methods should encapsulate setting
the default headers and addresses, to clean the area a bit further.

Preparation:
- You need a valid SMTP configuration in koha-conf.xml. If you use Gmail
  you can generate an 'app password' and set things like this:

 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>5</timeout>
    <ssl_mode>STARTTLS</ssl_mode>
    <user_name>youraddress@gmail.com</user_name>
    <password>youpassword</password>
    <debug>1</debug>
 </smtp_server>
- Set KohaAdminAddress to your address.

To test:
1. Pick a patron. Make sure it doesn't have any email address (Acevedo?)
2. Set an overdue notice trigger for its category
3. Check something out, with due date in the past to force an overdue
4. Run:
   $ kshell
  k$ misc/cronjobs/overdue_notices.pl -v
  k$ exit
   $ koha-mysql kohadev
   > SELECT * FROM message_queue WHERE borrowernumber=the_borrowernumber;
=> SUCCESS: A notice has been created
5. Run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl --verbose
=> SUCCESS: SMTP is ok => Email is sent
=> FAIL: Your inbox shows an email with weird content
6. Apply this patches
7. Run:
   $ koha-mysql kohadev
   > DELETE FROM message_queue;
8. Repeat 4 and 5
=> SUCCESS: You got an email with an attachment!
=> SUCCESS: The attachment contains an email that couldn't be delivered!
9. Try all the things that enqueue messages :-D
=> SUCCESS: No behavior change
10. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2021-11-05 19:45:32 UTC
Ok, here's my attempt. I tried several things, and this is the simplest I could come up with. Thinking of a backportable patch.

I chose 'MIME' as the content_type because Email::MIME doesn't seem to return the same thing MIME::Lite returns. and, well.

On bug 29365 we could implement high level methods for setting 'the addresses' and  'the default headers' and removing the FIXME I introduce.
Comment 21 David Nind 2021-11-06 22:38:59 UTC
Created attachment 127413 [details] [review]
Bug 29330: Add Koha::Email->new_from_string

This patch introduces a new method to Koha::Email. This method allows us
to parse a MIME email to initialize the Koha::Email object. This is
particularly important when we are restoring emails from the DB. i.e.
from the *message_queue* table.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass! Koha::Email->new_from_string is the correct
counterpart for Koha::Email->as_string!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2021-11-06 22:39:04 UTC
Created attachment 127414 [details] [review]
Bug 29330: Restore handling of serialized MIME messages in message_queue

This patch changes how multipart MIME messages are handled on the
message_queue table.

The email, with the required attachments, is now generated using
Koha::Email, and serialized using Koha::Email->as_string.

This bug also adds Koha::Email->new_from_string which is used to read
that data from the DB, and produce a Koha::Email object, that can be
further augmented/modified using regular Koha::Email methods.

This implementation should be considered a middle ground, with
backportability in mind. higher-level methods should encapsulate setting
the default headers and addresses, to clean the area a bit further.

Preparation:
- You need a valid SMTP configuration in koha-conf.xml. If you use Gmail
  you can generate an 'app password' and set things like this:

 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>5</timeout>
    <ssl_mode>STARTTLS</ssl_mode>
    <user_name>youraddress@gmail.com</user_name>
    <password>youpassword</password>
    <debug>1</debug>
 </smtp_server>
- Set KohaAdminAddress to your address.

To test:
1. Pick a patron. Make sure it doesn't have any email address (Acevedo?)
2. Set an overdue notice trigger for its category
3. Check something out, with due date in the past to force an overdue
4. Run:
   $ kshell
  k$ misc/cronjobs/overdue_notices.pl -v
  k$ exit
   $ koha-mysql kohadev
   > SELECT * FROM message_queue WHERE borrowernumber=the_borrowernumber;
=> SUCCESS: A notice has been created
5. Run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl --verbose
=> SUCCESS: SMTP is ok => Email is sent
=> FAIL: Your inbox shows an email with weird content
6. Apply this patches
7. Run:
   $ koha-mysql kohadev
   > DELETE FROM message_queue;
8. Repeat 4 and 5
=> SUCCESS: You got an email with an attachment!
=> SUCCESS: The attachment contains an email that couldn't be delivered!
9. Try all the things that enqueue messages :-D
=> SUCCESS: No behavior change
10. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 David Nind 2021-11-06 22:40:15 UTC
Testing notes (using koha-testing-docker):

- To set notice triggers (step 2): Tools > Overdue notice/status triggers

- Additional test for email (step 9)
  . [NON QUEUED] Sending cart and lists to email address from the OPAC
  . [NON QUEUED] Password recovery (enable OpacResetPassword)
  . Sent overdue message as an HTML message (added paragraph tags and bolded some text)
  . Sent messages for items checked in and checked out
Comment 24 Victor Grousset/tuxayo 2021-11-08 05:59:31 UTC
Created attachment 127418 [details] [review]
Bug 29330: Add Koha::Email->new_from_string

This patch introduces a new method to Koha::Email. This method allows us
to parse a MIME email to initialize the Koha::Email object. This is
particularly important when we are restoring emails from the DB. i.e.
from the *message_queue* table.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass! Koha::Email->new_from_string is the correct
counterpart for Koha::Email->as_string!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Victor Grousset/tuxayo 2021-11-08 05:59:36 UTC
Created attachment 127419 [details] [review]
Bug 29330: Restore handling of serialized MIME messages in message_queue

This patch changes how multipart MIME messages are handled on the
message_queue table.

The email, with the required attachments, is now generated using
Koha::Email, and serialized using Koha::Email->as_string.

This bug also adds Koha::Email->new_from_string which is used to read
that data from the DB, and produce a Koha::Email object, that can be
further augmented/modified using regular Koha::Email methods.

This implementation should be considered a middle ground, with
backportability in mind. higher-level methods should encapsulate setting
the default headers and addresses, to clean the area a bit further.

Preparation:
- You need a valid SMTP configuration in koha-conf.xml. If you use Gmail
  you can generate an 'app password' and set things like this:

 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>5</timeout>
    <ssl_mode>STARTTLS</ssl_mode>
    <user_name>youraddress@gmail.com</user_name>
    <password>youpassword</password>
    <debug>1</debug>
 </smtp_server>
- Set KohaAdminAddress to your address.

To test:
1. Pick a patron. Make sure it doesn't have any email address (Acevedo?)
2. Set an overdue notice trigger for its category
3. Check something out, with due date in the past to force an overdue
4. Run:
   $ kshell
  k$ misc/cronjobs/overdue_notices.pl -v
  k$ exit
   $ koha-mysql kohadev
   > SELECT * FROM message_queue WHERE borrowernumber=the_borrowernumber;
=> SUCCESS: A notice has been created
5. Run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl --verbose
=> SUCCESS: SMTP is ok => Email is sent
=> FAIL: Your inbox shows an email with weird content
6. Apply this patches
7. Run:
   $ koha-mysql kohadev
   > DELETE FROM message_queue;
8. Repeat 4 and 5
=> SUCCESS: You got an email with an attachment!
=> SUCCESS: The attachment contains an email that couldn't be delivered!
9. Try all the things that enqueue messages :-D
=> SUCCESS: No behavior change
10. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
  «Try all the things that enqueue messages»: I only testes an overdue
  notice email.
Comment 26 Victor Grousset/tuxayo 2021-11-08 06:13:07 UTC
Thanks David, you were faster ^^

As for QA all I can help about is say is that the QA script is happy :D
Comment 27 Martin Renvoize 2021-11-08 09:10:38 UTC
Created attachment 127429 [details] [review]
Bug 29330: Add Koha::Email->new_from_string

This patch introduces a new method to Koha::Email. This method allows us
to parse a MIME email to initialize the Koha::Email object. This is
particularly important when we are restoring emails from the DB. i.e.
from the *message_queue* table.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass! Koha::Email->new_from_string is the correct
counterpart for Koha::Email->as_string!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2021-11-08 09:10:43 UTC
Created attachment 127430 [details] [review]
Bug 29330: Restore handling of serialized MIME messages in message_queue

This patch changes how multipart MIME messages are handled on the
message_queue table.

The email, with the required attachments, is now generated using
Koha::Email, and serialized using Koha::Email->as_string.

This bug also adds Koha::Email->new_from_string which is used to read
that data from the DB, and produce a Koha::Email object, that can be
further augmented/modified using regular Koha::Email methods.

This implementation should be considered a middle ground, with
backportability in mind. higher-level methods should encapsulate setting
the default headers and addresses, to clean the area a bit further.

Preparation:
- You need a valid SMTP configuration in koha-conf.xml. If you use Gmail
  you can generate an 'app password' and set things like this:

 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>5</timeout>
    <ssl_mode>STARTTLS</ssl_mode>
    <user_name>youraddress@gmail.com</user_name>
    <password>youpassword</password>
    <debug>1</debug>
 </smtp_server>
- Set KohaAdminAddress to your address.

To test:
1. Pick a patron. Make sure it doesn't have any email address (Acevedo?)
2. Set an overdue notice trigger for its category
3. Check something out, with due date in the past to force an overdue
4. Run:
   $ kshell
  k$ misc/cronjobs/overdue_notices.pl -v
  k$ exit
   $ koha-mysql kohadev
   > SELECT * FROM message_queue WHERE borrowernumber=the_borrowernumber;
=> SUCCESS: A notice has been created
5. Run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl --verbose
=> SUCCESS: SMTP is ok => Email is sent
=> FAIL: Your inbox shows an email with weird content
6. Apply this patches
7. Run:
   $ koha-mysql kohadev
   > DELETE FROM message_queue;
8. Repeat 4 and 5
=> SUCCESS: You got an email with an attachment!
=> SUCCESS: The attachment contains an email that couldn't be delivered!
9. Try all the things that enqueue messages :-D
=> SUCCESS: No behavior change
10. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2021-11-08 09:16:15 UTC
Testing yields positive results in all cases I tried.. QA scripts is happy.

That's some somewhat fnky looking code here, but I understand it's in aide of backports.. we can clean up during the next cycle I think.

The only bit that raised slight alarm bells.. but that's only because my email understanding isn't at 'expert god level' is the internal content type of the attachments being hardcoded to `application/octet-stream`.. in all my testing I couldn't find a problem with that.. but I don't understand it either so would love a comment from Tomas explaining?

Passing QA anyway as this does indeed fix the issues as described and doesn't raise and real red flags.
Comment 30 Tomás Cohen Arazi 2021-11-08 10:56:17 UTC
(In reply to Martin Renvoize from comment #29)
> The only bit that raised slight alarm bells.. but that's only because my
> email understanding isn't at 'expert god level' is the internal content type
> of the attachments being hardcoded to `application/octet-stream`.. in all my
> testing I couldn't find a problem with that.. but I don't understand it
> either so would love a comment from Tomas explaining?

Good catch, Martin. I'll post a follow-up patch for that. application/octet-stream is the fallback in Email::Stuffer when it cannot make a better guess on the content. I overlooked it because the email clients I tested didn't care about that and displayed things correctly. But we should explicitly pass text/plain.
Comment 31 Tomás Cohen Arazi 2021-11-08 11:14:15 UTC
Created attachment 127437 [details] [review]
Bug 29330: (QA follow-up) Use passed MIME type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

https://bugs.koha-community.org/show_bug.cgi?id=29303
Comment 32 Martin Renvoize 2021-11-08 12:09:56 UTC
Created attachment 127444 [details] [review]
Bug 29330: (QA follow-up) Use passed MIME type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2021-11-08 12:10:35 UTC
Thanks for the explanation and follow-up.. adding my QA stamp to the final patch :)
Comment 34 David Cook 2021-11-09 22:50:54 UTC
Cool workaround. I had looked at Email::MIME->new(), but wasn't sure how we'd make it work.

Haven't tried out the code but thanks for working on this, Tomas!
Comment 35 Katrin Fischer 2021-11-10 10:13:16 UTC
I tried to apply on 21.11.11 for testing and there are lots of conflicts right now - might need work for the maintenance branches:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 29330: Add Koha::Email->new_from_string
Using index info to reconstruct a base tree...
M	Koha/Email.pm
M	t/Koha/Email.t
Falling back to patching base and 3-way merge...
Auto-merging t/Koha/Email.t
CONFLICT (content): Merge conflict in t/Koha/Email.t
Auto-merging Koha/Email.pm
CONFLICT (content): Merge conflict in Koha/Email.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 29330: Add Koha::Email->new_from_string
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-29330-Add-KohaEmail-newfromstring-R_84LL.patch
Comment 36 Katrin Fischer 2021-11-10 10:19:34 UTC
Actually these first conflicts are not so bad, the one in Letters.pm is the bad one I am failing to resolve.
Comment 37 Marcel de Rooy 2021-11-12 07:40:49 UTC
Still having a look here
Comment 38 Marcel de Rooy 2021-11-12 08:55:20 UTC
w3.org
The Message/rfc822 (primary) subtype: A Content-Type of "message/rfc822" indicates that the body contains an encapsulated message, with the syntax of an RFC 822 message.
Better than using MIME as content-type..
Comment 39 Marcel de Rooy 2021-11-12 09:16:42 UTC
Created attachment 127570 [details] [review]
Bug 29330: Add Koha::Email->new_from_string

This patch introduces a new method to Koha::Email. This method allows us
to parse a MIME email to initialize the Koha::Email object. This is
particularly important when we are restoring emails from the DB. i.e.
from the *message_queue* table.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Email.t
=> SUCCESS: Tests pass! Koha::Email->new_from_string is the correct
counterpart for Koha::Email->as_string!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2021-11-12 09:16:46 UTC
Created attachment 127571 [details] [review]
Bug 29330: Restore handling of serialized MIME messages in message_queue

This patch changes how multipart MIME messages are handled on the
message_queue table.

The email, with the required attachments, is now generated using
Koha::Email, and serialized using Koha::Email->as_string.

This bug also adds Koha::Email->new_from_string which is used to read
that data from the DB, and produce a Koha::Email object, that can be
further augmented/modified using regular Koha::Email methods.

This implementation should be considered a middle ground, with
backportability in mind. higher-level methods should encapsulate setting
the default headers and addresses, to clean the area a bit further.

Preparation:
- You need a valid SMTP configuration in koha-conf.xml. If you use Gmail
  you can generate an 'app password' and set things like this:

 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>5</timeout>
    <ssl_mode>STARTTLS</ssl_mode>
    <user_name>youraddress@gmail.com</user_name>
    <password>youpassword</password>
    <debug>1</debug>
 </smtp_server>
- Set KohaAdminAddress to your address.

To test:
1. Pick a patron. Make sure it doesn't have any email address (Acevedo?)
2. Set an overdue notice trigger for its category
3. Check something out, with due date in the past to force an overdue
4. Run:
   $ kshell
  k$ misc/cronjobs/overdue_notices.pl -v
  k$ exit
   $ koha-mysql kohadev
   > SELECT * FROM message_queue WHERE borrowernumber=the_borrowernumber;
=> SUCCESS: A notice has been created
5. Run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl --verbose
=> SUCCESS: SMTP is ok => Email is sent
=> FAIL: Your inbox shows an email with weird content
6. Apply this patches
7. Run:
   $ koha-mysql kohadev
   > DELETE FROM message_queue;
8. Repeat 4 and 5
=> SUCCESS: You got an email with an attachment!
=> SUCCESS: The attachment contains an email that couldn't be delivered!
9. Try all the things that enqueue messages :-D
=> SUCCESS: No behavior change
10. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Edited the POD, restoring a few lines that describe the needed hash
keys of the attachments.
Comment 41 Marcel de Rooy 2021-11-12 09:16:51 UTC
Created attachment 127572 [details] [review]
Bug 29330: (QA follow-up) Use passed MIME type

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: tidied.
Comment 42 Marcel de Rooy 2021-11-12 09:16:54 UTC
Created attachment 127573 [details] [review]
Bug 29330: (QA follow-up) Change to message/rfc822

This content-type might be more appropriated to use as a temporary
label for the serialized email message with attachments.

WARNING: perl -cw tells you that the constant is redefined. This has
to do with an already existing module dependency loop of Letters.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2021-11-12 09:18:01 UTC
Good job, Tomas. Looks good to me. Tested with multiple attachments including some weird UTF8 stuff.
Note that we do not have tests (yet) for adding attachments.
Comment 44 Marcel de Rooy 2021-11-12 09:30:55 UTC
Hmm. Still having an issue?
Comment 45 Marcel de Rooy 2021-11-12 09:36:56 UTC
(In reply to Marcel de Rooy from comment #44)
> Hmm. Still having an issue?

False alarm. Back to green.
Comment 46 Jonathan Druart 2021-11-15 11:40:11 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 47 Kyle M Hall 2021-11-19 15:44:32 UTC
If this is needed to for 21.05.x please rebase and notify me!
Comment 48 Katrin Fischer 2021-11-19 15:45:53 UTC
It is needed... but I can't provide the reabase. :(
Comment 49 Victor Grousset/tuxayo 2021-11-20 01:10:06 UTC
conflict is due to bug 28803
Comment 50 Victor Grousset/tuxayo 2021-11-20 01:11:52 UTC
No actual dependency it seems. bug 28803 adds a catch and this patch adds code in the try.
Comment 51 Victor Grousset/tuxayo 2021-11-20 02:26:43 UTC
Nonetheless with bug 28803[1] applied, only minor conflicts remain. Other than some attention the module use conflict, the patch can be applied.


[1] which is major and is not able to apply on 21.05.x so will likely be backported so no need to circumvent it's absence.


For 20.11.x it might be trickier. Bug 28803 depends on bug 28813 which depends on 14723 which might not be an easy 20.11.x backport. So circumventing bug 28803 might be the way.
Comment 52 Victor Grousset/tuxayo 2021-11-20 23:29:54 UTC
errata:

> and is not able to apply on 21.05.x

and is *now* able to be applied on 21.05.x
Comment 53 David Cook 2021-11-21 22:32:13 UTC
(In reply to Victor Grousset/tuxayo from comment #52)
> errata:
> 
> > and is not able to apply on 21.05.x
> 
> and is *now* able to be applied on 21.05.x

Could you clarify this one a bit more? I'd like to apply these patches to 21.05. What did you need to do that?
Comment 54 Victor Grousset/tuxayo 2021-11-22 02:20:44 UTC
(clarified in chat, no change in the backport plan)
Comment 55 David Cook 2021-11-22 03:36:46 UTC
(In reply to Victor Grousset/tuxayo from comment #54)
> (clarified in chat, no change in the backport plan)

Well... I'd say that I still don't really understand and that I'll just wait to see what Kyle comes up with.
Comment 56 Kyle M Hall 2021-11-22 18:22:36 UTC
Pushed to 21.05.x for 21.05.05
Comment 57 Fridolin Somers 2021-11-23 06:56:50 UTC
(In reply to Victor Grousset/tuxayo from comment #51)
> 
> For 20.11.x it might be trickier. Bug 28803 depends on bug 28813 which
> depends on 14723 which might not be an easy 20.11.x backport. So
> circumventing bug 28803 might be the way.

So we can skip for 20.11.x ?
Comment 58 Victor Grousset/tuxayo 2021-11-24 20:33:19 UTC
I don't think there is a dependency from bug 28803 (and it's dep bug 28813) **for the bug occurrence**.
20.11.x should be affected since bug 22343 is there

28803 gets in the way to seamlessly backport the fix.

To reword the results of my testing, here are the possibilities
1. backport this bug 29330 directly even if it conflicts due to the lack of 28803. It adds a catch without changing the code in the try and this patch (29330) adds code in the try.

2. try to backport 28803 without it's dependency 28813 (it might not be actually a dependency our case)
Comment 59 David Cook 2021-11-24 22:42:24 UTC
(In reply to Victor Grousset/tuxayo from comment #50)
> No actual dependency it seems. bug 28803 adds a catch and this patch adds
> code in the try.

So you're saying that the merge conflict is just due to the surrounding lines being different so it can't auto-merge without manual intervention? That's not so bad.
Comment 60 Victor Grousset/tuxayo 2021-11-25 13:43:44 UTC
Yes, that's how it looks :)

And bug 28803 made it to oldstable anyway. Possibility 2 of comment 58 happened ^^
Comment 61 Fridolin Somers 2021-11-25 21:04:41 UTC
Ich not easy.
I prefer not risking breaking all email feature just before 20.11.12 release.
Comment 62 Victor Grousset/tuxayo 2021-11-26 01:07:08 UTC
Indeed.

Wait, I though I really needed this one for 20.05 but it's not even affected since it's about a feature that came in 20.11

So it turns out I don't care about this bug :P

See ya!
🚪<= 🏃