SMF
SMF copied to clipboard
SimpleMachines
Delete unread alerts upon an unlike
Partial for #7423
Just as it says, if there is an unread alert for a like, and the user changes their mind and unlikes it, get rid of the alert & adjust the recipient's alert counter.
Well, you have two choices here and I've seen both of them. For me I actually think it's better to leave the alert when I read it even though the like is gone, I would realize when I visit the post again, but I don't know if this is the same for the general user.
The problem I see here (and this would apply to any function that automatically deletes alerts) is that the alerts are created by a background task, whereas this code deletes the alert immediately. This creates a race condition where the attempt to delete the alert could easily occur before the alert has been created. In particular, if the user rapidly clicks "Like" and then "Unlike", the attempt to delete the alert might happen before the background task to create it runs.
To avoid this problem, the delete also needs to happen via a background task.
It cannot do so before the alert is created - because it queries the alerts above, and only deletes it if it exists already...
Sure, but then when the background task runs, the alert will be created after it was supposed to be deleted.
Put simply, since alerts are created via the background tasks queue, they also need to be deleted via the background tasks queue. That will ensure that the operations happen in the correct chronological order.
If you have already started to work on code to do the deletes via background tasks, then I will leave you to it, @sbulen. But if you'd prefer, I can take a shot at it instead. I've got an initial version for deleting obsolete unread alerts about likes that seems to work pretty well, and I should be able to implement similar functionality for other alert types fairly quickly.
Yes, it is conceivable that the cleanup is attempted before the alert is posted (mainly if you're having system issues & cron isn't functioning). But that is such a narrow possibility, and the downside of that happening is so negligible, that I think we should stay with what we have here.
I modeled these PRs after pre-existing logic used elsewhere in SMF for cleaning up alerts, e.g., the cleanup after quote/mention removal logic.
You wrote that logic, emulated here.
For moderation activity and approvals, there will be sufficient time between those actions this gap should be virtually non-existent (again, unless there are system issues). For likes, especially if someone sits there twiddling the like link incessantly, there may be a discrepancy. But, it should be noted, that there was, in fact, a like performed. It's not inaccurate.
For 0.01% of the time, a once valid alert may remain after the fact. But the truth remains it was once valid.
This is not an accounting application with millions of transactions & critical sequencing. If so, I agree a rewrite of all of these functions would be necessary.
Now is not the time for a redesign. It is unnecessary.
These PRs had extensive testing before they were submitted, and have been executing in an active production environment for months after that with no issues.