Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2, 2.3
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: General, Reports
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      • A few courses
      • A few categories

      Test steps

      1. Visit course/index.php and course/category.php
      2. Make sure each possible action is being logged in the log reports (Home ► Site pages ► Reports ► Logs ► Reports ► Logs)

      Note: Actions that being log are: Add/Delete/Move/Hide/Show

      Show
      Test pre-requisites A few courses A few categories Test steps Visit course/index.php and course/category.php Make sure each possible action is being logged in the log reports (Home ► Site pages ► Reports ► Logs ► Reports ► Logs) Note: Actions that being log are: Add/Delete/Move/Hide/Show
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34435-master

      Description

      I've just noticed that it appears that actions in categories (creating categories, renaming, viewing, adding blocks etc.) are not currently being logged in the mdl_log table. I came across this when trying to work out why a category block had disappeared (hoping to identify which user had deleted it). I've tested on both our Moodle 2.2.2 live server and 2.3 test server - neither log this info.

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that.

          I tried a few actions in categories while watching the Live logs and you are correct, there are no entries being generated.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for reporting that. I tried a few actions in categories while watching the Live logs and you are correct, there are no entries being generated. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          Michael de Raadt added a comment -

          In relation to other similar issues, there is no need to classify this as a security issue. Certainly the risk of this being exploited is very low.

          Show
          Michael de Raadt added a comment - In relation to other similar issues, there is no need to classify this as a security issue. Certainly the risk of this being exploited is very low.
          Hide
          Frédéric Massart added a comment -

          Added some calls to add_to_log() from the pages course/index.php and course/category.php.

          I tried to be as descriptive as I could in the info field without having to call get information from the DB. Also, when an action triggers a lot of other ones (hiding a category) I decided not to add a log for each changes, but just for the main action.

          Cheers!

          Show
          Frédéric Massart added a comment - Added some calls to add_to_log() from the pages course/index.php and course/category.php. I tried to be as descriptive as I could in the info field without having to call get information from the DB. Also, when an action triggers a lot of other ones (hiding a category) I decided not to add a log for each changes, but just for the main action. Cheers!
          Hide
          Mark Nelson added a comment -

          [N] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Hi Fred, looks good. Few trivial points:

          1. In course/editcategory.php you have not placed quotes around $newcategory->name, but have quoted that parameter in every other use of add_to_log.
          2. For some reason there was a conflict in course/lib.php MOODLE_23_STABLE with $actionurl = $CFG->wwwroot. make_log_url($log->module,$log->url); but not in the other files. I was going to suggest adding spaces in this code when concatenating and between the comma and variable names but since this conflict only happened in one branch it is probably not recommended to just make this change for one specific branch. I can't actually see the difference between the lines in the diff (git is odd sometimes)
          3. I know you just moved the line "$DB->set_field('course_categories', 'parent', $parentid, array('id'=>$category->id));" down, but since it is included in your diff you could add spaces between the variables and '=>' to make it look nicer.
          4. The testing instruction step "Make sure each possible action is logged (/report/log/index.php)" is pretty vague. The file you mention does not provide any obvious information about what you mean.
          Show
          Mark Nelson added a comment - [N] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hi Fred, looks good. Few trivial points: In course/editcategory.php you have not placed quotes around $newcategory->name, but have quoted that parameter in every other use of add_to_log. For some reason there was a conflict in course/lib.php MOODLE_23_STABLE with $actionurl = $CFG->wwwroot. make_log_url($log->module,$log->url); but not in the other files. I was going to suggest adding spaces in this code when concatenating and between the comma and variable names but since this conflict only happened in one branch it is probably not recommended to just make this change for one specific branch. I can't actually see the difference between the lines in the diff (git is odd sometimes) I know you just moved the line "$DB->set_field('course_categories', 'parent', $parentid, array('id'=>$category->id));" down, but since it is included in your diff you could add spaces between the variables and '=>' to make it look nicer. The testing instruction step "Make sure each possible action is logged (/report/log/index.php)" is pretty vague. The file you mention does not provide any obvious information about what you mean.
          Hide
          Frédéric Massart added a comment -

          Thanks Mark!

          1/ Fixed.

          2/ Yup, no idea why this line appears different. I did not edit it at all.

          3/ Fixed.

          4/ Fixed.

          Pushing for integration. Thanks!

          Show
          Frédéric Massart added a comment - Thanks Mark! 1/ Fixed. 2/ Yup, no idea why this line appears different. I did not edit it at all. 3/ Fixed. 4/ Fixed. Pushing for integration. Thanks!
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          Reopening sorry, I think it would be well worth adding the new log entries to lib/db/log.php.
          Also noting I am unsure about the value in adding two log entries when moving a course up/down.
          I wonder what the benefit is to the user when viewing reports and seeing two consecutive "update course" entries, perhaps that would confuse the user given it is only a single action to move a single course.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, Reopening sorry, I think it would be well worth adding the new log entries to lib/db/log.php. Also noting I am unsure about the value in adding two log entries when moving a course up/down. I wonder what the benefit is to the user when viewing reports and seeing two consecutive "update course" entries, perhaps that would confuse the user given it is only a single action to move a single course. Many thanks Sam
          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
          Frédéric Massart added a comment -

          Thanks for your comments Sam, I didn't know about lib/db/logs.php, I have added things there. I have also removed the duplicated line in the logs and replaces the info with the ID of the course or category so that lib/db/logs.php becomes useful. I only didn't do that for delete actions where I hardcoded the names and ids in the info field.

          Pushing for peer review again, should be straight forward Mark as very few things have changed.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Thanks for your comments Sam, I didn't know about lib/db/logs.php, I have added things there. I have also removed the duplicated line in the logs and replaces the info with the ID of the course or category so that lib/db/logs.php becomes useful. I only didn't do that for delete actions where I hardcoded the names and ids in the info field. Pushing for peer review again, should be straight forward Mark as very few things have changed. Cheers, Fred
          Hide
          Mark Nelson added a comment -

          Logic looks good, pushing to integration.

          Show
          Mark Nelson added a comment - Logic looks good, pushing to 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 Fred, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Fred, this has been integrated now.
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao
          Hide
          Richard Heath added a comment -

          Great stuff, thanks everyone for your efforts in fixing this.

          Show
          Richard Heath added a comment - Great stuff, thanks everyone for your efforts in fixing this.
          Hide
          Mary Cooch added a comment -

          Just made a quick note of this in the docs: http://docs.moodle.org/24/en/Logs#Logs_of_site_activity

          Show
          Mary Cooch added a comment - Just made a quick note of this in the docs: http://docs.moodle.org/24/en/Logs#Logs_of_site_activity

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: