Support both Text & HTML in the same email #14715

hishamco · 2023-11-17T15:02:39Z

hishamco · 2023-11-17T15:07:33Z

@Mike4tel could you please try to send via EmailTask all the 3 cases to ensure it was similar to the old behavior. Also, I might add unit tests too

MikeKry · 2023-11-25T15:32:42Z

@hishamco
Thanks for looking at this. Code looks good and I have tested all 3 scenarios.

Html - OK
Text - OK
"All" (may be renamed to multipart/alternative or something) - Works OK (as seen below). Just one more thing. There is need to have two Body fields - text and html (similar as it was before). Because you need to have different formatting in text and html.

hishamco · 2023-11-25T15:45:28Z

"All" (may be renamed to multipart/alternative or something)

All or Both make sense to me

There is need to have two Body fields - text and html (similar as it was before). Because you need to have different formatting in text and HTML.

This will revert the changes and let the body TextBody & HtmlBody

hishamco · 2023-11-25T16:50:44Z

@sebastienros are we fine to add two bodies into the MailMessage or shall we let the sender decide if the body is HTML or Text

One option if we use MailMessageFormat.All is to supply the HTML body and then stripe all the HTML tags to make it fine for the plain-text body. This way we can use a single body instead of two

Mike4tel · 2023-11-25T21:10:07Z

Just pointing out that only stripping html will not let users format text nicely for plain text.. it is needed for good marketing.

But I can see using stripped html version as another feature just to fight spam filters.

MikeKry · 2024-01-20T10:06:45Z

Is it possible to merge this one? :)

hishamco · 2024-01-20T12:48:50Z

We forgot this one :) I will do a final review and then merge

MikeAlhayek

@hishamco what is the problem that you are trying to solve? Why would we need All as an enum value?

Maybe we should have an interface for the message like INotificationMessage which we already have. Or something similar

public interface IEmailMessage 
{
    string Message { get; }
    bool IsHtml { get; }
}

hishamco · 2024-01-21T09:57:19Z

Maybe we should have an interface for the message like INotificationMessage which we already have. Or something similar

I don't think so, the issue was the email can be sent in both formats. In the past we already had IsHtml but it didn't fit anymore, that's why I introduced something similar to the MimiKit mail body

github-actions · 2024-01-29T13:28:52Z

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions · 2024-02-27T19:50:20Z

This pull request has merge conflicts. Please resolve those before requesting a review.

# Conflicts: # src/OrchardCore.Modules/OrchardCore.Email/Controllers/AdminController.cs # src/OrchardCore.Modules/OrchardCore.Users/Controllers/EmailAuthenticatorController.cs # src/OrchardCore/OrchardCore.Email.Core/Services/SmtpService.cs

hishamco · 2024-03-14T19:27:46Z

@MikeKry could you please do a last test, because we made a massive changes in the OC.Email module

Piedone · 2024-03-22T18:52:10Z

src/OrchardCore.Modules/OrchardCore.Email/Workflows/Activities/EmailTask.cs

@@ -76,15 +76,15 @@ public WorkflowExpression<string> Subject
            set => SetProperty(value);
        }

-        public WorkflowExpression<string> Body
+        public WorkflowExpression<string> TextBody


Since GetProperty() below uses this as the name by default, this will break existing Email Tasks, since they'll be empty. Add a migration that reads out the bodies of all Email Tasks, and depending on IsHtmlBody, updates one of the fields with the old body value.

The idea is to generate both bodies, so isHtmlBody is not make sense here

For the consumers of the old API, nothing will change. For the consumers of the new API, Obsolete will tell that they shouldn't use this.

Piedone · 2024-03-22T18:54:13Z

src/OrchardCore.Modules/OrchardCore.Email/Views/Items/EmailTask.Fields.Edit.cshtml

