Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

        1. backup-moodle2-course-qatest21-20110622-1811.mbz
          152 kB
          Javier Sola Aréchaga
        2. backup-moodle2-course-testgradecalc-20110629-1807.mbz
          62 kB
          Eloy Lafuente (stronk7)
        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

            jsola60 Javier Sola Aréchaga created issue -
            salvetore Michael de Raadt made changes -
            Field Original Value New Value
            Link This issue will help resolve MDLQA-1110 [ MDLQA-1110 ]
            Hide
            salvetore Michael de Raadt added a comment -

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

            Show
            salvetore Michael de Raadt added a comment - Promoting this issue as it is related to a QA test.
            salvetore Michael de Raadt made changes -
            Fix Version/s DEV Sprint 2.1 [ 10650 ]
            Priority Minor [ 4 ] Blocker [ 1 ]
            Labels mdlqa triaged
            Hide
            salvetore 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
            salvetore 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            samhemelryk 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
            samhemelryk 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
            stronk7 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
            stronk7 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!
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Open [ 1 ] Waiting for integration review [ 10010 ]
            Fix Version/s 2.0.4 [ 10652 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator stronk7
            Currently in integration Yes
            Hide
            stronk7 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
            stronk7 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.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Link This issue has a non-specific relationship to MDL-23362 [ MDL-23362 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Pull Master Diff URL https://github.com/samhemelryk/moodle/compare/master...wip-MDL-27988-master
            Pull Master Branch wip-MDL-27988-master
            Testing Instructions 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).
            Pull from Repository git://github.com/samhemelryk/moodle.git
            Assignee Eloy Lafuente (stronk7) [ stronk7 ] Sam Hemelryk [ samhemelryk ]
            Difficulty Difficult
            stronk7 Eloy Lafuente (stronk7) made changes -
            Affects Version/s 2.0.4 [ 10652 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Affects Version/s 2.0.3 [ 10537 ]
            Affects Version/s 2.0.4 [ 10652 ]
            Hide
            stronk7 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
            stronk7 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.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Hide
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester nobody
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Testing in progress [ 10011 ] Waiting for testing [ 10005 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester nobody stronk7
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Passing test without further action. Will be tested by MDLQA-1110 once this meets upstream.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            salvetore Michael de Raadt made changes -
            Link This issue has been marked as being related by MDL-28116 [ MDL-28116 ]
            Hide
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment - Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes
            Integration date 1/Jul/11
            stronk7 Eloy Lafuente (stronk7) made changes -
            Link This issue has been marked as being related by MDL-28156 [ MDL-28156 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Labels mdlqa triaged triaged
            stronk7 Eloy Lafuente (stronk7) made changes -
            Fix Version/s DEV Sprint 2.1 [ 10650 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  1/Aug/11