1/ overridden has two DD, right?

2/ I would expect hidden field with default in else following if (count($calendartypes) > 1) {

3/ could we have some tests for this in externallib pleae?

4/ why not add the missing fields centrally in user_create_user()?

Ciao and thanks, I agree this is necessary.

Thanks Petr,

1. Cheers! You are indeed correct.
2. Is this necessary? There is already a DB default, so not specifying a calendar type is not really an issue when inserting records in the DB. Also, the only time this condition will be false is if only the gregorian calendar is present (it can not be deleted via the GUI), so the DB default of 'gregorian' is fine.
3. I have moved the logic to core functions in user/lib.php rather than in the web service, so I am not sure it is necessary to create tests for externallib.php for this one.
4. I moved all the logic into this function. Please let me know if this is what you were talking abt.

Cheers!

2. I guess we should not rely on DB defaults here, the site might configure different default calendar, I believe it is better to use hidden fields consistently.

more comments:

a/ // Calendar settings - trailing . missing

b/ adding email restrictions is not OK, there was no real internal requirement for email presence or uniqueness before - this is going to break stuff big time!

c/ if (!empty($user->auth)) { if not ok when creating users, it must be present

d/ the user update allows you to delete user->auth value which is not allowed

e/ hmm, these two methods for creation and update of users modify the $user parameter - that seems wrong, we should do clone there and it should be properly documented

f/ seems that user_udpate_user may create duplicate usernames and emails

g/ user_update_user should not allow modification the mnethostid

Hmm maybe this all needs a new cleanup issue and this could be kept for calendar stuff only...

Hi Petr, those comments are related to MDL-42935, can you comment there? I simply rebased this on top of that one as I knew there would be conflicts. Sorry.

oh, the calendar commit looks ok, the other one needs a bit more work imho

great, +1 for integration

Thanks Petr.

To integrators, I personally think this should go in 2.6, but it is up to you whether or not this does.

How can i find a simple patch file to apply on my moodle?

Hi Mark,

I personally think you should've got the feature finished by 2.6 and then this would've been available.

This is an improvement, not a bug and so will only go into master. Please file a backport request to request its backport to 2.6 and we'll vote on it: http://docs.moodle.org/dev/Integration_Review#Process_for_requesting_a_non_bug-fix_backport

Integrated to master only - thanks.

Yep, I unfortunately overlooked this setting, so have only just implemented it now. I have created MDL-43169 requesting this to be backported. Cheers.

Hi Mark,

core_userliblib_testcase::test_create_users

Undefined variable: CFG

/Users/Shared/Jenkins/Home/git_repositories/master/user/lib.php:71

/Users/Shared/Jenkins/Home/git_repositories/master/user/tests/userlib_test.php:107

/Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:80

auth_db_testcase::test_plugin

Undefined variable: CFG

/Users/Shared/Jenkins/Home/git_repositories/master/user/lib.php:71

/Users/Shared/Jenkins/Home/git_repositories/master/lib/moodlelib.php:4039

/Users/Shared/Jenkins/Home/git_repositories/master/auth/db/tests/db_test.php:300

/Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:80

This looks completely untested? You are not doing your backport request very much good..

I am not sure why I missed this. I tested using two scripts I created (which I also attached to MDL-42935). Originally the logic was here, but was moved out to two separate issues and maybe this broke something. Sorry.

Hi Dan, please pull https://github.com/markn86/moodle/commit/4c61097cc7

Thanks.

Thanks Mark

Failing again:

03. Moodle exception: Error writing to database More information about this error

    Debug info:

     ERROR:  null value in column "calendartype" violates not-null constraint

    DETAIL:  Failing row contains (3, manual, 1, 0, 0, 0, 1, jonest, $2y$04$j1kwOixwTZhed.Gi1JtV7.0MKBkAxDEayuQ3Njyy3XwtpPVDuXwa2, , Tom, Jones, jonest@someplace.edu, 0, , , , , , , , , , , Perth, AU, en, null, , 99, 0, 0, 0, 0, , , 0, , , 1, 1, 0, 2, 1, 0, 1386049828, 1386049828, 0, null, null, null, null, null).

    INSERT INTO euser (username,password,firstname,lastname,email,mnethostid,city,country,lang,timezone,mailformat,maildisplay,maildigest,autosubscribe,institution,department,idnumber,phone1,phone2,address,url,description,descriptionformat,auth,confirmed,timemodified,timecreated,suspended,calendartype) VALUES($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27,$28,$29) RETURNING id

    [array (

      'username' => 'jonest',

      'password' => '$2y$04$j1kwOixwTZhed.Gi1JtV7.0MKBkAxDEayuQ3Njyy3XwtpPVDuXwa2',

      'firstname' => 'Tom',

      'lastname' => 'Jones',

      'email' => 'jonest@someplace.edu',

      'mnethostid' => '1',

      'city' => 'Perth',

      'country' => 'AU',

      'lang' => 'en',

      'timezone' => '99',

      'mailformat' => '1',

      'maildisplay' => '2',

      'maildigest' => '0',

      'autosubscribe' => '1',

      'institution' => '',

      'department' => '',

      'idnumber' => '',

      'phone1' => '',

      'phone2' => '',

      'address' => '',

      'url' => '',

      'description' => '',

      'descriptionformat' => '1',

      'auth' => 'manual',

      'confirmed' => 1,

      'timemodified' => 1386049828,

      'timecreated' => 1386049828,

      'suspended' => 0,

      'calendartype' => NULL,

    )]

    Error code: dmlwriteexception

    Stack trace:

    line 444 of /lib/dml/moodle_database.php: dml_write_exception thrown

    line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()

    line 848 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()

    line 900 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->insert_record_raw()

    line 78 of /user/lib.php: call to pgsql_native_moodle_database->insert_record()

    line 805 of /admin/tool/uploaduser/index.php: call to user_create_user()

    In step `And I press "Upload users"'.                              # behat_forms::press_button()

    From scenario `Upload users enrolling them on courses and groups'. # /Users/danp/moodles/im_behating/moodle/admin/tests/behat/upload_users.feature:8

    Of feature `Upload users'.                                         # /Users/danp/moodles/im_behating/moodle/admin/tests/behat/upload_users.feature

Hi Mark,

This is breaking the complete behat suite combined with the other failure, so i'm reverting this issue.

Please resubmit once you've fixed it and tested it more throughly.

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

I couldn't replicate this breakage on behat .. I suspect $CFG->calendartype had not been defined, and the only way this could be (that I can think of) is if the site had not been upgraded.

Trying again by incrementing version

So far so good, (only part way through). So looks like you are probably right, sorry Mark.

No problem. Let me know how it goes and I will send this back to integration if all goes well.

Please instead of $string['calendartype'] and $string['configcalendartype'] can we start using the calendartype_desc variant? I know that consistency with the current code is important and we should convert all existing strings first. But maybe we can just stop introducing new config* strings first. See http://docs.moodle.org/dev/Help_strings for reference. Having these two strings with the same prefix will keep them together in alphabetical sorting which significantly helps translators to guess the context and the relationship of both strings.

Hey David, I agree. It would be ideal if the strings were together in the language file. However, I just copied what the existing standard was, as I did not realise we had a doc regarding help string changes so assumed the standard used in the file was ideal. I have amended the commit to use 'calendartype_desc', rather than 'configcalendartype'.

Thanks Mark - this has been integrated now

All testing instructions passed and I tested around a few more things and the validation seems pretty solid.

Only tested on master. Passing!

Thanks for the code, its now upstream!

Heres a fun trick to try in the spirit of Friday the 13th.
I hear if you stand in front a mirror, alone, in the dark, and say "Oracle" three times Petr Skoka will appear in the mirror and you'll see him deleting the Oracle driver from Moodle.

Thanks Mark for amending the string names. See the attached screenshot that hopefully illustrates it clearly that the new naming policy is just better. Cheers.

Is this included in 2.6.1 (Build: 20140113)?
I just installed 2.6.1 and the <yoursite>/admin/settings.php?section=calendar page does not have a setting to set the default calendar.

Hello, this new feature will be available in 2.7 only - see the "Fix version:" field at the top of this page.

Currently it is only in 2.7, Shamin.

Petr, Mary,
Just noticed the fix version is 2.7
Thanks for your reply

Just asking for clarification here as I document this - I see that the default calendar type is also available in 2.6.2 (ie on the sandbox site) so does that mean it was backported as well as being available in 2.7?

It's ok - I saw the other issue for 2.6.2 Removing docs_required as this is documented here http://docs.moodle.org/27/en/Calendar_settings - please check in case anything needs editing/improving.

Hi Mary
you're right. It's added to 2.6.2