Moodle
  1. Moodle
  2. MDL-32499

Fatal error trying to restore a course containing an assignment using a rubric

    Details

    • Testing Instructions:
      Hide
      1. Restore the backup attached to this issue in a new course selecting all the activities
      2. It SHOULD NOT crash
      3. Create / goto a course with student/s
      4. Add an assignment (the old or the new one) with rubrics as a grading method
      5. Create a rubric with 3 criteria
      6. Grade a user using the rubric
      7. Edit the rubric and delete 2 of the 3 criteria, you will receive a warning, but click the continue button
      8. Backup the course
      9. Extract the backup into a folder and open the activities/assignment_??/grading.xml file of the graded activity, there SHOULD NOT be a filling tag pointing to a non defined criterion
      10. Restore the course, it SHOULD NOT crash
      Show
      Restore the backup attached to this issue in a new course selecting all the activities It SHOULD NOT crash Create / goto a course with student/s Add an assignment (the old or the new one) with rubrics as a grading method Create a rubric with 3 criteria Grade a user using the rubric Edit the rubric and delete 2 of the 3 criteria, you will receive a warning, but click the continue button Backup the course Extract the backup into a folder and open the activities/assignment_??/grading.xml file of the graded activity, there SHOULD NOT be a filling tag pointing to a non defined criterion Restore the course, it SHOULD NOT crash
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32499_master
    • Rank:
      39391

      Description

      As part of my review for MDL-31270 I have been working on creating a test suite of assignments that I can upgrade.
      As part of that test suite I have a course that contains several assignments, a couple of which are using advanced grading, some using rubrics, and some using the marking guide plugin (MDL-31731) that will be arriving in master in the next couple of weeks in time for 2.3.
      I've just tried restoring a backup of the course that I have just taken and found that I get the following fatal error.

      Debug info: ERROR: duplicate key value violates unique constraint "mdl_gradrubrfill_inscri_uix"
      DETAIL: Key (instanceid, criterionid)=(34, 0) already exists.
      INSERT INTO mdl_gradingform_rubric_fillings (criterionid,levelid,remark,remarkformat,instanceid) VALUES($1,$2,$3,$4,$5) RETURNING id
      [array (
      'criterionid' => 0,
      'levelid' => 0,
      'remark' => 'Good',
      'remarkformat' => '0',
      'instanceid' => 34,
      )]
      Stack trace:

      line 413 of /lib/dml/moodle_database.php: dml_write_exception thrown
      line 237 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      line 795 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
      line 847 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->insert_record_raw()
      line 112 of /grade/grading/form/rubric/backup/moodle2/restore_gradingform_rubric_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_gradingform_rubric_plugin->process_gradinform_rubric_filling()
      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 187 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()

      Of course to be sure I can reproduce it I have tried to restore the course a couple of times and in a couple of different ways but always the same outcome.
      I'll attach my course backup after I create this issue so that you can test it out as well, please note though you will need to install the marking guide plugin (MDL-31731) in order to test this.
      The restore may work without it, it may not, I don't know but either way its not the issue at hand

      Cheers
      Sam

        Activity

        Hide
        Sam Hemelryk added a comment -

        OK I've attached the course backup now, and I've raised the priority to critical.
        It's trying to insert a record with criteriaid = 0 which really can't be good.

        Not a blocker as I can work around it by snapshotting my database, running the upgrade, and then restoring things so that I can do it over and over again (gotta love testing)

        Show
        Sam Hemelryk added a comment - OK I've attached the course backup now, and I've raised the priority to critical. It's trying to insert a record with criteriaid = 0 which really can't be good. Not a blocker as I can work around it by snapshotting my database, running the upgrade, and then restoring things so that I can do it over and over again (gotta love testing)
        Hide
        Marina Glancy added a comment -

        David, can you please look at it

        Show
        Marina Glancy added a comment - David, can you please look at it
        Hide
        Sam Hemelryk added a comment -

        Hmm don't start work on this quite yet guys.

        I had a quick look into this and found I could only reproduce it on the first site I tried on.
        That site had been set up with 10 off assignments all with various settings and various grading methods.
        To test this again I just created a second new site from scratch, still with only 1 course but with only 2 assignments, one using rubrics, the other using marking guides.
        Both had several submission and both were graded.

        I'm not too sure whether it is being caused by a bug in the assign module, or being caused by something I've done during setup/submitting/grading.
        I'll be testing the new and old assignment modules more in the next couple of days so I will leave this open.
        If I find I can reproduce it then hopefully I will have more information, otherwise this can be closed as a freak occurrence.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hmm don't start work on this quite yet guys. I had a quick look into this and found I could only reproduce it on the first site I tried on. That site had been set up with 10 off assignments all with various settings and various grading methods. To test this again I just created a second new site from scratch, still with only 1 course but with only 2 assignments, one using rubrics, the other using marking guides. Both had several submission and both were graded. I'm not too sure whether it is being caused by a bug in the assign module, or being caused by something I've done during setup/submitting/grading. I'll be testing the new and old assignment modules more in the next couple of days so I will leave this open. If I find I can reproduce it then hopefully I will have more information, otherwise this can be closed as a freak occurrence. Cheers Sam
        Hide
        Mark Nielsen added a comment -

        Sam, we have run into this very same problem. I'm looking into it now to see if I can find the cause.

        Show
        Mark Nielsen added a comment - Sam, we have run into this very same problem. I'm looking into it now to see if I can find the cause.
        Hide
        Sam Hemelryk added a comment -

        Thanks for commenting Mark, its good to know this is an issue and not just a problem that I managed to create myself.
        I'll be sure to leave this issue open now so that David, or Marina can look into it for us.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks for commenting Mark, its good to know this is an issue and not just a problem that I managed to create myself. I'll be sure to leave this issue open now so that David, or Marina can look into it for us. Cheers Sam
        Hide
        Mark Nielsen added a comment - - edited

        Going to try to explain my findings the best that I can...

        • On failure, both levelid and criterionid are false after attempting to get their mapped IDs in restore_gradingform_rubric_plugin::process_gradinform_rubric_filling
        • I searched for the criterionid that it was trying to get in the backup XML, and it didn't exist. EG: <criterion id="23">

        After checking and double checking how things are being backed up, my best guess is that these rubric fillings that are failing to restore should never have been backed up because their corresponding criterionid and levelid no longer exist. These fillings are backed up on instanceid only and the validity of the criterionid and levelids are never confirmed.

        So, I searched for 'gradingform_rubric_criteria' under grade/grading/form and the only DB criteria delete I found does not include deletes to the gradingform_rubric_fillings table (Around line 230 in grade/grading/form/rubric/lib.php).

        So, proposed fixes:

        • Clean gradingform_rubric_fillings on criteria, instance or level deletion (or update accordingly)
        • On restore, in restore_gradingform_rubric_plugin::process_gradinform_rubric_filling, if levelid or criterionid are false, prevent insert. This makes sure that old backups will restore.
        • (Optional though would be good) Only backup fillings that have valid levelid and criterionids.
        Show
        Mark Nielsen added a comment - - edited Going to try to explain my findings the best that I can... On failure, both levelid and criterionid are false after attempting to get their mapped IDs in restore_gradingform_rubric_plugin::process_gradinform_rubric_filling I searched for the criterionid that it was trying to get in the backup XML, and it didn't exist. EG: <criterion id="23"> After checking and double checking how things are being backed up, my best guess is that these rubric fillings that are failing to restore should never have been backed up because their corresponding criterionid and levelid no longer exist. These fillings are backed up on instanceid only and the validity of the criterionid and levelids are never confirmed. So, I searched for 'gradingform_rubric_criteria' under grade/grading/form and the only DB criteria delete I found does not include deletes to the gradingform_rubric_fillings table (Around line 230 in grade/grading/form/rubric/lib.php). So, proposed fixes: Clean gradingform_rubric_fillings on criteria, instance or level deletion (or update accordingly) On restore, in restore_gradingform_rubric_plugin::process_gradinform_rubric_filling, if levelid or criterionid are false, prevent insert. This makes sure that old backups will restore. (Optional though would be good) Only backup fillings that have valid levelid and criterionids.
        Hide
        Mark Nielsen added a comment - - edited

        Considering the table structure of gradingform_rubric_fillings, it looks like levelid might not always be set? I'm not very familiar with rubrics just yet...

        Show
        Mark Nielsen added a comment - - edited Considering the table structure of gradingform_rubric_fillings, it looks like levelid might not always be set? I'm not very familiar with rubrics just yet...
        Hide
        Mark Nielsen added a comment - - edited

        This attached patch allowed my course to restore, haven't confirmed 100% if there is any data loss Also, patch doesn't address all of the problems and even the patch might need to also check levelid field.

        Show
        Mark Nielsen added a comment - - edited This attached patch allowed my course to restore, haven't confirmed 100% if there is any data loss Also, patch doesn't address all of the problems and even the patch might need to also check levelid field.
        Hide
        Mark Nielsen added a comment -

        Changed the version to 2.2.2, was able to reproduce in the Moodle2.2 branch

        Show
        Mark Nielsen added a comment - Changed the version to 2.2.2, was able to reproduce in the Moodle2.2 branch
        Hide
        Marina Glancy added a comment -

        (MD sending to STABLE)

        Show
        Marina Glancy added a comment - (MD sending to STABLE)
        Hide
        Michael de Raadt added a comment -

        This should be picked up in the next sprint.

        Show
        Michael de Raadt added a comment - This should be picked up in the next sprint.
        Hide
        David Monllaó added a comment -

        Adding pull branches and testing instructions

        Show
        David Monllaó added a comment - Adding pull branches and testing instructions
        Hide
        David Monllaó added a comment -

        Thanks for the tips Mark. After talking with Marina I've been able to reproduce the problem creating an assignment with rubrics, creating a rubric with 3 criterion, grading a user and editing the rubrics to remove 2 of the criterion. When the criteria is removed the gradingform_rubrics_fillings records remains in the database until the student is regraded (I'm not 100% sure why, but I guess it has something to do with the grading instances status) If a user performs a backup before the students regrading there are gradingform_rubrics_fillings pointing to non-existing records that are restored with id = 0, if there is more than one of it's records moodle throws the duplicate unique key exception.

        I modified both backup and restore process to ensure there is always an existing criterionid, the non-empty levelid checking is performed in the backup process nesting both elements.

        Show
        David Monllaó added a comment - Thanks for the tips Mark. After talking with Marina I've been able to reproduce the problem creating an assignment with rubrics, creating a rubric with 3 criterion, grading a user and editing the rubrics to remove 2 of the criterion. When the criteria is removed the gradingform_rubrics_fillings records remains in the database until the student is regraded (I'm not 100% sure why, but I guess it has something to do with the grading instances status) If a user performs a backup before the students regrading there are gradingform_rubrics_fillings pointing to non-existing records that are restored with id = 0, if there is more than one of it's records moodle throws the duplicate unique key exception. I modified both backup and restore process to ensure there is always an existing criterionid, the non-empty levelid checking is performed in the backup process nesting both elements.
        Hide
        Mark Nielsen added a comment -

        Thanks so much for fixing!

        Show
        Mark Nielsen added a comment - Thanks so much for fixing!
        Hide
        Adrian Greeve added a comment -

        Hello David,

        The code looks fine. The testing instructions make sense to me. In general everything looks good.
        The comments that you have made in the code probably don't need the MDL number included. The commit log will have the mdl number.

        Thanks.

        Show
        Adrian Greeve added a comment - Hello David, The code looks fine. The testing instructions make sense to me. In general everything looks good. The comments that you have made in the code probably don't need the MDL number included. The commit log will have the mdl number. Thanks.
        Hide
        David Monllaó added a comment -

        Thanks for the comments Adrian, I removed the issue numbers

        Show
        David Monllaó added a comment - Thanks for the comments Adrian, I removed the issue numbers
        Hide
        Aparup Banerjee added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        David Monllaó added a comment -

        Hi, pull branches rebased against upstream

        Show
        David Monllaó added a comment - Hi, pull branches rebased against upstream
        Hide
        Sam Hemelryk added a comment -

        Thanks David, this has been integrated.
        Thanks for explaining you findings on the issue btw, it helps a lot during integration.

        Show
        Sam Hemelryk added a comment - Thanks David, this has been integrated. Thanks for explaining you findings on the issue btw, it helps a lot during integration.
        Hide
        Jason Fowler added a comment -

        all good there David

        Show
        Jason Fowler added a comment - all good there David
        Hide
        Dan Poltawski added a comment -

        asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ!

        Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

        Show
        Dan Poltawski added a comment - asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ! Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: