Skip to content
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

Refactor and migrate notifications from core to modules #785

Closed
dlpzx opened this issue Oct 11, 2023 · 1 comment
Closed

Refactor and migrate notifications from core to modules #785

dlpzx opened this issue Oct 11, 2023 · 1 comment
Assignees
Milestone

Comments

@dlpzx
Copy link
Contributor

dlpzx commented Oct 11, 2023

Is your idea related to a problem? Please describe.
The notification module in core is very specific to datasets, which goes against the design principles of keeping in modules the module specific functionalities and in core the bare minimum application modules. For this reason we would like to refactor the notifications code and turn it into a module the same way it is done for feeds or votes

Describe the solution you'd like
The list of tasks that need to be complete are:

  • Add a registry in notifications and move it from core to modules
  • Add the dependencies in the data_sharing or in the dataset module init file
  • In the config.json, we need to declare the module data_sharing and add the feature notifications to it. Instead of doing it from core . Alternatively we could do it from the datasets module, whatever works best.
  • Code for SES notifications developed in Email Notification for Actions on Share Requests #734 would be divided into the notifications module and the way we register the notifications in the dataset/data_sharing module
@dlpzx dlpzx self-assigned this Oct 13, 2023
@dlpzx dlpzx added this to the v2.1.0 milestone Oct 13, 2023
dlpzx added a commit that referenced this issue Oct 26, 2023
### 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.
noah-paige pushed a commit that referenced this issue Nov 3, 2023
### Feature or Bugfix
- Feature ( Documentation ) 

### Detail

When enabling email notification on share workflow, an SES identity is
created on AWS SES. This PR contains documentation about the manual
steps that need to be taken towards setting up SES and also additional
steps to setup monitoring.

### Relates
- #734
- #785

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

- 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)? N/A
  - 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? N/A
- 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? N/A
  - 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? N/A
  - 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.
@dlpzx
Copy link
Contributor Author

dlpzx commented Nov 8, 2023

Merged and released with v2.1.0 🚀

@dlpzx dlpzx closed this as completed Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant