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

      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

      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: