Putting this up for peer review. I'll backport it once that is done.

Hi Andrew,

This works when grading from the grading table, but if you click on the edit icon for a single student grade and change it there it is not logged. This is the same page you would use to "unoverride" a grade (which should also be logged).

Also the info parameter for the add_to_log call does not need the course (it's in the log info already) and should probably be just data - not a sentence - e.g. "$modulename: $fullname".

Cheers, Damyon

I think I've got this all working. I've added logging to the page you get taken to when you click that edit icon.

I did originally have "override" and "override removal" as the event types but if you want the events to appear in a log search you're limited to add, update etc. So they're all grade updates. It sucks a little as it means you can't tell the difference between the adding and the removal of a grade override.

Putting this up for peer review.

Hi Andrew,

Two small things I spotted:
1. you are making the same SQL call to load a user record twice in "grade/edit/tree/grade.php" - The second one could just check if the data is already loaded
2. you need to add a restore_log_rule to "backup/moodle2/restore_final_task.class.php" (I think this gets overlooked a bit).

[Y] Syntax
[-] Output
[Y] Whitespace
[Y] Language
[-] Databases
[Y] Testing - You should add a backup and restore test here once you've added that rule
[-] Security
[-] Documentation
[Y] Git
[Y] Sanity check

Thumbs up once you fix 1 and 2.

Damyon

I'm pushing an updated version now.

re #1, it's not immediately obvious but you will only execute one query or the other, never both. You can either override the grade or remove the override flag. Not both.

re #2, I've added a restore_log_rule but I'd be lying if I said that I was confident that what I have is completely correct. I've based it on the rules around it.

The restore_log_rule should just be this:

$rules[] = new restore_log_rule('grade', 'update', 'report/grader/index.php?id=

', null);

You don't want to do any mapping on the info string on restore because you just used a string (the alternative is to use a single id and then have a definition in db/log.php that defines the columns to display - but this doesn't work when we need to join tables to get the info).

The logging gets a bit complicated when we want to add multiple bits of info from different tables to the log info which is a shame because ideally the logs would have as much info as possible.

It would be good add testing the backup/restore of the log entries to the test instructions.

Fixed up the restore log rule and added backup/restore to the testing instructions. Putting this up for integration.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

Cheers!

Hi Andrew,

I'm quite concerned about the performance implications of adding the user lookup to each logging call. Particularly for bulk grading operations - this could be significant.

• Can you explain why we can't use "a single id and then have a definition in db/log.php"?
• Have you examined the impact of this (db queries before/after) when doing some large grading operation?

I'm reopening this for more consideration regarding thoes points. Thanks.

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

A better URL than the course grader report might be this. Its the grade edit page for a single student grade. Not sure why I didnt think of it before.
/grade/edit/tree/grade.php?courseid=2&id=3&gpr_type=report&gpr_plugin=grader&gpr_courseid=2

There may be another way we can do this. Maybe.

Add something like this to lib/db/log.php
array('module'=>'grade', 'action'=>'update'...

For comparison here is a messaging entry. The mtable and field columns are use with the $info supplied to add_to_log() to look up data to display when the log entry is viewed (as I understand it).
array('module'=>'message', 'action'=>'remove contact', 'mtable'=>'user', 'field'=>$DB->sql_concat('firstname', "' '" , 'lastname')),

So the add_to_log() call would still look something like
add_to_log($course->id, 'grade', 'update', $url, $info);

But what goes in $info? Currently I have "{$grade_item->itemname}: $fullname" but I gather that it should be a single ID if we're to use db/log.php as it gets fed into a query.

The problem is that when displaying the logs we need both the student name and the activity name so what goes in the mtable column in the db/log.php entry?

Switching to that new URL also causes a problem. In restore_final_task.class.php there was this

$rules[] = new restore_log_rule('grade', 'update', 'report/grader/index.php?id=

&id=GRADEGRADEID&gpr_type=report&gpr_plugin=grader&gpr_courseid=

I've now pretty much come full circle. The current solution appears to be the best we're going to do. The db/log.php and log restoration don't appear to allow what I was trying to do.

I have however made my change.
"Have you examined the impact of this (db queries before/after) when doing some large grading operation?"
Only one of these log calls exists within a loop where it could be called repeatedly. Fortunately that is in a situation where there is actually already an array of user objects. I just didn't spot it before. I've switched it over to accessing this array instead of hitting the DB.

Putting this back up for peer review.

Rebased.

Hi Andrew,

This gets approach looks fine to me. Sorry I made you bash your head against the logging api for so long - at least we are looking at improving this for 2.6. I think the approach you have here looks fine for now. I am thinking of this as a bug (it should really be logging this information) so can you add backport branches and push for integration?

(There is nothing to report from the PR checklist)

Thanks!

Adding versions for 2.4 and 2.3. Putting this up for integration.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Thanks Andrew, this has been integrated now.

Test result: Success

Tested in 2.3, 2.4, 2.5 and master.

Thanks for your awesome work! This has now become a part of Moodle.

Closing as fixed!