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

Fixes #19147: Email notification should be disabled when no host server is configured #360

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Apr 14, 2021

emailBody <- getContentFromTemplate(mf, emailConf, params)
emailSubject <- getSubjectFromTemplate(mf, emailConf.subject, params)
_ <- emailService.sendEmail(serverConfig, emailConf.toEnvelop(emailBody).copy(subject = emailSubject))
_ <- ZIO.when(!serverConfig.smtpHostServer.isBlank) {
Copy link
Member

Choose a reason for hiding this comment

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

isEmpty not isBlank (we still support some old Java8 systems that does not support isBlank

@ElaadF ElaadF force-pushed the bug_19147/email_notification_should_be_disabled_when_no_host_server_is_configured branch from 5d75b3e to 4a648cb Compare April 14, 2021 12:18
@ElaadF
Copy link
Member Author

ElaadF commented Apr 14, 2021

Commit modified

emailSubject <- getSubjectFromTemplate(mf, emailConf.subject, params)
_ <- emailService.sendEmail(serverConfig, emailConf.toEnvelop(emailBody).copy(subject = emailSubject))
_ <- ZIO.when(serverConfig.smtpHostServer.nonEmpty) {
for {
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be an info log line in each case - oh, no the config is read on each notification.
OK, we need to add the info in the plugin page in a next version. (ie in the plugin page, something like "Email notification are [disabled (configure email server in /... to enable) | enable (comment config in .... to disable) ]

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean we want to log whether active or not when we try to send an email ?

Copy link
Member

Choose a reason for hiding this comment

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

yes, but I forgot that we reread conf each time, so it would hard to spot when it changes from enable to disable (or opposite).
So in place of having a log when the config is read, we need to display the info somewhere else.

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

You need to update documentation of the plugin to reflect that change in behavior

@ElaadF ElaadF force-pushed the bug_19147/email_notification_should_be_disabled_when_no_host_server_is_configured branch from 4a648cb to 5c6f211 Compare April 14, 2021 13:34
@ElaadF
Copy link
Member Author

ElaadF commented Apr 14, 2021

Commit modified

@ElaadF ElaadF force-pushed the bug_19147/email_notification_should_be_disabled_when_no_host_server_is_configured branch from 5c6f211 to 9ab2a2a Compare April 15, 2021 10:38
@ElaadF
Copy link
Member Author

ElaadF commented Apr 15, 2021

Commit modified

@ElaadF ElaadF closed this Apr 26, 2021
@ElaadF ElaadF force-pushed the bug_19147/email_notification_should_be_disabled_when_no_host_server_is_configured branch from 9ab2a2a to 8b1bc88 Compare April 26, 2021 07:55
@ElaadF
Copy link
Member Author

ElaadF commented Apr 26, 2021

Moved to #364

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