+            mode: { name: "liquid" },
+        });
+
+        $('select').onchange(function () {


I get this when opening a previously existing Email Task:

Piedone · 2024-03-22T18:56:17Z

src/OrchardCore.Modules/OrchardCore.Email/Workflows/Drivers/EmailTaskDisplayDriver.cs

+            activity.TextBody = new WorkflowExpression<string>(model.TextBody);
+            activity.HtmlBody = new WorkflowExpression<string>(model.HtmlBody);


With this logic, if you switch from Text to HTML (not All), for example, then the old Text value will still exist, and will be used, even though you won't see it and won't be able to edit it on the UI.

Piedone · 2024-03-22T19:00:42Z

src/OrchardCore/OrchardCore.Email.Abstractions/MailMessage.cs

-        /// <remarks>This property is work in conjunction with <see cref="IsHtmlBody"/> to determine the body type..</remarks>
-        public string Body { get; set; }
-
-        /// <summary>
-        /// Gets or sets the message content as plain text.
-        /// </summary>
-        [Obsolete("This property is deprecated, please use Body instead.")]
-        public string BodyText
-        {
-            get => Body;
-            set
-            {
-                Body = value;
-                IsHtmlBody = false;
-            }
-        }
-
-        /// <summary>
-        /// Gets or sets whether the message body is an HTML.
-        /// </summary>
-        [Obsolete("This property is deprecated, please use IsHtmlBody instead.")]
-        public bool IsBodyHtml
-        {
-            get => IsHtmlBody;
-            set => IsHtmlBody = value;
-        }
-
-        /// <summary>
-        /// Gets or sets whether the message body is plain text.
-        /// </summary>
-        [Obsolete("This property is deprecated, please use IsHtmlBody instead.")]
-        public bool IsBodyText
-        {
-            get => !IsHtmlBody;
-            set => IsHtmlBody = !value;
-        }
-
-        /// <summary>
-        /// Gets or sets whether the message body is an HTML or not. Default is <c>false</c> which is plain text.
-        /// </summary>
-        public bool IsHtmlBody { get; set; }


Since this is a published interface, keep the old properties with Obsolete, and make them use the new Body behind the scenes.

Piedone · 2024-03-22T19:01:37Z

src/docs/releases/1.8.0.md

+### Email
+
+The `MailMessage.Body` now returns `MailMessageBody` instead of `string` to support both plain text & HTML formats.


This isn't needed in the 1.8 release notes.

It was planned for 1.8 :)

src/docs/releases/1.9.0.md

Piedone · 2024-03-22T19:09:20Z

src/OrchardCore/OrchardCore.Email.Abstractions/MailMessageBody.cs

+    /// <summary>
+    /// Gets or sets the body in HTML format.
+    /// </summary>
+    public string Html { get; set; }


Maybe IHtmlContent instead to be better typed? With a convenience conversion method to/from string.

For simplicity, I prefer string instead, like other email messaging APIs

Yeah but string is really overused. IHtmlContent is for when you want to store a string that's actually HTML.

Piedone · 2024-03-22T19:10:30Z

test/OrchardCore.Tests/Email/EmailTests.cs

@@ -14,7 +14,7 @@ public async Task SendEmail_WithToHeader()
            {
                To = "[email protected]",
                Subject = "Test",
-                Body = "Test Message"
+                Body = new MailMessageBody { Text = "Test Message" }


How about an implicit cast operator in MailMessageBody from string (fills Text) and IHtmlContent (fills Html)? Then here you could write Body = "Test Message".

Piedone · 2024-03-22T19:11:18Z

src/OrchardCore/OrchardCore.Email.Abstractions/MailMessageBody.cs

+    /// <summary>
+    /// Gets or sets the body in plain text format.
+    /// </summary>
+    public string Text { get; set; }


Suggested change

public string Text { get; set; }

public string PlainText { get; set; }

I used Text something similar to BodyBuilder.Text

I just think that in contrast to "HTML" it's clearer to have "plain text". It's also a term commonly used for this when talking about e-mail.

No problem we can rename it, but I usually "PlainText" in security :)

Piedone · 2024-03-22T19:11:37Z

src/OrchardCore.Modules/OrchardCore.Email/Workflows/Activities/EmailTask.cs

+                    Text = textBody?.Trim(),
+                    Html = htmlBody?.Trim()


Have you checked that actually both are sent if both are provided?

Co-authored-by: Zoltán Lehóczky <[email protected]>

github-actions · 2024-04-01T19:19:50Z

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions · 2024-06-05T01:59:57Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

hishamco · 2024-06-05T02:07:00Z

I will revise this soon

# Conflicts: # src/OrchardCore.Modules/OrchardCore.Users/Controllers/EmailAuthenticatorController.cs # src/OrchardCore.Modules/OrchardCore.Users/Extensions/ControllerExtensions.cs # src/OrchardCore.Modules/OrchardCore.Users/Workflows/Activities/RegisterUserTask.cs # src/OrchardCore/OrchardCore.Email.Abstractions/MailMessage.cs # src/OrchardCore/OrchardCore.Notifications.Core/Services/EmailNotificationProvider.cs # src/docs/releases/1.9.0.md # test/OrchardCore.Tests/Email/EmailTests.cs

hishamco · 2024-06-19T19:09:48Z

@MikeKry could you please do one more test if you have time

github-actions · 2024-07-19T13:56:24Z

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions · 2024-08-14T13:22:10Z

This pull request has merge conflicts. Please resolve those before requesting a review.

MikeAlhayek · 2024-08-15T17:44:36Z

for instructions on how to resolve the merge conflicts due to #16572 please follow the step listed in this comment.

github-actions · 2024-10-14T20:40:40Z

It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up.

hishamco · 2024-10-15T12:11:12Z

Seems I need to revisit this soon

Support both Text & HTML in the same email

d492809

hishamco mentioned this pull request Nov 17, 2023

Simplify email body #13426

Merged

Remove extra whitespace

fefd6ff

hishamco added 4 commits November 26, 2023 19:20

Introduce MainMessageBody

57403f1

Cleanup

9d56318

Revert some changes

9f52deb

Content -> Body

5843a39

hishamco requested a review from agriffard as a code owner November 27, 2023 12:13

hishamco added 4 commits January 20, 2024 23:51

Merge branch 'main' into hishamco/email-message-format

a587476

Merge branch 'main' into hishamco/email-message-format

dd37fd1

Remove obsolete memebers

a0bc7a6

Add release notes docs

3c079dd

hishamco requested review from MikeAlhayek and removed request for agriffard January 20, 2024 21:29

MikeAlhayek reviewed Jan 21, 2024

View reviewed changes

github-actions bot added the merge conflict label Jan 29, 2024

Merge branch 'main' into hishamco/email-message-format

4f8b04f

github-actions bot removed the merge conflict label Feb 4, 2024

github-actions bot added the merge conflict label Feb 27, 2024

github-actions bot removed the merge conflict label Mar 14, 2024

hishamco requested a review from sebastienros March 14, 2024 19:36

Piedone requested changes Mar 22, 2024

View reviewed changes

Piedone mentioned this pull request Mar 22, 2024

Add ISmtpService extension methods #14772

Merged

hishamco and others added 2 commits March 23, 2024 00:20

Update src/docs/releases/1.9.0.md

0ea8cd1

Co-authored-by: Zoltán Lehóczky <[email protected]>

Merge branch 'main' into hishamco/email-message-format

c3a546d

github-actions bot added the merge conflict label Apr 1, 2024

github-actions bot added the stale label Jun 5, 2024

hishamco removed the stale label Jun 5, 2024

github-actions bot removed the merge conflict label Jun 17, 2024

hishamco added 3 commits June 17, 2024 18:00

Add implicit operator to MailMessageBody

c86399f

Text -> PlainText

d1f6ce7

Merge branch 'main' into hishamco/email-message-format

279ae07

github-actions bot added the merge conflict label Jul 19, 2024

Merge branch 'main' into hishamco/email-message-format

55aa5d6

github-actions bot removed the merge conflict label Jul 19, 2024

github-actions bot added the merge conflict label Aug 14, 2024

github-actions bot added the stale label Oct 14, 2024

github-actions bot removed the stale label Oct 15, 2024

		activity.TextBody = new WorkflowExpression<string>(model.TextBody);
		activity.HtmlBody = new WorkflowExpression<string>(model.HtmlBody);

		### Email

		The `MailMessage.Body` now returns `MailMessageBody` instead of `string` to support both plain text & HTML formats.

	public string Text { get; set; }
	public string PlainText { get; set; }

Support both Text & HTML in the same email #14715

Are you sure you want to change the base?

Support both Text & HTML in the same email #14715

Conversation

hishamco commented Nov 17, 2023 • edited Loading

hishamco commented Nov 17, 2023

MikeKry commented Nov 25, 2023

hishamco commented Nov 25, 2023 • edited Loading

hishamco commented Nov 25, 2023

Mike4tel commented Nov 25, 2023 • edited Loading

MikeKry commented Jan 20, 2024

hishamco commented Jan 20, 2024

MikeAlhayek left a comment

Choose a reason for hiding this comment

hishamco commented Jan 21, 2024 • edited Loading

github-actions bot commented Jan 29, 2024

github-actions bot commented Feb 27, 2024

hishamco commented Mar 14, 2024 • edited Loading

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

hishamco Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 1, 2024

github-actions bot commented Jun 5, 2024

hishamco commented Jun 5, 2024

hishamco commented Jun 19, 2024

github-actions bot commented Jul 19, 2024

github-actions bot commented Aug 14, 2024

MikeAlhayek commented Aug 15, 2024

github-actions bot commented Oct 14, 2024

hishamco commented Oct 15, 2024

hishamco commented Nov 17, 2023 •

edited

Loading

hishamco commented Nov 25, 2023 •

edited

Loading

Mike4tel commented Nov 25, 2023 •

edited

Loading

hishamco commented Jan 21, 2024 •

edited

Loading

hishamco commented Mar 14, 2024 •

edited

Loading

hishamco Jun 17, 2024 •

edited

Loading