diff --git a/remarkbox/lib/notify.py b/remarkbox/lib/notify.py index 0685be6..751f91b 100644 --- a/remarkbox/lib/notify.py +++ b/remarkbox/lib/notify.py @@ -151,6 +151,37 @@ def get_email_notifications(dbsession, frequency): return notification_dict +def filter_orphaned_notifications(notification_dict): + """ + Filter out notifications with null node_event from the notification dictionary. + + Args: + notification_dict: Dictionary mapping user_id to list of notifications + + Returns: + dict: Filtered notification dictionary with orphaned notifications removed + """ + filtered_dict = {} + total_orphaned = 0 + + for user_id, notifications in notification_dict.items(): + valid_notifications = [] + for notification in notifications: + if notification.node_event is None: + total_orphaned += 1 + log.warning(f"Skipping orphaned notification {notification.id} for user {user_id}") + else: + valid_notifications.append(notification) + + if valid_notifications: + filtered_dict[user_id] = valid_notifications + + if total_orphaned > 0: + log.warning(f"Filtered out {total_orphaned} orphaned notifications") + + return filtered_dict + + def deliver_scheduled_notifications(request=None): from datetime import datetime from pyramid.scripting import prepare @@ -160,12 +191,14 @@ def deliver_scheduled_notifications(request=None): request = env["request"] notification_dict = get_email_notifications(request.dbsession, "daily") - send_digest_notifications(request, notification_dict, "daily") + filtered_dict = filter_orphaned_notifications(notification_dict) + send_digest_notifications(request, filtered_dict, "daily") # Send weekly on Monday. if datetime.today().weekday() == 0: notification_dict = get_email_notifications(request.dbsession, "weekly") - send_digest_notifications(request, notification_dict, "weekly") + filtered_dict = filter_orphaned_notifications(notification_dict) + send_digest_notifications(request, filtered_dict, "weekly") def send_immediate_notifications(request, notifications): @@ -210,10 +243,14 @@ def group_notifications_by_root(notifications): """ group notifications by root node, where the key is the root node and the value is a list of notification objects. + Skips orphaned notifications (where node_event is None). """ groups = defaultdict(list) for notification in notifications: - groups[notification.node_event.node.root].append(notification) + if notification.node_event is not None: + groups[notification.node_event.node.root].append(notification) + else: + log.warning(f"Skipping orphaned notification {notification.id} in group_notifications_by_root") return groups diff --git a/remarkbox/scripts/delete_disabled_nodes.py b/remarkbox/scripts/delete_disabled_nodes.py index a5c686e..1fcfb18 100644 --- a/remarkbox/scripts/delete_disabled_nodes.py +++ b/remarkbox/scripts/delete_disabled_nodes.py @@ -4,6 +4,7 @@ import transaction import logging from pyramid.paster import bootstrap, setup_logging from ..models import Node, get_tm_session, now_timestamp +from ..models.notification import NodeEventNotification from . import base_parser @@ -29,10 +30,40 @@ def is_node_anonymized(node): ) +def cleanup_orphaned_notifications(dbsession): + """ + Clean up any existing orphaned NodeEventNotification records. + + Args: + dbsession: Database session + + Returns: + int: Number of orphaned notifications cleaned up + """ + # Find notifications with null node_event references + orphaned = ( + dbsession.query(NodeEventNotification) + .filter(NodeEventNotification.node_event_id.is_(None)) + .all() + ) + + count = len(orphaned) + if count > 0: + print(f"Found {count} orphaned notifications, cleaning them up...") + for notification in orphaned: + dbsession.delete(notification) + print(f"Cleaned up {count} orphaned notifications") + else: + print("No orphaned notifications found") + + return count + + def delete_disabled_nodes(request): """ Delete disabled leaf nodes and anonymize disabled parent nodes while preserving children. Skips re-anonymizing already anonymized parent nodes. + Also cleans up any existing orphaned notifications. Args: request: Pyramid request object with transaction manager @@ -46,6 +77,10 @@ def delete_disabled_nodes(request): request.registry["dbsession_factory"], transaction.manager ) + # First, clean up any existing orphaned notifications + print("Checking for orphaned notifications...") + orphaned_count = cleanup_orphaned_notifications(dbsession) + # Fetch all disabled nodes in one query disabled_nodes = dbsession.query(Node).filter(Node.disabled == True).all() print(f"Found {len(disabled_nodes)} disabled nodes.") @@ -92,8 +127,16 @@ def delete_disabled_nodes(request): anonymized_count += 1 else: # Delete leaf node and its related data - # Handle events deletion + # Handle events deletion and cleanup associated notifications for event in node.events: + # Delete all notifications associated with this event + notifications = dbsession.query(NodeEventNotification).filter( + NodeEventNotification.node_event_id == event.id + ).all() + for notification in notifications: + dbsession.delete(notification) + + # Delete the event itself dbsession.delete(event) # Handle watchers deletion with a loop @@ -123,7 +166,7 @@ def delete_disabled_nodes(request): # Print summary of operations print( - f"Summary: Deleted {deleted_count} leaf nodes, anonymized {anonymized_count} parent nodes with children, skipped {skipped_count} already anonymized nodes" + f"Summary: Cleaned up {orphaned_count} orphaned notifications, deleted {deleted_count} leaf nodes, anonymized {anonymized_count} parent nodes with children, skipped {skipped_count} already anonymized nodes" ) if remaining_disabled_nodes > 0: @@ -150,7 +193,7 @@ def main(): """ # Set up argument parser with description parser = base_parser( - "Delete disabled leaf nodes and anonymize disabled parent nodes with children." + "Delete disabled leaf nodes, anonymize disabled parent nodes with children, and clean up orphaned notifications." ) args = parser.parse_args() diff --git a/remarkbox/scripts/send_node_digest_notifications.py b/remarkbox/scripts/send_node_digest_notifications.py index cf61eec..e99e851 100644 --- a/remarkbox/scripts/send_node_digest_notifications.py +++ b/remarkbox/scripts/send_node_digest_notifications.py @@ -14,7 +14,7 @@ import logging import transaction from pyramid.paster import bootstrap, setup_logging -from ..lib.notify import deliver_scheduled_notifications +from ..lib.notify import get_email_notifications, send_digest_notifications, filter_orphaned_notifications from ..models import get_tm_session from ..models.notification import NodeEventNotification from ..models.meta import now_timestamp @@ -55,6 +55,32 @@ def should_have_been_sent(notification): return False +def safe_deliver_scheduled_notifications(request, dbsession): + """ + Deliver scheduled notifications with protection against orphaned notifications. + """ + from datetime import datetime + + # Temporarily replace request.dbsession with our transaction-managed session + old_dbsession = request.dbsession + request.dbsession = dbsession + + try: + # Get daily notifications and filter out orphaned ones + notification_dict = get_email_notifications(dbsession, "daily") + filtered_dict = filter_orphaned_notifications(notification_dict) + send_digest_notifications(request, filtered_dict, "daily") + + # Send weekly on Monday + if datetime.today().weekday() == 0: + notification_dict = get_email_notifications(dbsession, "weekly") + filtered_dict = filter_orphaned_notifications(notification_dict) + send_digest_notifications(request, filtered_dict, "weekly") + finally: + # Restore original dbsession + request.dbsession = old_dbsession + + def get_unsent_notification_count(request): """ Get count of unsent notifications using a fresh database session. @@ -77,6 +103,7 @@ def get_unsent_notification_count(request): def mark_ready_notifications_as_sent(request): """ Mark notifications as sent if they were ready to be delivered based on frequency rules. + Only marks non-orphaned notifications (those with valid node_event). Args: request: Pyramid request object @@ -88,36 +115,42 @@ def mark_ready_notifications_as_sent(request): dbsession = get_tm_session( request.registry["dbsession_factory"], transaction.manager ) - + # Get all unsent notifications unsent_notifications = dbsession.query(NodeEventNotification).filter( NodeEventNotification.sent == False ).all() - + total_unsent = len(unsent_notifications) - + if total_unsent == 0: return total_unsent, 0, 0 - + # Only mark notifications that should have been sent based on frequency + # and are not orphaned (have valid node_event) ready_notification_ids = [ - n.id for n in unsent_notifications - if should_have_been_sent(n) + n.id for n in unsent_notifications + if should_have_been_sent(n) and n.node_event is not None ] - + + # Count orphaned notifications separately + orphaned_count = sum(1 for n in unsent_notifications if n.node_event is None) + if orphaned_count > 0: + log.warning(f"Found {orphaned_count} orphaned notifications that will not be marked as sent") + marked_as_sent = 0 if ready_notification_ids: marked_as_sent = dbsession.query(NodeEventNotification).filter( NodeEventNotification.id.in_(ready_notification_ids) ).update({ - 'sent': True, + 'sent': True, 'updated_timestamp': now_timestamp() }, synchronize_session=False) - + # Ensure changes are persisted dbsession.flush() transaction.commit() - + left_for_later = total_unsent - marked_as_sent return total_unsent, marked_as_sent, left_for_later @@ -142,10 +175,14 @@ def main(): log.info("No notifications to process") return - # Deliver scheduled notifications + # Deliver scheduled notifications with orphaned notification protection log.info("Delivering scheduled notifications...") - deliver_scheduled_notifications(request) - transaction.commit() + with transaction.manager: + dbsession = get_tm_session( + request.registry["dbsession_factory"], transaction.manager + ) + safe_deliver_scheduled_notifications(request, dbsession) + transaction.commit() # Check final state and mark ready notifications as sent log.info("Checking for notifications that need to be marked as sent...")