Moodle

Also related is whether a user should be able to view messages of deleted users. From MoodleHQ chat:

mathieu: what happens to messages received (and read) from a user that's now deleted? can they be accessed?
(10:02:26 PM) mathieu: I suppose we should make both case do the same
(10:03:27 PM) arborrow: I'm not sure, I'd suspect not
(10:06:37 PM) arborrow: actually if you put in the URL for /message/history.php you can get the history as it is using a get_record("user", "id", $userid1)
(10:06:51 PM) arborrow: we could add a check to see if the user has been deleted
(10:07:14 PM) arborrow: and display a message indicating such

Following on the same issue (even though this is a different feature), when deleting a user, there could be an option to delete the messages as well. It should not be the default, but with spammers, it would at least make it possible to not keep an eternal backup of spams in the database.

Eloy - Here is a patch to implement moving the unread messages from message to message_read for users that are being deleted. Since AFAIK there is no way to restore a deleted user via the Moodle interface these messages would remain in message_read until the DBA moved them manually. We could probably create some documentation around this if needed. At least we are being consistent though and not losing or removing any data. Also in the patch, I added a check to history.php to see if the person is trying to view the history between themselves and a deleted user or if an admin between two users in which one of them is deleted. The data is still in the message_read table; however, it is marked with a timeread of 0. This seems to be consistent with how messages were being handled elsewhere. Let me know if you have any questions. Peace - Anthony

Perhaps to ensure when a user is restored from a CSV update, we should add some checks to /admin/uploaduser.php around line 470 to check:

if the $column is deleted then move messages from message_read back to message

// move all unread messages from message_read table to message
if ($messages = get_records_select('message_read', "useridfrom = $user->id", 'timecreated')) {
foreach ($messages as $message) {
$message = addslashes_object($message);
$messageid = $message->id;
unset($message->id);
unset($message->timeread);
if (insert_record('message', $message)) { delete_records('message_read', 'id', $messageid); }
}
}

If we do that I think that would make this a reasonably complete fix but I'm going to bed so it will have to wait for another. Peace - Anthony

Mathieu - I would prefer to see us be consistent about not deleting data when deleting a user; however, perhaps we could add a delete spammer option which would go through and delete data. Peace - Anthony

In looking at this issue, I noticed that the delete_user function stores the email and time of deletion as the new username. This strikes me as dataloss of the original username. I would think that we should store that information somewhere. I understand the rationale to free up the email and the username; however, we should be able to do a restore. FYI, currently as I understand the logic in processing the CSV files, there is no way to undelete a user since the deleted field is unset. As such, the ability to restore the messages is not critical; however, we could add the capability to restore a user but then we have to deal with how to restore the username. If the username field were changed to something like $username.'.'.$email.'.'.$time that is the concatenation of the username, email, and time we would be OK; however, then we are going to run into issues of long usernames or emails getting truncated. I think the best place to put the data is the description field where we could prepend it during the deletion and retrieve it during the undeletion and remove it from the description field. Of course, all of this assumes that we want to allow undeleting with CSV files and what we do with CSV files we should be able to do (IMHO) via the UI. I've asked for a little input from MoodleHQ chat on this issue. Peace - Anthony

Eloy - Here is an initial stab at a patch for what I'm thinking for the upload user file. I ran into the issue with the username being changed by the delete_user function but the basic functionality here is that it would restore the user and move the unread emails back from message_read to message. Use it in so far as it it helpful. Peace - Anthony

Sorry to be the newbie here, but what do I do with this patch?

Peter - To be honest, I would ignore the patch and not do anything with it. It was added as a way of showing how I might approach the issue and to possibly help Eloy come up with some ideas. If you are not able to apply a patch on a test installation and test it then I would be very hesitant about using it. That being said, if you are still interested in it I do not want to appear to be 'elitist' as in only those who really know about these things can make use of them so if you want patched versions of the files send me an email but please use with due prudence. Peace - Anthony

Increasing the priority of this issue, and adding a fix version (being hopeful!) as lots of people are reporting problems.

Helen - Perhaps at the very least, to prevent folks from experiencing what Mathieu was with no way of being able to stop the empty message popup from appearing, Eloy may want to implement the first part of the patch I provided to move all the messages of a deleted user to read messages table. Then we can come back later and work on how to handle those messages is the user is undeleted. In the interim we could provide instructions for admins to manually move the messages back after un-deleting but I suspect the number of request for that will be few and far between. The main problem seems to be creating the orphaned unread messages. Peace - Anthony

Hi Anthony,

The logic in your comments looks perfect.

1) Move messages to message_read when user is deleted.
2) Move them back to message when user is "revived" (knowing that right now, that "reviving" option isn't possible at all because of the lost username problem.

I'd add one more step:

3) Add one upgrade step in order to perform the (1) move sitewide.

I've been looking to your proposed patches. And they look 99% fine.

In 1) you are storing those messages with timeread=0, perfect. Also, any attempt to see a discussion with one deleted user will end with a "user deleted" error message. Perfect too.

In 2) when "reviving" users (although, once more, that's not possible right now), it seems that, when selecting messages to be moved from mesage_read back to message, you are missing one condition, the "and timeread = 0". Else ALL read messages from the user will be moved to message table. And we don't want that IMO.

About 3) it's missing and I think it could be interesting to have it in order to clean current repetitive messages. It's basically 1) but iterating over all deleted users having messages in message table. Can you do that? Or should I try it?

TIA and ciao

Eloy - Classes are starting back up again full force so I'll be pressed for time. While it is tempting to go in and clean up the patch, I should probably resist that urge and remain focused on my studies at this point. Perhaps around the end of November I'll have time to play with code again. My intention with the patch was simply to lay out a possible path to resolving it so if you can get to it before the end of November by all means feel free. We definitely want to only move the message_read to message where timeread=0 - leaving that out was an oversight but what I was intending. I agree that checking for any orphaned messages would be good. My question is what do we want to use to trigger the check? I'm guessing that a version check would make the most sense so that if the site is being upgraded, then check. Peace - Anthony

Eloy - OK, I added the the 'AND timeread = 0' condition to prevent from moving all messages back for undeleted users. I also created a function to facilitate the processing in upgrade.php. So now we should be able to help clean up a site that has orphaned messages. I did very minimal testing with this so please give it a run before committing. I saw another recent post on this and then Helen's comment on increasing the priority so I wanted to get something submitted. Peace - Anthony

Eloy - Just an FYI, I forgot to include the version.php file in my diff but that should be pretty simple especially considering that I am hoping this will be part of 1.9.5 release. Peace - Anthony

I've moved function to /message/lib.php and improved a bit the upgrade (to save some queries).

I've tested upgrade, user deletion and messaging interface. All worked as expected.

It's pending the uploaduser.php script that is under revision right now.

Ciao

About code in uploaduser.php:

1) Deleted users have lost their usernames (as part of the normal delete_user() process).
2) uploaduser.php looks for existing users by username.

Obviously, this causes that uploaduser.php won't ever match one deleted user... so it won't be able to revive a user at all. Said that, it hasn't sense at all to revive messages for those users because we'll be creating orphaned messages again.

If someday uploaduser.php is able to revive users successfully... then their messages will be recoverable too. Until then, IMO, it's better not to introduce the revival of messages, so I'm not going to apply that part of the patch.

Ciao

Resolving this as fixed. Thanks a lot Anthony!

As reference, MDL-16838 is the bug where the revival of users is being discussed. Fell free to vote/watch/comment there...ciao

nice, thanks