Moodle
  1. Moodle
  2. MDL-41170

user_updated event not triggered when user password modified in moodlelib.php functions

    Details

    • Testing Instructions:
      Hide
      1. Run unit tests in moodlelib_test.php and make sure they pass (master only)
      2. Change your password, create a user, update a user as admin
      3. Verify no errors or debugging messages.
      4. On master, a log entry should be created for those actions.
      Show
      Run unit tests in moodlelib_test.php and make sure they pass (master only) Change your password, create a user, update a user as admin Verify no errors or debugging messages. On master, a log entry should be created for those actions.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-41170-master
    • Story Points (Obsolete):
      8
    • Sprint:
      BACKEND Sprint 6

      Description

      user_updated event not triggered when user password modified in moodlelib.php functions:

      • update_internal_user_password()
      • setnew_password_and_mail()

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Debbie McDonald added a comment -

            When students change their password in the system, this bug rejects the change keeping the original password. Updates please? This is a fairly significant item. Every time I update a users information the password reverts to the originally loaded password.

            Show
            Debbie McDonald added a comment - When students change their password in the system, this bug rejects the change keeping the original password. Updates please? This is a fairly significant item. Every time I update a users information the password reverts to the originally loaded password.
            Hide
            Brent Boghosian added a comment -

            Thanks to Akin for the patches ...

            Pull 2.2 Branch: MDL-41170-m22
            Pull 2.2 Diff URL: https://github.com/number33/moodle/compare/MOODLE_22_STABLE...MDL-41170-m22

            Show
            Brent Boghosian added a comment - Thanks to Akin for the patches ... Pull 2.2 Branch: MDL-41170 -m22 Pull 2.2 Diff URL: https://github.com/number33/moodle/compare/MOODLE_22_STABLE...MDL-41170-m22
            Hide
            Debbie McDonald added a comment -

            Thank you so much, I will pass this along!

            Show
            Debbie McDonald added a comment - Thank you so much, I will pass this along!
            Hide
            Michael de Raadt added a comment -

            Thanks to everyone involved in this so far.

            Show
            Michael de Raadt added a comment - Thanks to everyone involved in this so far.
            Hide
            Mark Nelson added a comment -

            Hi Ankit, this is blocked by MDL-40045 as I am creating an event user_password_updated which you should use here. The new event was created to replace a legacy add_to_log call.

            Show
            Mark Nelson added a comment - Hi Ankit, this is blocked by MDL-40045 as I am creating an event user_password_updated which you should use here. The new event was created to replace a legacy add_to_log call.
            Hide
            Ankit Agarwal added a comment -

            Hi Mark,
            As I commented on MDL-40045, we should not have separate event just for password update. Hence am going forward with this patch.

            I have left the stable pull branches as well, although am not sure if we want to add new events in stables. I will leave that for integrators to decide.

            Pushing forward for review.
            Thanks

            Show
            Ankit Agarwal added a comment - Hi Mark, As I commented on MDL-40045 , we should not have separate event just for password update. Hence am going forward with this patch. I have left the stable pull branches as well, although am not sure if we want to add new events in stables. I will leave that for integrators to decide. Pushing forward for review. Thanks
            Hide
            Petr Skoda added a comment -

            Is this going to result in two updated events when admin edits or creates a user?

            Show
            Petr Skoda added a comment - Is this going to result in two updated events when admin edits or creates a user?
            Hide
            Ankit Agarwal added a comment -

            No, this triggers only one event for these situations.

            Show
            Ankit Agarwal added a comment - No, this triggers only one event for these situations.
            Hide
            Petr Skoda added a comment -

            looks ok, +1, submitting for integration

            Show
            Petr Skoda added a comment - looks ok, +1, submitting for integration
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            Damyon Wiese added a comment -

            Looks OK and seems safe/reasonable for backporting - but I think that this change should be mentioned in upgrade.txt to be safe. Can you please add this (even for stables)?

            Thanks

            Show
            Damyon Wiese added a comment - Looks OK and seems safe/reasonable for backporting - but I think that this change should be mentioned in upgrade.txt to be safe. Can you please add this (even for stables)? Thanks
            Hide
            Ankit Agarwal added a comment -

            Thanks Damyon, patch updated.
            Thanks

            Show
            Ankit Agarwal added a comment - Thanks Damyon, patch updated. Thanks
            Hide
            Damyon Wiese added a comment -

            Thanks Ankit,

            Looks fine now.

            Integrated to 24, 25 and master.

            Show
            Damyon Wiese added a comment - Thanks Ankit, Looks fine now. Integrated to 24, 25 and master.
            Hide
            Damyon Wiese added a comment -

            I put some (lame) testing instructions on this issue, Ankit can you please improve them?

            Show
            Damyon Wiese added a comment - I put some (lame) testing instructions on this issue, Ankit can you please improve them?
            Hide
            Ankit Agarwal added a comment -

            updated. Thanks

            Show
            Ankit Agarwal added a comment - updated. Thanks
            Hide
            Mark Nelson added a comment -

            How about checking that that an entry was created in the log report?

            Show
            Mark Nelson added a comment - How about checking that that an entry was created in the log report?
            Hide
            Ankit Agarwal added a comment -

            Mark, log entry is created on master only and it is already tested in unit tests. Anyway I will mention it in manual testing again, as well.

            Thanks

            Show
            Ankit Agarwal added a comment - Mark, log entry is created on master only and it is already tested in unit tests. Anyway I will mention it in manual testing again, as well. Thanks
            Hide
            Jason Fowler added a comment -

            all good Ankit, thanks for the patch

            Show
            Jason Fowler added a comment - all good Ankit, thanks for the patch
            Hide
            Dan Poltawski added a comment -

            Hurrah! Thanks for your contribution - this fix is part of Moodle.

            Show
            Dan Poltawski added a comment - Hurrah! Thanks for your contribution - this fix is part of Moodle.

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile