Mark orphaned alerts as read

[sbulen]

Partial for #7423

Unread alerts are not purged. Alerts that point to messages that have been deleted or moved to boards that the user does not have visibility to, are not displayed.

So... These unread alerts will stay around forever as invisible orphans... (Although a nit, this really bugs me...)

This can be easily detected during the count function, which is frequently invoked, so this PR cleans them up by marking them as read.

An alternate approach would be to change the purge function to purge alerts whether or not they have been read...

[sbulen]

OK, after running this on my site for a while, I think there are two issues with it:

1. It should delete, not mark unread. The alerts themselves point to invalid messages for that user, so they'll get ugly messages if they click on them.
2. It doesn't update the member count to reflect the change in status, so it can knock the count out of sync (e.g., say there's 1 unread but there are none). Although this seems like an easy fix via a call to updateMemberData(), the problem is that updateMemberData() calls this function, causing a loop...

So...

Either I need to update the member record directly via sql, OR, replicate a big chunk of alert_count() elsewhere... I'm leaning towards the former... Input welcome.

[live627]

. I'm leaning towards the former

That seems to be quick and easy, the better solution here. And would probably result in fewer operations for the patch, a win in my book.

[sbulen]

Change made.

[sbulen]

Found another tweak needed while testing... $user_info isn't populated when this is invoked via cron, so don't use it...

[Sesquipedalian]

A followup question for @sbulen:

Since this PR added code to update the value of the alerts field in the member table every time the alert_count() function is called, is there any reason not to make the following change to the updateMemberData() function?

Current: https://github.com/SimpleMachines/SMF/blob/aee494fe17322c24c2076cc084b3f4559e61db5b/Sources/Subs.php#L375-L390

New:

		if ($var == 'alerts' && ($val === '+' || $val === '-'))
		{
			include_once($sourcedir . '/Profile-Modify.php');

			foreach ((array) $members as $k => $v)
				alert_count($v, true);

			continue;
		}

It seems like setting the new value again further down in updateMemberData() is redundant, since we just did it in alert_count(). Is there something I am missing here, or would this be a good change for a further PR?

[sbulen]

It seems like setting the new value again further down in updateMemberData() is redundant, since we just did it in alert_count(). Is there something I am missing here, or would this be a good change for a further PR?

Sorry, missed this question earlier.

This is noted above in my 2nd comment above. Since updateMemberData() in turn calls this function, it causes a tight loop.

Trust me... It's not a good thing...

2. It doesn't update the member count to reflect the change in status, so it can knock the count out of sync (e.g., say there's 1 unread but there are none). Although this seems like an easy fix via a call to updateMemberData(), the problem is that updateMemberData() calls this function, causing a loop...