Uploaded image for project: '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
    • Sprint:
      BACKEND Sprint 6
    • 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

          Attachments

            Issue Links

              Activity

              Hide
              dlmcdonald 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
              dlmcdonald 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
              brentboghosian 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
              brentboghosian 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
              dlmcdonald Debbie McDonald added a comment -

              Thank you so much, I will pass this along!

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

              Thanks to everyone involved in this so far.

              Show
              salvetore Michael de Raadt added a comment - Thanks to everyone involved in this so far.
              Hide
              markn 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
              markn 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_frenz 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_frenz 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
              skodak Petr Skoda added a comment -

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

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

              No, this triggers only one event for these situations.

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

              looks ok, +1, submitting for integration

              Show
              skodak Petr Skoda added a comment - looks ok, +1, submitting for integration
              Hide
              stronk7 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
              stronk7 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 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 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_frenz Ankit Agarwal added a comment -

              Thanks Damyon, patch updated.
              Thanks

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

              Thanks Ankit,

              Looks fine now.

              Integrated to 24, 25 and master.

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

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

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

              updated. Thanks

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

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

              Show
              markn Mark Nelson added a comment - How about checking that that an entry was created in the log report?
              Hide
              ankit_frenz 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_frenz 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
              phalacee Jason Fowler added a comment -

              all good Ankit, thanks for the patch

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

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

              Show
              poltawski 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:
                    Fix Release Date:
                    11/Nov/13