Cleanup approval alerts after approvals

[sbulen]

Fixes #7423 (last one)

Cleanup alerts that posts need approval after posts have been approved.

The wrinkle here was that the post alerts were not identified by msg ids, but by topic ids, which made it impossible to identify the specific alerts to delete for that message. This required changing the content of the alert a bit, which in turn, required changing the key of the language string, as it is dynamically built.

[Sesquipedalian]

My comment here also applies to this PR, I'm afraid. 🙁

[sbulen]

As in the others - this cannot attempt a delete before the alert exists - because it's driven by a query of existing alerts.

[Sesquipedalian]

See #7516 (comment) 🙂

[sbulen]

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.

[Sesquipedalian]

Why the change to content_type for unapproved replies? I agree that it would make better sense for it to be msg rather than topic in such cases, but changing this part of the current behaviour doesn't seem strictly necessary to make this code work. Am I missing something?

[Sesquipedalian]

I ask because I'd rather not force the translators to redo the $txt string if we can avoid it.

[sbulen]

It's necessary to properly identify the alert. A "topic reply" that only ids the topic isn't specific enough to uniquely the message that was approved.

Suzie might have 2 unapproved posts in topic 555. George may have approved post 1 and Randy approved post 2. Both posts just put alerts out there for topic 555 - nothing message specific.

It's conceivable to find the alert by doing a preg match against the url stuffed in there... But that's a hackish workaround.

In all other alerts, the content ids uniquely id the record. Here, he messages are flagged as topic replies.

Note I only fixed the ones I had to here. There are other alerts that use the wrong content identifiers. All post & post approval related. Sometimes the content points to topics vs messages; sometimes it labeled as a board when the id is a topic id, etc. But I only cleaned up what I absolutely had to for this task.

So... Existing alerts at the time this goes in may not be cleaned properly, since they are not uniquely identified.

The "Mark read" link fixes everything, and resets the counters. It's magic....

[Sesquipedalian]

Makes sense. Thanks.

[sbulen]

I believe this one is ready now.