Moodle
  1. Moodle
  2. MDL-22051

Deleting one block/activity... must delete dependent information...

    Details

    • Database:
      Any
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      While trying backup I've detected some orphan records in DB here and there.

      Each time one context (block, activity, course) is deleted... dependent information should be deleted too. This includes:

      • ratings
      • comments
      • filters
      • role assignments and overrides
      • block instances and positions
      • logs
      • tags?
      • files?

      Please, revise all the block/activity/course deletions... we are leaving a lot of (mainly context based information) 100% orphaned.

      Ciao

        Gliffy Diagrams

        1. comments_block.patch
          0.5 kB
          Dongsheng Cai
        2. course_deletion_remnants_raw_sql_diff.txt
          371 kB
          Aparup Banerjee
        3. db_test_results.tar.gz
          2.08 MB
          Aparup Banerjee
        4. html_block.patch
          0.9 kB
          Dongsheng Cai
        5. start_to_end_state_db_(test1-6).txt
          139 kB
          Aparup Banerjee
        6. start_to_end_state_db_with_student_activity.txt
          165 kB
          Aparup Banerjee

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding some watchers...

          Show
          Eloy Lafuente (stronk7) added a comment - Adding some watchers...
          Hide
          Dongsheng Cai added a comment -

          Hi Eloy

          About deleting comment records when delete comments block, since we decide use page context for comments, is it a good idea to delete them all? After all comments block doesn't own the comments records..

          And files:
          Are you talking about private files block or generate file storage in blocks? For private file blocks, I don't think we should delete files, because the private file area can be used elsewhere, for file storage, currently only html block has files, I will commit a patch later

          Show
          Dongsheng Cai added a comment - Hi Eloy About deleting comment records when delete comments block, since we decide use page context for comments, is it a good idea to delete them all? After all comments block doesn't own the comments records.. And files: Are you talking about private files block or generate file storage in blocks? For private file blocks, I don't think we should delete files, because the private file area can be used elsewhere, for file storage, currently only html block has files, I will commit a patch later
          Hide
          Martin Dougiamas added a comment -

          I agree about comments, the comments belong to the context, and the block just shows/edits them.

          BUT, the comments should definitely be deleted when the context is deleted. Are we doing this at the moment for activity deletions and course deletions?

          Show
          Martin Dougiamas added a comment - I agree about comments, the comments belong to the context, and the block just shows/edits them. BUT, the comments should definitely be deleted when the context is deleted. Are we doing this at the moment for activity deletions and course deletions?
          Hide
          Martin Dougiamas added a comment -

          Thanks Dongsheng!

          Show
          Martin Dougiamas added a comment - Thanks Dongsheng!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, I'm not sure if this was properly fixed... I've continued finding orphan records in a lot of places (see MDL-24087, MDL-24091 for example).

          IMO this needs a complete review and an structured way to delete things. All the bugs above raised following these steps:

          1) create one new 2.0 installation with MySQL
          2) create one course, with one activity of each type, using everything within them (comments, ratings, repositories, blocks, tags, files, course completion, activity completion and availability, enrollments ...)
          3) go to admin/courses and delete the course
          4) look to all the candidate tables, they must be empty. If not... we are keeping orphaned records.

          And that is only testing course deletion, IMO all these should be tested:

          • course deletion
          • course clear contents
          • course reset
          • activity deletion
          • block deletion

          Not sure if this is the place or a new one is necessary to cover the whole thing but all those orphans shouldn't be there, just that. I'm not reopening this, but needs some attention indeed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, I'm not sure if this was properly fixed... I've continued finding orphan records in a lot of places (see MDL-24087 , MDL-24091 for example). IMO this needs a complete review and an structured way to delete things. All the bugs above raised following these steps: 1) create one new 2.0 installation with MySQL 2) create one course, with one activity of each type, using everything within them (comments, ratings, repositories, blocks, tags, files, course completion, activity completion and availability, enrollments ...) 3) go to admin/courses and delete the course 4) look to all the candidate tables, they must be empty. If not... we are keeping orphaned records. And that is only testing course deletion, IMO all these should be tested: course deletion course clear contents course reset activity deletion block deletion Not sure if this is the place or a new one is necessary to cover the whole thing but all those orphans shouldn't be there, just that. I'm not reopening this, but needs some attention indeed. Ciao
          Hide
          Martin Dougiamas added a comment -

          Yes, thanks Eloy.

          I'll ask Aparup to perform the test (create course on new install, add blocks/ratings/comments/tags etc to course, delete course, examine tables) and report back here with results.

          Show
          Martin Dougiamas added a comment - Yes, thanks Eloy. I'll ask Aparup to perform the test (create course on new install, add blocks/ratings/comments/tags etc to course, delete course, examine tables) and report back here with results.
          Hide
          Aparup Banerjee added a comment -

          I've looked at the tables between a fresh install and after a course with all activities and resources were deleted. This is with only admin. There was no students acting on this course. (i should do that next)

          I've taken a look at this by diff DB dumps of the start (fresh install) and the end states (course with resources and acitvities deleted).
          (please ignore backup stuff as i did perform a course backup)

          Heres what i gleaned from the diff :-

          tables incompletely cleaned,

          • files (still has draft file records , see attached diff)

          tables missed (insert records still around),

          • cache_flags (records contain context paths that don't exist anymore, although they are marked 'dirty' so it might be ok?)
          • course_display
          • enrol

          tables cleaned of records (auto_increment increased with no corresponding record inserts),

          • assignment
          • block_instances
          • chat
          • choice , choice_options
          • context
          • course, course_modules
          • course_sections (cleaned appropriately except the default course)
          • data
          • event
          • folder
          • forum
          • glossary
          • grade_categories, grade_items
          • imscp
          • label
          • lesson
          • page
          • quiz, quiz_feedback
          • resource
          • role_names
          • scale
          • scorm, scorm_scoes, scorm_scoes_data
          • survey
          • url
          • user_lastaccess
          • wiki
          • workshop

          tables ignored for posterity reasons (guessing from table and comments),

          • cache_text
          • config
          • grade_categories_history, grade_items_history
          • log
          • sessions
          • user, user_preferences
          Show
          Aparup Banerjee added a comment - I've looked at the tables between a fresh install and after a course with all activities and resources were deleted. This is with only admin. There was no students acting on this course. (i should do that next) I've taken a look at this by diff DB dumps of the start (fresh install) and the end states (course with resources and acitvities deleted). (please ignore backup stuff as i did perform a course backup) Heres what i gleaned from the diff :- tables incompletely cleaned, files (still has draft file records , see attached diff) tables missed (insert records still around), cache_flags (records contain context paths that don't exist anymore, although they are marked 'dirty' so it might be ok?) course_display enrol tables cleaned of records (auto_increment increased with no corresponding record inserts), assignment block_instances chat choice , choice_options context course, course_modules course_sections (cleaned appropriately except the default course) data event folder forum glossary grade_categories, grade_items imscp label lesson page quiz, quiz_feedback resource role_names scale scorm, scorm_scoes, scorm_scoes_data survey url user_lastaccess wiki workshop tables ignored for posterity reasons (guessing from table and comments), cache_text config grade_categories_history, grade_items_history log sessions user, user_preferences
          Hide
          Dongsheng Cai added a comment -

          draft files won't be deleted when delete courses/activities, because it is not in course context. But it will be cleaned up by cron.

          Show
          Dongsheng Cai added a comment - draft files won't be deleted when delete courses/activities, because it is not in course context. But it will be cleaned up by cron.
          Hide
          Martin Dougiamas added a comment -

          Apu what about the list of things Eloy mentions at the top?

          1. ratings
          2. comments
          3. filters
          4. role assignments and overrides
          5. block instances and positions
          6. logs
          Show
          Martin Dougiamas added a comment - Apu what about the list of things Eloy mentions at the top? ratings comments filters role assignments and overrides block instances and positions logs
          Hide
          Aparup Banerjee added a comment -


          I've just done a test (see ..student_activity.txt ) with activities being used by students and i'm getting a few more tables not being cleared of relevant records after course deletion,
          tables missed :-

          • comments
          • glossary_entries
          • tag, tag_instance had some records left around due to wiki, not sure if this is supposed to be or not.

          ok heres the test for course deletion after using everything in 1-5 (see ..(test1-6).txt)

          1. ratings [ forum,data,glossary all have ratings with comments.]
          2. comments from block [all modules + main ]
          3. filters [all modules set custom filter switches ]
          4. role assignments [all modules set custom assignments ]
          5. block instances and positions [all modules + main created and moved blocks left]

          tables with data that seems orphaned :-

          • comments (contains comments from 'page_comments' and also 'wiki_page')
          • course_display (still references course deleted record )
          • enrol (table still has records referencing the deleted courseid.)
          • question_categories (i'm not sure if this is ok but it has records referencing a contextid=3)

          tables with lots of data for no current reason (to be cleaned by cron?) :-

          • cache_flags (seems to still contain dirtycontexts after cron)

          files table contains draft files but seems like it will be removed after 4 days by cron (thanks DS & MD).

          in terms of the tests, no orphaned data left behind by 1,3,4,5. just 2.
          -----------------------

          Show
          Aparup Banerjee added a comment - — I've just done a test (see ..student_activity.txt ) with activities being used by students and i'm getting a few more tables not being cleared of relevant records after course deletion, tables missed :- comments glossary_entries tag, tag_instance had some records left around due to wiki, not sure if this is supposed to be or not. ok heres the test for course deletion after using everything in 1-5 (see ..(test1-6).txt) 1. ratings [ forum,data,glossary all have ratings with comments.] 2. comments from block [all modules + main ] 3. filters [all modules set custom filter switches ] 4. role assignments [all modules set custom assignments ] 5. block instances and positions [all modules + main created and moved blocks left] tables with data that seems orphaned :- comments (contains comments from 'page_comments' and also 'wiki_page') course_display (still references course deleted record ) enrol (table still has records referencing the deleted courseid.) question_categories (i'm not sure if this is ok but it has records referencing a contextid=3) tables with lots of data for no current reason (to be cleaned by cron?) :- cache_flags (seems to still contain dirtycontexts after cron) files table contains draft files but seems like it will be removed after 4 days by cron (thanks DS & MD). in terms of the tests, no orphaned data left behind by 1,3,4,5. just 2. -----------------------
          Hide
          Dongsheng Cai added a comment -

          Just moved comments deletion code from delete_course to delete_context, have to use comments methods in lib/accesslib.php

          Show
          Dongsheng Cai added a comment - Just moved comments deletion code from delete_course to delete_context, have to use comments methods in lib/accesslib.php
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Dong,

          just a side comment pointing you to MDL-24087 (and the patch there). Now that you're deleting comments from delete_context(), you should take rid of the deletion in the xxx_delete_instance() methods. Plz, continue there.

          Hi Aparup, I assume you've tested:

          1) One course_delete() execution

          nice work! Could you also test what happens (looking for orphans) when:

          2) One delete_course_contents() is used.
          3) The course->reset feature is used.

          I guess we'll find some more orphans there (specially in the reset feature).

          Then, make a final summary of found orphans for the 1) 2) 3) above to see where we must fix things.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Dong, just a side comment pointing you to MDL-24087 (and the patch there). Now that you're deleting comments from delete_context(), you should take rid of the deletion in the xxx_delete_instance() methods. Plz, continue there. Hi Aparup, I assume you've tested: 1) One course_delete() execution nice work! Could you also test what happens (looking for orphans) when: 2) One delete_course_contents() is used. 3) The course->reset feature is used. I guess we'll find some more orphans there (specially in the reset feature). Then, make a final summary of found orphans for the 1) 2) 3) above to see where we must fix things. Ciao
          Hide
          Dongsheng Cai added a comment -

          Hi, Eloy

          thanks for reminding me the code in *_delete_instance(), comments deletion code removed now.

          Show
          Dongsheng Cai added a comment - Hi, Eloy thanks for reminding me the code in *_delete_instance(), comments deletion code removed now.
          Hide
          Aparup Banerjee added a comment -

          Hi Eloy,
          thanks, i'm working on setting up testing the reset feature now.
          not sure yet about how do i test (2)?

          Show
          Aparup Banerjee added a comment - Hi Eloy, thanks, i'm working on setting up testing the reset feature now. not sure yet about how do i test (2)?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Aparup,

          surely delete_course() is invoking remove_course_contents() but I would create one simple php script in your root moodle dir "remove_course_contents.php" and simply add this to it and execute via browser:

          $course = XXX; // replace by your course id
          require_once('config.php');
          remove_course_contents($course, true);
          

          something like that should do the trick (unless there are some extra lines needed to set the $PAGE or some missing lib). Once executed, look for orphaned records (I guess they will be, practically, the same than the ones found in course_delete()).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Aparup, surely delete_course() is invoking remove_course_contents() but I would create one simple php script in your root moodle dir "remove_course_contents.php" and simply add this to it and execute via browser: $course = XXX; // replace by your course id require_once('config.php'); remove_course_contents($course, true); something like that should do the trick (unless there are some extra lines needed to set the $PAGE or some missing lib). Once executed, look for orphaned records (I guess they will be, practically, the same than the ones found in course_delete()). Ciao
          Hide
          Aparup Banerjee added a comment -

          ok here's the summary of all the three types of tests (attached test data & results):

          tables still with data after
          (1) course_delete(gui)
          (2) remove_course_contents(code test)
          (3) course_reset(gui)

          • course_display (1) : still references course deleted record
          • enrol (1) : table still has records referencing the deleted courseid.
          • question_categories (1)
          • user_preferences (1, 3) : has block instances data
          • comments (1, 3) : contains comments from 'page_comments' and also 'wiki_page' , (reset_course_userdata() doesn't seem to call remove_context() )
          • mdl_assignment_submissions (3) : has submissions
          • glossary_alias (3) : has data although glossary_entries has been cleared.
          • grade_items (3)
          • quiz_feedback (3)
          • tag and tag_instance (3) : a wiki_page tag
          • wiki_locks (3)
          • mdl_course(2) : course_delete clears this but remove_course_contents() doesn't (as expected?).

          notes:

          • data_fields (3) :still has data but data_content and data_records has been truncated so i guess that seems right as this is probably deemed the 'setup' data.
          Show
          Aparup Banerjee added a comment - ok here's the summary of all the three types of tests (attached test data & results): tables still with data after (1) course_delete(gui) (2) remove_course_contents(code test) (3) course_reset(gui) — course_display (1) : still references course deleted record enrol (1) : table still has records referencing the deleted courseid. question_categories (1) user_preferences (1, 3) : has block instances data comments (1, 3) : contains comments from 'page_comments' and also 'wiki_page' , (reset_course_userdata() doesn't seem to call remove_context() ) mdl_assignment_submissions (3) : has submissions glossary_alias (3) : has data although glossary_entries has been cleared. grade_items (3) quiz_feedback (3) tag and tag_instance (3) : a wiki_page tag wiki_locks (3) mdl_course(2) : course_delete clears this but remove_course_contents() doesn't (as expected?). notes: data_fields (3) :still has data but data_content and data_records has been truncated so i guess that seems right as this is probably deemed the 'setup' data.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Great Aparup,

          now two questions:

          1) who is going to fix what?
          2) do we need to add upgrade scripts to delete already existing orphaned?

          Ciao

          PS: I'd number each case above in another bug so anybody can pick and mark what has been fixed (something like MDL-22151). Once everything is fixed, tests should be run again to check there is nothing pending.

          Show
          Eloy Lafuente (stronk7) added a comment - Great Aparup, now two questions: 1) who is going to fix what? 2) do we need to add upgrade scripts to delete already existing orphaned? Ciao PS: I'd number each case above in another bug so anybody can pick and mark what has been fixed (something like MDL-22151 ). Once everything is fixed, tests should be run again to check there is nothing pending.
          Hide
          Aparup Banerjee added a comment -

          Eloy, definitely for comments we need a script as changes from DS only affect the current course being acted on. not yet sure about other records.
          ok i'll create subtasks for the issues seen in the test.

          Show
          Aparup Banerjee added a comment - Eloy, definitely for comments we need a script as changes from DS only affect the current course being acted on. not yet sure about other records. ok i'll create subtasks for the issues seen in the test.
          Hide
          Martin Dougiamas added a comment - - edited

          I've assigned all the subtasks here.

          There seems to be some confusion in the functions for all this. Here is my summary of it all.

          delete_course() - deletes the course record and context, as well as calling remove_course_contents(). Called from a variety of places.

          remove_course_contents() - removes ALL course content related to courseid. Calls xxx_delete_instance and xxx_delete_course in all activities. Called by delete_course() and during restore.

          reset_course_userdata() - retains structure of course, deletes MOST user data. Calls xxx_reset_userdata in all activities. Is ONLY called during a course reset.

          delete_context() - deletes anything associated to the context directly (not courseid). Can be used for any context, not just course context. Is called by remove_course_contents() and delete_course() to remove all contexts.

          Show
          Martin Dougiamas added a comment - - edited I've assigned all the subtasks here. There seems to be some confusion in the functions for all this. Here is my summary of it all. delete_course() - deletes the course record and context, as well as calling remove_course_contents(). Called from a variety of places. remove_course_contents() - removes ALL course content related to courseid. Calls xxx_delete_instance and xxx_delete_course in all activities. Called by delete_course() and during restore. reset_course_userdata() - retains structure of course, deletes MOST user data. Calls xxx_reset_userdata in all activities. Is ONLY called during a course reset. delete_context() - deletes anything associated to the context directly (not courseid). Can be used for any context, not just course context. Is called by remove_course_contents() and delete_course() to remove all contexts.
          Hide
          Petr Skoda added a comment -

          Hello,

          I was fixing my new enrolment code to properly delete data and I realised there were other multiple problems there, so I fixed them too:

          • fixed order of deleting in course and context
          • adding course context to course delete/remove event data because it is not available alter and some stuff may depend on old context id
          • adding option for context purging (keeps the context record because it might be still referenced later and it would be recreated)
          • new course enrol cleanup
          • removing content from some course fields that were referencing deleted content
          • general coding style and phpdocs improvements

          Petr Skoda

          Show
          Petr Skoda added a comment - Hello, I was fixing my new enrolment code to properly delete data and I realised there were other multiple problems there, so I fixed them too: fixed order of deleting in course and context adding course context to course delete/remove event data because it is not available alter and some stuff may depend on old context id adding option for context purging (keeps the context record because it might be still referenced later and it would be recreated) new course enrol cleanup removing content from some course fields that were referencing deleted content general coding style and phpdocs improvements Petr Skoda
          Hide
          Martin Dougiamas added a comment -

          Awesome, thanks Petr!

          Show
          Martin Dougiamas added a comment - Awesome, thanks Petr!
          Hide
          Martin Dougiamas added a comment -

          Yay!

          Show
          Martin Dougiamas added a comment - Yay!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: