Moodle
  1. Moodle
  2. MDL-30466

error while writing to database while restoring a backed up course that uses course completion

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Create a course with course completion tracking
      2. Create a database activity and set "Require grade" in Activity completion to "Student must receive a grade to complete this activity"
      3. Backup and try restore
      4. no error should be encountered.

      Also, run test from MDL-28116 and MDLQA-1356
      NOTE:
      Make sure to test on 20_STABLE (if integrated)

      Show
      Create a course with course completion tracking Create a database activity and set "Require grade" in Activity completion to "Student must receive a grade to complete this activity" Backup and try restore no error should be encountered. Also, run test from MDL-28116 and MDLQA-1356 NOTE: Make sure to test on 20_STABLE (if integrated)
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-30466
    • Rank:
      33137

      Description

      I set up a course as per the instructions in MDLQA-1356. I had previously run an unrelated QA test testing course completion so it contained a database activity that required students to get a grade to before the activity would be marked complete. One student had completed the activity.

      I performed a full back up then restored into a new course and got the following error.

      Error writing to database

      More information about this error

      Debug info: Duplicate entry '3-8' for key 'mdl_courmoducomp_usecou_uix'
      INSERT INTO mdl_course_modules_completion (userid,completionstate,viewed,timemodified,coursemoduleid) VALUES(?,?,?,?,?)
      [array (
      0 => '3',
      1 => '1',
      2 => '0',
      3 => 1322192283,
      4 => 8,
      )]
      Stack trace:
      
          line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown
          line 893 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          line 935 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
          line 2489 of /backup/moodle2/restore_stepslib.php: call to mysqli_native_moodle_database->insert_record()
          line 131 of /backup/util/plan/restore_structure_step.class.php: call to restore_userscompletion_structure_step->process_completion()
          line 103 of /backup/util/helper/restore_structure_parser_processor.class.php: call to restore_structure_step->process()
          line 125 of /backup/util/xml/parser/processors/grouped_parser_processor.class.php: call to restore_structure_parser_processor->dispatch_chunk()
          line 91 of /backup/util/helper/restore_structure_parser_processor.class.php: call to grouped_parser_processor->postprocess_chunk()
          line 148 of /backup/util/xml/parser/processors/simplified_parser_processor.class.php: call to restore_structure_parser_processor->postprocess_chunk()
          line 92 of /backup/util/xml/parser/processors/progressive_parser_processor.class.php: call to simplified_parser_processor->process_chunk()
          line 169 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser_processor->receive_chunk()
          line 253 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->publish()
          line ? of unknownfile: call to progressive_parser->end_tag()
          line 158 of /backup/util/xml/parser/progressive_parser.class.php: call to xml_parse()
          line 137 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->parse()
          line 105 of /backup/util/plan/restore_structure_step.class.php: call to progressive_parser->process()
          line 153 of /backup/util/plan/base_task.class.php: call to restore_structure_step->execute()
          line 182 of /backup/moodle2/restore_activity_task.class.php: call to base_task->execute()
          line 148 of /backup/util/plan/base_plan.class.php: call to restore_activity_task->execute()
          line 157 of /backup/util/plan/restore_plan.class.php: call to base_plan->execute()
          line 302 of /backup/controller/restore_controller.class.php: call to restore_plan->execute()
          line 147 of /backup/util/ui/restore_ui.class.php: call to restore_controller->execute_plan()
          line 46 of /backup/restore.php: call to restore_ui->execute()
      

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Discovered this while running a QA test

          Show
          Andrew Davis added a comment - Discovered this while running a QA test
          Hide
          Rajesh Taneja added a comment -

          Hello Andrew,

          Can you please provide backup file, which you tried to restore.

          Show
          Rajesh Taneja added a comment - Hello Andrew, Can you please provide backup file, which you tried to restore.
          Hide
          Andrew Davis added a comment -

          Rajesh was able to reproduce this before I got him the backup file.

          Show
          Andrew Davis added a comment - Rajesh was able to reproduce this before I got him the backup file.
          Hide
          Sam Marshall added a comment -

          I'm OK with the change (it doubles the number of queries made when restoring completion data, which is potentially a large number of queries - typically on order of 'number of students' * 'number of activities', so something like 100,000 is feasible, and this would make it 200,000 - but if you restore user data on a large course you're asking for trouble anyway).

          But! I'm a bit confused as to why it is necessary - the backup should not contain duplicate rows any more (since that fix quoted in the comment in the code), and if you're restoring to a new course, there shouldn't be any there. Even if you're restoring to an existing course, it should be creating new cmids I think.

          I checked the attached backup file and it doesn't contain duplicate completion rows (only completion data is in the 'data' activity, and it is for 4 different users).

          Could you explain why it's happening?

          If there is a good reason that I just haven't spotted, feel free to submit to integration, the change looks fine to me. I just wonder if there's some possibility there might be another bug this is hiding (like, it could be somehow running this multiple times for the same entry?).

          Show
          Sam Marshall added a comment - I'm OK with the change (it doubles the number of queries made when restoring completion data, which is potentially a large number of queries - typically on order of 'number of students' * 'number of activities', so something like 100,000 is feasible, and this would make it 200,000 - but if you restore user data on a large course you're asking for trouble anyway). But! I'm a bit confused as to why it is necessary - the backup should not contain duplicate rows any more (since that fix quoted in the comment in the code), and if you're restoring to a new course, there shouldn't be any there. Even if you're restoring to an existing course, it should be creating new cmids I think. I checked the attached backup file and it doesn't contain duplicate completion rows (only completion data is in the 'data' activity, and it is for 4 different users). Could you explain why it's happening? If there is a good reason that I just haven't spotted, feel free to submit to integration, the change looks fine to me. I just wonder if there's some possibility there might be another bug this is hiding (like, it could be somehow running this multiple times for the same entry?).
          Hide
          Sam Marshall added a comment -

          ah sorry just noticed test steps - grade completion? OK, I guess what's happening is, it is restoring grades before it restores completion data, and then it recalculates the grade completion (at the point where it sets the new grade value). That's kind of ugly, but I guess it is not a bug so this fix is correct.

          Show
          Sam Marshall added a comment - ah sorry just noticed test steps - grade completion? OK, I guess what's happening is, it is restoring grades before it restores completion data, and then it recalculates the grade completion (at the point where it sets the new grade value). That's kind of ugly, but I guess it is not a bug so this fix is correct.
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam
          I haven't removed after_execute() function, do you want me remove this as part of the fix?

          FYI:
          @integrators: 28116 was not cherry-picked on 20_STABLE, but the problem is visible on 20_STABLE as well. Please feel free to leave/integrate on 20_STABLE.

          Show
          Rajesh Taneja added a comment - Thanks Sam I haven't removed after_execute() function, do you want me remove this as part of the fix? FYI: @integrators: 28116 was not cherry-picked on 20_STABLE, but the problem is visible on 20_STABLE as well. Please feel free to leave/integrate on 20_STABLE.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Integrated to 20, 21 and master, thanks R & S!

          Edited: I've deleted the now empty after_execute() method with extra commit in 21 and master.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Integrated to 20, 21 and master, thanks R & S! Edited: I've deleted the now empty after_execute() method with extra commit in 21 and master.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nobody tested this, MDLQA-1356 will (the method is 100% the same now in all branches, yay, risks!)

          Show
          Eloy Lafuente (stronk7) added a comment - Nobody tested this, MDLQA-1356 will (the method is 100% the same now in all branches, yay, risks!)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days.

          Thanks for the hard work! Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days. Thanks for the hard work! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: