Bug 32575

Most of my clients don't use this script, but I have bumped into this issue with those that do.

I'm also able to reproduce it using koha-testing-docker, a quick Perl script, and an external SMTP server.

--

If you look at Email::Stuffer::email() and Email::MIME::parts_set(), it becomes clear that if Email::Stuffer only has 1 part (e.g. an attachment), it will always be sent out as a single part email (ie direct attachment) and not a multipart. 

It seems that different email clients handle single part emails differently. As Magnus has observed, sometimes the HTML is in the body of an email. I see CSVs get added as attachments with the subject line of the email as the filename plus ".txt" at the end. 

I think the best practice is to use "multipart/mixed" even when there is only a single part as it has the best email client compatibility. But that's not how Email::MIME seems to work.

--

In the past, I've solved this the same way Magnus has by adding a "text_body" value. Even if it's just " ", it would work.

However, it appears that an alternative is just to set the "Content-Type" header at the email level.

Here's a little test program for using in koha-testing-docker:

#!/usr/bin/perl
use Modern::Perl;
use C4::Context;
use Koha::Email;
use Koha::SMTP::Servers;
my $transport = Koha::SMTP::Servers->get_default->transport;
my $email = Koha::Email->create(
        {
            from    => C4::Context->preference('KohaAdminEmailAddress'),
            to      => 'your@email.address',
            subject => 'My test email',
        }
);
$email->header('Content-Type' => 'multipart/mixed');
my $filepath = '/tmp/notices_ODUE3-2024-06-17.csv';
my $filename = 'notices_ODUE3-2024-06-17.csv';
my $mimetype = 'text/csv';
$email->attach_file(
    $filepath,
    content_type => $mimetype,
    charset      => 'UTF-8',
    name         => $filename,
    disposition  => 'attachment',
);
$email->send_or_die( { transport => $transport } );

--

Some steps:
1. Set up the SMTP server config in koha-conf.xml for a server that works
2. Set up a valid email sender in KohaAdminEmailAddress
3. Create a little CSV file at /tmp/notices_ODUE3-2024-06-17.csv 
4. Run the program

One disadvantage of explicitly setting the Content-Type at the email level in lieu of a text body (empty or otherwise) is that it appears SMTP injected footers don't get added. 

So I'm actually inclined to go with an empty text body, so that SMTP servers can still add their footers. 

I'm hesitant to put in a text body of "See attached file" just because it would be hard-coded to English, and I think creating a whole notice template for gather_print_notices.pl is a bit overkill at this point.

Created attachment 167779 [details] [review]
Bug 32575: Tidy patch

This is a tough one to test on koha-testing-docker, as you need an available SMTP server. 

To test the script itself, you'd need to generate notices as well.

It's a pretty simple change overall, and the program I added in the comments earlier can prove the concept for anyone who has a SMTP server available for their ktd...

Of course, I notice that the subject is hard coded to English with: "Print notices for " . $today_syspref.

Maybe it's not a drama to have the body text in English?

Or have some default text that is overridden using a CLI option?

Could we maybe clarify the description of the bug a bit?

(In reply to Katrin Fischer from comment #9)
> Could we maybe clarify the description of the bug a bit?

When no text/html body is added to the email (which is always with this email), attachments aren't attached correctly.

Created attachment 167820 [details] [review]
Bug 32575: Add an empty text body to fix multipart/mixed handling

By adding an empty text body, we force Email::Stuffer/Email::MIME
to use multipart/mixed handling for the attachment instead of forcing
a single part (ie direct attachment) email, which is not consistently
handled by different email clients.

An empty text body is language-neutral (ie not imposing English),
and it allows SMTP servers to inject organisational footers into
the email (e.g. confidentiality notices).

Signed-off-by: Magnus Enger <magnus@libriotech.no>
I have tested this solution in production, and it works for me.

Created attachment 167821 [details] [review]
Bug 32575: Tidy patch

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Created attachment 168225 [details] [review]
Bug 32575: Add an empty text body to fix multipart/mixed handling

By adding an empty text body, we force Email::Stuffer/Email::MIME
to use multipart/mixed handling for the attachment instead of forcing
a single part (ie direct attachment) email, which is not consistently
handled by different email clients.

An empty text body is language-neutral (ie not imposing English),
and it allows SMTP servers to inject organisational footers into
the email (e.g. confidentiality notices).

Signed-off-by: Magnus Enger <magnus@libriotech.no>
I have tested this solution in production, and it works for me.

Bug 32575: Tidy patch

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Hm, bit odd since the patches were just re-attached this morning:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 32575: Tidy patch
error: sha1 information is lacking or useless (misc/cronjobs/gather_print_notices.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 32575: Tidy patch
hint: Use 'git am --show-current-patch=diff' to see the failed 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-32575-Tidy-patch-ghgaof9i.patch

Ah, wrong sequence.

Looks like the Tidy patch was squashed with the other patch in this case. Obsoleted the tidy patch, QA tools pass, patch applies that way.

(In reply to Katrin Fischer from comment #16)
> Looks like the Tidy patch was squashed with the other patch in this case.
> Obsoleted the tidy patch, QA tools pass, patch applies that way.

I noticed the status is "Patch doesn't apply". Is that still the case or should it be changed?

No, it's actually pushed, but the script didn't pick it up for the wrong status.

Pushed to main for 24.11, thanks all!