Moodle
  1. Moodle
  2. MDL-17065

No easy way to remove pictures of deleted users

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.3
    • Fix Version/s: 1.8.8, 1.9.4
    • Component/s: Administration
    • Labels:
      None
    • Rank:
      30295

      Description

      If a user account has been deleted, their picture remains attached to forum posts. However, if the admin decides that they don't like the picture (say because it is offensive) there is now no way to delete or edit it.

      Why do profiles of deleted users simply not remain editable to admins? There could even be a simple (and useful) undelete function!

      1. f1.png
        5 kB
      2. f2.png
        1 kB

        Issue Links

          Activity

          Hide
          Howard Miller added a comment -

          It's worse than that. If you have a link to the picture via pix.php then it's viewable by anyone. Surely, pix.php should check if the picture associated with a user is "deleted" when accessed by guest users?

          Show
          Howard Miller added a comment - It's worse than that. If you have a link to the picture via pix.php then it's viewable by anyone . Surely, pix.php should check if the picture associated with a user is "deleted" when accessed by guest users?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Assigning to Petr. I agree with Howard about pictures of deleted users not being accessible anymore:

          1) at least from guests.
          2) perhaps from everybody.

          Not sure if that implies deleting the picture or just detecting it and sending the "default" image for deleted users.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Assigning to Petr. I agree with Howard about pictures of deleted users not being accessible anymore: 1) at least from guests. 2) perhaps from everybody. Not sure if that implies deleting the picture or just detecting it and sending the "default" image for deleted users. Ciao
          Hide
          Anthony Borrow added a comment -

          I wonder if instead of actually deleting the picture instead we just do a check and if it is a deleted picture then use the generic smiley face or perhaps even a grayed out smiley face to indicate that it is a deleted user. It could be as simple as using grayscale images (which I'll attach) or overlaying 'Deleted User' or something over the image. I think our general approach of not actually removing data is good as it does leave the door open should we want at some point to implement a restore/undelete user option via the UI. Peace - Anthony

          Show
          Anthony Borrow added a comment - I wonder if instead of actually deleting the picture instead we just do a check and if it is a deleted picture then use the generic smiley face or perhaps even a grayed out smiley face to indicate that it is a deleted user. It could be as simple as using grayscale images (which I'll attach) or overlaying 'Deleted User' or something over the image. I think our general approach of not actually removing data is good as it does leave the door open should we want at some point to implement a restore/undelete user option via the UI. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          ATW was asking about voting on this issue. I'm guessing that because it is a security issue that normal users cannot vote on it. Peace - Anthony

          Show
          Anthony Borrow added a comment - ATW was asking about voting on this issue. I'm guessing that because it is a security issue that normal users cannot vote on it. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          here are the grayscaled f1 and f2 files

          Show
          Anthony Borrow added a comment - here are the grayscaled f1 and f2 files
          Hide
          Anthony Borrow added a comment -

          Just a quick follow up, my idea would be to make a small modification to the print_user_picture function in /lib/weblib.php to check if the user is deleted and if so to use the deleted user image files.

          Show
          Anthony Borrow added a comment - Just a quick follow up, my idea would be to make a small modification to the print_user_picture function in /lib/weblib.php to check if the user is deleted and if so to use the deleted user image files.
          Hide
          Petr Škoda added a comment -

          I have added a test into user/pix.php to validate that user is not deleted, if yes default image is used.
          Changing print_user_picture might not be enough because the images are also stored in emails

          There is no "undelete user" feature planned (either manual or automatic) and I would strongly oppose it, if you really want it you can use db editor or some custom script.
          Instead we should have easy to use "user suspend" and proper user delete+purge - we must clean up the old data because it is becoming a real performance problem.

          I am planning to rework the profile image handling in 2.0 - I suppose it will use the same trick as SCORM to work around the caching problems, I agree the gray images is a good idea (+1 for HEAD).

          Show
          Petr Škoda added a comment - I have added a test into user/pix.php to validate that user is not deleted, if yes default image is used. Changing print_user_picture might not be enough because the images are also stored in emails There is no "undelete user" feature planned (either manual or automatic) and I would strongly oppose it, if you really want it you can use db editor or some custom script. Instead we should have easy to use "user suspend" and proper user delete+purge - we must clean up the old data because it is becoming a real performance problem. I am planning to rework the profile image handling in 2.0 - I suppose it will use the same trick as SCORM to work around the caching problems, I agree the gray images is a good idea (+1 for HEAD).
          Hide
          Howard Miller added a comment -

          I agree with you, Petr. However, the underlying problem is that delete was never actually delete at all. It creates an expectation that is not true. Simply renaming it to suspend would have helped.

          Show
          Howard Miller added a comment - I agree with you, Petr. However, the underlying problem is that delete was never actually delete at all. It creates an expectation that is not true. Simply renaming it to suspend would have helped.
          Hide
          Petr Škoda added a comment -

          well, there is a big difference between delete and suspend
          1/ delete should delete stuff that is not needed like assignment submissions (no more user access expected) + purge should remove all traces of users including posts, etc.
          2/ suspend should just prevent logins while keeping ALL user data and preventing creation of account with the same email/username

          I believe that we need enrolment suspend and proper delete too. It is very similar to this problem imho.

          Show
          Petr Škoda added a comment - well, there is a big difference between delete and suspend 1/ delete should delete stuff that is not needed like assignment submissions (no more user access expected) + purge should remove all traces of users including posts, etc. 2/ suspend should just prevent logins while keeping ALL user data and preventing creation of account with the same email/username I believe that we need enrolment suspend and proper delete too. It is very similar to this problem imho.
          Hide
          Helen Foster added a comment -

          Thanks for reporting this issue Howard, thanks for everyone's comments, and thanks for fixing it Petr

          Show
          Helen Foster added a comment - Thanks for reporting this issue Howard, thanks for everyone's comments, and thanks for fixing it Petr
          Hide
          Martin Dougiamas added a comment -

          I'm removing security status to open this up.

          Show
          Martin Dougiamas added a comment - I'm removing security status to open this up.

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: