[MDL-43797] User "My Home" records are not being cleaned up on user deletion - Moodle Tracker

Details

Type: Bug
Status: Closed
Priority: Minor
Resolution: Fixed
Affects Version/s: 2.5.4, 2.6.1, 2.7
Fix Version/s: 2.5.5, 2.6.2
Component/s: Dashboard (My home), Database SQL/XMLDB
Labels:
- triaged

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:
https://github.com/totara/moodle
Pull Master Branch:
master_~~MDL-43797~~
Pull Master Diff URL:
https://github.com/totara/moodle/compare/9788e26805cdb59d77e28faaa006644f157a7e05...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

Attachments

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

CiBoT added a comment - 22/Jan/14 4:28 AM

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

Show

CiBoT added a comment - 22/Jan/14 4:28 AM 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

Permalink

Rossiani Wijaya added a comment - 22/Jan/14 6:14 AM

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

Rossiani Wijaya added a comment - 22/Jan/14 6:14 AM 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

Permalink

Yuliya Bozhko added a comment - 22/Jan/14 8:47 PM

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

Show

Yuliya Bozhko added a comment - 22/Jan/14 8:47 PM Yep, can do that. Was wondering if you needed fixing that too. Will upload additional fix today.

Hide

Permalink

Yuliya Bozhko added a comment - 22/Jan/14 11:05 PM

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

Yuliya Bozhko added a comment - 22/Jan/14 11:05 PM Hi Rossiani, Updated branches. Can you please have a look again if everything is in place and I will submit for integration. Thanks! Yuliya

Hide

Permalink

CiBoT added a comment - 22/Jan/14 11:29 PM

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

Show

CiBoT added a comment - 22/Jan/14 11:29 PM 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

Permalink

Rossiani Wijaya added a comment - 23/Jan/14 6:58 AM

Hi Yuliya,

Patch looks fine.

Please feel free to submit it for integration review.

Show

Rossiani Wijaya added a comment - 23/Jan/14 6:58 AM Hi Yuliya, Patch looks fine. Please feel free to submit it for integration review.

Hide

Permalink

CiBoT added a comment - 26/Jan/14 7:13 PM

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

Show

CiBoT added a comment - 26/Jan/14 7:13 PM Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 27/Jan/14 4:38 PM - 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

Eloy Lafuente (stronk7) added a comment - 27/Jan/14 4:38 PM - 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

Permalink

CiBoT added a comment - 27/Jan/14 6:22 PM

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

Show

CiBoT added a comment - 27/Jan/14 6:22 PM Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Hide

Permalink

Yuliya Bozhko added a comment - 27/Jan/14 8:58 PM

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

Yuliya Bozhko added a comment - 27/Jan/14 8:58 PM 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

Permalink

Eloy Lafuente (stronk7) added a comment - 27/Jan/14 11:48 PM

Thanks! (didn't realise the false)

Show

Eloy Lafuente (stronk7) added a comment - 27/Jan/14 11:48 PM Thanks! (didn't realise the false)

Hide

Permalink

Yuliya Bozhko added a comment - 28/Jan/14 12:05 AM

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

Thanks!

Show

Yuliya Bozhko added a comment - 28/Jan/14 12:05 AM Branches updated and rebased. Ready to be reviewed when you have time. Thanks!

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:24 AM - 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

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:24 AM - 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

Permalink

Yuliya Bozhko added a comment - 28/Jan/14 12:26 AM

Probably MySQL, because we tested it on Postgres.

Show

Yuliya Bozhko added a comment - 28/Jan/14 12:26 AM Probably MySQL, because we tested it on Postgres.

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:35 AM

(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

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:35 AM (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

Permalink

Yuliya Bozhko added a comment - 28/Jan/14 12:37 AM

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

Show

Yuliya Bozhko added a comment - 28/Jan/14 12:37 AM Thanks! Just updated commits, so should be better now.

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:40 AM

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

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:40 AM 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

Permalink

Yuliya Bozhko added a comment - 28/Jan/14 12:42 AM

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

Show

Yuliya Bozhko added a comment - 28/Jan/14 12:42 AM Thanks, Eloy! It's always good to learn something new

Hide

Permalink

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:44 AM

Integrated (25, 26 & master), thanks!

Show

Eloy Lafuente (stronk7) added a comment - 28/Jan/14 12:44 AM Integrated (25, 26 & master), thanks!

Hide

Permalink

Michael de Raadt added a comment - 29/Jan/14 6:37 AM

Test result: Success!

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

My page records are removed for deleted users.

Show

Michael de Raadt added a comment - 29/Jan/14 6:37 AM Test result: Success! Tested in 2.5, 2.6 and master. Tested using PostgreSQL. My page records are removed for deleted users.

Hide

Permalink

Marina Glancy added a comment - 31/Jan/14 8:29 AM

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

Show

Marina Glancy added a comment - 31/Jan/14 8:29 AM Thanks for your hard work. Your code has now become a part of Moodle!

People

Assignee:

Yuliya Bozhko

Reporter:

Yuliya Bozhko

Peer reviewer:

Rossiani Wijaya

Integrator:

Eloy Lafuente (stronk7)

Tester:

Michael de Raadt

Participants:

CiBoT, Eloy Lafuente (stronk7), Marina Glancy, Michael de Raadt, Rossiani Wijaya, Yuliya Bozhko

Votes:

0 Vote for this issue

Watchers:

6 Start watching this issue

Dates

Created:

22/Jan/14 2:03 AM

Updated:

31/Jan/14 8:29 AM

Resolved:

31/Jan/14 8:29 AM

Fix Release Date:

10/Mar/14

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

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