Opened 7 years ago

Last modified 6 years ago

#5544 review defect

Orphaned notifications

Reported by: ayleph Owned by:
Priority: critical Milestone:
Component: programming Keywords: notifications
Cc: Parent Tickets:


My database contains a number of orphaned notifications that result in server errors with the following output.

File '/path/to/mediagoblin/mediagoblin/templates/mediagoblin/fragments/header_notifications.html', line 8 in top-level template code
  {% set comment_object = comment_wrapper.comment() %}
UndefinedError: 'None' has no attribute 'comment'

For users that want notifications, this results in server errors every time they log in or try to navigate to any page while logged in, effectively locking them out of the instance.

Change History (3)

comment:1 by ayleph, 7 years ago

Here's a high level overview of my tables.

I have 1300 rows in core__comment_links.

  • 1209 rows have an id that matches the obj_pk from core__generic_model_reference.
  • 91 rows have an id that does not exist in core__generic_model_reference.

I have 1428 rows in core__generic_model_reference with model_type of core__comment_links.

  • 1209 rows have an obj_pk that matches the id from core__comment_links, as above.
  • 219 rows have an obj_pk that does not exist in core__comment_links.

I have 45 rows in core__notifications that reference one of the 219 rows in core__generic_model_reference which does not have a corresponding entry in core__comment_links.

This leads me to believe that when comments are deleted, they are removed from the core__comment_links table but not from the core__generic_model_reference and core__notifications tables, resulting in orphaned notifications.

(As for the 91 comments that don't exist in GMR, I haven't dug deep enough to determine what caused those, but my first guess would be those existed before we switched to the new comment model.)

comment:2 by ayleph, 7 years ago

I can use the following band-aid to prevent this from throwing a server error. It still shows a notification count and the "mark all read" link, even though it might not show any notifications, but that's better than crashing. I'm going to apply this to my local instance while I continue to investigate. It might not be a bad idea to apply this patch to the codebase too, just as an extra layer of error checking.

--- a/mediagoblin/templates/mediagoblin/fragments/header_notifications.html
+++ b/mediagoblin/templates/mediagoblin/fragments/header_notifications.html
@@ -4,6 +4,7 @@
     <h3>{% trans %}New comments{% endtrans %}</h3>
         {% for notification in  notifications %}
+        {% if notification.obj() %}
         {% set comment_wrapper = notification.obj() %}
         {% set comment_object = comment_wrapper.comment() %}
         {% set comment_author = comment_object.get_actor %}
@@ -35,6 +36,7 @@
+        {% endif %}
         {% endfor %}
     <a href="{{ request.urlgen('mediagoblin.notifications.mark_all_comment_notifications_seen') }}?next={{

comment:3 by ShawnRisk, 6 years ago

Status: newreview

This should be made into a patch.

Note: See TracTickets for help on using tickets.