Skip to content

Commit

Permalink
Feat: Refactor notifications from core to modules (#822)
Browse files Browse the repository at this point in the history
### Feature or Bugfix
- Refactoring

### Detail
As a rule of thumb, we encourage customization of `modules` while
changes in `core` should be avoided when possible. `notifications` is a
component initially in core which is only used by `dataset_sharing`. To
facilitate customization of the `notifications` module and also to
clearly see its dependencies we have:

- Moved `notifications` code from core to modules as it is a reusable
component that is not needed by any core component.
- Moved dataset_sharing references inside dataset_sharing module and
left `notifications` independent from any other module (done mostly in
#734, so credits to @TejasRGitHub)
- Added depends_on in the dataset_sharing module to load notifications
if the data_sharing module is imported.
- Modified frontend navigation bar to make it conditional of the
notifications module
- Added migration script to modify the notification type column
- Fix tests from #734, some references on the payload of the
notification tasks were wrong
- Small fixes to SES stack: added account in KMS policy and email_id as
input

### [WIP] Testing
Local testing
- [ ] loading of notifications with datasets enabled
- [ ] ...

AWS testing
- [ ] CICD pipeline succeds

### Other remarks
Not for this PR, but as a general note, we should clean up deprecated
ECS tasks

### Relates
- #785 
- #734 

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

`N/A` just refactoring


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
  • Loading branch information
dlpzx authored Oct 26, 2023
1 parent 48c32e5 commit 6d727e9
Show file tree
Hide file tree
Showing 47 changed files with 429 additions and 322 deletions.
6 changes: 2 additions & 4 deletions backend/api_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ def handler(event, context):

if 'authorizer' in event['requestContext']:
username = event['requestContext']['authorizer']['claims']['email']
email_id = event['requestContext']['authorizer']['claims']['email']
try:
groups = get_groups(event['requestContext']['authorizer']['claims'])
with ENGINE.scoped_session() as session:
Expand All @@ -138,14 +137,13 @@ def handler(event, context):
print(f'Error managing groups due to: {e}')
groups = []

set_context(RequestContext(ENGINE, username, email_id, groups))
set_context(RequestContext(ENGINE, username, groups))

app_context = {
'engine': ENGINE,
'username': username,
'groups': groups,
'schema': SCHEMA,
'emailId': email_id,
'schema': SCHEMA
}

else:
Expand Down
6 changes: 4 additions & 2 deletions backend/dataall/base/aws/cognito.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ def __init__(self):
def get_user_emailids_from_group(self, groupName):
try:
envname = os.getenv('envname', 'local')
if envname == 'local' or envname == 'dkrcompose':
raise Exception('Cognito is not supported in local dev environment')
parameter_path = f'/dataall/{envname}/cognito/userpool'
ssm = boto3.client('ssm', region_name=os.getenv('AWS_REGION', 'eu-west-1'))
user_pool_id = ssm.get_parameter(Name=parameter_path)['Parameter']['Value']
Expand All @@ -29,6 +27,10 @@ def get_user_emailids_from_group(self, groupName):
group_email_ids.extend([x['Value'] for x in attributes if x['Name'] == 'email'])

except Exception as e:
envname = os.getenv('envname', 'local')
if envname in ['local', 'dkrcompose']:
log.error('Local development environment does not support Cognito')
return ['anonymous@amazon.com']
log.error(
f'Failed to get email ids for Cognito group {groupName} due to {e}'
)
Expand Down
4 changes: 4 additions & 0 deletions backend/dataall/base/aws/ses.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,9 @@ def send_email(self, toList, message, subject):
}
)
except Exception as e:
envname = os.getenv('envname', 'local')
if envname in ['local', 'dkrcompose']:
log.error('Local development environment does not support SES notifications')
return True
log.error(f'Error while sending email {e})')
raise e
1 change: 0 additions & 1 deletion backend/dataall/base/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ class RequestContext:
"""Contains API for every graphql request"""
db_engine: Engine
username: str
email_id: str
groups: List[str]


Expand Down
1 change: 0 additions & 1 deletion backend/dataall/core/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from dataall.core import (
permissions,
stacks,
notifications,
cognito_groups,
environment,
organizations,
Expand Down
1 change: 0 additions & 1 deletion backend/dataall/core/notifications/__init__.py

This file was deleted.

9 changes: 0 additions & 9 deletions backend/dataall/core/notifications/api/__init__.py

This file was deleted.

48 changes: 0 additions & 48 deletions backend/dataall/core/notifications/api/resolvers.py

This file was deleted.

3 changes: 0 additions & 3 deletions backend/dataall/core/notifications/handlers/__init__.py

This file was deleted.

3 changes: 1 addition & 2 deletions backend/dataall/core/permissions/api/types.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
from dataall.base.api import gql
from dataall.core.notifications.db.notification_models import Notification
from dataall.core.permissions.db.permission_models import PermissionType


def resolve_enum(context, source: Notification):
def resolve_enum(context, source: PermissionType):
return source.type.name if source.type else PermissionType.TENANT.name


Expand Down
6 changes: 4 additions & 2 deletions backend/dataall/modules/dataset_sharing/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ def is_supported(modes: Set[ImportMode]) -> bool:

@staticmethod
def depends_on() -> List[Type['ModuleInterface']]:
return [DatasetBaseModuleInterface]
from dataall.modules.notifications import NotificationsModuleInterface
return [DatasetBaseModuleInterface, NotificationsModuleInterface]

def __init__(self):
from dataall.modules.dataset_sharing import api
Expand All @@ -35,7 +36,8 @@ def is_supported(modes: List[ImportMode]):

@staticmethod
def depends_on() -> List[Type['ModuleInterface']]:
return [DatasetBaseModuleInterface]
from dataall.modules.notifications import NotificationsModuleInterface
return [DatasetBaseModuleInterface, NotificationsModuleInterface]

def __init__(self):
import dataall.modules.dataset_sharing.handlers
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ def revoke_items_share_object(uri, revoked_uris):
resource_uri=dataset.datasetUri,
)

ShareNotificationService.notify_share_object_rejection(session, context.username, context.email_id, dataset, share)
ShareNotificationService(
session=session,
dataset=dataset,
share=share
).notify_share_object_rejection(email_id=context.username)

revoke_share_task: Task = Task(
action='ecs.share.revoke',
Expand Down
Loading

0 comments on commit 6d727e9

Please sign in to comment.