Moodle
  1. Moodle
  2. MDL-32061

Backup is broken if there is a lesson with attempts in the course

    Details

    • Rank:
      38753

      Description

      -> Backup a course which has atleast one lesson with attempts
      -> Try to restore as a new course
      Following error is generated :-

      Debug info: Column 'answerid' cannot be null
      INSERT INTO mdl_lesson_attempts (userid,retry,correct,useranswer,timeseen,lessonid,pageid,answerid) VALUES(?,?,?,?,?,?,?,?)
      [array (
      0 => '5',
      1 => '0',
      2 => '0',
      3 => 'O:8:"stdClass":5:{s:4:"sent";i:0;s:6:"graded";i:0;s:5:"score";i:0;s:6:"answer";s:9:"sadsadsad";s:8:"response";s:0:"";}',
      4 => 1331524772,
      5 => 5,
      6 => 7,
      7 => NULL,
      )]
      Stack trace:
      
          line 413 of /lib/dml/moodle_database.php: dml_write_exception thrown
          line 901 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          line 943 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
          line 122 of /mod/lesson/backup/moodle2/restore_lesson_stepslib.php: call to mysqli_native_moodle_database->insert_record()
          line 131 of /backup/util/plan/restore_structure_step.class.php: call to restore_lesson_activity_structure_step->process_lesson_attempt()
          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 192 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 310 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
          Ankit Agarwal added a comment -

          raising the priority to blocker as this needs immediate attention

          Show
          Ankit Agarwal added a comment - raising the priority to blocker as this needs immediate attention
          Hide
          Michael de Raadt added a comment -

          I think there must be some more specific circumstanes involved here.

          Show
          Michael de Raadt added a comment - I think there must be some more specific circumstanes involved here.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I bet this is a recent regression introduced recently. My test course has currently lessons with attempts and it started failing a few weeks ago. Although I use it often, I cannot guarantee when it was successfully executed last time, though.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I bet this is a recent regression introduced recently. My test course has currently lessons with attempts and it started failing a few weeks ago. Although I use it often, I cannot guarantee when it was successfully executed last time, though. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Offtopic: recent & recently. Although and though, lol. Sorry!

          Show
          Eloy Lafuente (stronk7) added a comment - Offtopic: recent & recently. Although and though, lol. Sorry!
          Hide
          Sam Chaffee added a comment -

          It looks like this bug is regression from MDL-31589. I believe that the problem is that inserting the lesson_answer records during restore was moved to the after_execute() method along with the corresponding set_mapping() call that tracks the new answer id. Therefore, when execution of process_lesson_attempt reaches this line:

          $data->answerid = $this->get_new_parentid('lesson_answer');
          

          get_new_parentid() returns NULL, since there is no mapped id for the answer yet. This issue looks like it will affect any lesson being restored with attempts.

          One possible fix is to delay the lesson_attempt record restore by also moving it into the after_execute() method since no other process methods use the new lesson attempt id.

          Show
          Sam Chaffee added a comment - It looks like this bug is regression from MDL-31589 . I believe that the problem is that inserting the lesson_answer records during restore was moved to the after_execute() method along with the corresponding set_mapping() call that tracks the new answer id. Therefore, when execution of process_lesson_attempt reaches this line: $data->answerid = $ this ->get_new_parentid('lesson_answer'); get_new_parentid() returns NULL, since there is no mapped id for the answer yet. This issue looks like it will affect any lesson being restored with attempts. One possible fix is to delay the lesson_attempt record restore by also moving it into the after_execute() method since no other process methods use the new lesson attempt id.
          Hide
          Chris Follin added a comment -

          Changing "non specific relationship" to a regression.

          Show
          Chris Follin added a comment - Changing "non specific relationship" to a regression.
          Hide
          Andrew Nicols added a comment -

          Agreed - this does look like a regression caused by MDL-31589. Sorry for missing this case.

          Unfortunately, as I recall, it's not possible to sort XML data as it's parsed so the only way to correctly handle mis-ordered lesson answers is to store them for later processing in the after_execute function so reverting isn't really a solution.

          I can only give it a cursory glance now and will be able to take a proper look on Thursday morning, but the solution that you've suggested looks to be the most appropriate and I'd say that it's the right way given the limitations of the XML parser.

          My only comments on the patch actually relate to the comments. IIRC the code checker now enforce a space after the //, and it's probably not necessary to put the bug number in the comments, though that's my personal opinion.

          As I say, I'll be able to take a more detailed look on Thursday and will make it my first priority of the morning.

          For testing, I'd suggest adding in the testing instructions for MDL-31589 as well as the instructions specific to this issue.

          Thanks!

          Show
          Andrew Nicols added a comment - Agreed - this does look like a regression caused by MDL-31589 . Sorry for missing this case. Unfortunately, as I recall, it's not possible to sort XML data as it's parsed so the only way to correctly handle mis-ordered lesson answers is to store them for later processing in the after_execute function so reverting isn't really a solution. I can only give it a cursory glance now and will be able to take a proper look on Thursday morning, but the solution that you've suggested looks to be the most appropriate and I'd say that it's the right way given the limitations of the XML parser. My only comments on the patch actually relate to the comments. IIRC the code checker now enforce a space after the //, and it's probably not necessary to put the bug number in the comments, though that's my personal opinion. As I say, I'll be able to take a more detailed look on Thursday and will make it my first priority of the morning. For testing, I'd suggest adding in the testing instructions for MDL-31589 as well as the instructions specific to this issue. Thanks!
          Hide
          Andrew Nicols added a comment -

          This should cherry-pick cleanly to all stable branches

          Show
          Andrew Nicols added a comment - This should cherry-pick cleanly to all stable branches
          Hide
          Andrew Nicols added a comment -

          I've updated Sam's commit to add some improve the comments and explain what's going on a little more.

          Confirmed fix on master.

          Show
          Andrew Nicols added a comment - I've updated Sam's commit to add some improve the comments and explain what's going on a little more. Confirmed fix on master.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 to send this straight to integration once the in_memory storage is replaced by DB storage as commented @ HQ chat. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - +1 to send this straight to integration once the in_memory storage is replaced by DB storage as commented @ HQ chat. Thanks!
          Hide
          Andrew Nicols added a comment -

          Updated based on Eloy's comments.

          Also discovered MDL-32461 whilst testing so have provided an alternative test backup file for testing

          Show
          Andrew Nicols added a comment - Updated based on Eloy's comments. Also discovered MDL-32461 whilst testing so have provided an alternative test backup file for testing
          Hide
          Andrew Nicols added a comment -

          Sending straight to integration as per Eloy's request

          Show
          Andrew Nicols added a comment - Sending straight to integration as per Eloy's request
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! (21, 22 and master)

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (21, 22 and master)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing this, I've tested your backup against master but my own one against 21 and 22 because onto older versions is not recommended (my backup file is 2.0 one and also had lesson attempts). Everything looked ok within the restored lesson.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing this, I've tested your backup against master but my own one against 21 and 22 because onto older versions is not recommended (my backup file is 2.0 one and also had lesson attempts). Everything looked ok within the restored lesson.
          Hide
          Dan Poltawski added a comment -

          Bonza mate!

          Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

          Hooroo

          Show
          Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: