Moodle
  1. Moodle
  2. MDL-30718

Method sync_users() @ "/auth/db/auth.php" does not register mdl_user->timecreated property.

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Enable the auth/db plugin, create some users on the external database and try to sync those users using the sync_users() method. The created users will be missing their timecreated property.

      Show
      Enable the auth/db plugin, create some users on the external database and try to sync those users using the sync_users() method. The created users will be missing their timecreated property.
    • Workaround:
      Hide

      The included patch.diff file adds the missing field if the user is really a new user and should be created on sync_users() method call. It sets the user's timecreated property as the timemodified property, that in this case are the same.

      Show
      The included patch.diff file adds the missing field if the user is really a new user and should be created on sync_users() method call. It sets the user's timecreated property as the timemodified property, that in this case are the same.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w02_MDL-30718_m23_usertimedb

      Description

      When it is run the sync_users() method, the new users created (if any) are created without having their timecreated properties registered.

        Gliffy Diagrams

        1. patch.diff
          0.7 kB
          Luis Gustavo Mueller de Alcantara
        2. patch2.diff
          1 kB
          Luis Gustavo Mueller de Alcantara

          Issue Links

            Activity

            Hide
            Petr Skoda added a comment -

            Thanks for the report!

            Show
            Petr Skoda added a comment - Thanks for the report!
            Hide
            Luis Gustavo Mueller de Alcantara added a comment -

            I hope it helps.

            Show
            Luis Gustavo Mueller de Alcantara added a comment - I hope it helps.
            Hide
            Luis Gustavo Mueller de Alcantara added a comment -

            I did not see that $user->timemodified was set as $user->modified. Updating that too.

            Show
            Luis Gustavo Mueller de Alcantara added a comment - I did not see that $user->timemodified was set as $user->modified. Updating that too.
            Hide
            Petr Skoda added a comment -

            I have added updates of timemodified field too, there is one place left that does not add it, but it is a dead code because we do not undelete users via old user name any more.

            to integrators: please cherry pick to 2.1.x and 2.2.x

            Show
            Petr Skoda added a comment - I have added updates of timemodified field too, there is one place left that does not add it, but it is a dead code because we do not undelete users via old user name any more. to integrators: please cherry pick to 2.1.x and 2.2.x
            Hide
            Luis Gustavo Mueller de Alcantara added a comment -

            Every help is much appreciated.

            Thanks again Petr.

            Show
            Luis Gustavo Mueller de Alcantara added a comment - Every help is much appreciated. Thanks again Petr.
            Hide
            Aparup Banerjee added a comment -

            Hi Petr,

            • could you link to the issue (or create?) about the $old_user dead code bit, it would make sense to have that cleaned up.
            • about test: "try to sync those users using the sync_users() method" might need a little more explaining for the passer-by tester.
            • i suppose unit testing on sync_users() would help prevent this sort of issue/regression in future.

            Code wise this looks ok to integrate currently but i'll wait for your comment.

            Show
            Aparup Banerjee added a comment - Hi Petr, could you link to the issue (or create?) about the $old_user dead code bit, it would make sense to have that cleaned up. about test: "try to sync those users using the sync_users() method" might need a little more explaining for the passer-by tester. i suppose unit testing on sync_users() would help prevent this sort of issue/regression in future. Code wise this looks ok to integrate currently but i'll wait for your comment.
            Hide
            Petr Skoda added a comment -

            1/ I do not know the exact issue numbers, sorry, I was planning to work on that this week. I think I saw multiple issues related to undeleting and unsuspending.

            2/ Testers should know something about this plugin and enrolments in general - depending on plugin type users are synchronised either via cron or CLI script; It should be documented somewhere for admins already and if not the testers could create new issue and fix it.

            3/ unit testing is not going to help here much because we would need to first abstract the user creation/updates to some centralised API and then we would ahve to teach our simpletest framework something about external databases. I would personally love to cleanup all enrol+auth plugins to use core APIs for any course/user/category manipulation + add necessary unit tests.

            Show
            Petr Skoda added a comment - 1/ I do not know the exact issue numbers, sorry, I was planning to work on that this week. I think I saw multiple issues related to undeleting and unsuspending. 2/ Testers should know something about this plugin and enrolments in general - depending on plugin type users are synchronised either via cron or CLI script; It should be documented somewhere for admins already and if not the testers could create new issue and fix it. 3/ unit testing is not going to help here much because we would need to first abstract the user creation/updates to some centralised API and then we would ahve to teach our simpletest framework something about external databases. I would personally love to cleanup all enrol+auth plugins to use core APIs for any course/user/category manipulation + add necessary unit tests.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            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
            Aparup Banerjee added a comment -

            Petr,
            point taken, it would certainly be more feasible to form some proper unit tests when they're more centralized. It might be worth creating issue(s) for your point (3) if there aren't any already.

            that said, integration wise thats been integrated into 2.1.x, 2.2.x and master.

            cheers!

            Show
            Aparup Banerjee added a comment - Petr, point taken, it would certainly be more feasible to form some proper unit tests when they're more centralized. It might be worth creating issue(s) for your point (3) if there aren't any already. that said, integration wise thats been integrated into 2.1.x, 2.2.x and master. cheers!
            Hide
            Jason Fowler added a comment -

            I am unable to test this due to the lack of information in the test instructions.

            Show
            Jason Fowler added a comment - I am unable to test this due to the lack of information in the test instructions.
            Hide
            Aparup Banerjee added a comment - - edited

            Jason : have you read Petr's further explanation

            also i googled 'moodle auth' to get http://docs.moodle.org/22/en/Authentication_FAQ which should help you to learn more about auth plugins.

            Show
            Aparup Banerjee added a comment - - edited Jason : have you read Petr's further explanation also i googled 'moodle auth' to get http://docs.moodle.org/22/en/Authentication_FAQ which should help you to learn more about auth plugins.
            Hide
            Aparup Banerjee added a comment -

            just trying to clarify the test here.

            1) enable the auth/db plugin
            2) create data in external database table.
            3) then as Petr said in his comment : 'depending on plugin type users are synchronised either via cron or CLI script'

            • NOTE: this does use sync_users()
              (Petr, linking synchronisation with sync_users() would have helped clarify it just a bit imo)
            Show
            Aparup Banerjee added a comment - just trying to clarify the test here. 1) enable the auth/db plugin 2) create data in external database table. 3) then as Petr said in his comment : 'depending on plugin type users are synchronised either via cron or CLI script' NOTE: this does use sync_users() (Petr, linking synchronisation with sync_users() would have helped clarify it just a bit imo)
            Hide
            Jason Fowler added a comment -

            thanks for the clarification Aparup

            Show
            Jason Fowler added a comment - thanks for the clarification Aparup
            Hide
            Jason Fowler added a comment -

            Test passed.

            Show
            Jason Fowler added a comment - Test passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This virus has been spread upstream, everybody will be infected soon. Congrats, you did it!

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - This virus has been spread upstream, everybody will be infected soon. Congrats, you did it! Closing, ciao
            Hide
            Luis Gustavo Mueller de Alcantara added a comment -

            Thank you all.

            Show
            Luis Gustavo Mueller de Alcantara added a comment - Thank you all.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: