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

disable notification feature from UI #198

Merged

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Nov 17, 2020

Issue #, if available:

Description of changes:
remove notification from following pages

  • report detail
  • report definition detail
  • create/update report definition

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 November 17, 2020 08:58
@@ -13,4 +13,4 @@
* permissions and limitations under the License.
*/

export const POLL_INTERVAL = 1000 * 60 * 5; // in milliseconds (5 min)
export const POLL_INTERVAL = 1000 * 60 * 0.5; // in milliseconds (5 min)
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment should be 30 seconds instead of 5 min, and what does POLL_INTERVAL do?

Copy link
Member Author

Choose a reason for hiding this comment

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

I always forgot to revert that value, thx! We are using a fixed interval to poll background jobs for reporting. For develop purpose, I always change it to a small value.

@@ -620,7 +620,7 @@ export function ReportDefinitionDetails(props) {
<EuiSpacer />
{triggerSection}
<EuiSpacer />
<EuiTitle>
{/* <EuiTitle>
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 unused code?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be added back once we plan to add notification. Do you want me to put a comment here?

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

@zhongnansu zhongnansu merged commit e755c43 into opendistro-for-elasticsearch:dev Nov 18, 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