Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(mail-connector): Adding Dropdown for Outbound Connector to switch content type between plain text and html #3569

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

itsnuyen
Copy link

@itsnuyen itsnuyen commented Oct 30, 2024

Description

image

Adding a Dropdown to set the content type of an email so that html templates can be used in this email outbound connector.

Also set the right content type in the smtp send mail

Checklist

  • PR has a milestone or the no milestone label.

…n mime type of plain text or html to send html templates
@itsnuyen itsnuyen requested a review from a team as a code owner October 30, 2024 16:49
@itsnuyen itsnuyen changed the title Adding Dropdown for Outbound Connector to switch content type between plain text and html feat: Adding Dropdown for Outbound Connector to switch content type between plain text and html Oct 30, 2024
@itsnuyen itsnuyen self-assigned this Oct 30, 2024
@itsnuyen itsnuyen changed the title feat: Adding Dropdown for Outbound Connector to switch content type between plain text and html feat(mail-connector): Adding Dropdown for Outbound Connector to switch content type between plain text and html Oct 31, 2024
@itsnuyen itsnuyen marked this pull request as draft October 31, 2024 11:28
@itsnuyen itsnuyen marked this pull request as ready for review October 31, 2024 14:46
@sbuettner
Copy link
Contributor

@itsnuyen Thanks for your contribution. We will get back to you!

@sbuettner
Copy link
Contributor

@itsnuyen Thanks again for the PR. We discussed this internally with product. We would prefer to offer a Dropdown that allows the following options:

  • Plaintext
  • HTML
  • Plaintext + HTML

Would that work for you and if so could you adapt your PR? Thanks!

@itsnuyen
Copy link
Author

itsnuyen commented Nov 5, 2024

Hi @sbuettner,

I can adapt the PR with the suggested recommendations.
How should the HTML + Plaintext work?
Does the user enter a HTML template as well as a plain text and both are send out?

@sbuettner
Copy link
Contributor

@itsnuyen Yes, exactly. Its common practice to send both in case the users email client is not able to show HTML:
https://stackoverflow.com/questions/3902455/mail-multipart-alternative-vs-multipart-mixed

@ingorichtsmeier
Copy link

Hi @sbuettner, as Minh is on vacation for a month and the customer request the HTML emails, it would be good to separate the complicated html/text mix from the current pull request to a new issue and resolve it later.

Would it be OK to merge this change now and add a new issue to follow up on the combination of HTML and plain text?

Or find a way, to extract the plain text from the html and add it to the mail body automatically?

@itsnuyen
Copy link
Author

itsnuyen commented Nov 7, 2024

Hi
I added a part, where the user can choose a Multi line Mixed as the third option.
I think adding the possibility to use FEEL in the HTML templating would be a separate feature.


@Test
void executeSmtpSendEmailAsMultiPart() throws MessagingException {
buildSmtpTest(ContentType.MULTIPART, "<html><body>body</body></html>", "multipart/mixed");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to add the second part (in plain text) to the email body?

@sbuettner
Copy link
Contributor

Thanks for your contriburation and considering the feedback @itsnuyen

@ingorichtsmeier @mathias-vandaele from the Connectors team will do a review, fix and merge any upcoming changes.

@sbuettner sbuettner added this to the 8.7.0-alpha2 milestone Nov 7, 2024
Copy link
Contributor

@mathias-vandaele mathias-vandaele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few comments, but overall it's a good PR 👍

HTML,
MULTIPART;

public static class Constants {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having Constants, we could manage this directly within the enum, like this

PLAIN("plain"),
HTML("html"),
MULTIPART("multipart");

@sbuettner
Copy link
Contributor

@mathias-vandaele As @itsnuyen is on vacation would you take care of doing the requested changes yourself?

"tooltip" : "Email's contentType",
"type" : "Dropdown",
"choices" : [ {
"name" : "Plain",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use Plaintext

"name" : "Plain",
"value" : "PLAIN"
}, {
"name" : "Html",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"HTML"

"name" : "Html",
"value" : "HTML"
}, {
"name" : "Html+Plain",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"HTML & Plaintext"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants