Bug 32575 - gather_print_notices.pl sends attachment as body of email or poorly named txt file
Summary: gather_print_notices.pl sends attachment as body of email or poorly named txt...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-01-06 10:40 UTC by Magnus Enger
Modified: 2024-11-18 17:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.06
Circulation function:


Attachments
Bug 32575: Tidy patch (912 bytes, patch)
2024-06-17 02:27 UTC, David Cook
Details | Diff | Splinter Review
Bug 32575: Add an empty text body to fix multipart/mixed handling (1.15 KB, patch)
2024-06-17 02:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 32575: Tidy patch (912 bytes, patch)
2024-06-17 02:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 32575: Add an empty text body to fix multipart/mixed handling (1.26 KB, patch)
2024-06-18 09:05 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 32575: Tidy patch (963 bytes, patch)
2024-06-18 09:05 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 32575: Add an empty text body to fix multipart/mixed handling (1.63 KB, patch)
2024-06-28 07:33 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 Magnus Enger 2023-01-06 10:40:50 UTC
gather_print_notices.pl can send print notices to the library, as an HTML file attached to an email. At least some libraries report that the HTML does not come as an attached file, but as the body of the email. This breaks printing the notices properly. 

I am not sure if this is a problem with the emails generated by gather_print_notices.pl or with the email clients (or other software) used by the libraries that have this problem. 

A quick solution has been to add some body text to the emails generated by gather_print_notices.pl: 

299     my $email = Koha::Email->create(
300         {
301             from    => $from,
302             to      => $to,
303             subject => 'Print notices for ' . $today_syspref,
304             text_body => 'See attached file.', # <==== Added this
305         }
306     );

This seems to force the attachment to be seen as an attachment, and not as the body of the email. 

In the headers of the emails sent by the standard gather_print_notices.pl I have seen this: 

    Content-Transfer-Encoding: base64
    Content-Type: text/html; charset=iso-8859-1
    
    PCFET0NU... [lots of base64]

When I have added some body text it looks more like this: 

    Content-Transfer-Encoding: 7bit
    Content-Type: multipart/mixed; boundary="16633311691.0Ab5.3555417"
    
    --16633311691.0Ab5.3555417
    Date: Fri, 16 Sep 2022 14:26:09 +0200
    MIME-Version: 1.0
    Content-Type: text/plain; charset=us-ascii
    Content-Transfer-Encoding: quoted-printable
    
    See attached file.
    --16633311691.0Ab5.3555417
    Date: Fri, 16 Sep 2022 14:26:09 +0200
    MIME-Version: 1.0
    Content-Type: text/html; name="notices_all-2022-09-16-KOHA.html"
    Content-Disposition: attachment; filename="notices_all-2022-09-16-KOHA.html"
    Content-Transfer-Encoding: base64
    
    PCFET0... [lots of base64]

So unless something is changed on the way from Koha to the mail client, it looks like gather_print_notices.pl sends the HTML from the document as the body of the email. 

Not sure if this is a problem in Koha::Email->create that should be fixed there, or if it makes sense to just add some default text to the email, to fix the problem in the least obtrusive way?
Comment 1 David Cook 2024-06-17 01:53:00 UTC
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.
Comment 2 David Cook 2024-06-17 01:56:14 UTC
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
Comment 3 David Cook 2024-06-17 02:20:19 UTC
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.
Comment 4 David Cook 2024-06-17 02:27:08 UTC
Created attachment 167779 [details] [review]
Bug 32575: Tidy patch
Comment 5 David Cook 2024-06-17 02:28:00 UTC Comment hidden (obsolete)
Comment 6 David Cook 2024-06-17 02:28:03 UTC Comment hidden (obsolete)
Comment 7 David Cook 2024-06-17 02:30:36 UTC
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...
Comment 8 David Cook 2024-06-17 04:48:08 UTC
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?
Comment 9 Katrin Fischer 2024-06-17 18:46:16 UTC
Could we maybe clarify the description of the bug a bit?
Comment 10 David Cook 2024-06-17 23:05:02 UTC
(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.
Comment 11 Magnus Enger 2024-06-18 09:05:04 UTC
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.
Comment 12 Magnus Enger 2024-06-18 09:05:06 UTC
Created attachment 167821 [details] [review]
Bug 32575: Tidy patch

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 13 Marcel de Rooy 2024-06-28 07:33:33 UTC
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>
Comment 14 Katrin Fischer 2024-06-28 11:24:30 UTC
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
Comment 15 Katrin Fischer 2024-06-28 11:24:56 UTC
Ah, wrong sequence.
Comment 16 Katrin Fischer 2024-06-28 11:26:54 UTC
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.
Comment 17 David Cook 2024-06-30 23:16:18 UTC
(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?
Comment 18 Katrin Fischer 2024-07-01 07:29:02 UTC
No, it's actually pushed, but the script didn't pick it up for the wrong status.
Comment 19 Katrin Fischer 2024-07-01 07:29:30 UTC
Pushed to main for 24.11, thanks all!
Comment 20 Lucas Gass (lukeg) 2024-11-18 17:11:11 UTC
Backported to 24.05.x for upcoming 24.05.06