Moodle
  1. Moodle
  2. MDL-37939

Moving modules between sections is not properly working

    Details

    • Testing Instructions:
      Hide

      On 2.3 only:

      Before applying upgrade:

      • take a course with a fair few CMs
      • check it's section data in the database using a query like:
        select id, section, sequence from mdl_course_sections where course = 5 order by section asc; select id, section from mdl_course_modules where course = 5;
        
      • move some course modules around. It's easiest to replicate if you have some CMs in the same section, and then move some of them out
      • check that they're broken - you should see duplicate cmids in the sections.sequence data, and no updates made to the modules.section data.
      • refresh the page and make some more changes: confirm that although the drag/drop appears to work, when you actually refresh the page it doesn't persist across a refresh
      • now apply the patch and perform the upgrade
      • confirm that the CMs didn't appear to move from where they were before the upgrade
      • confirm that the DB looks correct now (no duplicates in sequence data)
      • move some CMs around sections
      • refresh
        • confirm that the section sequence data is correct
        • confirm that the cm.section is correct

      On 2.4 and 2.5

      • take a course with a fair few CMs
      • apply the upgrade
      • confirm that the CMs didn't appear to move from where they were before the upgrade
      Show
      On 2.3 only: Before applying upgrade: take a course with a fair few CMs check it's section data in the database using a query like: select id, section, sequence from mdl_course_sections where course = 5 order by section asc; select id, section from mdl_course_modules where course = 5; move some course modules around. It's easiest to replicate if you have some CMs in the same section, and then move some of them out check that they're broken - you should see duplicate cmids in the sections.sequence data, and no updates made to the modules.section data. refresh the page and make some more changes: confirm that although the drag/drop appears to work, when you actually refresh the page it doesn't persist across a refresh now apply the patch and perform the upgrade confirm that the CMs didn't appear to move from where they were before the upgrade confirm that the DB looks correct now (no duplicates in sequence data) move some CMs around sections refresh confirm that the section sequence data is correct confirm that the cm.section is correct On 2.4 and 2.5 take a course with a fair few CMs apply the upgrade confirm that the CMs didn't appear to move from where they were before the upgrade
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-37939-m24
    • Pull Master Branch:
    • Rank:
      47698

      Description

      When moving modules between sections in Moodle 2.3.4+, section sequence data and course_module section data gets out of skew. As a result modules can be moved once but then it becomes impossible to move them again.

      When drag and drop moving is used, they will appear to move, but refreshing the page shows that they do not move.

        Issue Links

          Activity

          Hide
          Tõnis Tartes added a comment -

          To clear the duplicate values from the sequence field, could be probably done with a nice DB query, but i'm scared of letting those loose on live DB.
          So i made a little cleanup patch to course/lib.php file.
          At function delete_mod_from_section(), i modified the clearing the keys by following hack.

                  if ($key = array_keys ($modarray, $mod)) {
                      if (count($key) > 1) {
                          foreach ($key as $k1 => $v1) {
                              unset($modarray[$v1]);
                          }
                      } else {
                          array_splice($modarray, $key[0], 1);                
                      }
                      $newsequence = implode(",", $modarray);
                      return $DB->set_field("course_sections", "sequence", $newsequence, array("id"=>$section->id));
                  } else {
                      return false;
                  }
          

          So when starting to move the modules from section to section it will clear up those duplicate values eventually(after multiple tries tho).
          Not all courses are affected with this problem, so it will take time to clear up, but for me it works.

          Show
          Tõnis Tartes added a comment - To clear the duplicate values from the sequence field, could be probably done with a nice DB query, but i'm scared of letting those loose on live DB. So i made a little cleanup patch to course/lib.php file. At function delete_mod_from_section(), i modified the clearing the keys by following hack. if ($key = array_keys ($modarray, $mod)) { if (count($key) > 1) { foreach ($key as $k1 => $v1) { unset($modarray[$v1]); } } else { array_splice($modarray, $key[0], 1); } $newsequence = implode(",", $modarray); return $DB->set_field("course_sections", "sequence", $newsequence, array("id"=>$section->id)); } else { return false; } So when starting to move the modules from section to section it will clear up those duplicate values eventually(after multiple tries tho). Not all courses are affected with this problem, so it will take time to clear up, but for me it works.
          Hide
          Andrew Nicols added a comment -

          Adding Damyon as a watcher.
          Also increasing the priority of this as it causes damage to course metadata which could be tricky to repair.

          Show
          Andrew Nicols added a comment - Adding Damyon as a watcher. Also increasing the priority of this as it causes damage to course metadata which could be tricky to repair.
          Hide
          Andrew Nicols added a comment -

          Tõnis Tartes thanks for tracking this down. I came across the issue when helping someone in IRC on Thursday but hadn't quite worked out the cause before I had to disappear.

          I think that we really do need to do this as a database upgrade to fix existing data. There's no telling at what point the APIs may change and we may become more reliant upon this field.

          Show
          Andrew Nicols added a comment - Tõnis Tartes thanks for tracking this down. I came across the issue when helping someone in IRC on Thursday but hadn't quite worked out the cause before I had to disappear. I think that we really do need to do this as a database upgrade to fix existing data. There's no telling at what point the APIs may change and we may become more reliant upon this field.
          Hide
          Tõnis Tartes added a comment -

          +1 for the database fix, but i think its a tricky one. I ain't that big of sql guru to suggest a decent fix query.
          If something goes wrong with the sequence field(accidentally some correct IDs are being deleted), then that would do a lot of damage to courses.
          We have around 2000(1400 active) courses and i cant think of losing the sequence(course front page being emptied) in the middle of semester.
          Luckily the module data will be still there, but re-linking them would be a pain.

          Show
          Tõnis Tartes added a comment - +1 for the database fix, but i think its a tricky one. I ain't that big of sql guru to suggest a decent fix query. If something goes wrong with the sequence field(accidentally some correct IDs are being deleted), then that would do a lot of damage to courses. We have around 2000(1400 active) courses and i cant think of losing the sequence(course front page being emptied) in the middle of semester. Luckily the module data will be still there, but re-linking them would be a pain.
          Hide
          Andrew Nicols added a comment -

          Although it would be an upgrade performed in lib/db/upgrade.php, the calculation would actually be performed in the database.

          I guess we'd need to:

          • go through each course_sections record and split out it's sequence field by ,
          • confirm that the relevant course_module belongs to that section
          • re-implode the sequence

          Unfortunately, this could add up to a lot of database queries too though (at least two reads per course section, and potentially a write too).

          Show
          Andrew Nicols added a comment - Although it would be an upgrade performed in lib/db/upgrade.php, the calculation would actually be performed in the database. I guess we'd need to: go through each course_sections record and split out it's sequence field by , confirm that the relevant course_module belongs to that section re-implode the sequence Unfortunately, this could add up to a lot of database queries too though (at least two reads per course section, and potentially a write too).
          Hide
          Andrew Nicols added a comment -

          Completely untested but this is just a first thought for a fix. I need to generate some broken data first before I can actually test this:

          // Retrieve the list of course_sections as a recordset to save memory
          $coursesections = $DB->get_recordset('course_sections', null, 'course, id', 'id, course, sequence');
          foreach ($coursesections as $coursesection) {
              // Retrieve all of the actual modules in this course and section combination to reduce DB calls
              $actualsectionmodules = $DB->get_records('course_modules',
                      array('course' => $coursesection->course, 'section' => $coursesection->id), '', 'id, section');
          
              // Break out the current sequence so that we can compare it
              $currentsequence = explode(',', $coursesection->sequence);
              $newsequence = array();
          
              // Check each of the modules in the current sequence
              foreach ($currentsequence as $module) {
                  if (isset($actualsectionmodules[$module])) {
                      $newsequence[] = $module;
                      unset($actualsectionmodules[$module]);
                  }   
              }   
          
              // Append any modules which have somehow been orphaned
              foreach ($actualsectionmodules as $module) {
                  $newsequence[] = $module->id;
              }   
          
              // Piece it all back together
              $sequence = implode(',', $newsequence);
          
              // Only update if there have been changes
              if ($sequence !== $coursesection->sequence) {
                  $coursesection->sequence = $sequence;
                  $DB->update_record('coures_sections', $coursesection);
              }   
          }
          $coursesection->close();
          
          Show
          Andrew Nicols added a comment - Completely untested but this is just a first thought for a fix. I need to generate some broken data first before I can actually test this: // Retrieve the list of course_sections as a recordset to save memory $coursesections = $DB->get_recordset('course_sections', null , 'course, id', 'id, course, sequence'); foreach ($coursesections as $coursesection) { // Retrieve all of the actual modules in this course and section combination to reduce DB calls $actualsectionmodules = $DB->get_records('course_modules', array('course' => $coursesection->course, 'section' => $coursesection->id), '', 'id, section'); // Break out the current sequence so that we can compare it $currentsequence = explode(',', $coursesection->sequence); $newsequence = array(); // Check each of the modules in the current sequence foreach ($currentsequence as $module) { if (isset($actualsectionmodules[$module])) { $newsequence[] = $module; unset($actualsectionmodules[$module]); } } // Append any modules which have somehow been orphaned foreach ($actualsectionmodules as $module) { $newsequence[] = $module->id; } // Piece it all back together $sequence = implode(',', $newsequence); // Only update if there have been changes if ($sequence !== $coursesection->sequence) { $coursesection->sequence = $sequence; $DB->update_record('coures_sections', $coursesection); } } $coursesection->close();
          Hide
          Andrew Nicols added a comment -

          Oh, and we may need to clear the sectioncache too.

          Show
          Andrew Nicols added a comment - Oh, and we may need to clear the sectioncache too.
          Hide
          Andrew Nicols added a comment -

          I've only been able to replicate this on MOODLE_23_STABLE. It seems that later branches aren't affected by the bug in the same way. May need to dig deeper to work out why I'm not seeing it as from the face of it, I think I should be.

          Show
          Andrew Nicols added a comment - I've only been able to replicate this on MOODLE_23_STABLE. It seems that later branches aren't affected by the bug in the same way. May need to dig deeper to work out why I'm not seeing it as from the face of it, I think I should be.
          Hide
          Andrew Nicols added a comment -

          Also note, that it's possible for the same CMID to be listed multiple times in a single sequence. The above code does deal with this too.

          moodle-MOODLE_23_STABLE=# select * from mdl_course_sections where course = 2;
           id | course | section | name | summary | summaryformat |  sequence   | visible | availablefrom | availableuntil | showavailability | groupingid 
          ----+--------+---------+------+---------+---------------+-------------+---------+---------------+----------------+------------------+------------
            1 |      2 |       0 |      |         |             1 | 8,8,5       |       1 |             0 |              0 |                0 |          0
           10 |      2 |       1 |      |         |             1 | 5,5,5,5,5,8 |       1 |             0 |              0 |                0 |          0
          (2 rows)
          
          moodle-MOODLE_23_STABLE=# select * from mdl_course_sections where course = 2;
           id | course | section | name | summary | summaryformat | sequence | visible | availablefrom | availableuntil | showavailability | groupingid 
          ----+--------+---------+------+---------+---------------+----------+---------+---------------+----------------+------------------+------------
            1 |      2 |       0 |      |         |             1 | 5        |       1 |             0 |              0 |                0 |          0
           10 |      2 |       1 |      |         |             1 | 8        |       1 |             0 |              0 |                0 |          0
          (2 rows)
          
          
          Show
          Andrew Nicols added a comment - Also note, that it's possible for the same CMID to be listed multiple times in a single sequence. The above code does deal with this too. moodle-MOODLE_23_STABLE=# select * from mdl_course_sections where course = 2; id | course | section | name | summary | summaryformat | sequence | visible | availablefrom | availableuntil | showavailability | groupingid ----+--------+---------+------+---------+---------------+-------------+---------+---------------+----------------+------------------+------------ 1 | 2 | 0 | | | 1 | 8,8,5 | 1 | 0 | 0 | 0 | 0 10 | 2 | 1 | | | 1 | 5,5,5,5,5,8 | 1 | 0 | 0 | 0 | 0 (2 rows) moodle-MOODLE_23_STABLE=# select * from mdl_course_sections where course = 2; id | course | section | name | summary | summaryformat | sequence | visible | availablefrom | availableuntil | showavailability | groupingid ----+--------+---------+------+---------+---------------+----------+---------+---------------+----------------+------------------+------------ 1 | 2 | 0 | | | 1 | 5 | 1 | 0 | 0 | 0 | 0 10 | 2 | 1 | | | 1 | 8 | 1 | 0 | 0 | 0 | 0 (2 rows)
          Hide
          Andrew Nicols added a comment -

          I've tracked this issue down and worked out why it only affects 2.3 and not 2.4 or master.

          It turns out to be that course_module.section is not updated when the section is changed. This is fine for the first move of that item, but on subsequent moves the workflow is such:

          • retrieve course_module record
          • delete this cm from cm->section
          • insert into new section

          Since the section ID was previously not updated, this means that on subsequent moves, the delete operation is unsuccessful.

          The way that this is handled was rewritten in 2.4 so neither 2.4 nor master are affected.

          I think that the proposed DB update should fix things and mean that they can be moved around again.

          Show
          Andrew Nicols added a comment - I've tracked this issue down and worked out why it only affects 2.3 and not 2.4 or master. It turns out to be that course_module.section is not updated when the section is changed. This is fine for the first move of that item, but on subsequent moves the workflow is such: retrieve course_module record delete this cm from cm->section insert into new section Since the section ID was previously not updated, this means that on subsequent moves, the delete operation is unsuccessful. The way that this is handled was rewritten in 2.4 so neither 2.4 nor master are affected. I think that the proposed DB update should fix things and mean that they can be moved around again.
          Hide
          Andrew Nicols added a comment -

          The bug only exists in 2.3, but data must be fixed in all versions to address it for those upgrading from their current (broken_ version straight to the next major.

          Show
          Andrew Nicols added a comment - The bug only exists in 2.3, but data must be fixed in all versions to address it for those upgrading from their current (broken_ version straight to the next major.
          Hide
          Andrew Nicols added a comment -

          n.b. this is not restricted to drag/drop movements.

          Show
          Andrew Nicols added a comment - n.b. this is not restricted to drag/drop movements.
          Hide
          Andrew Nicols added a comment -

          I've added unit tests to confirm this fix.

          Reminder: They will only fail on 2.3 before the fix is applied as this is not a problem in 2.4 onwards.

          Show
          Andrew Nicols added a comment - I've added unit tests to confirm this fix. Reminder: They will only fail on 2.3 before the fix is applied as this is not a problem in 2.4 onwards.
          Hide
          Trevor Cunnnigham added a comment -

          This issue has come up for us as well, and I reached the same conclusion. The problem is cm->section isn't being updated after a move.

          My one hesitation is with the db/upgrade fix. From my experience trying to replicate this, it seems difficult to know which section the module is actually displaying in. Since the cm->section wasn't updated, I wonder if its position is read from the cache or something? Is it the first or last instance of the cm->id in the course sections/sequences?

          My concern is that the upgrade may reposition course modules, so those courses affected may need review to ensure that the modules are located where expected.

          I'll try applying this fix to a copy of our data and report my findings.

          Show
          Trevor Cunnnigham added a comment - This issue has come up for us as well, and I reached the same conclusion. The problem is cm->section isn't being updated after a move. My one hesitation is with the db/upgrade fix. From my experience trying to replicate this, it seems difficult to know which section the module is actually displaying in. Since the cm->section wasn't updated, I wonder if its position is read from the cache or something? Is it the first or last instance of the cm->id in the course sections/sequences? My concern is that the upgrade may reposition course modules, so those courses affected may need review to ensure that the modules are located where expected. I'll try applying this fix to a copy of our data and report my findings.
          Hide
          Dan Poltawski added a comment - - edited

          Hi Andrew,

          The fix looks good to me. We came across a very similar problem om moodle.org a while ago. Basically you're just adding the modules to the end if they aren't already in the sequence field.

          Do we need to be concerned about modules which aren't in a section? Or is that already handled properly in the 'orphaned activities' code?

          Also, it'd be good if someone could test this with some real data and verify that it isn't changing things which don't need to be changed.

          I've added Marina here - Marina, it'd be good if you could cast your eye on this. In fact, the more eyes the better.

          Show
          Dan Poltawski added a comment - - edited Hi Andrew, The fix looks good to me. We came across a very similar problem om moodle.org a while ago. Basically you're just adding the modules to the end if they aren't already in the sequence field. Do we need to be concerned about modules which aren't in a section? Or is that already handled properly in the 'orphaned activities' code? Also, it'd be good if someone could test this with some real data and verify that it isn't changing things which don't need to be changed. I've added Marina here - Marina, it'd be good if you could cast your eye on this. In fact, the more eyes the better.
          Hide
          Andrew Nicols added a comment -

          Dan Poltawski WRT orphaned activities:
          There is a not null constraint on course_modules.section so that should prevent a module from not being in a section at all.
          In this fix, we don't handle orphaned activities at all. Since they don't belong to a section, they have no sequence data which we can fix.
          We could move those orphaned activities into a valid section but that would be wrong IMHO - it would also break mod_subpage in its current implementation.

          I don't have any real data to test on at present - this issue currently only affects weeklies. It's a regression from MDL-37430. A user on IRC has a test environment with real affected data who may be able to test this.

          Trevor Cunnnigham I think that this will do the right thing and put the CM in the correct location. I'm going to try and test that now and will report back. If you could report your findings too, that would be great.

          Andrew

          Show
          Andrew Nicols added a comment - Dan Poltawski WRT orphaned activities: There is a not null constraint on course_modules.section so that should prevent a module from not being in a section at all. In this fix, we don't handle orphaned activities at all. Since they don't belong to a section, they have no sequence data which we can fix. We could move those orphaned activities into a valid section but that would be wrong IMHO - it would also break mod_subpage in its current implementation. I don't have any real data to test on at present - this issue currently only affects weeklies. It's a regression from MDL-37430 . A user on IRC has a test environment with real affected data who may be able to test this. Trevor Cunnnigham I think that this will do the right thing and put the CM in the correct location. I'm going to try and test that now and will report back. If you could report your findings too, that would be great. Andrew
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,
          >There is a not null constraint on course_modules.section so that should prevent a module from not being in a section at all.

          But there can be sections which don't exist. Not asking you to 'fix' this, in fact you shouldn't, for reasons you point out. Just wondering if you've checked its ok.

          >I don't have any real data to test on at present

          Well, you could test it does nothing to your real sites with no broken data, which is a good piece of verification. Someone on the linked issues has also tested and found this fixes it.

          Show
          Dan Poltawski added a comment - Hi Andrew, >There is a not null constraint on course_modules.section so that should prevent a module from not being in a section at all. But there can be sections which don't exist. Not asking you to 'fix' this, in fact you shouldn't, for reasons you point out. Just wondering if you've checked its ok. >I don't have any real data to test on at present Well, you could test it does nothing to your real sites with no broken data, which is a good piece of verification. Someone on the linked issues has also tested and found this fixes it.
          Hide
          Andrew Nicols added a comment -

          I suspect that the upgrade script will move things around after all.
          Looking at how the order is currently calculated to find an alternative.

          Show
          Andrew Nicols added a comment - I suspect that the upgrade script will move things around after all. Looking at how the order is currently calculated to find an alternative.
          Hide
          Andrew Nicols added a comment -

          I've rewritten the upgrade side of things. It should now calculate the correct order as is currently displayed and does so without using any library calls (harder than I expected to work out).

          I'd appreciate a Peer Review of this again if that's okay.
          At present, I've only done the m23 branch. I'll update the 24 and master branches following peer review.

          Show
          Andrew Nicols added a comment - I've rewritten the upgrade side of things. It should now calculate the correct order as is currently displayed and does so without using any library calls (harder than I expected to work out). I'd appreciate a Peer Review of this again if that's okay. At present, I've only done the m23 branch. I'll update the 24 and master branches following peer review.
          Hide
          Trevor Cunnnigham added a comment -

          The rewritten upgrade script appears to work as expected. I can confirm that the previous version moved things around.

          We have a pilot and test instance running 2.3.4+ with real data. Between them, about 8 courses contained this bug. After running the most recent upgrade script on a copy of both data sets and comparing that to the actual versions, all labels, modules, etc, appear to remain in place.

          I had also written a quick report plugin to show courses with duplicates in the section sequences, and after applying the upgrade, everything looks good, no duplicates.

          Nice work!

          Show
          Trevor Cunnnigham added a comment - The rewritten upgrade script appears to work as expected. I can confirm that the previous version moved things around. We have a pilot and test instance running 2.3.4+ with real data. Between them, about 8 courses contained this bug. After running the most recent upgrade script on a copy of both data sets and comparing that to the actual versions, all labels, modules, etc, appear to remain in place. I had also written a quick report plugin to show courses with duplicates in the section sequences, and after applying the upgrade, everything looks good, no duplicates. Nice work!
          Hide
          Marina Glancy added a comment -

          I'm linking it to MDL-37028 which also deals with broken sequence field

          Show
          Marina Glancy added a comment - I'm linking it to MDL-37028 which also deals with broken sequence field
          Hide
          Russell Smith added a comment - - edited

          I would like to try and understand what's going on here. We've experienced this issue and applied the part of the patch that isn't the unit-test or upgrade. No failures of subjects have been reported since. To investigate which subjects were affected, I've created a modified version of the sql presented in the testing instructions.

          select cs.course, cs.id, array_upper(string_to_array(sequence,','),1), other.count, other.id_array, sequence
          from mdl_course_sections cs
          JOIN (select course, section, count(id), array_agg(id) as id_array
          from mdl_course_modules group by course, section) other ON cs.id = other.section
          where other.count <> array_upper(string_to_array(sequence,','),1) order by cs.course;

          There are 458 sections that don't have a matching count, let alone duplicate id's. I've run this a few times since the implementation of the script and the number of sections has risen. One example row result is;
          course| cs.id | counts| cm data | sequence data
          28512 | 165870 | 3 | 1 |

          {513711}

          | 915334,915375,915376

          They don't match. Are they supposed to in all cases or are there situations where these will be different that aren't involved with this bug?

          Show
          Russell Smith added a comment - - edited I would like to try and understand what's going on here. We've experienced this issue and applied the part of the patch that isn't the unit-test or upgrade. No failures of subjects have been reported since. To investigate which subjects were affected, I've created a modified version of the sql presented in the testing instructions. select cs.course, cs.id, array_upper(string_to_array(sequence,','),1), other.count, other.id_array, sequence from mdl_course_sections cs JOIN (select course, section, count(id), array_agg(id) as id_array from mdl_course_modules group by course, section) other ON cs.id = other.section where other.count <> array_upper(string_to_array(sequence,','),1) order by cs.course; There are 458 sections that don't have a matching count, let alone duplicate id's. I've run this a few times since the implementation of the script and the number of sections has risen. One example row result is; course| cs.id | counts| cm data | sequence data 28512 | 165870 | 3 | 1 | {513711} | 915334,915375,915376 They don't match. Are they supposed to in all cases or are there situations where these will be different that aren't involved with this bug?
          Hide
          Andrew Nicols added a comment -

          Hi Russell,

          Without the upgrade script, the existing data won't be fixed. As you move course_modules around, they should gradually correct their own data, but not all of the other data in the sequence. These two patches were designed to go together and you may still see issues with sequences not moving correctly if you don't apply both.

          Looking at your SQL, the WHERE clause tests on the overall count of IDs in the sequence, rather than the content of the sequence so your results may vary and you may get both false positives and negatives.

          I am intrigued as to how the result that you included occurred as the section.sequence data doesn't include the module id which actually belongs to that section. From my diagnosis of this bug, this should not have happened - module IDs are always added to the sequence, but not always taken away.

          Another thing that I've seen on our live data is a very small number (2) of sections which contain sequence data with modules which have been removed. These are all pretty old course_modules so I suspect that this may be residue from an old bug which has now been fixed. The upgrade script will also fix these scenarios.

          I'd suggest running the upgrade on your test environment and seeing if you still continue to see a rising number from that query.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Russell, Without the upgrade script, the existing data won't be fixed. As you move course_modules around, they should gradually correct their own data, but not all of the other data in the sequence. These two patches were designed to go together and you may still see issues with sequences not moving correctly if you don't apply both. Looking at your SQL, the WHERE clause tests on the overall count of IDs in the sequence, rather than the content of the sequence so your results may vary and you may get both false positives and negatives. I am intrigued as to how the result that you included occurred as the section.sequence data doesn't include the module id which actually belongs to that section. From my diagnosis of this bug, this should not have happened - module IDs are always added to the sequence, but not always taken away. Another thing that I've seen on our live data is a very small number (2) of sections which contain sequence data with modules which have been removed. These are all pretty old course_modules so I suspect that this may be residue from an old bug which has now been fixed. The upgrade script will also fix these scenarios. I'd suggest running the upgrade on your test environment and seeing if you still continue to see a rising number from that query. Andrew
          Hide
          Andrew Nicols added a comment -

          Rebased all branches for integration.

          Show
          Andrew Nicols added a comment - Rebased all branches for integration.
          Hide
          Andrew Nicols added a comment -

          I've done some testing on a copy of one of our production databases. Everything appears to be correct in the new output - again there were some long-since-deleted modules listed in the sequence field before I applied the update and this has been updated to only show current modules.

          I'll leave this waiting for peer review, but this could really do with being integrated ASAP because it is causes data corruption and loss of functionality to anyone using 2.3.4+.

          Show
          Andrew Nicols added a comment - I've done some testing on a copy of one of our production databases. Everything appears to be correct in the new output - again there were some long-since-deleted modules listed in the sequence field before I applied the update and this has been updated to only show current modules. I'll leave this waiting for peer review, but this could really do with being integrated ASAP because it is causes data corruption and loss of functionality to anyone using 2.3.4+.
          Hide
          Dan Marsden added a comment -

          pushing this up for integration as it MUST be resolved this week and we need to make sure HQ includes it - this is causing major issues for people and the longer it is left, the more corrupt 2.3 installs become.

          Show
          Dan Marsden added a comment - pushing this up for integration as it MUST be resolved this week and we need to make sure HQ includes it - this is causing major issues for people and the longer it is left, the more corrupt 2.3 installs become.
          Hide
          Damyon Wiese added a comment -

          The incorrect sequences are written to the course backup files - but new courses created from the backups do not have duplicates (but understandably do have a confusing order for the activities).

          Show
          Damyon Wiese added a comment - The incorrect sequences are written to the course backup files - but new courses created from the backups do not have duplicates (but understandably do have a confusing order for the activities).
          Hide
          Damyon Wiese added a comment -

          Hi Andrew,

          I had a good look at the code and verified it on my own install and it looks correct.

          I've pushed this to 23, 24 and master as requested.

          Thanks!

          Show
          Damyon Wiese added a comment - Hi Andrew, I had a good look at the code and verified it on my own install and it looks correct. I've pushed this to 23, 24 and master as requested. Thanks!
          Hide
          Russell Smith added a comment -

          I'm not sure if this is the right place, but we have now run the upgrade script on a non-production environment and the sections have repaired themselves.

          However in line with Andrew Nicols question about the missing id's we are up to 513 rows. Running the query I posted recently shows examples like the following;

          course|id|array_upper|count|id_array|sequence
          28417|164751|6|4|

          {910671,910672,910673,910674}

          |910671,910672,910673,910674,910642,913715
          28523|166013|6|1|

          {11}

          |916414,916415,916416,916417,916418,916939

          Should I raise another issue to investigate this further?
          I also don't understand as my reading of the schema documentation makes me think that sequence and modules.id should be the same number and same ID's. sequence just has a different order. Ours are all over the place for some sections.

          Show
          Russell Smith added a comment - I'm not sure if this is the right place, but we have now run the upgrade script on a non-production environment and the sections have repaired themselves. However in line with Andrew Nicols question about the missing id's we are up to 513 rows. Running the query I posted recently shows examples like the following; course|id|array_upper|count|id_array|sequence 28417|164751|6|4| {910671,910672,910673,910674} |910671,910672,910673,910674,910642,913715 28523|166013|6|1| {11} |916414,916415,916416,916417,916418,916939 Should I raise another issue to investigate this further? I also don't understand as my reading of the schema documentation makes me think that sequence and modules.id should be the same number and same ID's. sequence just has a different order. Ours are all over the place for some sections.
          Hide
          Andrew Nicols added a comment -

          Russell, I'm not sure what you mean by:

          I also don't understand as my reading of the schema documentation makes me think that sequence and modules.id should be the same number and same ID's. sequence just has a different order.

          I'm not quite sure how/why you're seeing sequence issues still. Are you able to replicate this in a consistent fashion - can you describe how? Perhaps just limit to a single course and try to replicate?

          Show
          Andrew Nicols added a comment - Russell, I'm not sure what you mean by: I also don't understand as my reading of the schema documentation makes me think that sequence and modules.id should be the same number and same ID's. sequence just has a different order. I'm not quite sure how/why you're seeing sequence issues still. Are you able to replicate this in a consistent fashion - can you describe how? Perhaps just limit to a single course and try to replicate?
          Hide
          Trevor Cunnnigham added a comment - - edited

          Still noticing an issue with this. The upgrade goes well and all duplicates are removed from the course section sequence info. I have noticed in a few courses, however, that after the upgrade the module's section info does not match the course section sequence info.

          So as an example, I end up with..

          mdl_course_sections
          id | section | sequence
          307 | 0 | 380
          ...
          311 | 4 | 1049
          ...
          317 | 10 | 1048

          mdl_course_modules
          id | section
          380 | 307
          1048 | 308 (should be 317)
          1049 | 309 (should be 311)

          Since the course modules' section info doesn't match, when I move it I get duplicates occurring again. The mdl_course_sections sequence info appears to indicate the correct location, or the location the activity/resource was intended to appear in, so I think the issue is with updating the mdl_course_modules section info.

          When I manually update the mdl_course_modules section data to match the section id of the sequence that it is listed in, everything appears and works as expected, and I can move elements around freely without duplicate info coming up again.

          It seems to me that the course_module section data needs to be updated during the upgrade process as well, to reflect the course section id that relates to the sequence the module is actually in.

          Show
          Trevor Cunnnigham added a comment - - edited Still noticing an issue with this. The upgrade goes well and all duplicates are removed from the course section sequence info. I have noticed in a few courses, however, that after the upgrade the module's section info does not match the course section sequence info. So as an example, I end up with.. mdl_course_sections id | section | sequence 307 | 0 | 380 ... 311 | 4 | 1049 ... 317 | 10 | 1048 mdl_course_modules id | section 380 | 307 1048 | 308 (should be 317) 1049 | 309 (should be 311) Since the course modules' section info doesn't match, when I move it I get duplicates occurring again. The mdl_course_sections sequence info appears to indicate the correct location, or the location the activity/resource was intended to appear in, so I think the issue is with updating the mdl_course_modules section info. When I manually update the mdl_course_modules section data to match the section id of the sequence that it is listed in, everything appears and works as expected, and I can move elements around freely without duplicate info coming up again. It seems to me that the course_module section data needs to be updated during the upgrade process as well, to reflect the course section id that relates to the sequence the module is actually in.
          Hide
          Trevor Cunnnigham added a comment -

          I applied the attached code and re-ran the upgrade against my data set. Everything now matches between course_section sequence info and course_modules section info, so I can once again drag and drop without creating any duplicates.

          The code I inserted may belong in a better spot, but should give you the idea anyway. The relevant course_modules section info should be updated to match the final course_section sequence info.

          Show
          Trevor Cunnnigham added a comment - I applied the attached code and re-ran the upgrade against my data set. Everything now matches between course_section sequence info and course_modules section info, so I can once again drag and drop without creating any duplicates. The code I inserted may belong in a better spot, but should give you the idea anyway. The relevant course_modules section info should be updated to match the final course_section sequence info.
          Hide
          Russell Smith added a comment -

          I don't have instructions to reproduce. Maybe to explain a little more clearly.

          We have a copy of production with mismatches in it. We applied the upgrade and ran the SQL. I would have expected an answer of zero rows are the upgrade. Instead there were still a number is differences.

          Is it correct that the element count of sequence should match the element count of course_modules once a section is cleaned up by this patch?

          If that is the case, then I will have enough knowledge to correct it and start looking for places and reasons it might be going awry.

          In support of Trevor's comments, I wanted to be clear about the expected outcomes so I could be confident about the patch.

          I will review Trevor's work today and see what happens in our environment in that situation.

          Show
          Russell Smith added a comment - I don't have instructions to reproduce. Maybe to explain a little more clearly. We have a copy of production with mismatches in it. We applied the upgrade and ran the SQL. I would have expected an answer of zero rows are the upgrade. Instead there were still a number is differences. Is it correct that the element count of sequence should match the element count of course_modules once a section is cleaned up by this patch? If that is the case, then I will have enough knowledge to correct it and start looking for places and reasons it might be going awry. In support of Trevor's comments, I wanted to be clear about the expected outcomes so I could be confident about the patch. I will review Trevor's work today and see what happens in our environment in that situation.
          Hide
          Andrew Nicols added a comment -

          Hi Russell,

          Ideally yes, this should now produce no differences at all. There are two
          potential cases where you may see a non-zero result, depending on how your
          code works. One of these issues is not an issue at all, but a quirk of the
          way that core handles course modules in deleted sections (orphaned
          activities). The other is a completely different breaking of your data and
          this patch was never designed to handle it.

          Basically, the code works by retrieving a course_section, retrieving all
          course modules which match both the section ID, and course ID, and then
          attempts to rebuild the section sequence data from those fields.

          The potential edge case comes in where a course_module has been associated
          with a course_section belonging to a different course. In this case, there is no
          'right' answer and there is something else wrong with your data which these
          patches were not designed to address. It should be relatively trivial to
          work out if you're affected by this one by just comparing the section, and
          course details for some of the affected course_modules and the sections
          that they believe they belong to.

          Trevor's patch uses the $clearcaches state variable, which means it's
          actually the original version of this patch which I've already stated does
          slightly the wrong thing. It has been completely rewritten and the edge
          case which Trevor looks to be accounting for no longer exists.

          Please verify that you are running with the correct patches applied.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Russell, Ideally yes, this should now produce no differences at all. There are two potential cases where you may see a non-zero result, depending on how your code works. One of these issues is not an issue at all, but a quirk of the way that core handles course modules in deleted sections (orphaned activities). The other is a completely different breaking of your data and this patch was never designed to handle it. Basically, the code works by retrieving a course_section, retrieving all course modules which match both the section ID, and course ID, and then attempts to rebuild the section sequence data from those fields. The potential edge case comes in where a course_module has been associated with a course_section belonging to a different course. In this case, there is no 'right' answer and there is something else wrong with your data which these patches were not designed to address. It should be relatively trivial to work out if you're affected by this one by just comparing the section, and course details for some of the affected course_modules and the sections that they believe they belong to. Trevor's patch uses the $clearcaches state variable, which means it's actually the original version of this patch which I've already stated does slightly the wrong thing. It has been completely rewritten and the edge case which Trevor looks to be accounting for no longer exists. Please verify that you are running with the correct patches applied. Cheers, Andrew
          Hide
          Adrian Greeve added a comment -

          Tested pre and post patch on the 2.3, 2.4 and master integration branches.
          I replicated the problem and the patch worked as described.
          No problems found.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested pre and post patch on the 2.3, 2.4 and master integration branches. I replicated the problem and the patch worked as described. No problems found. Test passed.
          Hide
          Russell Smith added a comment -

          Thanks Andrew for your persistent answers. They have helped a lot. I've discovered the issues and why I'm getting strange results.

          moodle=# select course, section, count(cm.id), array_agg(cm.id) as id_array from mdl_course_modules cm where section = 145747 group by course, section
          ;
          course | section | count | id_array
          -------------------+----------------
          16264 | 145747 | 1 |

          {514313}

          26815 | 145747 | 2 |

          {916432,916660}

          We have a situation where a section is linked to two separate courses in course_modules. This explains why I get the results I do above, the join is finding the failing match and showing that. So that looks like some nasty issue that I'll investigate. Thanks for all your input. I've learnt a lot to help with debugging this stuff and will be able to make better reports in the future.

          Show
          Russell Smith added a comment - Thanks Andrew for your persistent answers. They have helped a lot. I've discovered the issues and why I'm getting strange results. moodle=# select course, section, count(cm.id), array_agg(cm.id) as id_array from mdl_course_modules cm where section = 145747 group by course, section ; course | section | count | id_array ------- ------- ----- + ---------------- 16264 | 145747 | 1 | {514313} 26815 | 145747 | 2 | {916432,916660} We have a situation where a section is linked to two separate courses in course_modules. This explains why I get the results I do above, the join is finding the failing match and showing that. So that looks like some nasty issue that I'll investigate. Thanks for all your input. I've learnt a lot to help with debugging this stuff and will be able to make better reports in the future.
          Hide
          Andrew Nicols added a comment -

          Hi Russell,
          Glad that you've got to the bottom of it. Just wanted to point out that you have to be careful when correcting this one as a course module has a context, and a context has parent information. Both of these can be used in things like capability checks, etc so they are quite important.
          Andrew

          Show
          Andrew Nicols added a comment - Hi Russell, Glad that you've got to the bottom of it. Just wanted to point out that you have to be careful when correcting this one as a course module has a context, and a context has parent information. Both of these can be used in things like capability checks, etc so they are quite important. Andrew
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon
          Hide
          Marina Glancy added a comment -
          Show
          Marina Glancy added a comment - Example of how this script can make course modules completely unaccessible is described here: https://tracker.moodle.org/browse/MDL-38228?focusedCommentId=244855&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-244855

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              25 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: