Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Add email body template & Optimize notification setting UI #141

Merged
merged 11 commits into from
Oct 23, 2020

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Oct 23, 2020

Issue #, if available:

Description of changes:

  • update notification settings UI and flow behind in create report definition
    image

  • Add email template
    image

  • update notification setting UI in report details page

  • Add better logging and error handling for email

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@zhongnansu zhongnansu marked this pull request as ready for review October 23, 2020 07:05
@@ -97,7 +97,6 @@ export function EditReportDefinition(props) {

const callUpdateAPI = async (metadata) => {
const { httpClient } = props;

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this file from the PR?

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 edit_report_definition.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah- it looks like the only change is an empty line removal?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -143,13 +142,12 @@ export const channelSchema = schema.object({
}),
{ minSize: 1 }
),
// TODO: consider add this field next to url-related fields.
// Need this to build the links in email
origin: schema.uri(), //e.g. https://xxxxx.com
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need input validation for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

the .uri() will check whether it is valid uri in this format https://xxxxx.com

);
/**
* notification plugin response example:
* {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this comment block?

Copy link
Contributor

@davidcui1225 davidcui1225 left a comment

Choose a reason for hiding this comment

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

LGTM - please address comments before merging

@zhongnansu zhongnansu requested a review from akbhatta October 23, 2020 17:13
@zhongnansu zhongnansu merged commit 6ed2912 into opendistro-for-elasticsearch:dev Oct 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants