Implement Reply-To header support when replying

[vermeeren]

The Reply-To header, if present, will take precedence over the From header. If the Reply-To data is undefined old behaviour is used.

Note that Horde libraries appear to internally determine reply_to property in the Horde_Imap_Client_Data_Envelope (unless Dovecot does it, but doubt). It was observed that reply_to property had the same value as from property in a mail that contained no Reply-To header. It is assumed that the Horde implementation makes sense, so just use the value as-is.

This notably fixes replying to mail from software reliant on the Reply-To header such as GitLab and GitHub notifications and many types of mail based ticketing (support) systems.

Closes #4075
Closes #7538

[welcome]

Thanks for opening your first pull request in this repository! ✌️

[vermeeren]

This has been tested based upon a production instance dataset with both Reply to all and Reply to sender only, behaviour is fully correct in my opinion and labels are also correctly used in the UI.

Tested on Nextcloud 25.0.9, the commit being directly on top of Mail 2.2.7. This would need to be forward-ported to newer stables and main if accepted.

[vermeeren]

Rebased on top of stable2.2 in case it helps with CI.

[ChristophWurst]

Thanks a lot

Please rebase to main. We will backport the fix to stable branches.

https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#automatic-backport

[ChristophWurst]

Code looks good so far

[vermeeren]

@ChristophWurst Thanks for the info, rebased on top of main and conflicts fixed. Did a self-review and looks good.

I don't have a dev setup for latest Nextcloud set up so cannot test if there are behaviour changes that influence this, do you have a way to check?

Here is patch for stable2.2 version in case it's useful (because of conflicts): 0001-Implement-Reply-To-header-support-when-replying.patch.txt (GitHub didn't let me upload .patch for some reason)

Cheers

[hamza221]

Tested ✅

[vermeeren]

@ChristophWurst @GretaD Is there anything that still needs checking or that I need to change? Just checking in case it's stuck somewhere, no particular rush to get this merged. Though I would appreciate it if it could be in the next release version as I'm currently patching in production as a workaround.

[vermeeren]

@ChristophWurst @GretaD I took some time to check the failing test, which is:

1) OCA\Mail\Tests\Unit\Model\IMAPMessageTest::testSerialize
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'dispositionNotificationTo' => 'disposition'
     'hasDkimSignature' => false
     'scheduling' => Array ()
+    'replyTo' => Array (...)
 )

/home/runner/actions-runner/_work/mail/mail/nextcloud/apps/mail/tests/Unit/Model/IMAPMessageTest.php:144

I am not sure if this is easily fixable in the template. Possibly it is just an empty AddressList instead of a NULL, but it could also be Horde's behaviour which adds the replyTo even if this field is actually missing from the original email. In such case, it sets it to be from header. From the commit message:

Note that Horde libraries appear to internally determine reply_to
property in the Horde_Imap_Client_Data_Envelope (unless Dovecot does it,
but doubt). It was observed that reply_to property had the same value as
from property in a mail that contained no Reply-To header. It is assumed
that the Horde implementation makes sense, so just use the value as-is.

This means that when passing data around, Horde will automagically populate the replyTo field. This is not really documented but I believe in the end it comes from somewhere related to this. https://dev.horde.org/api/master/lib/Imap_Client/classes/Horde_Imap_Client_Data_Envelope.html

---

I see some options:

1. Accept Horde's heuristics to replyTo handling and simply use the field when it is given by Horde.
   • Current patch does this. Could we exclude replyTo from this test?
2. Some deep dive into Horde to determine how and why it sets replyTo and possibilities to configure this, so it only does set it when the header is actually in the mail.
3. Inspect raw headers manually in Nextcloud mail app instead of using Horde implementation.
   • Seems to add a lot of maintenance burden and work, I don't suggest we do this.

Your thoughts on this? I'm not too familiar with the test framework so feel free to push any changes directly. Could also fix this specific test for now and leave the above for another day?

Thanks!

[vermeeren]

@ChristophWurst Revisiting this after a while, I'm thinking it might be best to update the test to exclude replyTo from assertion for now and perhaps add a comment referencing this PR?

The actual implementation now has been tested and does work correctly, I'd like to ship this to more users asap.

Your thoughts? Thanks!

[vermeeren]

Any help would be really appreciated, I'd love to see this merged so that other users can use the fix as well. Again it's purely the way works tests work I'm not sure how to make it ignore the replyTo as it re-uses internal Horde logic to retrieve the address-to-reply-to iirc. (Whether the header is actually present is not strictly related.)

[ChristophWurst]

diff --git a/tests/Unit/Model/IMAPMessageTest.php b/tests/Unit/Model/IMAPMessageTest.php
index 29f9c7ecd..1390a3831 100644
--- a/tests/Unit/Model/IMAPMessageTest.php
+++ b/tests/Unit/Model/IMAPMessageTest.php
@@ -147,6 +147,7 @@ class IMAPMessageTest extends TestCase {
                        'to' => [ [ 'label' => '[email protected]', 'email' => '[email protected]' ] ],
                        'cc' => [ [ 'label' => '[email protected]', 'email' => '[email protected]' ] ],
                        'bcc' => [ [ 'label' => '[email protected]', 'email' => '[email protected]' ] ],
+                       'replyTo' => [ [ 'label' => '[email protected]', 'email' => '[email protected]' ] ],
                        'subject' => 'subject',
                        'dateInt' => 1451606400,
                        'flags' => [

that will satisfy the tests

[vermeeren]

@ChristophWurst Many thanks for the test fix, seems so obvious in hindsight I must have overlooked this somehow.

Cheers!

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!