-
Notifications
You must be signed in to change notification settings - Fork 24
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 #19352: Illegal address when no replyTo configured #377
Fixes #19352: Illegal address when no replyTo configured #377
Conversation
<artifactId>javax.mail</artifactId> | ||
<version>1.6.2</version> | ||
<artifactId>jakarta.mail</artifactId> | ||
<version>2.0.0</version> |
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.
more recent (like, in that decade) release of javax.mail
Transport.send(message) | ||
} | ||
for { | ||
session <- IOResult.effect("Error when creating SMTP session")( |
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.
make IOResult
more granular to know where and error comes from.
def getTrimmedString(path: String) = config.getString(path).trim | ||
|
||
//shortcut to get email from a comma-separated string | ||
//We don't want any empty value, it causes smtp client to throw Illegal address. |
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.
it was causing the illegal address error
@@ -143,7 +161,7 @@ class NotificationService( | |||
} yield () | |||
} | |||
|
|||
private[this] def getConfig(path: String): IOResult[Config] = { | |||
protected[changevalidation] def getConfig(path: String): IOResult[Config] = { |
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.
change visibility to allow tests
@@ -246,7 +264,7 @@ class NotificationService( | |||
, "description" -> cr.info.description | |||
, "author" -> cr.owner | |||
// rudderBaseUrl should not contains "/" at the end | |||
, "link" -> (rudderBaseUrl + RudderConfig.linkUtil.changeRequestLink(cr.id)) |
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.
@ElaadF : you can't use RudderConfig in a service, it's a runtime class. You only have the right to use it when the plugin is init in the context of rudder, so: use a class parameter for services.
@@ -246,7 +264,7 @@ class NotificationService( | |||
, "description" -> cr.info.description | |||
, "author" -> cr.owner | |||
// rudderBaseUrl should not contains "/" at the end | |||
, "link" -> (rudderBaseUrl + RudderConfig.linkUtil.changeRequestLink(cr.id)) | |||
, "link" -> (rudderBaseUrl + linkUtil.baseChangeRequestLink(cr.id)) |
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.
baseChangeRequestLink
is necessary: changeRequestLink
uses lift S
(ie request session) to compute the base path - what we wanted to avoid by asking the user for the rudderBaseUrl
to use.
, configMailPath: String | ||
) { | ||
|
||
// we want all our string to be trimmed | ||
implicit class ConfigExtension(config: Config) { | ||
def getTrimmedString(path: String) = config.getString(path).trim |
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.
creating utility methods to enhance readability of code is good
OK, merging this PR |
https://issues.rudder.io/issues/19352