[GH-1420] Notification Improvements Stage 1 by TejasRGitHub · Pull Request #1748 · data-dot-all/dataall

TejasRGitHub · 2024-12-26T21:21:55Z

Feature or Bugfix

Feature [Enhancements]

Detail

Stage 1 as described in the Gh issue 1420 ( #1420 (comment))

Table on information about when notifications are sent to whom they are sent. I will add this as a part of documentation as well.

Data.all Resource	Notification	Recipient
Share	Share Object Submitted	Dataset owner, steward and share owner ( requestor )
Share	Share Object Approved	Dataset owner, steward and share owner ( requestor )
Share	Share Object Failed	Dataset owner, steward and share owner ( requestor )
Share	Share Object Revoked / Rejected	Dataset owner, steward and share owner ( requestor )
Share	Share verifier finds share is unhealthy	Share requestor team
Share	Share reapplier finds share is unhealthy	Share requestor team
Share	Share reapplier finds share is healthy	Share requestor team
Weekly reminders (Share Objects )	Submitted shares and unhealthy shares	Share requestor team
Weekly reminders (Dataset Objects )	Unhealthy dataset	Dataset owner and stewards team
Weekly reminders (Environment Objects )	Unhealthy environments	Environment owner team
ECS task	Any failures in ECS task	Data.all admin team ( DAAdministrators )
Dataset Table	Dataset table is removed	Dataset owners and stewards and if any shares exist on that table then notify all share object requestor teams
Dataset Table	Dataset table is added	Dataset owners and stewards
Dataset Table	Dataset table which was previously deleted was added back again	Dataset owners and stewards

Tests

Email Sent when something fails in ECS tasks ✅ Sample email -

2. Weekly Reminder email ✅ -

3. Email sent when shares are reapplied ✅ -

4. Email sent when share verifier finds share in unhealthy state ✅

5. Email sent when a glue table is deleted / resynced / added Email sent to dataset owners and dataset stewards when a table is deleted

Email sent to the share owner group when a table is deleted and there are active shares on that table

Email sent incase of share failure -

Relates

notification improvements #1420

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

Does this PR introduce or modify any input fields or queries - this includes
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
- Is the input sanitized?
- What precautions are you taking before deserializing the data you consume?
- Is injection prevented by parametrizing queries?
- Have you ensured no eval or similar functions are used?
Does this PR introduce any functionality or component that requires authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
- Are you logging failed auth attempts?
Are you using or adding any cryptographic features?
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?
Are you introducing any new policies/roles/users?
- Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

noah-paige

part 1 Review - still have a couple more files on Dataset Table notifications and weekly digest

backend/dataall/modules/catalog/tasks/catalog_indexer_task.py

backend/dataall/modules/notifications/services/admin_notifications.py

backend/dataall/modules/notifications/services/ses_email_notification_service.py

backend/dataall/modules/shares_base/services/share_notification_service.py

backend/dataall/modules/shares_base/services/sharing_service.py

backend/dataall/modules/shares_base/services/share_notification_service.py

noah-paige · 2025-01-10T15:33:43Z

backend/dataall/modules/shares_base/db/share_object_repositories.py

+    def find_share_by_dataset_attributes(session, dataset_uri, dataset_owner, groups = None):
+        if groups is None:
+            groups = []


I am guessing there is an edge case where groups could be passed in explicitly as None before and not being set to [] so the query failed?

tbh, I added it because my editor was suggesting an improvement. I think this article does a fine job at explaining why - https://nikos7am.com/posts/mutable-default-arguments/

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py

noah-paige

some additional comments from pending file review

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py

noah-paige · 2025-01-10T17:35:38Z

backend/dataall/modules/s3_datasets/tasks/tables_syncer.py

+                        if table_status_map:
+                            log.info('Sending email notification after dataset table updates were found')
+                            try:
+                                DatasetTableNotifications(dataset=dataset).notify_dataset_table_updates(session=session, table_status_map=table_status_map)
+                            except Exception as e:
+                                error_log = f"Error occurred while sending email to notify about changes to the glue tables for dataset with uri: {dataset.datasetUri} due to: {e}"
+                                task_exceptions.append(error_log)


table_status_map will always be defined so this will always execute right?

also should there be a try catch here or only on the loop over datasets?

table_status_map will always be defined so this will always execute right?

The table_status_map will be empty when there are no newly added tables, no table has been deleted , no table which was deleted previously has been added. And thus when it is empty the if part won't execute.

also should there be a try catch here or only on the loop over datasets?

I have added this try catch explicitly so that that any email sending failures are caught. Also with this , the rest of the code deosn't get affected and I don't want it to get affected

…et policu is removed

TejasRGitHub · 2025-01-15T00:58:01Z

Hi @noah-paige , I have updated the PR. Please let me know if you have any additional code comments. Let me know if you need any clarification / discussion on few of my replies on your comments .

Few additional things which I noticed and added as a part of new updated PR,

Admin group config ( admin_notifications ) in config.json to enable/disable admin email notifications. I am only keeping email admin notifications and not creating UI notifications - as UI notifications will look bloated with a lot of content.
Added try..catch in sharing_services file where missing and reformatted to have the same pattern of try except finally block across the file

noah-paige · 2025-01-16T16:25:36Z

backend/dataall/core/groups/db/constants.py

nit: should we also explicitly import db in the __init__ file? (at dataall/backend/dataall/core/groups/__init__.py) -- think right now it is implicitly imported via api

I wouldn't mind it getting imported implicitly. With a blank init all the group_models and constants file will be imported.

backend/dataall/modules/notifications/services/admin_notifications.py

backend/dataall/modules/notifications/services/ses_email_notification_service.py

backend/dataall/modules/shares_base/services/sharing_service.py

TejasRGitHub · 2025-01-24T16:23:41Z

backend/dataall/modules/shares_base/db/share_object_repositories.py

    @staticmethod
-    def find_dataset_shares(session, dataset_uri):
-        return session.query(ShareObject).filter(ShareObject.datasetUri == dataset_uri).all()
+    def find_dataset_shares(session, dataset_uri: str):


Nothing has changed here. Only returning via query.allI() instead of on the same line

TejasRGitHub · 2025-01-24T16:32:07Z

backend/dataall/modules/shares_base/services/share_notification_service.py

                    log.info(f'Notification type : {share_notification_config_type} is not active')
        else:
            log.info('Notifications are not active')
-


Removing this function from here and moving it to ses_email_notification_service.py

TejasRGitHub · 2025-01-24T16:38:27Z

config.json

                        "parameters": {
-                            "group_notifications": true
+                            "group_notifications": true,
+                            "admin_notifications": true


Adding this new parameter to switch on/off admin notifications. This can be later removed and admin notifications can be enabled by default when email notifications are enabled. Keeping this for now, to test and see how many notifications an admin might get with the current code change. There is a chance in which admins can be bombarded with a lot of notifications in which this will serve as a good way to stop creating admin notifications

TejasRGitHub · 2025-01-24T16:39:02Z

deploy/stacks/container.py

            container_id='container',
            ecr_repository=ecr_repository,
-            environment=self._create_env(),
+            environment=self.env_vars,


Switching to env_vars as those contains the same as self._create_env()

TejasRGitHub · 2025-01-24T16:44:41Z

Hi @petrkalos , @noah-paige , I have made code changes after taking a look at the review comments. Can you please take a look and let me if this PR looks good now

I have re-tested after code changes

Notifications after share is failed ( email + UI ) ✅
Notifications after share is Unhealthy ( email + UI ) ✅
Notifications after share is healthy ( email + UI ) ✅
Weekly digest reminder notifications ( email ) ✅
Notifications to owner ( when table are deleted, added, synced ) and share owners ( if there exists a share on the table ) - ✅
Notifications when ECS or any service fails to Admins ✅

noah-paige

thanks for all the work here Tejas - mostly nits from my end.

Mainly can we make sure to use table label vs GlueTableName comment and remove the unused imports - then I can approve

backend/dataall/base/feature_toggle_checker.py

backend/dataall/modules/omics/tasks/omics_workflows_fetcher.py

backend/dataall/modules/shares_base/tasks/share_reapplier_task.py

backend/dataall/modules/shares_base/services/sharing_service.py

...nd/dataall/modules/s3_datasets_shares/services/share_processors/s3_bucket_share_processor.py

...aall/modules/s3_datasets_shares/services/share_processors/s3_access_point_share_processor.py

...d/dataall/modules/s3_datasets_shares/services/share_processors/glue_table_share_processor.py

backend/dataall/modules/s3_datasets/services/dataset_table_notifications.py

TejasRGitHub · 2025-01-28T18:06:07Z

@noah-paige, Updated PR and made minor changes as per the review comments

noah-paige · 2025-01-28T19:26:08Z

@TejasRGitHub - looks like a test is failing and need to format with ruff

TejasRGitHub · 2025-01-28T19:41:34Z

@TejasRGitHub - looks like a test is failing and need to format with ruff

Updated PR . Now the ruff tests and the unit test should pass

noah-paige

Thanks for resolving ruff issues - looks like latest version of ruff had updated the way it formats f-strings causing a number of files to be updated for formatting outside the scope of this PR

TejasRGitHub · 2025-01-29T23:30:57Z

backend/dataall/base/feature_toggle_checker.py

 """

 import functools
+import logging


This file is a part of PR

TejasRGitHub · 2025-01-29T23:31:19Z

backend/dataall/core/groups/db/constants.py

@@ -0,0 +1,2 @@
+class DataallGroups:


This file is a part of PR

TejasRGitHub · 2025-01-29T23:31:38Z

backend/dataall/modules/catalog/tasks/catalog_indexer_task.py

 from dataall.base.db import get_engine
 from dataall.base.loader import load_modules, ImportMode
 from dataall.base.utils.alarm_service import AlarmService
+from dataall.modules.notifications.services.admin_notifications import AdminNotificationService


This file is a part of PR

TejasRGitHub · 2025-01-29T23:32:01Z

backend/dataall/modules/notifications/services/admin_notifications.py

@@ -0,0 +1,35 @@
+import logging


This file is a part of PR

TejasRGitHub · 2025-01-29T23:32:09Z

backend/dataall/modules/notifications/services/ses_email_notification_service.py

        email_provider = SESEmailNotificationService.get_email_provider_instance(
            recipient_groups_list, recipient_email_list
        )
+        identityProvider = ServiceProviderFactory.get_service_provider_instance()


This file is a part of PR

TejasRGitHub · 2025-01-29T23:32:14Z

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py

+import os
+from dataclasses import dataclass, field
+from typing import List, Dict, Any, Tuple, Set
+


This file is a part of PR

TejasRGitHub · 2025-01-29T23:35:28Z

backend/dataall/modules/shares_base/tasks/persistent_email_reminders_task.py

-            dataset = DatasetBaseRepository.get_dataset_by_uri(session, share.datasetUri)
-            ShareNotificationService(session=session, dataset=dataset, share=share).notify_persistent_email_reminder(
-                email_id=share.owner
+    task_exceptions = []


This file is a part of PR

TejasRGitHub · 2025-01-29T23:35:35Z

backend/dataall/modules/shares_base/tasks/share_expiration_task.py

-                    revoked_items_states = ShareStatusRepository.get_share_items_states(
-                        session, share.shareUri, item_uris
-                    )
+    task_exceptions = []


This file is a part of PR

TejasRGitHub · 2025-01-29T23:35:39Z

backend/dataall/modules/shares_base/tasks/share_manager_task.py

 import os
 import sys

+from dataall.modules.notifications.services.admin_notifications import AdminNotificationService


This file is a part of PR

TejasRGitHub · 2025-01-29T23:35:45Z

backend/dataall/modules/shares_base/tasks/share_reapplier_task.py

 import sys
+from typing import List

+from dataall.modules.notifications.services.admin_notifications import AdminNotificationService


This file is a part of PR

TejasRGitHub · 2025-01-29T23:35:49Z

backend/dataall/modules/shares_base/tasks/share_verifier_task.py

-            processed_share_objects.append(share_object.shareUri)
-            SharingService.verify_share(
-                engine, share_uri=share_object.shareUri, status=ShareItemStatus.Share_Succeeded.value, healthStatus=None
+    task_exceptions = []


This file is a part of PR

TejasRGitHub · 2025-01-29T23:37:58Z

Thanks for resolving ruff issues - looks like latest version of ruff had updated the way it formats f-strings causing a number of files to be updated for formatting outside the scope of this PR

Yes there are abut 60 - 70 files which got formatted 😢 . @petrkalos I have commented on the files which are a part of this PR . You can also view the changes made in the PR update prior to the formatting changes

TejasRGitHub added 6 commits December 26, 2024 15:21

Notification Improvements Stage 1, 70%

8750f63

Changes in the principal name

1cd2224

Weekly email task

1728d52

Weekly notifications code and minor changes

daa709e

Glue Table change notifications

84bf9fe

Additional comments and logs plus weekly reminder task

8c861ff

TejasRGitHub marked this pull request as ready for review January 3, 2025 00:27

noah-paige linked an issue Jan 10, 2025 that may be closed by this pull request

notification improvements #1420

Open

noah-paige reviewed Jan 10, 2025

View reviewed changes

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py Outdated Show resolved Hide resolved

noah-paige reviewed Jan 10, 2025

View reviewed changes

backend/dataall/modules/notifications/tasks/weekly_digest_reminder.py Outdated Show resolved Hide resolved

noah-paige reviewed Jan 10, 2025

View reviewed changes

TejasRGitHub added 6 commits January 10, 2025 14:01

Syncing changes from local deploy

99ed2ec

Adding new changes

c119c55

Minor correction in the create bucket policy code when access to buck…

f1e4079

…et policu is removed

New changes

870a835

Adding new changes for admin notifications

5b8ec01

Linting update

17158e2

TejasRGitHub added 2 commits January 15, 2025 11:45

Admin notification improvements

fd32987

Adding more refactoring changes

457ce82

noah-paige assigned TejasRGitHub Jan 16, 2025

noah-paige reviewed Jan 16, 2025

View reviewed changes

petrkalos requested changes Jan 17, 2025

View reviewed changes

TejasRGitHub added 6 commits January 21, 2025 14:15

Refactoring and corrections

878082a

Minor changes after testing

401b0de

Adding new changes and corrections

63dd9f1

Minor change and linting

828edf8

Formatting changes

8be4298

Adding improvements after review comments

a408553

TejasRGitHub added 2 commits January 24, 2025 10:12

Small slight changes

8b9b10c

Weekly notification enums

d83d85c

TejasRGitHub commented Jan 24, 2025

View reviewed changes

Few corrections

215ebe4

TejasRGitHub requested review from noah-paige and petrkalos January 24, 2025 16:44

noah-paige reviewed Jan 28, 2025

View reviewed changes

Minor corrections

7a92d9a

Merge branch 'main' into data-dot-allGH-1420-notification-improvements-1

60e0988

TejasRGitHub added 2 commits January 28, 2025 14:37

Resolving tests

f60e4ce

Filed reformatted by ruff

83156c7

noah-paige reviewed Jan 29, 2025

View reviewed changes

TejasRGitHub commented Jan 29, 2025

View reviewed changes

backend/dataall/base/feature_toggle_checker.py

"""

import functools

import logging

Copy link

Collaborator Author

TejasRGitHub Jan 29, 2025

Choose a reason for hiding this comment

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

This file is a part of PR

TejasRGitHub commented Jan 29, 2025

View reviewed changes

noah-paige approved these changes Jan 29, 2025

View reviewed changes

Conversation

TejasRGitHub commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Feature or Bugfix

Detail

Tests

Relates

Security

Uh oh!

noah-paige left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noah-paige left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TejasRGitHub commented Jan 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TejasRGitHub commented Jan 24, 2025

Uh oh!

noah-paige left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TejasRGitHub commented Jan 28, 2025

Uh oh!

noah-paige commented Jan 28, 2025

Uh oh!

TejasRGitHub commented Dec 26, 2024 •

edited

Loading

noah-paige left a comment •

edited

Loading