Add FromName and ReplyTo properties to SmtpSettingsPart #8420
Conversation
| @@ -12,12 +11,12 @@ | |||
| namespace Orchard.Email.Controllers { | |||
| [Admin] | |||
| public class EmailAdminController : Controller { | |||
| private readonly ISmtpChannel _smtpChannel; | |||
| private readonly IOrchardServices _orchardServices; | |||
| private readonly ISmtpChannel smtpChannel; | |||
There was a problem hiding this comment.
Can you revert all the names you changed?
There was a problem hiding this comment.
@sebastienros Okay. Since it is naming convention that Orchard team use, I will follow the standard.
| @@ -65,6 +72,11 @@ | |||
| </span> | |||
| </div> | |||
| </div> | |||
| <div> | |||
| <label for="@Html.FieldIdFor(m => m.ListUnsubscribe)">@T("List-Unsubscribe header")</label> | |||
There was a problem hiding this comment.
Is that something that mail clients understand natively, or just a field that can be used in a template?
There was a problem hiding this comment.
List-Unsubscribe is a STMP header that a mail client can understand natively.
https://www.postmastery.com/list-unsubscribe-header-critical-for-sustained-email-delivery/
It can help an email not go to junk.
Additional information:
https://blog.mailtrap.io/list-unsubscribe-header/#What_is_the_list-unsubscribe_header
I am not familiar with Orchard.WorkFlow area. There are two main places that use the setting.
|
|
@sebastienros Fixed all from your feedback, thank you so much. |
| @@ -53,7 +53,7 @@ public class EmailMessagingChannel : IMessagingChannel { | |||
| smtpClient.EnableSsl = smtpSettings.EnableSsl; | |||
| smtpClient.DeliveryMethod = SmtpDeliveryMethod.Network; | |||
|
|
|||
| context.MailMessage.From = new MailAddress(smtpSettings.Address); | |||
| context.MailMessage.From = new MailAddress(smtpSettings.FromAddress); | |||
There was a problem hiding this comment.
This is an important breaking change, since all existing sites will lose this setting. Maybe a migration step would be good to move the old value to the new properties.
There was a problem hiding this comment.
@sebastienros That's a good point, sorry for my mistake. I will continue the work on a migration.
This should be check list item for a contribution note.
There was a problem hiding this comment.
Update a migration with a test cases result.
However, there is an interesting issue that should fix by other PR.
The existing attribute do not remove from XML after SmtpSettingsPart get updated.
It is still there. I think a serialize XML of saving StmpSettingsPart should be responsible for this.
Therefore, I don't remove it manually.
Could you please check the data after migration in the test case 1.
Test case 1
SmtpSettingPart with existing value before migration
<Data>
<SiteSettingsPart SiteSalt="fdb3ec855e864eba91d2a3bdfa3c620a" SiteName="orchard-pr" PageTitleSeparator=" - " SiteTimeZone="SE Asia Standard Time" UseFileHash="true" SuperUser="admin" SiteCulture="en-US" PageSize="20" BaseUrl="http://localhost:61745" />
<CacheSettingsPart VaryByQueryStringIsExclusive="true" />
<CommentSettingsPart ModerateComments="true" />
<ThemeSiteSettingsPart CurrentThemeName="TheThemeMachine" />
<SmtpSettingsPart Address="[email protected]" Host="localhost" Port="25" EnableSsl="false" RequireCredentials="false" UseDefaultCredentials="false" UserName="" Password="" />
</Data>
SmtpSettingPart after migration
<Data>
<SiteSettingsPart SiteSalt="fdb3ec855e864eba91d2a3bdfa3c620a" SiteName="orchard-pr" PageTitleSeparator=" - " SiteTimeZone="SE Asia Standard Time" UseFileHash="true" SuperUser="admin" SiteCulture="en-US" PageSize="20" BaseUrl="http://localhost:61745" />
<CacheSettingsPart VaryByQueryStringIsExclusive="true" />
<CommentSettingsPart ModerateComments="true" />
<ThemeSiteSettingsPart CurrentThemeName="TheThemeMachine" />
<SmtpSettingsPart Address="[email protected]" Host="localhost" Port="25" EnableSsl="false" RequireCredentials="false" UseDefaultCredentials="false" UserName="" Password="" FromAddress="[email protected]" />
</Data>
Test case 2
SmtpSettingPart without existing value before migration
<Data>
<SiteSettingsPart SiteSalt="fdb3ec855e864eba91d2a3bdfa3c620a" SiteName="orchard-pr" PageTitleSeparator=" - " SiteTimeZone="SE Asia Standard Time" UseFileHash="true" SuperUser="admin" SiteCulture="en-US" PageSize="20" BaseUrl="http://localhost:61745" />
<CacheSettingsPart VaryByQueryStringIsExclusive="true" />
<CommentSettingsPart ModerateComments="true" />
<ThemeSiteSettingsPart CurrentThemeName="TheThemeMachine" />
</Data>
SmtpSettingPart after migration
<Data>
<SiteSettingsPart SiteSalt="fdb3ec855e864eba91d2a3bdfa3c620a" SiteName="orchard-pr" PageTitleSeparator=" - " SiteTimeZone="SE Asia Standard Time" UseFileHash="true" SuperUser="admin" SiteCulture="en-US" PageSize="20" BaseUrl="http://localhost:61745" />
<CacheSettingsPart VaryByQueryStringIsExclusive="true" />
<CommentSettingsPart ModerateComments="true" />
<ThemeSiteSettingsPart CurrentThemeName="TheThemeMachine" />
</Data>
Here is admin panel after a database migration.
Sent test email result
…tpSettingsPart.FromAddress
|
@sebastienros Fixed all your PR feedbacks. |
|
@HermesSbicego-Laser @ElenaRepository |
aaronamm
deleted the
add-from-name-reply-to-properties-to-smtp-settings-part
branch
Refer to :
I think adding from name, reply to and List-Unsubscribe can help passing an email spam filter.
Therefore, I have updated SmtpSettingsPart to include FromName, ReplyTo and ListUnsubscribe properties.
In addition, I also updated SmtpMessageChannel to make sure everything work correctly.
BTW, I have changed naming convention of fields to not use _ underscore and use a this keyword.
I know it is more convenient to use underscore but I think it is not .NET naming convention.
Hopefully, this PR is valid and will get approved. I am happy to update this PR from your feedback.
Thanks.