Merge branch 'MDL-64958-35' of git://github.com/junpataleta/moodle into MOODLE_35_STABLE

grade/import/csv/classes/load_data.php

@@ -221,20 +221,23 @@ class gradeimport_csv_load_data {
     protected function check_user_exists($value, $userfields) {
         global $DB;
 
-        $usercheckproblem = false;
         $user = null;
+        $errorkey = false;
         // The user may use the incorrect field to match the user. This could result in an exception.
         try {
-            $user = $DB->get_record('user', array($userfields['field'] => $value));
+            // Make sure the record exists and that there's only one matching record found.
-        } catch (Exception $e) {
+            $user = $DB->get_record('user', array($userfields['field'] => $value), '*', MUST_EXIST);
-            $usercheckproblem = true;
+        } catch (dml_missing_record_exception $missingex) {
+            $errorkey = 'usermappingerror';
+        } catch (dml_multiple_records_exception $multiex) {
+            $errorkey = 'usermappingerrormultipleusersfound';
         }
         // Field may be fine, but no records were returned.
-        if (!$user || $usercheckproblem) {
+        if ($errorkey) {
             $usermappingerrorobj = new stdClass();
             $usermappingerrorobj->field = $userfields['label'];
             $usermappingerrorobj->value = $value;
-            $this->cleanup_import(get_string('usermappingerror', 'grades', $usermappingerrorobj));
+            $this->cleanup_import(get_string($errorkey, 'grades', $usermappingerrorobj));
             unset($usermappingerrorobj);
             return null;
         }

grade/import/csv/tests/load_data_test.php

@@ -243,57 +243,116 @@ Bobby,Bunce,,"Moodle HQ","Rock on!",student5@example.com,75.00,,75.00,{exportdat
     }
 
     /**
-     * Check that the user matches a user in the system.
+     * Data provider for \gradeimport_csv_load_data_testcase::test_check_user_exists().
+     *
+     * @return array
      */
-    public function test_check_user_exists() {
+    public function check_user_exists_provider() {
+        return [
+            'Fetch by email' => [
+                'email', 's1@example.com', true
+            ],
+            'Fetch data using a non-existent email' => [
+                'email', 's2@example.com', false
+            ],
+            'Multiple accounts with the same email' => [
+                'email', 's1@example.com', false, 1
+            ],
+            'Fetch data using a valid user ID' => [
+                'id', true, true
+            ],
+            'Fetch data using a non-existent user ID' => [
+                'id', false, false
+            ],
+            'Fetch data using a valid username' => [
+                'username', 's1', true
+            ],
+            'Fetch data using an invalid username' => [
+                'username', 's2', false
+            ],
+            'Fetch data using a valid ID Number' => [
+                'idnumber', 's1', true
+            ],
+            'Fetch data using an invalid ID Number' => [
+                'idnumber', 's2', false
+            ],
+        ];
+    }
+
+    /**
+     * Check that the user matches a user in the system.
+     *
+     * @dataProvider check_user_exists_provider
+     * @param string $field The field to use for the query.
+     * @param string|boolean $value The field value. When fetching by ID, set true to fetch valid user ID, false otherwise.
+     * @param boolean $successexpected Whether we expect for a user to be found or not.
+     * @param int $allowaccountssameemail Value for $CFG->allowaccountssameemail
+     */
+    public function test_check_user_exists($field, $value, $successexpected, $allowaccountssameemail = 0) {
+        $this->resetAfterTest();
+
+        $generator = $this->getDataGenerator();
 
         // Need to add one of the users into the system.
-        $user = new stdClass();
+        $user = $generator->create_user([
-        $user->firstname = 'Anne';
+            'firstname' => 'Anne',
-        $user->lastname = 'Able';
+            'lastname' => 'Able',
-        $user->email = 'student7@example.com';
+            'email' => 's1@example.com',
-        $userdetail = $this->getDataGenerator()->create_user($user);
+            'idnumber' => 's1',
+            'username' => 's1',
+        ]);
+
+        if ($allowaccountssameemail) {
+            // Create another user with the same email address.
+            $generator->create_user(['email' => 's1@example.com']);
+        }
+
+        // Since the data provider can't know what user ID to use, do a special handling for ID field tests.
+        if ($field === 'id') {
+            if ($value) {
+                // Test for fetching data using a valid user ID. Use the generated user's ID.
+                $value = $user->id;
+            } else {
+                // Test for fetching data using a non-existent user ID.
+                $value = $user->id + 1;
+            }
+        }
+
+        $userfields = [
+            'field' => $field,
+            'label' => 'Field label: ' . $field
+        ];
 
         $testobject = new phpunit_gradeimport_csv_load_data();
 
-        $testarray = $this->csv_load($this->oktext);
+        // Check whether the user exists. If so, then the user id is returned. Otherwise, it returns null.
+        $userid = $testobject->test_check_user_exists($value, $userfields);
 
-        $userfields = array('field' => 'email', 'label' => 'Email address');
+        if ($successexpected) {
-        // If the user exists then the user id is returned.
+            // Check that the user id returned matches with the user that we created.
-        $userid = $testobject->test_check_user_exists($testarray[0][5] , $userfields);
+            $this->assertEquals($user->id, $userid);
-        // Check that the user id returned matches with the user that we created.
-        $this->assertEquals($userid, $userdetail->id);
 
-        // Check for failure.
+            // Check that there are no errors.
-        // Try for an exception.
+            $this->assertEmpty($testobject->get_gradebookerrors());
-        $userfields = array('field' => 'id', 'label' => 'userid');
-        $userid = $testobject->test_check_user_exists($testarray[0][0], $userfields);
-        // Check that the userid is null.
-        $this->assertNull($userid);
 
-        // Expected error message.
+        } else {
-        $mappingobject = new stdClass();
+            // Check that the userid is null.
-        $mappingobject->field = $userfields['label'];
+            $this->assertNull($userid);
-        $mappingobject->value = $testarray[0][0];
-        $expectederrormessage = get_string('usermappingerror', 'grades', $mappingobject);
-        // Check that expected error message and actual message match.
-        $gradebookerrors = $testobject->get_gradebookerrors();
-        $this->assertEquals($expectederrormessage, $gradebookerrors[0]);
 
-        // The field mapping is correct, but the student does not exist.
+            // Check that expected error message and actual message match.
-        $userid = $testobject->test_check_user_exists($testarray[1][5], $userfields);
+            $gradebookerrors = $testobject->get_gradebookerrors();
-        // Check that the userid is null.
+            $mappingobject = (object)[
-        $this->assertNull($userid);
+                'field' => $userfields['label'],
+                'value' => $value,
+            ];
+            if ($allowaccountssameemail) {
+                $expectederrormessage = get_string('usermappingerrormultipleusersfound', 'grades', $mappingobject);
+            } else {
+                $expectederrormessage = get_string('usermappingerror', 'grades', $mappingobject);
+            }
 
-        // Expected error message.
+            $this->assertEquals($expectederrormessage, $gradebookerrors[0]);
-        $mappingobject = new stdClass();
+        }
-        $mappingobject->field = $userfields['label'];
-        $mappingobject->value = $testarray[1][5];
-        $expectederrormessage = get_string('usermappingerror', 'grades', $mappingobject);
-        // Check that expected error message and actual message match.
-        $gradebookerrors = $testobject->get_gradebookerrors();
-        // This is the second error in the array of gradebook errors.
-        $this->assertEquals($expectederrormessage, $gradebookerrors[1]);
     }
 
     /**

lang/en/grades.php

@@ -831,6 +831,7 @@ $string['usergrade'] = 'User {$a->fullname} ({$a->useridnumber}) on item {$a->gr
 $string['userid'] = 'User ID';
 $string['useridnumberwarning'] = 'Users without an ID number are excluded from the export as they cannot be imported';
 $string['usermappingerror'] = 'User mapping error: Could not find user with {$a->field} of "{$a->value}".';
+$string['usermappingerrormultipleusersfound'] = 'User mapping error: Multiple users found with {$a->field} of "{$a->value}". Please use a more unique mapping field.';
 $string['usermappingerrorusernotfound'] = 'User mapping error. Could not find user.';
 $string['usermappingerrorcurrentgroup'] = 'User is not a member of current group.';
 $string['userpreferences'] = 'User preferences';