Notification: cancel notifications automatically

[humitos]

Closes #10981

[ericholscher]

This is a really nice UX improvement, but I'm a little worried about performance. It might not be an issue, but I can see our Notifications table getting pretty huge, so might be worth considering, or at least tracking on deploy.

[ericholscher]

Seems this we're going to be calling this query a lot on projects that were never skipped. We don't save projects super frequently, but it seems less than ideal to add overhead like this for very rare outcomes.

Can we cancel the notification on project same if the skip field has been changed or similar?

[humitos]

Can we cancel the notification on project same if the skip field has been changed or similar?

I'm not sure how we can detect this. pre_save and post_save signals receive a update_fields (https://docs.djangoproject.com/en/5.0/ref/signals/#post-save) but it's only passed when using .save(update_fields=[...]), which we don't use.

I remember we were checking if some field was updated in a form and perform something in particular, but I'm not finding it. However, that approach won't be useful here either because this won't work when the field is updated outside that particular form (Django Admin or Django Shell).

Do you an idea in mind here?

[ericholscher]

I think this approach is fine for now, we just need to monitor performance. It's likely not a real issue.

[ericholscher]

This could get pretty slow if we have a huge Notifications table. I assume we have an index on some subset of these that hopefully makes it faster?

[humitos]

We have an index on state that I think will work fine here. We should probably create an index_together on attached_to_content_type and attached_to_id, which is something we will use a lot: https://docs.djangoproject.com/en/5.0/ref/models/options/#index-together

[humitos]

I went ahead and created a PR for this at #11050

[humitos]

@ericholscher thanks for the review! It helped me to realized that I was executing the query for verified email on each logged in request as well. I moved that to a Django signal attached to EmailAddress. Besides, I added some extra test cases 💯

[ericholscher]

This is called every time a Email, Organization, Project are saved. It seems we're depending on add to not keep adding multiple notifications, and de-dupe things smartly. Is it safe to make that assumption? In particular, if they dismiss it, we won't try to re-add it right?

[humitos]

Yes, that assumption is correct. That's how .add() should work. If it's not working like that, it's a bug and we should fix it.

If they dismiss it, we will add a new one, yes. We won't add a new one only if there is already one in READ or UNREAD state only:

readthedocs.org/readthedocs/notifications/querysets.py

Lines 26 to 43 in 444bd6f

	notification = self.filter(
	attached_to_content_type=content_type,
	attached_to_id=attached_to.id,
	message_id=message_id,
	# Update only ``READ`` and ``UNREAD`` notifications because we want
	# to keep track of ``DISMISSED`` and ``CANCELLED`` ones.
	state__in=(UNREAD, READ),
	).first()
	if notification:
	self.filter(pk=notification.pk).update(
	*args,
	modified=timezone.now(),
	state=UNREAD,
	**kwargs,
	)
	notification.refresh_from_db()
	return notification