-
Notifications
You must be signed in to change notification settings - Fork 66
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
Support sending email message via Notifications passthrough API #406
Conversation
Signed-off-by: Mohammad Qureshi <[email protected]>
Signed-off-by: Mohammad Qureshi <[email protected]>
…ugh API Signed-off-by: Mohammad Qureshi <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #406 +/- ##
=============================================
+ Coverage 70.40% 86.40% +16.00%
=============================================
Files 123 51 -72
Lines 3849 1479 -2370
Branches 612 352 -260
=============================================
- Hits 2710 1278 -1432
+ Misses 960 198 -762
+ Partials 179 3 -176
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
If anyone is wondering why the Notification Dashboards CI passed when the backend isn't building yet, it's because the workflow just checks out the backend plugin and is running the unit test for the frontend. Once we fix the Cypress tests, we can add them to the |
{ key: String -> | ||
SecureSetting.secureString( | ||
key, | ||
fallback(key, LEGACY_EMAIL_USERNAME, "opensearch\\.notifications\\.core", "plugins.alerting.destination") |
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.
do we have a unit test for fallback case?
// The naming is a little confusing but need to use the subject from LegacyEmailMessage as the title, | ||
// so it appears as the subject of the email | ||
message = MessageContent(title = baseMessage.subject, textDescription = baseMessage.messageContent) | ||
runBlocking { |
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.
Do we plan to simplify this blocking call in the future?
@@ -291,6 +292,45 @@ object SendMessageActionHelper { | |||
LegacyDestinationResponse.Builder().withStatusCode(status.statusCode) | |||
.withResponseContent(status.statusText).build() | |||
} | |||
LegacyDestinationType.LEGACY_EMAIL -> { |
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.
Can we not re-use the logic from sendEmailMessage
below by abstracting out to the pieces in a util method, to avoid code duplication?
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.
LGTM. Few suggestions/clarifications which are non-blocking
Description
Depends on opensearch-project/common-utils#158 so GitHub Actions are expected to fail until that is merged in.
This PR includes the following:
EMAIL_USERNAME
andEMAIL_PASSWORD
to correctly use Alerting's when they aren't set for an SMTP email accountIssues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.