Feat: Refactor notifications from core to modules

[dlpzx]

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 Email Notification for Actions on Share Requests #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 Email Notification for Actions on Share Requests #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

• Refactor and migrate notifications from core to modules #785
• Email Notification for Actions on Share Requests #734

Security

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

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.

[noah-paige]

Just a couple of notes that I meant to leave on the previous Email notifciation PR - @dlpzx

• Can we make iam.AnyPrincipal() drilled down to iam.AccountPrincipal() in the KMS Key Policy for SNS Topic in SES Stack

• Enhancement Idea (can be implemented later): On revokes it is for particular items but we just say revoked/rejected on dataset level

  • Should we have different notifications for revoked share items and display the item names that are revoked verse rejected share objects?

[dlpzx]

@noah-paige, @TejasRGitHub I got an enhancement idea. Currently who receives a notification is still a little messy:

Current state

---

SHARE_SUBMISSION
Notification in UI sent to:
- dataset.owner
Email sent to:
- share submitter user: user that clicked on "submit" share request OR share.groupUri (if group_notifications=true)
- dataset.SamlAdminGroupName
- dataset.stewards

---

SHARE_APPROVAL
Notification in UI sent to:
- dataset.owner
- dataset.SamlAdminGroupName
- dataset.stewards
- share.owner: user that created the share request
Email sent to:
- share approver user: user that clicked on "approve" share request OR share.groupUri (if group_notifications=true)
- dataset.SamlAdminGroupName
- dataset.stewards

---

SHARE_REJECTED
Notification in UI sent to:
- dataset.owner
- dataset.SamlAdminGroupName
- dataset.stewards
- share.owner: user that created the share request
Email sent to:
- share approver user: user that clicked on "reject" share request OR share.groupUri (if group_notifications=true)
- dataset.SamlAdminGroupName
- dataset.stewards

Target state

---

ALL ACTIONS
Notification in UI sent to:
- dataset.SamlAdminGroupName
- dataset.stewards
- share.groupUri
Email sent to:
- dataset.SamlAdminGroupName
- dataset.stewards
- share.owner (person that opened the request) OR share.groupUri (if group_notifications=true)

[dlpzx]

After agreeing with @TejasRGitHub offline, the PR is ready for review with the latest changes to simplify the notifications recipients.

[noah-paige]

I tested this feature with datasets.share_notifications.email.active = False and all deployed successfully. One thing I noticed is that Notifications only get listed for the share.owner and not for the dataset.SamlAdminGroup or dataset.steward

I believe this is because in the ShareNotificationService for self.notification_target_users we add targeted users of dataset.SamlAdminGroupName (Group) , dataset.steward (Group), and share.owner (User Email) and have 3 notifications created in table for each with each of the above targeted users as the Notification.username

Then in NotificationPopover when we list Notifications for the use with API listNotifications() the backend resolver filters by context.username (i.e. models.Notification.username == username) and does not add another check for if user is a part of the group which is specified in the username

Either we have to add

1 - an additional conditional in the filter to say if user's username is the same email as in Notification.username or user's groups is one of the groups specified in Notification.username (and remove duplicates)

2 - get a list of user's emails that are within the dataset.SamlAdminGroup and dataset.stewards and add a new Notification object to the table for each distinct user email with Notification.username=user's email

[dlpzx]

@noah-paige, @TejasRGitHub
I am testing the following scenario:

"share_notifications": {
                "email": {
                    "active": false
                }
            }

[dlpzx]

@noah-paige @TejasRGitHub I still see one possible enhancement for this feature, but maybe it is out of the scope for this PR. I modified the notifications to take a group as recipient. That way notifications can be defined for a complete team. The issue is that a single user can delete the notification for the whole team, so only that user sees the notification although the whole team has received it.

Option 1: One of the options that you pointed out @noah-paige was to create a notification record for each user. But that means going into cognito and getting the emails of each of the users belonging to a group. The downside is that we depend on Cognito as underlying technology and that not all users in a group are data.all active users

Option2: have a window where all users can see all notifications. A proper frontend view for notifications. Even if a user marks a notification as read, all other members of the team can go and check the notifications so that the information is not lost. And we make notifications more user-friendly. At the moment I don't think they are that usable

Option 3: we store the users that have "marked as read" the notification in a different table --> not the ideal for a relational database to store

They are not exclusive. In my opinion option 2, having notifications views, is always a positive addition

[dlpzx]

@TejasRGitHub @noah-paige I have updated the PR, I made some final changes in the cdk.json and config.json. I also moved all the condition/checks logic to the same place and used if elses block (the one-liner conditionals was getting busy). Please have a look at my latest commits.

I will perform a final AWS test and as next steps we leave the "enhancements to notifications in UI" for future releases

[noah-paige]

Did another review of the latest commits and tested out the latest changes in an AWS Deployment with share_notifications.email.active set to false

Approving now to merge to v2m1m0 - looks good to me

[TejasRGitHub]

Looks Good to me as well. @dlpzx , please let me know where are you planning to keep the email_sender_id parameter value and I will update the documentation and create PR on the gh-pages.

[noah-paige]

I think the latest changes to add exceptions for local deployment look good as well - think good to merge @dlpzx

[TejasRGitHub]

Looks good to me