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

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

Hi, Eloy.

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

Michael;

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

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

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

//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?

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.

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

(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!

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.

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.

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

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

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