-
Notifications
You must be signed in to change notification settings - Fork 351
[ENG-9762] [ENG-9758] Refactor notification subscriptions and digest handling #11459
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
[ENG-9762] [ENG-9758] Refactor notification subscriptions and digest handling #11459
Conversation
cslzchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐
| ) | ||
| except NotificationSubscription.MultipleObjectsReturned: | ||
| pass | ||
| if not isinstance(resource, Preprint): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why only exclude Preprints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to the NR, we excluded the preprint. This piece was simply lost during the NR.
osf.io/website/notifications/listeners.py
Line 14 in 4c35e47
| subscribe_user_to_notifications(node, node.creator) |
osf.io/website/notifications/utils.py
Line 455 in 68c6d95
| if isinstance(node, Preprint): |
| ) | ||
| except NotificationSubscription.MultipleObjectsReturned: | ||
| pass | ||
| if not isinstance(resource, Preprint): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
| DESK_REQUEST_EXPORT = 'desk_request_export' | ||
| DESK_REQUEST_DEACTIVATION = 'desk_request_deactivation' | ||
| DESK_OSF_SUPPORT_EMAIL = 'desk_osf_support_email' | ||
| DESK_OSF_SUPPORT_EMAIL = 'desk_osf_support_email' # unused same as DESK_CROSSREF_ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, we need another ticket to review all unused types. We found bugs where we used the wrong types which led to the correct ones unused.
| subscription=self, | ||
| event_context=event_context | ||
| event_context=event_context, | ||
| sent=None if self.message_frequency != 'none' else timezone.now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, this is a temporary solution to avoid adding migrations. We need post-release improvements to be able to distinguish between none frequency sent vs actual sent.
1e812ee
into
CenterForOpenScience:feature/notification-refactor-p2-s
Purpose
refactor notification subscriptions and digest handling
Changes
TBD
QA Notes
TBD
Documentation
TBD
Side Effects
TBD
Ticket
https://openscience.atlassian.net/browse/ENG-9762
https://openscience.atlassian.net/browse/ENG-9758