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

#470 Notification Task and Logic #475

Merged
merged 41 commits into from
Nov 13, 2023
Merged

#470 Notification Task and Logic #475

merged 41 commits into from
Nov 13, 2023

Conversation

HesamKorki
Copy link
Member

@HesamKorki HesamKorki commented Oct 19, 2023

Related to #470

Please first apply the migration and then proceed.
You can create the celery task from the admin view and test it with periodic tasks.

  • Add a Mixin for entities to inherit from and get the ability to produce notifications
  • implement the Notification Business logic for the dataset, project, document, and access entities
  • Add tests
  • Add a celery task that calls the notification method of every entity that implements the Mixin
  • fix the related name of access from the user side
  • fix backward relationship of local custodians and project

@HesamKorki HesamKorki changed the base branch from develop to implement-notification-models October 23, 2023 12:14
@HesamKorki HesamKorki changed the base branch from implement-notification-models to develop November 8, 2023 13:09
notification/tasks.py Outdated Show resolved Hide resolved
docker-compose.yaml Show resolved Hide resolved
elixir_daisy/celery_app.py Outdated Show resolved Hide resolved
notification/models.py Outdated Show resolved Hide resolved
notification/__init__.py Outdated Show resolved Hide resolved
notification/__init__.py Outdated Show resolved Hide resolved
notification/__init__.py Outdated Show resolved Hide resolved
core/models/project.py Outdated Show resolved Hide resolved
core/models/dataset.py Outdated Show resolved Hide resolved
core/models/dataset.py Outdated Show resolved Hide resolved
@neoflex
Copy link
Member

neoflex commented Nov 9, 2023

logging could be made more extensive. can be done in a futur improvement too.
the output I got when nothing was generated:

[2023-11-09 16:00:34,553: INFO/ForkPoolWorker-6] Creating notifications for 2023-11-09
[2023-11-09 16:00:34,671: INFO/ForkPoolWorker-6] Task notification.tasks.create_notifications_for_entities[507437a6-b7d1-4778-ba21-d5be9ca61a01] succeeded in 0.12050887499935925s: None

@neoflex
Copy link
Member

neoflex commented Nov 9, 2023

it would be worth modifying the settings_locale template to update the CELERY_BEAT_SCHEDULE including the new task. Also to make sure the recommended schedule executes this task before the email sending task.

@HesamKorki
Copy link
Member Author

logging could be made more extensive. can be done in a futur improvement too. the output I got when nothing was generated:

[2023-11-09 16:00:34,553: INFO/ForkPoolWorker-6] Creating notifications for 2023-11-09
[2023-11-09 16:00:34,671: INFO/ForkPoolWorker-6] Task notification.tasks.create_notifications_for_entities[507437a6-b7d1-4778-ba21-d5be9ca61a01] succeeded in 0.12050887499935925s: None

I added some more logging at the Notification object creation time

@HesamKorki HesamKorki merged commit b7fe57e into develop Nov 13, 2023
2 checks passed
@vildead vildead deleted the 470-notif-user-loop branch July 10, 2024 09:29
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.

2 participants