Skip to content

[ENG-8064] Add New Notifications Data Model (Refactor Notifications Phase 2) #11151

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 413 commits into
base: develop
Choose a base branch
from
Open

[ENG-8064] Add New Notifications Data Model (Refactor Notifications Phase 2) #11151

wants to merge 413 commits into from

Conversation

Johnetordoff
Copy link
Contributor

@Johnetordoff Johnetordoff commented May 20, 2025 •
edited
Loading

Purpose

This system is designed to formalize and consolidate OSF Notifications under one system in order to better facilitate collaboration between Product, Engineers and QA and end persistent problems with notification email related tech debt. This project is the result of an extensive audit of all OSF emails and combined email digests and seeks to move the NotificationSubscription model out of it's transitional state having been migrated from a document based model, into it's final data model which is more regular for a relation our current relational database (django's postgress db).

In order to do this I have taken @mfraezz design documents and implemented them with minor changes. This means the responsibility for sending notifications in osf.io is shared by three new models, which will have the old data migrated into them via a migration script and management command. The models are:

  • NotificationType

    • Similar to RegistrationSchemas or Waffle flags these are in db instrances which are poulated from static config documents in this case yaml.
    • Since these are synced on migration with notification.yaml a developer will be able to see at a glance if where a notification template is and what it's purpose is.

  • NotificationSubscription

    • This model is somewhat analogous to the earlier EmailDigest model, this holds references to potentially many Notifcations models that can be compiled into a single digest this is sent at a periodic basis.

  • Notification

    • Holds message information and context
    • Unlike earlier implementations this will record each Notification creation and sending for metrics purposes.

Changes

  • Combines worker with beat in docker-compose
  • creates notifications.yaml to contain all notification types info it is populated via postmigrate hook
  • creates new Notification NotificationSubscription and NotificationType
  • adds migrations to rename legacy tables and a managment command to populate the new ones.
  • Deletes old send_mails method and replaces it with emit of NotificationType
  • adds tests and updates old mocking
  • updates views and permission classed
  • A slight change to handle_boa_error to pass the already decanted user object.
  • adds new data model for Notification, NotificationTypes and Subscriptions
  • creates a notifications.yaml for data dependency notificationtypes
  • add migrations
  • updates notifications to use NotificationTypes
  • updates Admin app to control email templates
  • updates metrics reporter to count notifications sent etc.
  • updates tests to all use capture_notifications mocking util
  • Removes queued_mail
  • Removes EmailDigest
  • Removes detect_duplicates for duplicate subscriptions

QA Notes

Please make verification statements inspired by your code and what your code touches.

  • Verify
  • Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-8064

@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch 4 times, most recently from 3a8b414 to 57b0bd1 Compare May 21, 2025 14:42
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch from 2b8ba0d to c449599 Compare June 9, 2025 13:40
@Johnetordoff Johnetordoff changed the base branch from feature/pbs-25-10 to refactor-notifications June 13, 2025 14:40
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch 3 times, most recently from ad18e9d to 300524c Compare July 3, 2025 14:20
@Johnetordoff Johnetordoff marked this pull request as ready for review July 8, 2025 20:05
@Johnetordoff Johnetordoff force-pushed the refactor-notifications branch from 2dee1b2 to 2dbcbf7 Compare July 10, 2025 15:50
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch 4 times, most recently from 9238258 to 37b419a Compare July 10, 2025 20:24
@Johnetordoff Johnetordoff force-pushed the refactor-notifications branch from afc3815 to b6bbeed Compare July 11, 2025 13:02
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch 14 times, most recently from 1a4314d to 2894a24 Compare July 18, 2025 14:38
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch 5 times, most recently from 30b3720 to 6239379 Compare September 5, 2025 16:59
Johnetordoff
Johnetordoff commented Sep 6, 2025
View reviewed changes
website/settings/defaults.py
@@ -435,12 +435,10 @@ class CeleryConfig:
'scripts.generate_sitemap',
'osf.management.commands.clear_expired_sessions',
'osf.management.commands.delete_withdrawn_or_failed_registration_files',
'osf.management.commands.check_crossref_dois',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Johnetordoff this sent notifications and was seemingly unused

Johnetordoff
Johnetordoff commented Sep 6, 2025
View reviewed changes
website/settings/defaults.py
'osf.management.commands.find_spammy_files',
'osf.management.commands.migrate_pagecounter_data',
'osf.management.commands.migrate_deleted_date',
'osf.management.commands.addon_deleted_date',
'osf.management.commands.migrate_registration_responses',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Johnetordoff this sent notifications and was seemingly unused

Johnetordoff
Johnetordoff commented Sep 6, 2025
View reviewed changes
website/settings/defaults.py
@@ -456,10 +454,9 @@ class CeleryConfig:

med_pri_modules = {
'framework.email.tasks',
'scripts.send_queued_mails',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Johnetordoff task removal add to notes.

Johnetordoff
Johnetordoff commented Sep 6, 2025
View reviewed changes
website/settings/defaults.py
@@ -598,156 +590,110 @@ class CeleryConfig:
# 'scripts.analytics.upload',
# )

# celery.schedule will not be installed when running invoke requirements the first time.
try:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Johnetordoff I'm removing this because the comment is no longer correct.

@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch 3 times, most recently from 8f41491 to a65fa08 Compare September 8, 2025 13:06
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch from a65fa08 to 33d6fda Compare September 8, 2025 13:27
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch from a12d8c8 to e2dcfe5 Compare September 8, 2025 13:48
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch from ee98f7f to 6ce12ec Compare September 8, 2025 15:32
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch from 6ce12ec to ffe8e89 Compare September 8, 2025 16:14
@Johnetordoff
Merge branch 'add-new-notifications-data-model' of https://github.com…
27d4ec4 
…/CenterForOpenScience/osf.io into add-new-notifications-data-model

# Conflicts:
#	api_tests/nodes/views/test_node_contributors_detail.py
#	api_tests/nodes/views/test_node_contributors_list.py
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch from ffe8e89 to 27d4ec4 Compare September 8, 2025 16:34
@Johnetordoff Johnetordoff force-pushed the add-new-notifications-data-model branch from 21dc509 to d9ca75f Compare September 9, 2025 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Johnetordoff @adlius @ihorsokhanexoft @antkryt @Ostap-Zherebetskyi @bodintsov