OrchardCore icon indicating copy to clipboard operation
OrchardCore copied to clipboard

Support both Text & HTML in the same email

Open hishamco opened this issue 1 year ago • 20 comments

/cc @MikeKry

hishamco avatar Nov 17 '23 15:11 hishamco

@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

hishamco avatar Nov 17 '23 15:11 hishamco

@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.

image

MikeKry avatar Nov 25 '23 15:11 MikeKry

"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 avatar Nov 25 '23 15:11 hishamco

@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

hishamco avatar Nov 25 '23 16:11 hishamco

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.

Mike4tel avatar Nov 25 '23 21:11 Mike4tel

Is it possible to merge this one? :)

MikeKry avatar Jan 20 '24 10:01 MikeKry

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

hishamco avatar Jan 20 '24 12:01 hishamco

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

hishamco avatar Jan 21 '24 09:01 hishamco

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

github-actions[bot] avatar Jan 29 '24 13:01 github-actions[bot]

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

github-actions[bot] avatar Feb 27 '24 19:02 github-actions[bot]

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

hishamco avatar Mar 14 '24 19:03 hishamco

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

github-actions[bot] avatar Apr 01 '24 19:04 github-actions[bot]

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.

github-actions[bot] avatar Jun 05 '24 01:06 github-actions[bot]

I will revise this soon

hishamco avatar Jun 05 '24 02:06 hishamco

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

hishamco avatar Jun 19 '24 19:06 hishamco

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

github-actions[bot] avatar Jul 19 '24 13:07 github-actions[bot]

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

github-actions[bot] avatar Aug 14 '24 13:08 github-actions[bot]

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

MikeAlhayek avatar Aug 15 '24 17:08 MikeAlhayek