Moodle
  1. Moodle
  2. MDL-21226

Add a 'timecreated' column to the mdl_user table

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 2.0
    • Component/s: Other
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      It would be nice if there was an additional column in the mdl_user table that tracked when the account was originally created.

      Currently there is no way that I can find, to determine when an account was created... we would like to find out how long users have been using the system, and since enrolment is completely automated, we do not have a paper trail of when somebody signed up.

      Thanks

        Gliffy Diagrams

        1. 20100107_MDL-21226_HEAD.patch
          5 kB
          Rossiani Wijaya
        2. 20100119_MDL-21226_HEAD.patch
          15 kB
          Rossiani Wijaya

          Issue Links

            Activity

            Jeff Sherk created issue -
            Hide
            Martin Dougiamas added a comment -

            Sounds like a good idea.

            It should be called "timecreated" (inserted just before timemodified) and during the upgrade the field should probably be set to the value of the "firstaccess" field for all existing user accounts. Newly created accounts would of course have the value set to time()

            Show
            Martin Dougiamas added a comment - Sounds like a good idea. It should be called "timecreated" (inserted just before timemodified) and during the upgrade the field should probably be set to the value of the "firstaccess" field for all existing user accounts. Newly created accounts would of course have the value set to time()
            Martin Dougiamas made changes -
            Field Original Value New Value
            Assignee Martin Dougiamas [ dougiamas ] Rossiani Wijaya [ rwijaya ]
            Fix Version/s 2.0 [ 10122 ]
            Hide
            Martin Dougiamas added a comment -

            (Note that in 1.9, Jeff, you could use the firstaccess field to get close to what you want)

            Show
            Martin Dougiamas added a comment - (Note that in 1.9, Jeff, you could use the firstaccess field to get close to what you want)
            Martin Dougiamas made changes -
            Summary Add a date_created column to the mdl_user table Add a 'timecreated' column to the mdl_user table
            Hide
            Jeff Sherk added a comment -

            Thanks Martin... yes, I already figured out about using the firstaccess field.

            Here is something to think about... what happens during the upgrade if firstaccess is still 0 (account not accessed yet)?

            Show
            Jeff Sherk added a comment - Thanks Martin... yes, I already figured out about using the firstaccess field. Here is something to think about... what happens during the upgrade if firstaccess is still 0 (account not accessed yet)?
            Hide
            Rossiani Wijaya added a comment -

            if firstaccess is still 0 during upgrade, i think timecreated should be set to time().

            Show
            Rossiani Wijaya added a comment - if firstaccess is still 0 during upgrade, i think timecreated should be set to time().
            Hide
            Jeff Sherk added a comment -

            Yes, Rossiani that makes sense.

            Show
            Jeff Sherk added a comment - Yes, Rossiani that makes sense.
            Hide
            Rossiani Wijaya added a comment - - edited

            working on patch for HEAD to:
            1. upgrade user table - as discussed above, if firstaccess=0, then timecreated will be set to time(). otherwise timecreated = firstaccess.
            2. set the value for timecreated field when new user is created (including upload user file).

            Show
            Rossiani Wijaya added a comment - - edited working on patch for HEAD to: 1. upgrade user table - as discussed above, if firstaccess=0, then timecreated will be set to time(). otherwise timecreated = firstaccess. 2. set the value for timecreated field when new user is created (including upload user file).
            Rossiani Wijaya made changes -
            Attachment 20100105_MDL-21226.patch [ 19327 ]
            Rossiani Wijaya made changes -
            Attachment 20100105_MDL-21226.patch [ 19327 ]
            Hide
            Rossiani Wijaya added a comment -

            attaching patch for HEAD.

            Please review and comments.

            thank you
            Rosie

            Show
            Rossiani Wijaya added a comment - attaching patch for HEAD. Please review and comments. thank you Rosie
            Rossiani Wijaya made changes -
            Attachment 20100107_MDL-21226_HEAD.patch [ 19351 ]
            Hide
            Sam Hemelryk added a comment -

            Hi Rossi,
            All looks good, the only thing I noticed was the version, for HEAD the version should be the current date rather than an incrementation of the last update.
            Other than good as gold, better get Martin to have a look at it just to be safe however.
            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Rossi, All looks good, the only thing I noticed was the version, for HEAD the version should be the current date rather than an incrementation of the last update. Other than good as gold, better get Martin to have a look at it just to be safe however. Cheers Sam
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Automatic message:

            new field in DB uses to mean (99% chance of) it needs to be also handled by backup & restore.

            Manual message:

            I really think we need to have one proper create_user() function ASAP in HEAD. Right now records are being inserted in the "user" table from a lot of places, and I'm not sure if are covering all them properly. Do externally authenticated users have this new field set? If so, how? With data from original auth store (ldap, db..) or following the rules above.

            I know it's not critical for this field, but we need one function doing those bloody inserts and able to detect wrong situations and so on. Here it's the list of places "creating" users (huge one!):

            admin/generator.php: $user->id = $DB->insert_record("user", $user);
            backup/restorelib.php: $newid = $DB->insert_record("user", $user);
            admin/uploaduser.php: if ($user->id = $DB->insert_record('user', $user)) {
            auth/cas/auth.php: if ($id = $DB->insert_record('user', $user))

            { auth/db/auth.php: }

            elseif ($id = $DB->insert_record ('user',$user)) { // it is truly a new user
            auth/email/auth.php: if (! ($user->id = $DB->insert_record('user', $user)) ) {
            auth/ldap/auth.php: if (! ($user->id = $DB->insert_record('user', $user)) ) {
            auth/ldap/auth.php: if ($id = $DB->insert_record('user', $user)) {
            auth/mnet/auth.php: $remoteuser->id = $DB->insert_record('user', $remoteuser);
            enrol/imsenterprise/enrol.php: if($id = $DB->insert_record('user', $person)){
            enrol/mnet/enrol.php: if ($userrecord->id = $DB->insert_record('user', $userrecord)) {
            lib/db/install.php: $guest->id = $DB->insert_record('user', $guest);
            lib/db/install.php: $admin->id = $DB->insert_record('user', $admin);
            lib/moodlelib.php: $DB->insert_record('user', $newuser);
            user/editadvanced.php: $usernew->id = $DB->insert_record('user', $usernew);

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Automatic message: new field in DB uses to mean (99% chance of) it needs to be also handled by backup & restore. Manual message: I really think we need to have one proper create_user() function ASAP in HEAD. Right now records are being inserted in the "user" table from a lot of places, and I'm not sure if are covering all them properly. Do externally authenticated users have this new field set? If so, how? With data from original auth store (ldap, db..) or following the rules above. I know it's not critical for this field, but we need one function doing those bloody inserts and able to detect wrong situations and so on. Here it's the list of places "creating" users (huge one!): admin/generator.php: $user->id = $DB->insert_record("user", $user); backup/restorelib.php: $newid = $DB->insert_record("user", $user); admin/uploaduser.php: if ($user->id = $DB->insert_record('user', $user)) { auth/cas/auth.php: if ($id = $DB->insert_record('user', $user)) { auth/db/auth.php: } elseif ($id = $DB->insert_record ('user',$user)) { // it is truly a new user auth/email/auth.php: if (! ($user->id = $DB->insert_record('user', $user)) ) { auth/ldap/auth.php: if (! ($user->id = $DB->insert_record('user', $user)) ) { auth/ldap/auth.php: if ($id = $DB->insert_record('user', $user)) { auth/mnet/auth.php: $remoteuser->id = $DB->insert_record('user', $remoteuser); enrol/imsenterprise/enrol.php: if($id = $DB->insert_record('user', $person)){ enrol/mnet/enrol.php: if ($userrecord->id = $DB->insert_record('user', $userrecord)) { lib/db/install.php: $guest->id = $DB->insert_record('user', $guest); lib/db/install.php: $admin->id = $DB->insert_record('user', $admin); lib/moodlelib.php: $DB->insert_record('user', $newuser); user/editadvanced.php: $usernew->id = $DB->insert_record('user', $usernew); Ciao
            Hide
            Sam Hemelryk added a comment -

            Wow nice list Eloy and yes it would be really nice to have some sort of structure for user.
            Rossi this certainly gives you a bit more to work on, let me know if you need a hand.
            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Wow nice list Eloy and yes it would be really nice to have some sort of structure for user. Rossi this certainly gives you a bit more to work on, let me know if you need a hand. Cheers Sam
            Hide
            Rossiani Wijaya added a comment -

            Hi Eloy

            I see there is a create_user_record in moodlelib within HEAD, it sets some but not all of the defaults for a user object and then creates it within the database.
            The function itself is only used in two places so I am thinking that it would be a good idea to modify this function to do what we want, change the two uses and then use it everywhere possible.
            Could you please have a look at the create_user_record function in moodlelib and the function that i wrote (attached), and let me know what you think?

            thank you

            Show
            Rossiani Wijaya added a comment - Hi Eloy I see there is a create_user_record in moodlelib within HEAD, it sets some but not all of the defaults for a user object and then creates it within the database. The function itself is only used in two places so I am thinking that it would be a good idea to modify this function to do what we want, change the two uses and then use it everywhere possible. Could you please have a look at the create_user_record function in moodlelib and the function that i wrote (attached), and let me know what you think? thank you
            Rossiani Wijaya made changes -
            Attachment 20100119_MDL-21226_HEAD.patch [ 19444 ]
            Hide
            Martin Dougiamas added a comment -

            Just a note, that the create_user_record function should be closely tied to the /user/external.php functions, and in fact the primary function everything uses should probably be in there.

            Show
            Martin Dougiamas added a comment - Just a note, that the create_user_record function should be closely tied to the /user/external.php functions, and in fact the primary function everything uses should probably be in there.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi,

            just to point that last Monday I introduced exactly this "problem" in HQ chat. And it seems that there is some agreement about which core functions should be used and so on. Just today Jerome ignited http://docs.moodle.org/en/Development:Libraries_Organization

            So, perhaps it's a good idea to have that organization defined and explained and then, go a fix all the "user creation" uses along Moodle? Note that, in Jerome's list above, it is the create_user_record() you commented that will be moved, for sure. Also note that the name of that function is really incorrect, as far as it no only creates the user record but does another things (setting prefs and so on). So, at the end, I think we need one function validating "important data" (username, email...) and creating all the minimum stuff (preferences...). Your function, Rosie, looks correct for the 1st part (validate important data), but skips the 2nd one (create minimum stuff).

            Plz, comment with Jerome/Martin about it, I think we need to define:

            • Which one is the create_user() we are going to use (only one IMO).
            • Where is it going to be (moodlelib, user/lib or userlib)
            • Which ones are going to be its responsibilitites (only validate and create user record, or also the rest of the preferences...).

            Once that has been agreed and documented... is when we'll be able to replace all current insert_record('user') but not before.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi, just to point that last Monday I introduced exactly this "problem" in HQ chat. And it seems that there is some agreement about which core functions should be used and so on. Just today Jerome ignited http://docs.moodle.org/en/Development:Libraries_Organization So, perhaps it's a good idea to have that organization defined and explained and then, go a fix all the "user creation" uses along Moodle? Note that, in Jerome's list above, it is the create_user_record() you commented that will be moved, for sure. Also note that the name of that function is really incorrect, as far as it no only creates the user record but does another things (setting prefs and so on). So, at the end, I think we need one function validating "important data" (username, email...) and creating all the minimum stuff (preferences...). Your function, Rosie, looks correct for the 1st part (validate important data), but skips the 2nd one (create minimum stuff). Plz, comment with Jerome/Martin about it, I think we need to define: Which one is the create_user() we are going to use (only one IMO). Where is it going to be (moodlelib, user/lib or userlib) Which ones are going to be its responsibilitites (only validate and create user record, or also the rest of the preferences...). Once that has been agreed and documented... is when we'll be able to replace all current insert_record('user') but not before. Ciao
            Hide
            Rossiani Wijaya added a comment -

            adding Jerome as a watcher.

            Jerome when you have time, could we talk about this?

            Thanks

            Show
            Rossiani Wijaya added a comment - adding Jerome as a watcher. Jerome when you have time, could we talk about this? Thanks
            Hide
            Jérôme Mouneyrac added a comment -

            Hi, I've just read this issue.

            1. First about the issue description and 'timecreated'.
            I've got an issue MDL-12673 that is in fact quite similar because we wondered if firstaccess column into the user table would be the "first time creation" or "first time successfull login". Finally it will be the "first time successfull login".
            This MDL-21226 issue description is "we would like to find out how long users have been using the system, and since enrolment is completely automated, we do not have a paper trail of when somebody signed up. ", fixing MDL-12673 will resolve the need => you can check firstaccess column to know how long the users have been using the system.
            So IMO we should close this MDL-21226 issue.

            2. About the second point that is the missing create_users function, as Eloy said we agree that we need to have one create_users function. However IMO we should create a new issue about it.

            Show
            Jérôme Mouneyrac added a comment - Hi, I've just read this issue. 1. First about the issue description and 'timecreated'. I've got an issue MDL-12673 that is in fact quite similar because we wondered if firstaccess column into the user table would be the "first time creation" or "first time successfull login". Finally it will be the "first time successfull login". This MDL-21226 issue description is "we would like to find out how long users have been using the system, and since enrolment is completely automated, we do not have a paper trail of when somebody signed up. ", fixing MDL-12673 will resolve the need => you can check firstaccess column to know how long the users have been using the system. So IMO we should close this MDL-21226 issue. 2. About the second point that is the missing create_users function, as Eloy said we agree that we need to have one create_users function. However IMO we should create a new issue about it.
            Hide
            Martin Dougiamas added a comment -

            We definitely should add the timecreated field, it's different to firstaccess. So the first patch can be applied now to HEAD, Rosie, thanks.

            The replacement of all the calls needs to be done next. That's a separate issue to this one, and can be done as soon as Jerome finishes the user_create_user() function in user/lib.php (http://docs.moodle.org/en/Development:Libraries_Organization) which he's working on now.

            Show
            Martin Dougiamas added a comment - We definitely should add the timecreated field, it's different to firstaccess. So the first patch can be applied now to HEAD, Rosie, thanks. The replacement of all the calls needs to be done next. That's a separate issue to this one, and can be done as soon as Jerome finishes the user_create_user() function in user/lib.php ( http://docs.moodle.org/en/Development:Libraries_Organization ) which he's working on now.
            Jérôme Mouneyrac made changes -
            Link This issue has been marked as being related by MDL-21497 [ MDL-21497 ]
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Agree!

            Please don't forget my 1st comment:

            Automatic message: new field in DB uses to mean (99% chance of) it needs to be also handled by backup & restore.

            Feel free to do so after committing current patch, np with that. And do it in a new bug, np neither. It should achieve:

            1) On backup, save the timecreated field to XML
            2) On restore, load the timecreated field from XML
            3) On restore, if new user is being created, set it to time()

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Agree! Please don't forget my 1st comment: Automatic message: new field in DB uses to mean (99% chance of) it needs to be also handled by backup & restore. Feel free to do so after committing current patch, np with that. And do it in a new bug, np neither. It should achieve: 1) On backup, save the timecreated field to XML 2) On restore, load the timecreated field from XML 3) On restore, if new user is being created, set it to time() Ciao
            Hide
            Rossiani Wijaya added a comment -

            committed to 2.0

            Show
            Rossiani Wijaya added a comment - committed to 2.0
            Rossiani Wijaya made changes -
            Status Open [ 1 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            Hide
            Martin Dougiamas added a comment -

            Rosie did you see Eloy's comment on backup/restore?

            Show
            Martin Dougiamas added a comment - Rosie did you see Eloy's comment on backup/restore?
            Martin Dougiamas made changes -
            Resolution Fixed [ 1 ]
            Status Resolved [ 5 ] Reopened [ 4 ]
            Hide
            Rossiani Wijaya added a comment -

            yes i did. I want to create new tracker for backup/restore. Would it be better if I just put it under this tracker?

            Either way is fine with me. i'm working on it right now.

            Show
            Rossiani Wijaya added a comment - yes i did. I want to create new tracker for backup/restore. Would it be better if I just put it under this tracker? Either way is fine with me. i'm working on it right now.
            Hide
            Rossiani Wijaya added a comment -

            I'm creating new tracker for backup and restore MDL-21509 (suggested by Jerome).

            Show
            Rossiani Wijaya added a comment - I'm creating new tracker for backup and restore MDL-21509 (suggested by Jerome).
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Thanks, Rosie. Re-resolving this as fixed.

            Show
            Eloy Lafuente (stronk7) added a comment - Thanks, Rosie. Re-resolving this as fixed.
            Eloy Lafuente (stronk7) made changes -
            Status Reopened [ 4 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            Eloy Lafuente (stronk7) made changes -
            Link This issue has a non-specific relationship to MDL-21509 [ MDL-21509 ]
            Martin Dougiamas made changes -
            Status Resolved [ 5 ] Closed [ 6 ]
            QA Assignee nobody
            Martin Dougiamas made changes -
            Workflow jira [ 34604 ] MDL Workflow [ 63634 ]
            Martin Dougiamas made changes -
            Workflow MDL Workflow [ 63634 ] MDL Full Workflow [ 92879 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: