-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Allow sending email without "To" if "Cc" or "Bcc" is provided #10853
Conversation
I may need to check work flow task too, to react to this change |
@Skrypt did I miss anything? |
Yes, here. Remove required. OrchardCore/src/OrchardCore.Modules/OrchardCore.Email/Workflows/ViewModels/EmailTaskViewModel.cs Lines 17 to 18 in e73a81e
|
var mimeMessage = FromMailMessage(message); | ||
|
||
if (mimeMessage.From.Count == 0 && mimeMessage.Cc.Count == 0 && mimeMessage.Bcc.Count == 0) | ||
{ | ||
return SmtpResult.Failed(S["The mail message should have at least one of these headers: To, Cc or Bcc."]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking myself if the Mailkit library now won't return proper error messages when we update it to the latest version like you saw here #10863
I would tend to let the message fail without throwing a specific error message here. Let the library do its work if that's possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no throwing here, I suggest to log an explicit message to make it clear. I'm not sure if the server reponse returns a specific or general message
Let's accept this as its, then we can add the server response to SmtpResult
as I suggested in related link
Addresses #10840