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).

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.

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.

Sam Hemelryk
added a comment - 29/Jun/11 1:38 PM 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

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.

Sam Hemelryk
added a comment - 29/Jun/11 2:36 PM 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

Andrew Davis
added a comment - 29/Jun/11 2:59 PM 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...

Andrew Davis
added a comment - 29/Jun/11 3:15 PM //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?

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##

Sam Hemelryk
added a comment - 29/Jun/11 3:48 PM 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

Eloy Lafuente (stronk7)
added a comment - 29/Jun/11 6:04 PM (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!

Eloy Lafuente (stronk7)
added a comment - 29/Jun/11 10:25 PM 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.

Eloy Lafuente (stronk7)
added a comment - 30/Jun/11 12:53 AM 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