Moodle
  1. Moodle
  2. MDL-26572

Course backup seems to produce different grades than the original course

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.0.3
    • Component/s: Backup, Gradebook
    • Labels:
    • Database:
      Any
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      16345

      Description

      Do a full course backup then restore into a new course.

      Comment from Elena Ivanova in MDLQA-706:
      Issue happens if you have SUM somewhere. In the restored course the category/course total with SUM will be set to 0.
      The rest of the aggregation methods seem to be OK.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          after chasing this through the backup/restore code it seems that the grade items are restored fine but then bad stuff happens the first time you view the gradebook after restoring the course from backup....

          Show
          Andrew Davis added a comment - after chasing this through the backup/restore code it seems that the grade items are restored fine but then bad stuff happens the first time you view the gradebook after restoring the course from backup....
          Hide
          Andrew Davis added a comment - - edited

          Ok, I have tracked down the root cause. It has to do with this function in /backup/util/plan/restore_structure_step.class.php

          /** * Return the new id of a mapping for the given itemname * */
          public function get_mappingid($itemname, $oldid) {
          $mapping = $this->get_mapping($itemname, $oldid);
          return $mapping ? $mapping->newitemid : false;
          }
          

          Note that if $oldid isnt found the function returns false. That means that if you do this...

          $data->outcomeid = $this->get_mappingid('outcome', $data->outcomeid);
          

          If $data->outcomeid is null it gets set to false. Then when $data is inserted into the database it gets inserted as 0 instead of null. Then, if that column has a "is null" type condition applied to it in a query, such as happens in grade_item::depends_on(), the column is incorrectly judged to be not null.

          This can be avoided by doing the following but having to do this everywhere I call get_mappingid() is kind of annoying.

          $data->outcomeid = $this->get_mappingid('outcome', $data->outcomeid);
          if ($data->outcomeid===false) {
          $data->outcomeid = null;
          }
          
          Show
          Andrew Davis added a comment - - edited Ok, I have tracked down the root cause. It has to do with this function in /backup/util/plan/restore_structure_step.class.php /** * Return the new id of a mapping for the given itemname * */ public function get_mappingid($itemname, $oldid) { $mapping = $ this ->get_mapping($itemname, $oldid); return $mapping ? $mapping->newitemid : false ; } Note that if $oldid isnt found the function returns false. That means that if you do this... $data->outcomeid = $ this ->get_mappingid('outcome', $data->outcomeid); If $data->outcomeid is null it gets set to false. Then when $data is inserted into the database it gets inserted as 0 instead of null. Then, if that column has a "is null" type condition applied to it in a query, such as happens in grade_item::depends_on(), the column is incorrectly judged to be not null. This can be avoided by doing the following but having to do this everywhere I call get_mappingid() is kind of annoying. $data->outcomeid = $ this ->get_mappingid('outcome', $data->outcomeid); if ($data->outcomeid=== false ) { $data->outcomeid = null ; }
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm,

          that function has suffered multiple modifications along its history (1 year). Initially it was returning the $oldid if no match was found, but later was decided to return "false" in case of no match found (because bool "false" seems to be the more correct value for that situation).

          The only possibility I can imagine is to return "NULL" if the requested id is "NULL" and return "0" if the requested id is "0". They don't collide with the meaning of "false" and won't be matching ever. But, in the other side... it will require to revisit all uses, because there are places where we are relying in returned values to decide execution flow.

          Another possibility could be to add one 3rd parameter to the function, able to specify which value we want to use if no match is found, so you will end with something like:

          $data->outcomeid = $this->get_mappingid('outcome', $data->outcomeid, NULL);
          

          It has the advantage of being 100x100 BC, so all the old uses will return the same bool "false" (default for param). And you will be able to alter that behavior for each use.

          How does that sound? Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, that function has suffered multiple modifications along its history (1 year). Initially it was returning the $oldid if no match was found, but later was decided to return "false" in case of no match found (because bool "false" seems to be the more correct value for that situation). The only possibility I can imagine is to return "NULL" if the requested id is "NULL" and return "0" if the requested id is "0". They don't collide with the meaning of "false" and won't be matching ever. But, in the other side... it will require to revisit all uses, because there are places where we are relying in returned values to decide execution flow. Another possibility could be to add one 3rd parameter to the function, able to specify which value we want to use if no match is found, so you will end with something like: $data->outcomeid = $ this ->get_mappingid('outcome', $data->outcomeid, NULL); It has the advantage of being 100x100 BC, so all the old uses will return the same bool "false" (default for param). And you will be able to alter that behavior for each use. How does that sound? Ciao
          Hide
          Andrew Davis added a comment -

          Ive gone with that idea. the optional third parameter that specifies what to return if the old id isnt found. Let me know if you can foresee any issues with what Ive done

          repo: git://github.com/andyjdavis/moodle.git
          branch: MDL-26572_backup_sum
          diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26572_backup_sum

          Show
          Andrew Davis added a comment - Ive gone with that idea. the optional third parameter that specifies what to return if the old id isnt found. Let me know if you can foresee any issues with what Ive done repo: git://github.com/andyjdavis/moodle.git branch: MDL-26572 _backup_sum diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26572_backup_sum
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Andrew,

          looks ok so far... the only point it that you shouldn't abuse that 3rd parameter and, I think you are doing so, for example in the grade_grades->userid column (that is defined as NOT NULL, so applying for NULLs there doesn't seem to have sense). Does it?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Andrew, looks ok so far... the only point it that you shouldn't abuse that 3rd parameter and, I think you are doing so, for example in the grade_grades->userid column (that is defined as NOT NULL, so applying for NULLs there doesn't seem to have sense). Does it? Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi again,

          I've already integrated PULL-378 and added 1-extra commit for implementing the 3rd param to both restore_plugin and restore_subplugin classes (wrappers over the restore_structure_step one).

          The only point remaining is if this should be really applied to so many fields (like the userid commented above), or only to the ones exhibiting the NULL problem.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi again, I've already integrated PULL-378 and added 1-extra commit for implementing the 3rd param to both restore_plugin and restore_subplugin classes (wrappers over the restore_structure_step one). The only point remaining is if this should be really applied to so many fields (like the userid commented above), or only to the ones exhibiting the NULL problem. Ciao
          Hide
          Andrew Davis added a comment -

          "looks ok so far... the only point it that you shouldn't abuse that 3rd parameter and, I think you are doing so, for example in the grade_grades->userid column (that is defined as NOT NULL, so applying for NULLs there doesn't seem to have sense). Does it?"

          I think maybe it does. If you cant have a null userid then at least youll get a fatal error if you try to insert the record. If we dont do that we need to add a check to the code otherwise we risk 0s being inserted ie orphan records. I think a fatal error is probably better than relying on us spotting all the places this could happen, missing one then having orphaned records inserted that we when then have to deal with later.

          Its a matter of opinion of course.

          Show
          Andrew Davis added a comment - "looks ok so far... the only point it that you shouldn't abuse that 3rd parameter and, I think you are doing so, for example in the grade_grades->userid column (that is defined as NOT NULL, so applying for NULLs there doesn't seem to have sense). Does it?" I think maybe it does. If you cant have a null userid then at least youll get a fatal error if you try to insert the record. If we dont do that we need to add a check to the code otherwise we risk 0s being inserted ie orphan records. I think a fatal error is probably better than relying on us spotting all the places this could happen, missing one then having orphaned records inserted that we when then have to deal with later. Its a matter of opinion of course.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, the point with 'userid' is that, on backup, all the different userids are annotated, so it should be impossible to have on record on restore not pointing to one userid having mapping.

          That's the point I think makes the case different from the outcomeid one (that is something optional).

          Doesn't hurt too much anyway, as you said, a matter of opinion. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, the point with 'userid' is that, on backup, all the different userids are annotated, so it should be impossible to have on record on restore not pointing to one userid having mapping. That's the point I think makes the case different from the outcomeid one (that is something optional). Doesn't hurt too much anyway, as you said, a matter of opinion. Ciao
          Hide
          David Mudrak added a comment -

          Tested. Thanks for the fix Andrew!

          Show
          David Mudrak added a comment - Tested. Thanks for the fix Andrew!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: