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

fix: send email with local name via custom client #607

Merged
merged 9 commits into from
Sep 13, 2023

Conversation

afdesk
Copy link
Collaborator

@afdesk afdesk commented Aug 28, 2023

There are cases when SMTP server needs fully-qualified client hostname.
so Postee should have a config for it.

it fixes #602

actions/email.go Outdated
@@ -26,6 +27,7 @@ type EmailAction struct {
Port int
Sender string
Recipients []string
LocalName string

Choose a reason for hiding this comment

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

is LocalName the right name for this variable? Not DomainName maybe (as derived from fqdn)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that LocalName isn't a right name. It should be changed.
also DomainName can be misleading for email settings, isn't it?

maybe something like client-host-name ?

@@ -89,6 +95,58 @@ func (email *EmailAction) Send(content map[string]string) error {
return nil
}

func (email EmailAction) sendEmailWithCustomClient(addr string, a smtp.Auth, from string, to []string, msg []byte) error {

Choose a reason for hiding this comment

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

maybe add some verbosity to this function? var names, logs, errors and docstrings(not necessary), to look feel and stay readable like the previous sendEmail implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, sure. I've added a comment. will think over it more.

@roeelivaqua
Copy link

Thanks @afdesk ! Left some comments. Please note we may need a cherry-pick into postee-for-enterprise branch, hopefully this isn't a breaking change :)

@afdesk
Copy link
Collaborator Author

afdesk commented Aug 30, 2023

Thanks @afdesk ! Left some comments. Please note we may need a cherry-pick into postee-for-enterprise branch, hopefully this isn't a breaking change :)

oh, thanks for the review. I missed it.
yes, there are no breaking changes here.

@roeelivaqua
Copy link

hey @afdesk ! I was away for a few days - how are we doing on this PR? Ready for review?

@afdesk afdesk marked this pull request as ready for review September 7, 2023 08:55
@roeelivaqua
Copy link

LGTM @afdesk ! let's go ahead and merge, and we will open a cherry-pick into postee-for-enterprise branch.

@simar7 simar7 merged commit ba3603d into main Sep 13, 2023
3 checks passed
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.

Email notifications: SMTP HELO check rejects "localhost"
3 participants