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

Feat: Move notifications from core to modules to facilitate customization #790

Closed
wants to merge 3 commits into from

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Oct 13, 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
  • Added depends_on in the dataset_sharing module to load notifications if the data_sharing module is imported.

TODO:

  • frontend changes in case notifications are not loaded
  • not for this PR, but as a general note, we should clean up deprecated ECS tasks

Relates

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.

@dlpzx dlpzx requested a review from noah-paige October 13, 2023 09:52
@dlpzx
Copy link
Contributor Author

dlpzx commented Oct 16, 2023

@noah-paige and I discussed internally that this PR is just for reference and will stay in Draft. First the PR for #734 needs to be merged and then we will do the necessary changes which developed in this Draft PR.

@dlpzx
Copy link
Contributor Author

dlpzx commented Oct 20, 2023

Closing, it will be merged in #822

@dlpzx dlpzx closed this Oct 20, 2023
@dlpzx dlpzx deleted the feat/notifications-to-modules branch November 8, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant