-
Notifications
You must be signed in to change notification settings - Fork 94
[GH-1420] Notification Improvements Stage 1 #1748
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
8750f63
1cd2224
1728d52
daa709e
84bf9fe
8c861ff
99ed2ec
c119c55
f1e4079
870a835
5b8ec01
17158e2
fd32987
457ce82
878082a
401b0de
63dd9f1
828edf8
8be4298
a408553
8b9b10c
d83d85c
215ebe4
7a92d9a
60e0988
f60e4ce
83156c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| from enum import Enum | ||
|
|
||
|
|
||
| class DataallGroups: | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is a part of PR |
||
| admin = 'DAAdministrators' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,42 @@ | ||
| import logging | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is a part of PR |
||
| from typing import List | ||
|
|
||
| from dataall.core.groups.db.constants import DataallGroups | ||
| from dataall.modules.notifications.services.ses_email_notification_service import SESEmailNotificationService | ||
| from dataall.base.config import config | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| class AdminNotificationService: | ||
| admin_group = 'DAAdministrators' | ||
|
|
||
| class AdminNotificationService: | ||
| """ | ||
| Send email notifications to Admin Group i.e. DAAdministrators in data.all | ||
| Args - | ||
| 1. process_error - string describing in short the error / exception details | ||
| 2. error_logs - List of all the exception error logs | ||
| 2. error_logs - List of all the exception error logs | ||
| 3. process_name - Code where the exception occurred. Example, inside an ECS task like cataloging task, etc or inside a graphql service | ||
| """ | ||
|
|
||
| @classmethod | ||
TejasRGitHub marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| def notify_admins_with_error_log(cls, process_error: str, error_logs: List[str], process_name:str = ''): | ||
| def notify_admins_with_error_log(cls, process_error: str, error_logs: List[str], process_name: str = ''): | ||
| if ( | ||
| config.get_property( | ||
| 'modules.datasets_base.features.share_notifications.email.parameters.admin_notifications', default=False | ||
| ) | ||
| is False | ||
| ): | ||
| log.info('Admin notifications are switched off') | ||
| return | ||
TejasRGitHub marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| subject = f'Data.all alert | Attention Required | Failure in : {process_name}' | ||
| email_message = f""" | ||
| Following error occurred - <br><br> {process_error} <br><br> | ||
| """ | ||
| for error_log in error_logs: | ||
| email_message += error_log + "<br><br>" | ||
| email_message += error_log + '<br><br>' | ||
|
|
||
| email_message += "Please check the logs in cloudwatch for more details" | ||
| email_message += 'Please check the logs in cloudwatch for more details' | ||
|
|
||
| SESEmailNotificationService.create_and_send_email_notifications( | ||
TejasRGitHub marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| subject=subject, | ||
| msg=email_message, | ||
| recipient_groups_list=[cls.admin_group] | ||
| ) | ||
| subject=subject, msg=email_message, recipient_groups_list=[DataallGroups.admin] | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,10 @@ | ||
| # Email Notification Provider implements the email notification service abstract method | ||
| import logging | ||
|
|
||
| from dataall.base.aws.cognito import Cognito | ||
| from dataall.base.aws.ses import Ses | ||
| from dataall.base.config import config | ||
| from dataall.base.services.service_provider_factory import ServiceProviderFactory | ||
| from dataall.core.groups.db.constants import DataallGroups | ||
| from dataall.modules.notifications.services.base_email_notification_service import BaseEmailNotificationService | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
@@ -38,9 +38,8 @@ def send_email_task(subject, message, recipient_groups_list, recipient_email_lis | |
| email_provider = SESEmailNotificationService.get_email_provider_instance( | ||
| recipient_groups_list, recipient_email_list | ||
| ) | ||
| identityProvider = ServiceProviderFactory.get_service_provider_instance() | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is a part of PR |
||
| try: | ||
| identityProvider = ServiceProviderFactory.get_service_provider_instance() | ||
|
|
||
| email_ids_to_send_emails = email_provider.get_email_ids_from_groupList( | ||
| email_provider.recipient_group_list, identityProvider | ||
| ) | ||
|
|
@@ -51,6 +50,15 @@ def send_email_task(subject, message, recipient_groups_list, recipient_email_lis | |
| SESEmailNotificationService.send_email_to_users(email_ids_to_send_emails, email_provider, message, subject) | ||
|
|
||
| except Exception as e: | ||
| email_ids_to_send_emails = email_provider.get_email_ids_from_groupList( | ||
| [DataallGroups.admin], identityProvider | ||
| ) | ||
| SESEmailNotificationService.send_email_to_users( | ||
| email_ids_to_send_emails, | ||
| email_provider, | ||
| f'Error sending email due to: {e}', | ||
| 'Data.all alert | Attention Required | Failure in: Email Notification Service', | ||
| ) | ||
| raise e | ||
| else: | ||
| return True | ||
|
|
@@ -74,10 +82,9 @@ def create_and_send_email_notifications(subject, msg, recipient_groups_list=None | |
| if recipient_email_ids is None: | ||
| recipient_email_ids = [] | ||
TejasRGitHub marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| share_notification_config = config.get_property( | ||
| if share_notification_config := config.get_property( | ||
| 'modules.datasets_base.features.share_notifications', default=None | ||
TejasRGitHub marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
| if share_notification_config: | ||
| ): | ||
| for share_notification_config_type in share_notification_config.keys(): | ||
| n_config = share_notification_config[share_notification_config_type] | ||
| if n_config.get('active', False) == True: | ||
TejasRGitHub marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
@@ -88,4 +95,4 @@ def create_and_send_email_notifications(subject, msg, recipient_groups_list=None | |
| else: | ||
| log.info(f'Notification type : {share_notification_config_type} is not active') | ||
| else: | ||
| log.info('Notifications are not active') | ||
| log.info('Notifications are not active') | ||
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.
nit: should we also explicitly import
dbin the__init__file? (atdataall/backend/dataall/core/groups/__init__.py) -- think right now it is implicitly imported viaapiThere 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.
I wouldn't mind it getting imported implicitly. With a blank init all the group_models and constants file will be imported.