fix: add ability to send alternet text (html and plain) #10477

SebastianKrupinski · 2024-12-11T02:27:38Z

Resolves #10415

Summary

This modifies the local_message table and mail transmission service to support alternative text for automated messages.

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

ChristophWurst · 2024-12-16T09:17:26Z

lib/Migration/Version4200Date20241210000000.php

+
+		$outboxTable = $schema->getTable('mail_local_messages');
+		if ($outboxTable->hasColumn('body')) {
+			$outboxTable->dropColumn('body');


What will be the migration path for production instances?

I did that before I realized that we use the same table for drafts, so at the time didn't think a migration was necessary.

Now, going to rename instead of dropping, then execute a sql command postSchemaChange to move the html versions to the body_html column based on the Html flag. Unless there is anything wrong with that?

Creating a new column, moving data over and dropping the old column is the correct approach 👍

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

ChristophWurst · 2024-12-17T07:44:50Z

Modify UI to send both plain and html

I'm not a fan of this, as it introduces a lot of complexity in the frontend.

Our backend could handle plain text and HTML messages before. Leave that unchanged. Only add the option to pass text and html alternatives for the mail provider API.

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

SebastianKrupinski · 2024-12-17T17:19:37Z

Our backend could handle plain text and HTML messages before. Leave that unchanged. Only add the option to pass text and html alternatives for the mail provider API.

Had no choice in this, because we Jsonize the database entity, then send it straight to the UI, the UI needs to be able handle the added database fields.

ChristophWurst · 2024-12-17T17:24:35Z

Simplified algorithm:

The client sends this:
body: bla
isHtml: true

-> you store bla into the text_html column, set text_plain null and html=true

The client sends this:
body: bla
isHtml: false

-> you store bla in the text_plain column, set text_html null and html=false.

Piping through the frontend is also fine 👍

What I'm against is pushing any kind of conversion to the client. The backend mechanism to convert HTML messages to a text alternative has to stay as-is.

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

kesselb · 2024-12-18T11:21:42Z

If I understood Christoph's latest message correctly, the part about creating a text version from the HTML version should remain and is currently missing. As we discussed in chat, it might be worth considering whether Hamaz's fork of html2text should be used instead of Horde's text filter.

kesselb · 2024-12-18T10:41:07Z

lib/Service/MimeMessage.php

-			$textPart->setDescription('Plaintext Version of Message');
-
-			/*
-			 * RFC1341: In general, user agents that compose multipart/alternative entities should place the


Let’s keep the comment, it provides useful context.

kesselb · 2024-12-18T10:41:13Z

lib/Service/MimeMessage.php

-			$alternativePart[] = $htmlPart;
-
-			/*
-			 * Wrap the multipart/alternative parts in multipart/related when inline images are found.


Let’s keep the comment, it provides useful context.

kesselb · 2024-12-18T10:41:22Z

lib/Service/MimeMessage.php

 		}

-		/*
-		 * For attachments wrap the body (multipart/related, multipart/alternative or text/plain) in


Let’s keep the comment, it provides useful context.

kesselb · 2024-12-18T10:45:50Z

lib/Service/MimeMessage.php

 	}

 	/**
 	 * A callback for Horde_Text_Filter.
 	 *
-	 * The purpose of this callback is to overwrite the default behaviour


The method is unused?

kesselb · 2024-12-18T10:46:28Z

lib/Service/MimeMessage.php

+	 *
+	 * @return Horde_Mime_Part
+	 */
+	public function buildPgpPart(string $content): Horde_Mime_Part {


Suggested change

public function buildPgpPart(string $content): Horde_Mime_Part {

private function buildPgpPart(string $content): Horde_Mime_Part {

kesselb · 2024-12-18T10:46:40Z

lib/Service/MimeMessage.php

+	 *
+	 * @return Horde_Mime_Part
+	 */
+	public function buildMessagePart(?string $contentPlain, ?string $contentHtml): Horde_Mime_Part {


Suggested change

public function buildMessagePart(?string $contentPlain, ?string $contentHtml): Horde_Mime_Part {

private function buildMessagePart(?string $contentPlain, ?string $contentHtml): Horde_Mime_Part {

kesselb · 2024-12-18T10:50:44Z

lib/Service/MimeMessage.php

+			$plainPart->setContents($contentPlain);
+		}
+
+		if (isset($plainPart) && isset($htmlPart)) {


Suggested change

if (isset($plainPart) && isset($htmlPart)) {

if (isset($plainPart, $htmlPart)) {

Done. Didn't even realize it can be used this way.

kesselb · 2024-12-18T10:54:28Z

tests/Unit/Service/MimeMessageTest.php

@@ -225,6 +225,8 @@ public function testMultipartMixedRelated() {
 	}

 	public function testMultipartAlternativeGreek() {
+		$this->markTestSkipped();


What's wrong with the test? ;)

It was disabled because the forced plain text generation was removed.

kesselb · 2024-12-18T11:08:09Z

lib/Service/MimeMessage.php

+	 * @param string $contentPlain
+	 * @param string $contentHtml


Suggested change

* @param string $contentPlain

* @param string $contentHtml

Let’s remove them. They don’t add much value over the type hints and don’t align with them anyway.

kesselb · 2024-12-18T11:17:46Z

lib/Service/MimeMessage.php

-			$bodyPart->setType('text/plain');
-			$bodyPart->setCharset('UTF-8');
-			$bodyPart->setContents($content);
+			$messagePart = new Horde_Mime_Part();


I’m not a fan of the method accepting an empty contentPlain and contentHtml and then returning an empty MIME part. Conceptually, this feels off, but I don’t have a better alternative in mind yet.

I agree. But the alternative was to make a separate function for all scenarios, which made it way to complex.

ChristophWurst · 2024-12-18T13:49:52Z

If I understood Christoph's latest message correctly, the part about creating a text version from the HTML version should remain and is currently missing.

Correct. We can argue about the quality of the Horde text transformation, and change it one day, but definitely not in the scope of this PR. Let's fix one problem at a time.

SebastianKrupinski · 2024-12-18T14:54:46Z

If I understood Christoph's latest message correctly, the part about creating a text version from the HTML version should remain and is currently missing.

Correct. We can argue about the quality of the Horde text transformation, and change it one day, but definitely not in the scope of this PR. Let's fix one problem at a time.

So I put it back in, but there are some issues doing it this way, even if we omit how bad the the horde text transformer is. By forcing automatic plain text, this produces unexpected behavior for any automated function that sends a message.

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

ChristophWurst · 2024-12-18T16:22:10Z

So I put it back in, but there are some issues doing it this way, even if we omit how bad the the horde text transformer is. By forcing automatic plain text, this produces unexpected behavior for any automated function that sends a message.

Dear @SebastianKrupinski, I think there is a discrepancy in goals.

The mail providers feature has broken emails that were previously sent through the system mailer, where the dav app has provided both HTML and the text alternatives. The mail app only works with one of the two types, and if the user writes an HTML message, it automagically generates a text alternative via Horde.

The goal of your fix should be to allow mail provider senders to either send a plain text message, an html message, or an alternative message with plain and html. In all cases the contents will be provided by the caller.
The mail app handling will stay the same. If the user sends a plain text message, it will be sent as-is. If the user sends a HTML message, Horde will also generate a plain text alternative.

I hope this helps.

fix: add ability to send alternet text (html and plain)

816d78f

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

SebastianKrupinski added the 2. developing label Dec 11, 2024

SebastianKrupinski requested review from ChristophWurst, st3iny and kesselb December 11, 2024 02:27

SebastianKrupinski self-assigned this Dec 11, 2024

SebastianKrupinski requested a review from GretaD as a code owner December 11, 2024 02:27

SebastianKrupinski marked this pull request as draft December 11, 2024 02:27

SebastianKrupinski removed the request for review from GretaD December 11, 2024 02:28

ChristophWurst reviewed Dec 16, 2024

View reviewed changes

fixup! fix: add ability to send alternet text (html and plain)

7ec09c2

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

SebastianKrupinski requested a review from ChristophWurst December 16, 2024 22:52

SebastianKrupinski added 4 commits December 16, 2024 17:53

fixup! fix: add ability to send alternet text (html and plain)

7c05793

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! fix: add ability to send alternet text (html and plain)

3e729fa

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! fix: add ability to send alternet text (html and plain)

61df418

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! fix: add ability to send alternet text (html and plain)

f8ff009

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

SebastianKrupinski and others added 3 commits December 17, 2024 10:59

fixup! fix: add ability to send alternet text (html and plain)

6e546c7

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! fix: add ability to send alternet text (html and plain)

04cec39

fixup! fix: add ability to send alternet text (html and plain)

e07e745

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

Merge branch 'main' into fix/issue-10415-send-plain-and-html

3b7c417

SebastianKrupinski added 3 commits December 17, 2024 18:01

fixup! fixup! fix: add ability to send alternet text (html and plain)

2269033

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! fixup! fix: add ability to send alternet text (html and plain)

82a8f13

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! fixup! fix: add ability to send alternet text (html and plain)

370081b

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

SebastianKrupinski marked this pull request as ready for review December 17, 2024 23:32

SebastianKrupinski added 3. to review and removed 2. developing labels Dec 17, 2024

kesselb reviewed Dec 18, 2024

View reviewed changes

SebastianKrupinski added 2 commits December 18, 2024 10:14

fixup! fixup! fix: add ability to send alternet text (html and plain)

07a11aa

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! fixup! fix: add ability to send alternet text (html and plain)

e44cfd5

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add ability to send alternet text (html and plain) #10477

fix: add ability to send alternet text (html and plain) #10477

SebastianKrupinski commented Dec 11, 2024 •

edited

Loading

ChristophWurst Dec 16, 2024

SebastianKrupinski Dec 16, 2024

ChristophWurst Dec 17, 2024

ChristophWurst commented Dec 17, 2024

SebastianKrupinski commented Dec 17, 2024

ChristophWurst commented Dec 17, 2024 •

edited

Loading

kesselb commented Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

kesselb Dec 18, 2024

SebastianKrupinski Dec 18, 2024

ChristophWurst commented Dec 18, 2024

SebastianKrupinski commented Dec 18, 2024 •

edited

Loading

ChristophWurst commented Dec 18, 2024

	public function buildPgpPart(string $content): Horde_Mime_Part {
	private function buildPgpPart(string $content): Horde_Mime_Part {

	public function buildMessagePart(?string $contentPlain, ?string $contentHtml): Horde_Mime_Part {
	private function buildMessagePart(?string $contentPlain, ?string $contentHtml): Horde_Mime_Part {

	if (isset($plainPart) && isset($htmlPart)) {
	if (isset($plainPart, $htmlPart)) {

fix: add ability to send alternet text (html and plain) #10477

Are you sure you want to change the base?

fix: add ability to send alternet text (html and plain) #10477

Conversation

SebastianKrupinski commented Dec 11, 2024 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented Dec 17, 2024

SebastianKrupinski commented Dec 17, 2024

ChristophWurst commented Dec 17, 2024 • edited Loading

kesselb commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented Dec 18, 2024

SebastianKrupinski commented Dec 18, 2024 • edited Loading

ChristophWurst commented Dec 18, 2024

SebastianKrupinski commented Dec 11, 2024 •

edited

Loading

ChristophWurst commented Dec 17, 2024 •

edited

Loading

SebastianKrupinski commented Dec 18, 2024 •

edited

Loading