From 5b529aca15e9338bd144060b9674cb4b2bbefb4e Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Mon, 6 Apr 2020 12:36:35 +0200 Subject: [PATCH] MDL-68333 testing: trigger user_created event in user generator --- .../manual/tests/behat/quickenrolment.feature | 4 +- grade/tests/report_graderlib_test.php | 2 +- grade/tests/reportuserlib_test.php | 4 +- lib/testing/generator/data_generator.php | 79 ++++++------------- lib/testing/tests/generator_test.php | 4 +- lib/tests/accesslib_test.php | 2 - lib/tests/moodlelib_test.php | 8 +- lib/tests/outputcomponents_test.php | 32 +++++--- lib/upgrade.txt | 1 + user/tests/externallib_test.php | 10 +-- user/tests/privacy_test.php | 8 +- user/tests/profilelib_test.php | 30 +++++++ user/tests/userlib_test.php | 8 +- 13 files changed, 102 insertions(+), 90 deletions(-) diff --git a/enrol/manual/tests/behat/quickenrolment.feature b/enrol/manual/tests/behat/quickenrolment.feature index f2f3f6cc139..cd3c979cf22 100644 --- a/enrol/manual/tests/behat/quickenrolment.feature +++ b/enrol/manual/tests/behat/quickenrolment.feature @@ -171,7 +171,7 @@ Feature: Teacher can search and enrol users one by one into the course When I log in as "admin" Then the following "users" exist: | username | firstname | lastname | email | phone1 | phone2 | department | institution | city | country | - | student100 | Student | 100 | student100@example.com | 1234567892 | 1234567893 | ABC1 | ABC2 | CITY1 | UK | + | student100 | Student | 100 | student100@example.com | 1234567892 | 1234567893 | ABC1 | ABC2 | CITY1 | GB | And the following config values are set as admin: | showuseridentity | idnumber,email,city,country,phone1,phone2,department,institution | When I am on "Course 001" course homepage @@ -179,7 +179,7 @@ Feature: Teacher can search and enrol users one by one into the course And I press "Enrol users" When I set the field "Select users" to "student100@example.com" And I click on ".form-autocomplete-downarrow" "css_element" in the "Select users" "form_row" - Then I should see "student100@example.com, CITY1, UK, 1234567892, 1234567893, ABC1, ABC2" + Then I should see "student100@example.com, CITY1, GB, 1234567892, 1234567893, ABC1, ABC2" # Remove identity field in setting User policies And the following config values are set as admin: | showuseridentity | idnumber,email,phone1,phone2,department,institution | diff --git a/grade/tests/report_graderlib_test.php b/grade/tests/report_graderlib_test.php index 152fc3b2ec1..d617f487cbe 100644 --- a/grade/tests/report_graderlib_test.php +++ b/grade/tests/report_graderlib_test.php @@ -47,7 +47,7 @@ class core_grade_report_graderlib_testcase extends advanced_testcase { $course = $this->getDataGenerator()->create_course(); // Create and enrol a student. - $student = $this->getDataGenerator()->create_user(array('username' => 'Student Sam')); + $student = $this->getDataGenerator()->create_user(array('username' => 'student_sam')); $role = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST); $this->getDataGenerator()->enrol_user($student->id, $course->id, $role->id); diff --git a/grade/tests/reportuserlib_test.php b/grade/tests/reportuserlib_test.php index 94ae2925e72..301b6bdad60 100644 --- a/grade/tests/reportuserlib_test.php +++ b/grade/tests/reportuserlib_test.php @@ -56,11 +56,11 @@ class core_grade_reportuserlib_testcase extends advanced_testcase { $coursecontext = context_course::instance($course->id); // Create and enrol test users. - $student = $this->getDataGenerator()->create_user(array('username' => 'Student Sam')); + $student = $this->getDataGenerator()->create_user(array('username' => 'student_sam')); $role = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST); $this->getDataGenerator()->enrol_user($student->id, $course->id, $role->id); - $teacher = $this->getDataGenerator()->create_user(array('username' => 'Teacher T')); + $teacher = $this->getDataGenerator()->create_user(array('username' => 'teacher_t')); $role = $DB->get_record('role', array('shortname' => 'editingteacher'), '*', MUST_EXIST); $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $role->id); diff --git a/lib/testing/generator/data_generator.php b/lib/testing/generator/data_generator.php index 2e46b8b4377..c8e031d6c2a 100644 --- a/lib/testing/generator/data_generator.php +++ b/lib/testing/generator/data_generator.php @@ -142,6 +142,7 @@ EOD; */ public function create_user($record=null, array $options=null) { global $DB, $CFG; + require_once($CFG->dirroot.'/user/lib.php'); $this->usercounter++; $i = $this->usercounter; @@ -206,10 +207,6 @@ EOD; if (isset($record['password'])) { $record['password'] = hash_internal_user_password($record['password']); - } else { - // The auth plugin may not fully support this, - // but it is still better/faster than hashing random stuff. - $record['password'] = AUTH_PASSWORD_NOT_CACHED; } if (!isset($record['email'])) { @@ -220,71 +217,41 @@ EOD; $record['confirmed'] = 1; } - if (!isset($record['lang'])) { - $record['lang'] = 'en'; - } - - if (!isset($record['maildisplay'])) { - $record['maildisplay'] = $CFG->defaultpreference_maildisplay; - } - - if (!isset($record['mailformat'])) { - $record['mailformat'] = $CFG->defaultpreference_mailformat; - } - - if (!isset($record['maildigest'])) { - $record['maildigest'] = $CFG->defaultpreference_maildigest; - } - - if (!isset($record['autosubscribe'])) { - $record['autosubscribe'] = $CFG->defaultpreference_autosubscribe; - } - - if (!isset($record['trackforums'])) { - $record['trackforums'] = $CFG->defaultpreference_trackforums; - } - - if (!isset($record['deleted'])) { - $record['deleted'] = 0; - } - - if (!isset($record['timecreated'])) { - $record['timecreated'] = time(); - } - - $record['timemodified'] = $record['timecreated']; - if (!isset($record['lastip'])) { $record['lastip'] = '0.0.0.0'; } - if ($record['deleted']) { - $delname = $record['email'].'.'.time(); - while ($DB->record_exists('user', array('username'=>$delname))) { - $delname++; - } - $record['idnumber'] = ''; - $record['email'] = md5($record['username']); - $record['username'] = $delname; - $record['picture'] = 0; + $tobedeleted = !empty($record['deleted']); + unset($record['deleted']); + + $userid = user_create_user($record, false, false); + + if ($extrafields = array_intersect_key($record, ['password' => 1, 'timecreated' => 1])) { + $DB->update_record('user', ['id' => $userid] + $extrafields); } - $userid = $DB->insert_record('user', $record); - - if (!$record['deleted']) { - context_user::instance($userid); - + if (!$tobedeleted) { // All new not deleted users must have a favourite self-conversation. $selfconversation = \core_message\api::create_conversation( \core_message\api::MESSAGE_CONVERSATION_TYPE_SELF, [$userid] ); \core_message\api::set_favourite_conversation($selfconversation->id, $userid); + + // Save custom profile fields data. + $hasprofilefields = array_filter($record, function($key){ + return strpos($key, 'profile_field_') === 0; + }, ARRAY_FILTER_USE_KEY); + if ($hasprofilefields) { + require_once($CFG->dirroot.'/user/profile/lib.php'); + $usernew = (object)(['id' => $userid] + $record); + profile_save_data($usernew); + } } $user = $DB->get_record('user', array('id' => $userid), '*', MUST_EXIST); - if (!$record['deleted'] && isset($record['interests'])) { + if (!$tobedeleted && isset($record['interests'])) { require_once($CFG->dirroot . '/user/editlib.php'); if (!is_array($record['interests'])) { $record['interests'] = preg_split('/\s*,\s*/', trim($record['interests']), -1, PREG_SPLIT_NO_EMPTY); @@ -292,6 +259,12 @@ EOD; useredit_update_interests($user, $record['interests']); } + \core\event\user_created::create_from_userid($userid)->trigger(); + + if ($tobedeleted) { + delete_user($user); + $user = $DB->get_record('user', array('id' => $userid)); + } return $user; } diff --git a/lib/testing/tests/generator_test.php b/lib/testing/tests/generator_test.php index d22a9101a4a..2d9990fae25 100644 --- a/lib/testing/tests/generator_test.php +++ b/lib/testing/tests/generator_test.php @@ -70,7 +70,6 @@ class core_test_generator_testcase extends advanced_testcase { $this->assertEquals($count + 1, $DB->count_records('user')); $this->assertSame($user->username, core_user::clean_field($user->username, 'username')); $this->assertSame($user->email, core_user::clean_field($user->email, 'email')); - $this->assertSame(AUTH_PASSWORD_NOT_CACHED, $user->password); $this->assertNotEmpty($user->firstnamephonetic); $this->assertNotEmpty($user->lastnamephonetic); $this->assertNotEmpty($user->alternatename); @@ -97,7 +96,6 @@ class core_test_generator_testcase extends advanced_testcase { 'password' => 'password1', 'email' => 'email@example.com', 'confirmed' => '1', - 'lang' => 'cs', 'maildisplay' => '1', 'mailformat' => '0', 'maildigest' => '1', @@ -128,7 +126,7 @@ class core_test_generator_testcase extends advanced_testcase { $this->assertEquals($count + 3, $DB->count_records('user')); $this->assertSame('', $user->idnumber); $this->assertSame(md5($record['username']), $user->email); - $this->assertFalse(context_user::instance($user->id, IGNORE_MISSING)); + $this->assertEquals(1, $user->deleted); // Test generating user with interests. $user = $generator->create_user(array('interests' => 'Cats, Dogs')); diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php index dcf3646c10a..0382f72e8e5 100644 --- a/lib/tests/accesslib_test.php +++ b/lib/tests/accesslib_test.php @@ -2749,8 +2749,6 @@ class core_accesslib_testcase extends advanced_testcase { $bi = $generator->create_block('online_users', array('parentcontextid'=>$usercontext->id)); $testblocks[] = $bi->id; } - // Deleted user - should be ignored everywhere, can not have context. - $generator->create_user(array('deleted'=>1)); // Add block to frontpage. $bi = $generator->create_block('online_users', array('parentcontextid'=>$frontpagecontext->id)); diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index fbfbbb6850d..1648a8fb9fb 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -2862,10 +2862,12 @@ class core_moodlelib_testcase extends advanced_testcase { * the user table and fire event. */ public function test_update_internal_user_password_no_cache() { + global $DB; $this->resetAfterTest(); $user = $this->getDataGenerator()->create_user(array('auth' => 'cas')); - $this->assertEquals(AUTH_PASSWORD_NOT_CACHED, $user->password); + $DB->update_record('user', ['id' => $user->id, 'password' => AUTH_PASSWORD_NOT_CACHED]); + $user->password = AUTH_PASSWORD_NOT_CACHED; $sink = $this->redirectEvents(); update_internal_user_password($user, 'wonkawonka'); @@ -3594,7 +3596,7 @@ class core_moodlelib_testcase extends advanced_testcase { $user = $this->getDataGenerator()->create_user( [ "username" => $username, - "confirmed" => false, + "confirmed" => 0, "email" => 'test@example.com', ] ); @@ -3623,7 +3625,7 @@ class core_moodlelib_testcase extends advanced_testcase { $user = $this->getDataGenerator()->create_user( [ "username" => "many_-.@characters@_@-..-..", - "confirmed" => false, + "confirmed" => 0, "email" => 'test@example.com', ] ); diff --git a/lib/tests/outputcomponents_test.php b/lib/tests/outputcomponents_test.php index 0a631aa1345..a5e1515f06c 100644 --- a/lib/tests/outputcomponents_test.php +++ b/lib/tests/outputcomponents_test.php @@ -140,11 +140,17 @@ class core_outputcomponents_testcase extends advanced_testcase { $user2 = $this->getDataGenerator()->create_user(array('picture'=>0, 'email'=>'user2@example.com')); $context2 = context_user::instance($user2->id); + // User 3 is deleted. $user3 = $this->getDataGenerator()->create_user(array('picture'=>1, 'deleted'=>1, 'email'=>'user3@example.com')); - $context3 = context_user::instance($user3->id, IGNORE_MISSING); + $this->assertNotEmpty(context_user::instance($user3->id)); $this->assertEquals(0, $user3->picture); $this->assertNotEquals('user3@example.com', $user3->email); - $this->assertFalse($context3); + + // User 4 is incorrectly deleted with its context deleted as well (testing legacy code). + $user4 = $this->getDataGenerator()->create_user(['picture' => 1, 'deleted' => 1, 'email' => 'user4@example.com']); + context_helper::delete_instance(CONTEXT_USER, $user4->id); + $this->assertEquals(0, $user4->picture); + $this->assertNotEquals('user4@example.com', $user4->email); // Try legacy picture == 1. $user1->picture = 1; @@ -186,13 +192,14 @@ class core_outputcomponents_testcase extends advanced_testcase { $this->assertSame($CFG->wwwroot.'/theme/image.php/boost/core/1/u/f2', $up3->get_url($page, $renderer)->out(false)); $this->assertEquals($reads, $DB->perf_get_reads()); - // Try incorrectly deleted users (with valid email and pciture flag) - some DB reads expected. - $user3->email = 'user3@example.com'; - $user3->picture = 1; + // Try incorrectly deleted users (with valid email and picture flag, but user context removed) - some DB reads expected. + unset($user4->deleted); + $user4->email = 'user4@example.com'; + $user4->picture = 1; $reads = $DB->perf_get_reads(); - $up3 = new user_picture($user3); + $up4 = new user_picture($user4); $this->assertEquals($reads, $DB->perf_get_reads()); - $this->assertSame($CFG->wwwroot.'/theme/image.php/boost/core/1/u/f2', $up3->get_url($page, $renderer)->out(false)); + $this->assertSame($CFG->wwwroot.'/theme/image.php/boost/core/1/u/f2', $up4->get_url($page, $renderer)->out(false)); $this->assertGreaterThan($reads, $DB->perf_get_reads()); // Test gravatar. @@ -203,6 +210,10 @@ class core_outputcomponents_testcase extends advanced_testcase { $user3->picture = 0; $up3 = new user_picture($user3); $this->assertSame($CFG->wwwroot.'/theme/image.php/boost/core/1/u/f2', $up3->get_url($page, $renderer)->out(false)); + $user4->email = 'deleted'; + $user4->picture = 0; + $up4 = new user_picture($user4); + $this->assertSame($CFG->wwwroot.'/theme/image.php/boost/core/1/u/f2', $up4->get_url($page, $renderer)->out(false)); // Http version. $CFG->wwwroot = str_replace('https:', 'http:', $CFG->wwwroot); @@ -237,11 +248,14 @@ class core_outputcomponents_testcase extends advanced_testcase { $up1 = new user_picture($user1); $this->assertSame($CFG->wwwroot.'/pluginfile.php/'.$context1->id.'/user/icon/boost/f2?rev=11', $up1->get_url($page, $renderer)->out(false)); + $up2 = new user_picture($user2); + $this->assertSame('https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?s=35&d=https%3A%2F%2Fwww.example.com%2Fmoodle%2Fpix%2Fu%2Ff2.png', $up2->get_url($page, $renderer)->out(false)); + $up3 = new user_picture($user3); $this->assertSame($CFG->wwwroot.'/theme/image.php/boost/core/1/u/f2', $up3->get_url($page, $renderer)->out(false)); - $up2 = new user_picture($user2); - $this->assertSame('https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?s=35&d=https%3A%2F%2Fwww.example.com%2Fmoodle%2Fpix%2Fu%2Ff2.png', $up2->get_url($page, $renderer)->out(false)); + $up4 = new user_picture($user4); + $this->assertSame($CFG->wwwroot.'/theme/image.php/boost/core/1/u/f2', $up4->get_url($page, $renderer)->out(false)); // TODO MDL-44792 Rewrite those tests to use a fixture. // Now test gravatar with one theme having own images (afterburner). diff --git a/lib/upgrade.txt b/lib/upgrade.txt index c6e0d0d38d8..ec9b9757502 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -71,6 +71,7 @@ information provided here is intended especially for developers. - cron_execute_plugin_type() - cron_bc_hack_plugin_functions() Please, use the Task API instead: https://docs.moodle.org/dev/Task_API +* Data generator create_user in both unittests and behat now validates user fields and triggers user_created event === 3.8 === * Add CLI option to notify all cron tasks to stop: admin/cli/cron.php --stop diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index fde87863b68..71de5348a70 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -223,8 +223,6 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { 'city' => 'Perth', 'url' => 'http://moodle.org', 'country' => 'AU', - 'lang' => 'kkl', - 'theme' => 'kkt', ); $user1 = self::getDataGenerator()->create_user($user1); if (!empty($CFG->usetags)) { @@ -330,11 +328,9 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { if (!empty($CFG->usetags) and !empty($generateduser->interests)) { $this->assertEquals(implode(', ', $generateduser->interests), $returneduser['interests']); } - // Check empty since incorrect values were used when creating the user. - if ($returneduser['id'] == $user1->id) { - $this->assertEmpty($returneduser['lang']); - $this->assertEmpty($returneduser['theme']); - } + // Default language and no theme were used for the user. + $this->assertEquals($CFG->lang, $returneduser['lang']); + $this->assertEmpty($returneduser['theme']); } } diff --git a/user/tests/privacy_test.php b/user/tests/privacy_test.php index cc5e6ed43aa..cedc5658bd3 100644 --- a/user/tests/privacy_test.php +++ b/user/tests/privacy_test.php @@ -151,7 +151,7 @@ class core_user_privacy_testcase extends provider_testcase { 'institution' => 'test', 'department' => 'Science', 'city' => 'Perth', - 'country' => 'au' + 'country' => 'AU' ]); $user2 = $this->getDataGenerator()->create_user(); $course = $this->getDataGenerator()->create_course(); @@ -222,7 +222,7 @@ class core_user_privacy_testcase extends provider_testcase { 'institution' => 'test', 'department' => 'Science', 'city' => 'Perth', - 'country' => 'au' + 'country' => 'AU' ]); $user2 = $this->getDataGenerator()->create_user(); $course = $this->getDataGenerator()->create_course(); @@ -328,7 +328,7 @@ class core_user_privacy_testcase extends provider_testcase { 'institution' => 'test', 'department' => 'Science', 'city' => 'Perth', - 'country' => 'au' + 'country' => 'AU' ]); $usercontext1 = \context_user::instance($user1->id); $userlist1 = new \core_privacy\local\request\userlist($usercontext1, $component); @@ -342,7 +342,7 @@ class core_user_privacy_testcase extends provider_testcase { 'institution' => 'test', 'department' => 'Science', 'city' => 'Perth', - 'country' => 'au' + 'country' => 'AU' ]); $usercontext2 = \context_user::instance($user2->id); $userlist2 = new \core_privacy\local\request\userlist($usercontext2, $component); diff --git a/user/tests/profilelib_test.php b/user/tests/profilelib_test.php index 0c4c5993dac..59d8b5dc603 100644 --- a/user/tests/profilelib_test.php +++ b/user/tests/profilelib_test.php @@ -210,4 +210,34 @@ class core_user_profilelib_testcase extends advanced_testcase { $this->assertTrue(user_not_fully_set_up($roaminghermione, true)); $this->assertTrue(user_not_fully_set_up($roaminghermione, false)); } + + /** + * Test that user generator sets the custom profile fields + */ + public function test_profile_fields_in_generator() { + global $CFG, $DB; + require_once($CFG->dirroot.'/mnet/lib.php'); + + $this->resetAfterTest(); + + // Add a required, visible, unlocked custom field. + $DB->insert_record('user_info_field', ['shortname' => 'house', 'name' => 'House', 'required' => 1, + 'visible' => 1, 'locked' => 0, 'categoryid' => 1, 'datatype' => 'text']); + + // Create some student accounts. + $hermione = $this->getDataGenerator()->create_user(['profile_field_house' => 'Gryffindor']); + $harry = $this->getDataGenerator()->create_user(); + + // Only students with required fields filled should be considered as fully set up. + $this->assertFalse(user_not_fully_set_up($hermione)); + $this->assertTrue(user_not_fully_set_up($harry)); + + // Test that the profile fields were actually set. + $profilefields1 = profile_user_record($hermione->id); + $this->assertEquals('Gryffindor', $profilefields1->house); + + $profilefields2 = profile_user_record($harry->id); + $this->assertObjectHasAttribute('house', $profilefields2); + $this->assertNull($profilefields2->house); + } } diff --git a/user/tests/userlib_test.php b/user/tests/userlib_test.php index c8327f3e235..28c37fd097c 100644 --- a/user/tests/userlib_test.php +++ b/user/tests/userlib_test.php @@ -833,23 +833,23 @@ class core_userliblib_testcase extends advanced_testcase { $this->resetAfterTest(true); $this->setAdminUser(); // We need capabilities to view the data. $user = self::getDataGenerator()->create_user([ - 'auth' => 'auth_something', + 'auth' => 'email', 'confirmed' => '0', 'idnumber' => 'someidnumber', 'lang' => 'en', 'theme' => $CFG->theme, - 'timezone' => '50', + 'timezone' => '5', 'mailformat' => '0', ]); // Fields that should get by default. $got = user_get_user_details($user); - self::assertSame('auth_something', $got['auth']); + self::assertSame('email', $got['auth']); self::assertSame('0', $got['confirmed']); self::assertSame('someidnumber', $got['idnumber']); self::assertSame('en', $got['lang']); self::assertSame($CFG->theme, $got['theme']); - self::assertSame('50', $got['timezone']); + self::assertSame('5', $got['timezone']); self::assertSame('0', $got['mailformat']); }