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:
    • Rank:
      42837

      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.

        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: