Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.8, 2.4.5, 2.5.1
    • Component/s: Gradebook
    • Labels:
    • Testing Instructions:
      Hide

      Log in as a teacher
      Navigate to Navigation > My courses > Course Name > Reports > Logs
      Click 'Get these logs'

      Open a second tab so you can keep the log screen handy.

      Navigate to a course
      Navigate to Settings > Course admin > Grades
      (If not already there) Navigate to the Grader report
      Override a student grade.
      Refresh the log screen and check a log entry is present.

      Toggle grader report ajax (so you will have tested this with both ajax on and off) override the student grade again.
      Check the log entry appears.

      Back up the course. Make sure to tick the option to include course logs
      Restore the back up into a new course.
      Go to the log screen in the new course.
      Click on the link in the action column, it will say "grade update", and you should be taken to the grader report for the new course (not the original course).

      Show
      Log in as a teacher Navigate to Navigation > My courses > Course Name > Reports > Logs Click 'Get these logs' Open a second tab so you can keep the log screen handy. Navigate to a course Navigate to Settings > Course admin > Grades (If not already there) Navigate to the Grader report Override a student grade. Refresh the log screen and check a log entry is present. Toggle grader report ajax (so you will have tested this with both ajax on and off) override the student grade again. Check the log entry appears. Back up the course. Make sure to tick the option to include course logs Restore the back up into a new course. Go to the log screen in the new course. Click on the link in the action column, it will say "grade update", and you should be taken to the grader report for the new course (not the original course).
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-36090_log2

      Description

      Adding/changing a student's grade through a gradebook is not logged.

      Replication steps:

      1. Log in as a teacher
      2. Navigate to a course
      3. Navigate to Settings > Course admin > Grades
      4. (If not already there) Navigate to the Grader report
      5. Turn editing on
      6. Add a new grade value
      7. Click update
      8. Modify an existing grade value
      9. Click update
      10. Navigate to Navigation > My courses > CourseXXX > Reports > Logs (or look in the log table in the DB)
      11. Click 'Get these logs'

      Expected result: Gradebook changes should be shown

      Actual result: No entries for adding/modifying grades are recorded

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            andyjdavis Andrew Davis added a comment -

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

            Show
            andyjdavis Andrew Davis added a comment - Putting this up for peer review. I'll backport it once that is done.
            Hide
            damyon Damyon Wiese added a comment -

            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

            Show
            damyon Damyon Wiese added a comment - 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
            Hide
            andyjdavis Andrew Davis added a comment -

            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.

            Show
            andyjdavis Andrew Davis added a comment - 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.
            Hide
            damyon Damyon Wiese added a comment -

            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

            Show
            damyon Damyon Wiese added a comment - 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
            Hide
            andyjdavis Andrew Davis added a comment -

            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.

            Show
            andyjdavis Andrew Davis added a comment - 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.
            Hide
            damyon Damyon Wiese added a comment -

            The restore_log_rule should just be this:

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

            {course}

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

            Show
            damyon Damyon Wiese added a comment - The restore_log_rule should just be this: $rules[] = new restore_log_rule('grade', 'update', 'report/grader/index.php?id= {course} ', 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.
            Hide
            andyjdavis Andrew Davis added a comment -

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

            Show
            andyjdavis Andrew Davis added a comment - Fixed up the restore log rule and added backup/restore to the testing instructions. Putting this up for integration.
            Hide
            damyon Damyon Wiese added a comment -

            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!

            Show
            damyon Damyon Wiese added a comment - 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!
            Hide
            poltawski Dan Poltawski added a comment -

            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?
            Show
            poltawski Dan Poltawski added a comment - 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?
            Hide
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - I'm reopening this for more consideration regarding thoes points. Thanks.
            Hide
            cibot CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            andyjdavis Andrew Davis added a comment -

            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?

            Show
            andyjdavis Andrew Davis added a comment - 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?
            Hide
            andyjdavis Andrew Davis added a comment -

            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=

            {course}', null);

            That URL would now be the below but Im not sure what token to put where I have GRADEGRADEID
            '/grade/edit/tree/grade.php?courseid={course}

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

            {course}
            Show
            andyjdavis Andrew Davis added a comment - 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= {course}', null); That URL would now be the below but Im not sure what token to put where I have GRADEGRADEID '/grade/edit/tree/grade.php?courseid={course} &id=GRADEGRADEID&gpr_type=report&gpr_plugin=grader&gpr_courseid= {course}
            Hide
            andyjdavis Andrew Davis added a comment -

            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.

            Show
            andyjdavis Andrew Davis added a comment - 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.
            Hide
            andyjdavis Andrew Davis added a comment -

            Putting this back up for peer review.

            Show
            andyjdavis Andrew Davis added a comment - Putting this back up for peer review.
            Hide
            andyjdavis Andrew Davis added a comment -

            Rebased.

            Show
            andyjdavis Andrew Davis added a comment - Rebased.
            Hide
            damyon Damyon Wiese added a comment -

            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!

            Show
            damyon Damyon Wiese added a comment - 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!
            Hide
            andyjdavis Andrew Davis added a comment -

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

            Show
            andyjdavis Andrew Davis added a comment - Adding versions for 2.4 and 2.3. Putting this up for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            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

            Show
            poltawski Dan Poltawski added a comment - 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
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Andrew, this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success

            Tested in 2.3, 2.4, 2.5 and master.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success Tested in 2.3, 2.4, 2.5 and master.
            Hide
            marina Marina Glancy added a comment -

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

            Closing as fixed!

            Show
            marina Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  8/Jul/13