Details

    • 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 2.4 Branch:
      MDL-36090_log2_24
    • Pull Master Branch:
      MDL-36090_log2
    • Rank:
      44851

      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

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

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

          Show
          Andrew Davis added a comment - Putting this up for peer review. I'll backport it once that is done.
          Hide
          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 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
          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
          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 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 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
          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
          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 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 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
          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
          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 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 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
          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
          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
          Dan Poltawski added a comment -

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

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

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          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
          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
          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
          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
          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
          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
          Andrew Davis added a comment -

          Putting this back up for peer review.

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

          Rebased.

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

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

          Show
          Andrew Davis added a comment - Adding versions for 2.4 and 2.3. Putting this up for integration.
          Hide
          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
          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
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now.

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

          Test result: Success

          Tested in 2.3, 2.4, 2.5 and master.

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

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

          Closing as fixed!

          Show
          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: