Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      Restore the file David attached. Or, for a more stringent test, make a backup from a different Moodle 2.0 course containing lots of questions, including matching questions, and restore that.

      Show
      Restore the file David attached. Or, for a more stringent test, make a backup from a different Moodle 2.0 course containing lots of questions, including matching questions, and restore that.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Rank:
      17652

      Description

      While testing MDL-22414 with the new QE2, I discovered some issues with the restore of the converted backup.

      Error writing to database
      
      Debug info: ERROR: null value in column "correctfeedback" violates not-null constraint
      INSERT INTO mdl_question_match (shuffleanswers,subquestions,question) VALUES($1,$2,$3) RETURNING id
      [array (
      'shuffleanswers' => '1',
      'subquestions' => '1,2,3',
      'question' => 1319,
      )]
      Stack trace:
      
          * line 394 of /lib/dml/moodle_database.php: dml_write_exception thrown
          * line 232 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
          * line 781 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
          * line 833 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->insert_record_raw()
          * line 80 of /question/type/match/backup/moodle2/restore_qtype_match_plugin.class.php: call to pgsql_native_moodle_database->insert_record()
          * line 131 of /backup/util/plan/restore_structure_step.class.php: call to restore_qtype_match_plugin->process_matchoptions()
          * 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 148 of /backup/util/plan/base_plan.class.php: call to base_task->execute()
          * line 157 of /backup/util/plan/restore_plan.class.php: call to base_plan->execute()
          * line 299 of /backup/controller/restore_controller.class.php: call to restore_plan->execute()
          * line 144 of /backup/util/ui/restore_ui.class.php: call to restore_controller->execute_plan()
          * line 45 of /backup/restore.php: call to restore_ui->execute()
      

      In the chat, Tim suggested that the restore process should supply defaults for the fields if they are not in the XML. Please note I did not have a chance to test further as this error stops the course restore.

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          This blocks the restore of my 1.9 backup reference course at the moment.

          Show
          David Mudrak added a comment - This blocks the restore of my 1.9 backup reference course at the moment.
          Hide
          David Mudrak added a comment -

          Attaching backup-refcourse.mbz which is a result of conversion of one 1.9 course. For the reference, I am attaching the 1.9 original, too.
          Please note that even though I tried to produce clean 2.0-like structures during the conversion, there might be some bugs that could influence the reported bug.

          Show
          David Mudrak added a comment - Attaching backup-refcourse.mbz which is a result of conversion of one 1.9 course. For the reference, I am attaching the 1.9 original, too. Please note that even though I tried to produce clean 2.0-like structures during the conversion, there might be some bugs that could influence the reported bug.
          Hide
          Tim Hunt added a comment -

          Eloy, please could you review this fix, and tell me if this is the right way to handle this sort of situation. Thanks.

          David, in the mean time, you could take this fix to un-block your work.

          Show
          Tim Hunt added a comment - Eloy, please could you review this fix, and tell me if this is the right way to handle this sort of situation. Thanks. David, in the mean time, you could take this fix to un-block your work.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm,

          1) If the fields exist since Moodle 2.0 "day 0", then restore must not invent defaults for them as far as ALL 2.0 backups must have them (unless they are missing because one bug, of course).

          2) If the fields are new (after 2.0 release, no matter if 2.0.x or 2.1.x), mandatory and without DB default then, for sure, restore must provide them, harcoded or whatever needs to be executed to calculate them.

          So, if the 3 conditions (new, not null and missing default) in 2) are true for the feedback[format] and shownumcorrect columns, then I think the code is ok.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, 1) If the fields exist since Moodle 2.0 "day 0", then restore must not invent defaults for them as far as ALL 2.0 backups must have them (unless they are missing because one bug, of course). 2) If the fields are new (after 2.0 release, no matter if 2.0.x or 2.1.x), mandatory and without DB default then, for sure, restore must provide them, harcoded or whatever needs to be executed to calculate them. So, if the 3 conditions (new, not null and missing default) in 2) are true for the feedback [format] and shownumcorrect columns, then I think the code is ok. Ciao
          Hide
          Tim Hunt added a comment -

          2. Almost applies. Some of the columns have database defaults, but in XMLDB, TEXT columns can't have defaults (https://github.com/timhunt/moodle/blob/master/question/type/match/db/install.xml).

          If I am setting $data->correctfeedback, it feels correct to me to also set $data->correctfeedbackformat.

          Since shownumcorrect has a DB default, I could delete that if statement, or I could leave it there to make the logic clear. Can you confirm what you would like me to do, then I will submit this for integration. Thanks.

          Show
          Tim Hunt added a comment - 2. Almost applies. Some of the columns have database defaults, but in XMLDB, TEXT columns can't have defaults ( https://github.com/timhunt/moodle/blob/master/question/type/match/db/install.xml ). If I am setting $data->correctfeedback, it feels correct to me to also set $data->correctfeedbackformat. Since shownumcorrect has a DB default, I could delete that if statement, or I could leave it there to make the logic clear. Can you confirm what you would like me to do, then I will submit this for integration. Thanks.
          Hide
          Tim Hunt added a comment -

          The fix for this bug is included in the branch for MDL-27639. Please see the integration info there.

          Show
          Tim Hunt added a comment - The fix for this bug is included in the branch for MDL-27639 . Please see the integration info there.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          marking as integrated (part of MDL-27639)

          Commits: d7ad6ac4efab812bbc07c69e13d3abc2328e15c9

          Show
          Eloy Lafuente (stronk7) added a comment - marking as integrated (part of MDL-27639 ) Commits: d7ad6ac4efab812bbc07c69e13d3abc2328e15c9
          Hide
          Sam Hemelryk added a comment -

          Thanks guys - test passed (restored David's and restored one I had)

          Show
          Sam Hemelryk added a comment - Thanks guys - test passed (restored David's and restored one I had)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Upstream, upstream, this is part of upstream, upstream... thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Upstream, upstream, this is part of upstream, upstream... thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: