Moodle

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()

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

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)?

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

Yes, Rossiani that makes sense.

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).

attaching patch for HEAD.

Please review and comments.

thank you
Rosie

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

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

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

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

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.

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

adding Jerome as a watcher.

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

Thanks

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.

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.

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

committed to 2.0

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

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.

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

Thanks, Rosie. Re-resolving this as fixed.