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

      Description

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

      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 Škoda added a comment -

          Thanks for the report!

          Show
          Petr Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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: