Moodle
  1. Moodle
  2. MDL-27988

ID Number activities into the calculated grade items are not restored in a full course restore

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1
    • Fix Version/s: 2.0.4
    • Component/s: Backup, Gradebook
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      mdlqa based. Basically:

      • make a backup of one course having some calculated columns based on others
      • restore the course. Formulae (and grades) should be ok (it was leading to error 100% of the times before the fix).
      Show
      mdlqa based. Basically: make a backup of one course having some calculated columns based on others restore the course. Formulae (and grades) should be ok (it was leading to error 100% of the times before the fix).
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull Master Branch:
      wip-MDL-27988-master
    • Rank:
      17626

      Description

      QA Test 2.1 Cycle 1 - 166

      The restore process is performed in a correct way on the categories, but the calculated grade items are not restored well. The ID numbers of activities are lost and a calculation error appear in the grade report.

      1. QA-test2-1-166 - Image1.png
        32 kB
      2. QA-test2-1-166 - Image2.png
        24 kB
      3. QA-test2-1-166 - Image3.png
        32 kB
      4. QA-test2-1-166 - Image4.png
        25 kB

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Promoting this issue as it is related to a QA test.

          Show
          Michael de Raadt added a comment - Promoting this issue as it is related to a QA test.
          Hide
          Michael de Raadt added a comment -

          Hi, Eloy.

          I assume you've been busy with integration. But have you had a chance to look at this?

          Michael;

          Show
          Michael de Raadt added a comment - Hi, Eloy. I assume you've been busy with integration. But have you had a chance to look at this? Michael;
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just been looking into this now.
          I'm certainly no expert on grades and I know only a little about restore however to me there are two clear bugs here:

          1. When restoring activity grade items if the idnumber has come from the course_modules table then it is not restored against the grade item despite being there in the backup. This isn't the big issue essentially this is just a side show bug.
          2. The calculation for gradeitems doesn't store the idnumber, before being saved to the database it appears the idnumbers in the calculation are being converted to the grade item id they reference. On restore we don't search and resolve those ids within a grade item calculation and as such calculated grade items within restored courses are almost guaranteed to be wrong.

          Shortly I'll attach a patch to fix both of these however it is probably best that someone who actually understands restore + grades review it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just been looking into this now. I'm certainly no expert on grades and I know only a little about restore however to me there are two clear bugs here: When restoring activity grade items if the idnumber has come from the course_modules table then it is not restored against the grade item despite being there in the backup. This isn't the big issue essentially this is just a side show bug. The calculation for gradeitems doesn't store the idnumber, before being saved to the database it appears the idnumbers in the calculation are being converted to the grade item id they reference. On restore we don't search and resolve those ids within a grade item calculation and as such calculated grade items within restored courses are almost guaranteed to be wrong. Shortly I'll attach a patch to fix both of these however it is probably best that someone who actually understands restore + grades review it. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Alright I've got a patch that fixes up this issue.
          It really isn't pretty but it does solve the two above bugs.

          repo: git://github.com/samhemelryk/moodle.git
          branch: wip-MDL-27988-master
          diff: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-27988-master

          In regards to testing I have tested this with the provided course backup (thanks Javier) but I haven't tested with anything else. I suppose at this point I just want those who are familiar with grade + restore to work out whether this solution is acceptable and factors in everything it should.

          The other thing to take into consideration is courses that have already been restored.
          It is possible to detect some problems with calculated grade items by checking that the grade items referenced in calculations relate to the same course that the referencing grade item belongs to... although there is still no way to work out what it should be we can at least instruct people to review the calculated grade items within courses.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Alright I've got a patch that fixes up this issue. It really isn't pretty but it does solve the two above bugs. repo: git://github.com/samhemelryk/moodle.git branch: wip- MDL-27988 -master diff: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-27988-master In regards to testing I have tested this with the provided course backup (thanks Javier) but I haven't tested with anything else. I suppose at this point I just want those who are familiar with grade + restore to work out whether this solution is acceptable and factors in everything it should. The other thing to take into consideration is courses that have already been restored. It is possible to detect some problems with calculated grade items by checking that the grade items referenced in calculations relate to the same course that the referencing grade item belongs to... although there is still no way to work out what it should be we can at least instruct people to review the calculated grade items within courses. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Is there a reason you haven't used the repo, branch and diff fields in tracker? And there's no testing instructions. Watch yourself Sam or the integrators will be very upset

          "@global moodle_database $DB"
          are we meant to put these in? I know theyre autogenerated but I see them so rarely I've got into the habit of stripping them out.

          I'll look at the actual code now...

          Show
          Andrew Davis added a comment - Is there a reason you haven't used the repo, branch and diff fields in tracker? And there's no testing instructions. Watch yourself Sam or the integrators will be very upset "@global moodle_database $DB" are we meant to put these in? I know theyre autogenerated but I see them so rarely I've got into the habit of stripping them out. I'll look at the actual code now...
          Hide
          Andrew Davis added a comment -

          //Because there potentially there will be an overlap of ids within the query and we
          // we substitue the wrong id.. safest way around this is the two step system

          Is it possible for you to add an example of a $grade_item->calculation where this two stage process is necessary?
          substitue eh?

          Show
          Andrew Davis added a comment - //Because there potentially there will be an overlap of ids within the query and we // we substitue the wrong id.. safest way around this is the two step system Is it possible for you to add an example of a $grade_item->calculation where this two stage process is necessary? substitue eh?
          Hide
          Andrew Davis added a comment -
          if (!$DB->record_exists_sql($sql, $params) && !$DB->record_exists('grade_items', array('courseid'=>$courseid, 'idnumber'=>$idnumber))) {
                          $idnumber = $data->idnumber;
                      }
          

          Shouldnt 'idnumber'=>$idnumber be 'idnumber'=>$data->idnumber

          Also, white space in those query params.

          Show
          Andrew Davis added a comment - if (!$DB->record_exists_sql($sql, $params) && !$DB->record_exists('grade_items', array('courseid'=>$courseid, 'idnumber'=>$idnumber))) { $idnumber = $data->idnumber; } Shouldnt 'idnumber'=>$idnumber be 'idnumber'=>$data->idnumber Also, white space in those query params.
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew,

          I've fixed up the typo in the comments, removed the @global from phpdocs, and replace $idnumber in the record exists with $data->idnumber.
          The @globals are an interesting one. I've got into the habit of manually adding them to the code the major code blocks that I write. Adding @globals is part of the phpdoc standard and it helps my netbeans remember what each is. However I have got in trouble for adding them to other people code as some people hate them (Tim for one).
          So I've got the habit of adding them to the libraries I maintain but never to other people's code. Either way the autogenerated ones should be removed or corrected (they have the wrong format I think? or at least use type or something).
          The whitespace within the query I couldn't spot.

          As for the substitution example imagine the following:
          Before backup you have three grade items:
          id: 31
          id: 32
          id: 33
          33 is a calculated grade item with the calculation: =##gi31##/##gi32##

          On backup that is all backed up as it is there and things are happy.

          On restore lets assume the 3 grade items are restored as follows:
          oldid: 31 - newid: 32
          oldid: 32 - newid: 33
          oldid: 33 - newid: 34

          When fixing the calculations preg_match_all finds two occurences 31, and 32 and processes the first, then the second.
          The code would be:
          $calculation = str_replace('##gi'.$oldid.'##', '##gi'.$newid.'##', $calculation);
          Or:
          $calculation = str_replace('##gi31##', '##gi32##', $calculation);
          $calculation = str_replace('##gi32##', '##gi33##', $calculation);

          The end outcome would be =##gi33##/##gi33## because of the clashing ids.
          The two step ensures that we never replace anything that has already been replaced.

          One other solution to this would be to walk or explode the calculation and work through things one by one, however this was quicker to write.

          The testing instructions and git diff fields I'll fill out if I put it up for integration

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Andrew, I've fixed up the typo in the comments, removed the @global from phpdocs, and replace $idnumber in the record exists with $data->idnumber. The @globals are an interesting one. I've got into the habit of manually adding them to the code the major code blocks that I write. Adding @globals is part of the phpdoc standard and it helps my netbeans remember what each is. However I have got in trouble for adding them to other people code as some people hate them (Tim for one). So I've got the habit of adding them to the libraries I maintain but never to other people's code. Either way the autogenerated ones should be removed or corrected (they have the wrong format I think? or at least use type or something). The whitespace within the query I couldn't spot. As for the substitution example imagine the following: Before backup you have three grade items: id: 31 id: 32 id: 33 33 is a calculated grade item with the calculation: =##gi31##/##gi32## On backup that is all backed up as it is there and things are happy. On restore lets assume the 3 grade items are restored as follows: oldid: 31 - newid: 32 oldid: 32 - newid: 33 oldid: 33 - newid: 34 When fixing the calculations preg_match_all finds two occurences 31, and 32 and processes the first, then the second. The code would be: $calculation = str_replace('##gi'.$oldid.'##', '##gi'.$newid.'##', $calculation); Or: $calculation = str_replace('##gi31##', '##gi32##', $calculation); $calculation = str_replace('##gi32##', '##gi33##', $calculation); The end outcome would be =##gi33##/##gi33## because of the clashing ids. The two step ensures that we never replace anything that has already been replaced. One other solution to this would be to walk or explode the calculation and work through things one by one, however this was quicker to write. The testing instructions and git diff fields I'll fill out if I put it up for integration Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (just submitting this to integration, knowing it's not ready, but to have it in the bag!)

          I think it looks perfect so, with backport and some specific instructions for being tested in the MDLQA I think it will be integrated. Will look to it later, thanks guys!

          Show
          Eloy Lafuente (stronk7) added a comment - (just submitting this to integration, knowing it's not ready, but to have it in the bag!) I think it looks perfect so, with backport and some specific instructions for being tested in the MDLQA I think it will be integrated. Will look to it later, thanks guys!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Offtopic, crap this escaped to the multiple iterations we did @ MDL-23362. Basically it means that ANY restored course in 2.0/2.1 up to now having calculation columns have them 100% borked, grrr.

          Show
          Eloy Lafuente (stronk7) added a comment - Offtopic, crap this escaped to the multiple iterations we did @ MDL-23362 . Basically it means that ANY restored course in 2.0/2.1 up to now having calculation columns have them 100% borked, grrr.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Attaching minimal course 2.0.x backup with:

          • 1 teacher
          • 1 student
          • 3 activities with grades
          • 3 calculation columns

          It reveals the problem under 20_STABLE and master and hopefully, will work once the fix is integrated, let's see.

          Show
          Eloy Lafuente (stronk7) added a comment - Attaching minimal course 2.0.x backup with: 1 teacher 1 student 3 activities with grades 3 calculation columns It reveals the problem under 20_STABLE and master and hopefully, will work once the fix is integrated, let's see.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated (and backported to 20_STABLE by cherry-pick).

          Also, I've tested both 20_STABLE and master with the (previously leading to error) course backup above and I'm pleased to say it restored PERFECTLY!

          Anyway, QA will re-ensure that! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated (and backported to 20_STABLE by cherry-pick). Also, I've tested both 20_STABLE and master with the (previously leading to error) course backup above and I'm pleased to say it restored PERFECTLY! Anyway, QA will re-ensure that! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing test without further action. Will be tested by MDLQA-1110 once this meets upstream.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing test without further action. Will be tested by MDLQA-1110 once this meets upstream.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: