Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as admin
      2. goto user list and delete a user
      3. Goto settings>reports>live log
      4. Make sure you can see the action of user deletion in the logs and make sure all data is correct.
      5. Click the button that says "test passed" on the top right of this page
      Show
      Login as admin goto user list and delete a user Goto settings>reports>live log Make sure you can see the action of user deletion in the logs and make sure all data is correct. Click the button that says "test passed" on the top right of this page
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-28387-master
    • Rank:
      18092

      Description

      When you delete a user from the 'Browse list of users' page no log entry is made.

      1. MDL-28387.patch
        0.5 kB
        Evan Irving-Pease

        Activity

        Hide
        Ankit Agarwal added a comment -

        Hi,
        I have modifed the patch a bit.
        Also I think it would be better to log directly inside the delete user api rather than embeding it somewhere in the work flow, since there are many places that call delete user api (example: spam cleaner)

        And finally I have directly used the name variables instead of calling "fullname" function that should reduce the DB call a bit, and at other places (user view) , we don't check for any caps (ex:- viewfullname?). Any other opinion appreciated.

        Submitting for review!
        Thanks

        Show
        Ankit Agarwal added a comment - Hi, I have modifed the patch a bit. Also I think it would be better to log directly inside the delete user api rather than embeding it somewhere in the work flow, since there are many places that call delete user api (example: spam cleaner) And finally I have directly used the name variables instead of calling "fullname" function that should reduce the DB call a bit, and at other places (user view) , we don't check for any caps (ex:- viewfullname?). Any other opinion appreciated. Submitting for review! Thanks
        Hide
        Dan Poltawski added a comment -

        As me and Ankit discussed:

        • We discussed if was probably preferable to use fullname() to display the name - however this will vary the entry added to the log depending on the user who deleted the users settings.
        • I suggested separating out the alphabetical ordering for reability
        Show
        Dan Poltawski added a comment - As me and Ankit discussed: We discussed if was probably preferable to use fullname() to display the name - however this will vary the entry added to the log depending on the user who deleted the users settings. I suggested separating out the alphabetical ordering for reability
        Hide
        Ankit Agarwal added a comment -

        Hi Dan,
        I have made changes as we discussed. and the readability commit is pushed to master only.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Dan, I have made changes as we discussed. and the readability commit is pushed to master only. Thanks
        Hide
        Sam Hemelryk added a comment -

        Hi Ankit,

        Why have you moved the user view entry in the log.php file?
        Were you meaning to duplicate it and change it to delete?

        Reviewing paused pending your reply

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Ankit, Why have you moved the user view entry in the log.php file? Were you meaning to duplicate it and change it to delete? Reviewing paused pending your reply Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        (forgot to mention only in master)

        Show
        Sam Hemelryk added a comment - (forgot to mention only in master)
        Hide
        Sam Hemelryk added a comment -

        Thanks Ankit this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Ankit this has been integrated now
        Hide
        Michael de Raadt added a comment - - edited

        Test result: Success in 2.1, 2.2 and master. I'm glad you described how to pass this test

        Show
        Michael de Raadt added a comment - - edited Test result: Success in 2.1, 2.2 and master. I'm glad you described how to pass this test
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

        icao_reverse('arreis olik rebemevon afla letoh ognat');
        

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: