Moodle
  1. Moodle
  2. MDL-14326

Moodle does not delete empty course_modules after the restore

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.5, 1.9
    • Fix Version/s: 1.8.7, 1.9.3
    • Component/s: Backup
    • Labels:
      None
    • Database:
      Any
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      31049

      Description

      PROBLEM
      ========
      During the restore process, an empty course_module is created by backup/restorelib.php (see around line 1000). "Empty" means there is no instance assigned into this module (slot) yet.

      If a module restore fails for some reason, the instance record is not created. User can then see just an icon of an module with no activity title.

      SOLUTION/PROPOSAL
      =================
      As a last step of restore, Moodle should delete all course_modules without instance:

      DELETE FROM course_modules where instance=0;

      Please note, the empty module is deleted anyway, if user clicks on the icon.

      1. MDL-14326-commited.patch
        1 kB
        David Mudrak
      2. restorelib.patch
        3 kB
        John T. Macklin

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          Please, forget the last sentence of the Description. The module has to be deleted manually. This is the reply:

          The required instance of this module didn't exist. Module deleted.

          Show
          David Mudrak added a comment - Please, forget the last sentence of the Description. The module has to be deleted manually. This is the reply: The required instance of this module didn't exist. Module deleted.
          Hide
          Petr Škoda added a comment -

          David, do you feel like making a patch for MOODLE_19_STABLE?

          Show
          Petr Škoda added a comment - David, do you feel like making a patch for MOODLE_19_STABLE?
          Hide
          David Mudrak added a comment -

          Finally it's just a single line of code that implements the proposed check. Works for me at MOODLE_19_STABLE @ Postgres 8.1

          Show
          David Mudrak added a comment - Finally it's just a single line of code that implements the proposed check. Works for me at MOODLE_19_STABLE @ Postgres 8.1
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks ok for me (I guess a lot of module-related data will be there - partially restore), but at least course won't show fake activities. +1

          Show
          Eloy Lafuente (stronk7) added a comment - Looks ok for me (I guess a lot of module-related data will be there - partially restore), but at least course won't show fake activities. +1
          Hide
          David Mudrak added a comment -

          I am waiting for another +1 from Petr or Martin. The I will commit into both STABLEs.

          Show
          David Mudrak added a comment - I am waiting for another +1 from Petr or Martin. The I will commit into both STABLEs.
          Hide
          Petr Škoda added a comment -

          I heard some ppl are having activities without entries in course_module table (ask sam from OU if needed
          but the opposite should never happen

          +1 for 1.8 and 1.9 branches

          Show
          Petr Škoda added a comment - I heard some ppl are having activities without entries in course_module table (ask sam from OU if needed but the opposite should never happen +1 for 1.8 and 1.9 branches
          Hide
          Petr Škoda added a comment -

          hmmm, maybe we could limit it to current course only and add upgrade code with clean up of all existing courses

          Show
          Petr Škoda added a comment - hmmm, maybe we could limit it to current course only and add upgrade code with clean up of all existing courses
          Hide
          Eloy Lafuente (stronk7) added a comment -

          100% agree with Petr last comment.

          Show
          Eloy Lafuente (stronk7) added a comment - 100% agree with Petr last comment.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oh, I think it's enough for now.

          Hehe, I don't use the web interface at all, at least for me (triaging everyone else bugs) the client is really better! B-)

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, I think it's enough for now. Hehe, I don't use the web interface at all, at least for me (triaging everyone else bugs) the client is really better! B-) Ciao
          Hide
          Jeffrey Silverman added a comment -

          Hi, all. I am experiencing this issue. Quick question: what does the "mdl_course_modules.instance" column signify?

          Thanks!

          Show
          Jeffrey Silverman added a comment - Hi, all. I am experiencing this issue. Quick question: what does the "mdl_course_modules.instance" column signify? Thanks!
          Hide
          Jeffrey Silverman added a comment -

          Never mind, figured it out. I'll answer here anyway for the legions of interested people flocking to this page.

          The "instance" column is the id of the module, pointing to the modulename table. So, for example, a "label":

          mdl_course_module.instance joins to mdl_label.id

          Show
          Jeffrey Silverman added a comment - Never mind, figured it out. I'll answer here anyway for the legions of interested people flocking to this page. The "instance" column is the id of the module, pointing to the modulename table. So, for example, a "label": mdl_course_module.instance joins to mdl_label.id
          Hide
          David Mudrak added a comment -

          Yes Jeffrey, you are right. The "instance" is the ID of the module record in its own table. In "mdl_course_modules" we define just "slots" where module instances can be put into.

          Ad this issue: I have not committed yet, because I want to check yet another approach: the restorelib should remember which slots (i.e. course_modules) were created during the restore process. At the end of the restore, only empty slots created in this restore process are deleted. This should prevent potential problems with concurrent (parallel) restore processes. In the current proposal, one running restore process could remove empty course_modules slots created by another (and still running!) restore process.

          Show
          David Mudrak added a comment - Yes Jeffrey, you are right. The "instance" is the ID of the module record in its own table. In "mdl_course_modules" we define just "slots" where module instances can be put into. Ad this issue: I have not committed yet, because I want to check yet another approach: the restorelib should remember which slots (i.e. course_modules) were created during the restore process. At the end of the restore, only empty slots created in this restore process are deleted. This should prevent potential problems with concurrent (parallel) restore processes. In the current proposal, one running restore process could remove empty course_modules slots created by another (and still running!) restore process.
          Hide
          Kenneth Newquist added a comment -

          I'm experiencing this problem in Moodle 1.9.2 (Build: 20080711). Is there a patch available for this yet? Faculty are starting to hit Moodle as they ramp up for the fall semester and are encountering this problem in droves. If it helps, I can confirm that "DELETE FROM course_modules where instance=0" gets ride of the ghost activities.

          Show
          Kenneth Newquist added a comment - I'm experiencing this problem in Moodle 1.9.2 (Build: 20080711). Is there a patch available for this yet? Faculty are starting to hit Moodle as they ramp up for the fall semester and are encountering this problem in droves. If it helps, I can confirm that "DELETE FROM course_modules where instance=0" gets ride of the ghost activities.
          Hide
          Kenneth Newquist added a comment -

          To clarify, what I'm seeing is the behavior reported in <a href="http://tracker.moodle.org/browse/MDL-10993"> MDL-10993: Restore creates duplicate activity/resource icons with no text</a>. We get these duplicates when importing a course into a new blank course (rather than restoring from a backup). It appears to be intermittent; I can reproduce it on my development system but not my production one, yet I have reports of this happening in production as well.

          The "DELETE FROM" line removed the duplicates however, as the tracker notes, fixing this will likely help with my problem as well.

          Show
          Kenneth Newquist added a comment - To clarify, what I'm seeing is the behavior reported in <a href="http://tracker.moodle.org/browse/MDL-10993"> MDL-10993 : Restore creates duplicate activity/resource icons with no text</a>. We get these duplicates when importing a course into a new blank course (rather than restoring from a backup). It appears to be intermittent; I can reproduce it on my development system but not my production one, yet I have reports of this happening in production as well. The "DELETE FROM" line removed the duplicates however, as the tracker notes, fixing this will likely help with my problem as well.
          Hide
          David Mudrak added a comment -

          Increasing the priority. I am on the holiday at the moment but I shall post a patch next week.

          Show
          David Mudrak added a comment - Increasing the priority. I am on the holiday at the moment but I shall post a patch next week.
          Hide
          John T. Macklin added a comment -

          I have had the same issues on multiple sites and this patch should resolve the issue with duplicate icons being created during Backup/Restore or import to new or existing course. moodle/backup/restorelib.php patch...

          Show
          John T. Macklin added a comment - I have had the same issues on multiple sites and this patch should resolve the issue with duplicate icons being created during Backup/Restore or import to new or existing course. moodle/backup/restorelib.php patch...
          Hide
          John T. Macklin added a comment -

          A work around for the issue with duplication ...

          Show
          John T. Macklin added a comment - A work around for the issue with duplication ...
          Hide
          David Mudrak added a comment -

          I am going to commit the patch today

          Show
          David Mudrak added a comment - I am going to commit the patch today
          Hide
          David Mudrak added a comment -

          MDL-14326-commited.patch - modified John's patch. It removes all course_modules that remained empty (i.e. with instance==0) after the particular restore process. The patch does not contain other Remote Learner modifications from the John's patch (i.e. ini_setting max executable time and ignoring mod/resource restore failures). Track these modification in a separate issue if needed.

          Show
          David Mudrak added a comment - MDL-14326 -commited.patch - modified John's patch. It removes all course_modules that remained empty (i.e. with instance==0) after the particular restore process. The patch does not contain other Remote Learner modifications from the John's patch (i.e. ini_setting max executable time and ignoring mod/resource restore failures). Track these modification in a separate issue if needed.
          Hide
          David Mudrak added a comment -

          Fixed in CVS

          Show
          David Mudrak added a comment - Fixed in CVS
          Hide
          Petr Škoda added a comment -

          thanks, closing

          Show
          Petr Škoda added a comment - thanks, closing
          Hide
          Petr Škoda added a comment -

          fixed the old DML syntax in HEAD, sorry for missing that

          Show
          Petr Škoda added a comment - fixed the old DML syntax in HEAD, sorry for missing that
          Hide
          Robert Levy added a comment -

          Hi. Has this patch been included in the 1.9.2x version of moodle?

          Show
          Robert Levy added a comment - Hi. Has this patch been included in the 1.9.2x version of moodle?
          Hide
          David Mudrak added a comment -

          Hi Robert. Yes, it has. Use the most recent wekly build of 1.9.2.

          Show
          David Mudrak added a comment - Hi Robert. Yes, it has. Use the most recent wekly build of 1.9.2.
          Hide
          Tim Hunt added a comment -

          Hmm. does this leave orphan contexts floating around? That is, should be be calling delete_course_module from course/lib.php rather than calling delete records?

          Show
          Tim Hunt added a comment - Hmm. does this leave orphan contexts floating around? That is, should be be calling delete_course_module from course/lib.php rather than calling delete records?
          Hide
          John T. Macklin added a comment -

          Tim,

          The issue here was additional modules instances if you check grade_item::fetch_all(array('itemtype'=>'mod', 'itemmodule'=>$modulename,
          'iteminstance'=>$cm->instance, 'courseid'=>$cm->course))) against one of these additional instances you will not find any grade_items or
          events since the $cm->instance is invalid. Hence why I did not use delete_course_module from course/lib.php .

          Show
          John T. Macklin added a comment - Tim, The issue here was additional modules instances if you check grade_item::fetch_all(array('itemtype'=>'mod', 'itemmodule'=>$modulename, 'iteminstance'=>$cm->instance, 'courseid'=>$cm->course))) against one of these additional instances you will not find any grade_items or events since the $cm->instance is invalid. Hence why I did not use delete_course_module from course/lib.php .
          Hide
          Susan Mangan added a comment -

          We are running version 1.8.8 and this problem still exists. Not sure about during restore but it occurred during import. After importing contents, empty resources appeared in various weeks and when clicking them the following error occurs:The required instance of this module didn't exist. Module deleted. But it doesn't actually delete. I could go back in and manually delete the empty modules, but why does this still occur? Same bug or different one?

          Show
          Susan Mangan added a comment - We are running version 1.8.8 and this problem still exists. Not sure about during restore but it occurred during import. After importing contents, empty resources appeared in various weeks and when clicking them the following error occurs:The required instance of this module didn't exist. Module deleted. But it doesn't actually delete. I could go back in and manually delete the empty modules, but why does this still occur? Same bug or different one?
          Hide
          Frédéric Hoogstoel added a comment -

          Hi !

          I get this problem with version 1.8.5. But the biggest problemn is not that the icons of non-existing modules appear, but that thsese modules have not been created by the restore !
          I did not find any issue about this : the restore stops with no error message. This problem is reported by several users in "Using Moodle" forum and in "Assistance Technique" forum of course "Moodle en Français" :
          See http://moodle.org/mod/forum/discuss.php?d=121608#p554771
          Should I create a new (related ?) issue ?

          Thank you.

          Show
          Frédéric Hoogstoel added a comment - Hi ! I get this problem with version 1.8.5. But the biggest problemn is not that the icons of non-existing modules appear, but that thsese modules have not been created by the restore ! I did not find any issue about this : the restore stops with no error message. This problem is reported by several users in "Using Moodle" forum and in "Assistance Technique" forum of course "Moodle en Français" : See http://moodle.org/mod/forum/discuss.php?d=121608#p554771 Should I create a new (related ?) issue ? Thank you.

            People

            • Votes:
              10 Vote for this issue
              Watchers:
              17 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: