Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43797

User "My Home" records are not being cleaned up on user deletion

    Details

    • Testing Instructions:
      Hide

      To test this, you need to have access to database to make sure that records are being removed.

      1. On fresh Moodle install create a test user (testuser)
      2. Log in as "testuser" and customise the blocks on their My Moodle page
      3. Run this SQL on the database "SELECT * FROM mdl_my_pages;" and the result should look similar to this (where the last record is a page for "testuser" with user id 3):

      id userid name private sortorder
      1   __default 0 0
      2   __default 1 0
      3 3 __default 1 0

      4. Delete "testuser"

      5. Run the SQL again, make sure that the record with userid 3 was removed:

      id userid name private sortorder
      1   __default 0 0
      2   __default 1 0
      Show
      To test this, you need to have access to database to make sure that records are being removed. 1. On fresh Moodle install create a test user (testuser) 2. Log in as "testuser" and customise the blocks on their My Moodle page 3. Run this SQL on the database "SELECT * FROM mdl_my_pages;" and the result should look similar to this (where the last record is a page for "testuser" with user id 3): id userid name private sortorder 1   __default 0 0 2   __default 1 0 3 3 __default 1 0 4. Delete "testuser" 5. Run the SQL again, make sure that the record with userid 3 was removed: id userid name private sortorder 1   __default 0 0 2   __default 1 0
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
      m26_MDL-43797
    • Pull Master Branch:
      master_MDL-43797

      Description

      Removal of all block instances on user pages is being handled by delete_context(CONTEXT_USER, $user->id) when user is deleted. However, if a user had a customised "My Home" page, a record will still remains in database.

      It looks like all we need to do is add:

      $DB->delete_records('my_pages', array('userid' => $user->id, 'private' => 1));

      to delete_user() method. We will submit proposed fix in a bit.

        Gliffy Diagrams

          Activity

          Show
          cibot CiBoT added a comment - Results for MDL-43797 Remote repository: https://github.com/totara/moodle Remote branch m25_ MDL-43797 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/695 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/695/artifact/work/smurf.html Remote branch m26_ MDL-43797 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/696 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/696/artifact/work/smurf.html Remote branch master_ MDL-43797 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/697 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/697/artifact/work/smurf.html
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Yuliya,

          Thank you for providing patch for this issue. The patch looks great.

          Additionally, it might be good to upgrade the table data by deleting rows of previously deleted user.

          Please let me know if you need assistance to push this issue for integration.

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Yuliya, Thank you for providing patch for this issue. The patch looks great. Additionally, it might be good to upgrade the table data by deleting rows of previously deleted user. Please let me know if you need assistance to push this issue for integration.
          Hide
          ybozhko Yuliya Bozhko added a comment -

          Yep, can do that. Was wondering if you needed fixing that too. Will upload additional fix today.

          Show
          ybozhko Yuliya Bozhko added a comment - Yep, can do that. Was wondering if you needed fixing that too. Will upload additional fix today.
          Hide
          ybozhko Yuliya Bozhko added a comment -

          Hi Rossiani,

          Updated branches. Can you please have a look again if everything is in place and I will submit for integration.

          Thanks!

          Yuliya

          Show
          ybozhko Yuliya Bozhko added a comment - Hi Rossiani, Updated branches. Can you please have a look again if everything is in place and I will submit for integration. Thanks! Yuliya
          Show
          cibot CiBoT added a comment - Results for MDL-43797 Remote repository: https://github.com/totara/moodle Remote branch m25_ MDL-43797 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/728 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/728/artifact/work/smurf.html Remote branch m26_ MDL-43797 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/729 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/729/artifact/work/smurf.html Remote branch master_ MDL-43797 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/730 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/730/artifact/work/smurf.html
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Yuliya,

          Patch looks fine.

          Please feel free to submit it for integration review.

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Yuliya, Patch looks fine. Please feel free to submit it for integration review.
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Sorry, I'm reopening this. I think the upgrade code is really wrong:

          1) Is it deleting the records for all the users with deleted = 0 ? Seems those are the active ones, not the deleted ones.

          2) No way that IN construction can work against million of users, not we can iterate over all them 1 by 1. Surely it can be executed with a raw delete + select sql, not needed to load/prepare anything in the php side. Something like this DELETE + SELECT may do the trick 100% in the SQL side. OR here there is another example. Those are the preferred way to delete orphaned data.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Sorry, I'm reopening this. I think the upgrade code is really wrong: 1) Is it deleting the records for all the users with deleted = 0 ? Seems those are the active ones, not the deleted ones. 2) No way that IN construction can work against million of users, not we can iterate over all them 1 by 1. Surely it can be executed with a raw delete + select sql, not needed to load/prepare anything in the php side. Something like this DELETE + SELECT may do the trick 100% in the SQL side. OR here there is another example . Those are the preferred way to delete orphaned data. Ciao
          Hide
          cibot CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          ybozhko Yuliya Bozhko added a comment -

          Hi Eloy,

          It selects correct users because the last parameter in get_in_or_equal() is false, but I can see that you mean by optimizing the code. We will take a look again. Thanks for the feedback!

          Yuliya

          Show
          ybozhko Yuliya Bozhko added a comment - Hi Eloy, It selects correct users because the last parameter in get_in_or_equal() is false , but I can see that you mean by optimizing the code. We will take a look again. Thanks for the feedback! Yuliya
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Thanks! (didn't realise the false)

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Thanks! (didn't realise the false)
          Hide
          ybozhko Yuliya Bozhko added a comment -

          Branches updated and rebased. Ready to be reviewed when you have time.

          Thanks!

          Show
          ybozhko Yuliya Bozhko added a comment - Branches updated and rebased. Ready to be reviewed when you have time. Thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Hi Yuliya,

          from memory, I don't remember if it's MySQL or Postgres (I think it's MySQL), but it sounds to me a lot that aliases are NOT allowed for the DELETE clause. So, in order to make the SQL 100% cross-db, you should replace all those "p." by "{my_pages}." and delete the alias from the DELETE clause. The second example linked above shows it.

          Edited: s/now/NOT

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Hi Yuliya, from memory, I don't remember if it's MySQL or Postgres (I think it's MySQL), but it sounds to me a lot that aliases are NOT allowed for the DELETE clause. So, in order to make the SQL 100% cross-db, you should replace all those "p." by "{my_pages}." and delete the alias from the DELETE clause. The second example linked above shows it. Edited: s/now/NOT
          Hide
          ybozhko Yuliya Bozhko added a comment -

          Probably MySQL, because we tested it on Postgres.

          Show
          ybozhko Yuliya Bozhko added a comment - Probably MySQL, because we tested it on Postgres.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          (yes, confirmed, mysql breaks with: "ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'p ....')

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - (yes, confirmed, mysql breaks with: "ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'p ....')
          Hide
          ybozhko Yuliya Bozhko added a comment -

          Thanks! Just updated commits, so should be better now.

          Show
          ybozhko Yuliya Bozhko added a comment - Thanks! Just updated commits, so should be better now.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Note the the "user => u" alias was ok, afaik, it's a normal SELECT, but it doesn't harm to have everything un-aliased... going to put this again in the integration bag, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Note the the "user => u" alias was ok, afaik, it's a normal SELECT, but it doesn't harm to have everything un-aliased... going to put this again in the integration bag, thanks!
          Hide
          ybozhko Yuliya Bozhko added a comment -

          Thanks, Eloy! It's always good to learn something new

          Show
          ybozhko Yuliya Bozhko added a comment - Thanks, Eloy! It's always good to learn something new
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (25, 26 & master), thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (25, 26 & master), thanks!
          Hide
          salvetore Michael de Raadt added a comment -

          Test result: Success!

          Tested in 2.5, 2.6 and master. Tested using PostgreSQL.

          My page records are removed for deleted users.

          Show
          salvetore Michael de Raadt added a comment - Test result: Success! Tested in 2.5, 2.6 and master. Tested using PostgreSQL. My page records are removed for deleted users.
          Hide
          marina Marina Glancy added a comment -

          Thanks for your hard work. Your code has now become a part of Moodle!

          Show
          marina Marina Glancy added a comment - Thanks for your hard work. Your code has now become a part of Moodle!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Mar/14