From ead32c158ee5d25e68397ef17bd449534536cb55 Mon Sep 17 00:00:00 2001 From: Julien Boulen Date: Fri, 22 Aug 2025 09:57:01 +0200 Subject: [PATCH] MDL-76318 gradebook: Improve csv import error message --- public/grade/import/csv/classes/load_data.php | 52 ++++++++++--- .../import/csv/lang/en/gradeimport_csv.php | 1 + .../phpunit_gradeimport_csv_load_data.php | 75 ++++++++++++++----- .../grade/import/csv/tests/load_data_test.php | 74 ++++++++++++++---- 4 files changed, 162 insertions(+), 40 deletions(-) diff --git a/public/grade/import/csv/classes/load_data.php b/public/grade/import/csv/classes/load_data.php index cfe472dc8de..17e24a18a07 100644 --- a/public/grade/import/csv/classes/load_data.php +++ b/public/grade/import/csv/classes/load_data.php @@ -300,9 +300,10 @@ class gradeimport_csv_load_data { * @param int $key The line that we are currently working on. * @param bool $verbosescales Form setting for grading with scales. * @param string $value The grade value. + * @param int $linenumber The line number that we are currently working on. * @return array grades to be updated. */ - protected function update_grade_item($courseid, $map, $key, $verbosescales, $value) { + protected function update_grade_item($courseid, $map, $key, $verbosescales, $value, int $linenumber) { // Case of an id, only maps id of a grade_item. // This was idnumber. if (!$gradeitem = new grade_item(array('id' => $map[$key], 'courseid' => $courseid))) { @@ -330,7 +331,10 @@ class gradeimport_csv_load_data { array_unshift($scales, '-'); // Scales start at key 1. $key = array_search($value, $scales); if ($key === false) { - $this->cleanup_import(get_string('badgrade', 'grades')); + $this->cleanup_import(get_string('badgrade', 'gradeimport_csv', [ + 'badgrade' => $value, + 'linenumber' => $linenumber, + ])); return null; } $value = $key; @@ -346,7 +350,10 @@ class gradeimport_csv_load_data { $value = $validvalue; } else { // Non numeric grade value supplied, possibly mapped wrong column. - $this->cleanup_import(get_string('badgrade', 'grades')); + $this->cleanup_import(get_string('badgrade', 'gradeimport_csv', [ + 'badgrade' => $value, + 'linenumber' => $linenumber, + ])); return null; } } @@ -378,9 +385,19 @@ class gradeimport_csv_load_data { * @param int $courseid The course ID. * @param int $feedbackgradeid The ID of the grade item that the feedback relates to. * @param bool $verbosescales Form setting for grading with scales. + * @param int $linenumber The line number that we are currently working on. */ - protected function map_user_data_with_value($mappingidentifier, $value, $header, $map, $key, $courseid, $feedbackgradeid, - $verbosescales) { + protected function map_user_data_with_value( + $mappingidentifier, + $value, + $header, + $map, + $key, + $courseid, + $feedbackgradeid, + $verbosescales, + int $linenumber + ) { // Fields that the user can be mapped from. $userfields = array( @@ -423,8 +440,14 @@ class gradeimport_csv_load_data { default: // Existing grade items. if (!empty($map[$key])) { - $this->newgrades = $this->update_grade_item($courseid, $map, $key, $verbosescales, $value, - $mappingidentifier); + $this->newgrades = $this->update_grade_item( + $courseid, + $map, + $key, + $verbosescales, + $value, + $linenumber + ); } // Otherwise, we ignore this column altogether because user has chosen // to ignore them (e.g. institution, address etc). @@ -493,11 +516,13 @@ class gradeimport_csv_load_data { $csvimport->init(); + $linenumber = 1; while ($line = $csvimport->next()) { if (count($line) <= 1) { // There is no data on this line, move on. continue; } + $linenumber++; // Array to hold all grades to be inserted. $this->newgrades = array(); @@ -527,8 +552,17 @@ class gradeimport_csv_load_data { $feedbackgradeid = ''; } - $this->map_user_data_with_value($mappingidentifier, $value, $header, $map, $key, $courseid, $feedbackgradeid, - $verbosescales); + $this->map_user_data_with_value( + $mappingidentifier, + $value, + $header, + $map, + $key, + $courseid, + $feedbackgradeid, + $verbosescales, + $linenumber + ); if ($this->status === false) { return $this->status; } diff --git a/public/grade/import/csv/lang/en/gradeimport_csv.php b/public/grade/import/csv/lang/en/gradeimport_csv.php index 06f06562c55..45c7c6f31c3 100644 --- a/public/grade/import/csv/lang/en/gradeimport_csv.php +++ b/public/grade/import/csv/lang/en/gradeimport_csv.php @@ -23,6 +23,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +$string['badgrade'] = 'Supplied grade (\'{$a->badgrade}\') is invalid on line {$a->linenumber}.'; $string['csv:view'] = 'Import grades from CSV'; $string['gradepenalties'] = 'Grade penalties will not be applied to imported grades.'; $string['pluginname'] = 'CSV file'; diff --git a/public/grade/import/csv/tests/fixtures/phpunit_gradeimport_csv_load_data.php b/public/grade/import/csv/tests/fixtures/phpunit_gradeimport_csv_load_data.php index 814102e19f6..4df25bf51d4 100644 --- a/public/grade/import/csv/tests/fixtures/phpunit_gradeimport_csv_load_data.php +++ b/public/grade/import/csv/tests/fixtures/phpunit_gradeimport_csv_load_data.php @@ -78,13 +78,6 @@ class phpunit_gradeimport_csv_load_data extends gradeimport_csv_load_data { return $this->create_feedback($courseid, $itemid, $value); } - /** - * Method to open up the appropriate method for unit testing. - */ - public function test_update_grade_item($courseid, $map, $key, $verbosescales, $value) { - return $this->update_grade_item($courseid, $map, $key, $verbosescales, $value); - } - /** * Method to open up the appropriate method for unit testing. * @@ -92,15 +85,66 @@ class phpunit_gradeimport_csv_load_data extends gradeimport_csv_load_data { * @param array $map Mapping information provided by the user. * @param int $key The line that we are currently working on. * @param bool $verbosescales Form setting for grading with scales. - * @param string $value The grade value . + * @param string $value The grade value. + * @param int $linenumber The line number that we are currently working on. * @return array grades to be updated. */ - public function test_map_user_data_with_value($mappingidentifier, $value, $header, $map, $key, $courseid, $feedbackgradeid, - $verbosescales) { + public function test_update_grade_item( + int $courseid, + array $map, + int $key, + bool $verbosescales, + string $value, + int $linenumber + ) { + return $this->update_grade_item( + $courseid, + $map, + $key, + $verbosescales, + $value, + $linenumber + ); + } + + /** + * Method to open up the appropriate method for unit testing. + * + * @param string $mappingidentifier The user field that we are matching together. + * @param string $value The value we are checking / importing. + * @param array $header The column headers of the csv file. + * @param array $map Mapping information provided by the user. + * @param int $key Current row identifier. + * @param int $courseid The course ID. + * @param int $feedbackgradeid The ID of the grade item that the feedback relates to. + * @param bool $verbosescales Form setting for grading with scales. + * @param int $linenumber The line number that we are currently working on. + * @return mixed Returns an integer representing a user ID, an array of feedbacks or an array of grades. + */ + public function test_map_user_data_with_value( + string $mappingidentifier, + string $value, + array $header, + array $map, + int $key, + int $courseid, + int $feedbackgradeid, + bool $verbosescales, + int $linenumber + ) { // Set an import code. $this->importcode = 00001; - $this->map_user_data_with_value($mappingidentifier, $value, $header, $map, $key, $courseid, $feedbackgradeid, - $verbosescales); + $this->map_user_data_with_value( + $mappingidentifier, + $value, + $header, + $map, + $key, + $courseid, + $feedbackgradeid, + $verbosescales, + $linenumber + ); switch ($mappingidentifier) { case 'userid': @@ -108,16 +152,11 @@ class phpunit_gradeimport_csv_load_data extends gradeimport_csv_load_data { case 'useremail': case 'username': return $this->studentid; - break; - case 'new': - return $this->newgrades; - break; case 'feedback': return $this->newfeedbacks; - break; + case 'new': default: return $this->newgrades; - break; } } } diff --git a/public/grade/import/csv/tests/load_data_test.php b/public/grade/import/csv/tests/load_data_test.php index dd5a85e20f9..0deddb83a06 100644 --- a/public/grade/import/csv/tests/load_data_test.php +++ b/public/grade/import/csv/tests/load_data_test.php @@ -382,11 +382,13 @@ Bobby,Bunce,,"Moodle HQ","Rock on!",student5@example.com,75.00,,75.00,{exportdat // We're not using scales so no to this option. $verbosescales = 0; + $linenumber = 2; // Map and key are to retrieve the grade_item that we are updating. $map = array(1); $key = 0; // We return the new grade array for saving. - $newgrades = $testobject->test_update_grade_item($this->courseid, $map, $key, $verbosescales, $testarray[0][6]); + $grade = $testarray[0][6]; + $newgrades = $testobject->test_update_grade_item($this->courseid, $map, $key, $verbosescales, $grade, $linenumber); $expectedresult = array(); $expectedresult[0] = new \stdClass(); @@ -396,10 +398,10 @@ Bobby,Bunce,,"Moodle HQ","Rock on!",student5@example.com,75.00,,75.00,{exportdat $this->assertEquals($newgrades, $expectedresult); // Try sending a bad grade value (A letter instead of a float / int). - $newgrades = $testobject->test_update_grade_item($this->courseid, $map, $key, $verbosescales, 'A'); + $newgrades = $testobject->test_update_grade_item($this->courseid, $map, $key, $verbosescales, 'A', $linenumber); // The $newgrades variable should be null. $this->assertNull($newgrades); - $expectederrormessage = get_string('badgrade', 'grades'); + $expectederrormessage = get_string('badgrade', 'gradeimport_csv', ['badgrade' => 'A', 'linenumber' => $linenumber]); // Check that the error message is what we expect. $gradebookerrors = $testobject->get_gradebookerrors(); $this->assertEquals($expectederrormessage, $gradebookerrors[0]); @@ -421,30 +423,67 @@ Bobby,Bunce,,"Moodle HQ","Rock on!",student5@example.com,75.00,,75.00,{exportdat // We're not using scales so no to this option. $verbosescales = 0; + $linenumber = 2; // Map and key are to retrieve the grade_item that we are updating. $map = array(1); $key = 0; // Test new user mapping. This should return the user id if there were no problems. - $userid = $testobject->test_map_user_data_with_value('useremail', $testarray[0][5], $this->columns, $map, $key, - $this->courseid, $map[$key], $verbosescales); + $userid = $testobject->test_map_user_data_with_value( + 'useremail', + $testarray[0][5], + $this->columns, + $map, + $key, + $this->courseid, + $map[$key], + $verbosescales, + $linenumber + ); $this->assertEquals($userid, $userdetail->id); - $newgrades = $testobject->test_map_user_data_with_value('new', $testarray[0][6], $this->columns, $map, $key, - $this->courseid, $map[$key], $verbosescales); + $newgrades = $testobject->test_map_user_data_with_value( + 'new', + $testarray[0][6], + $this->columns, + $map, + $key, + $this->courseid, + $map[$key], + $verbosescales, + $linenumber + ); // Check that the final grade is the same as the one inserted. $this->assertEquals($testarray[0][6], $newgrades[0]->finalgrade); - $newgrades = $testobject->test_map_user_data_with_value('new', $testarray[0][8], $this->columns, $map, $key, - $this->courseid, $map[$key], $verbosescales); + $newgrades = $testobject->test_map_user_data_with_value( + 'new', + $testarray[0][8], + $this->columns, + $map, + $key, + $this->courseid, + $map[$key], + $verbosescales, + $linenumber + ); // Check that the final grade is the same as the one inserted. // The testobject should now contain 2 new grade items. $this->assertEquals(2, count($newgrades)); // Because this grade item is empty, the value for final grade should be null. $this->assertNull($newgrades[1]->finalgrade); - $feedback = $testobject->test_map_user_data_with_value('feedback', $testarray[0][7], $this->columns, $map, $key, - $this->courseid, $map[$key], $verbosescales); + $feedback = $testobject->test_map_user_data_with_value( + 'feedback', + $testarray[0][7], + $this->columns, + $map, + $key, + $this->courseid, + $map[$key], + $verbosescales, + $linenumber + ); // Expected result. $resultarray = array(); $resultarray[0] = new \stdClass(); @@ -453,8 +492,17 @@ Bobby,Bunce,,"Moodle HQ","Rock on!",student5@example.com,75.00,,75.00,{exportdat $this->assertEquals($feedback, $resultarray); // Default behaviour (update a grade item). - $newgrades = $testobject->test_map_user_data_with_value('default', $testarray[0][6], $this->columns, $map, $key, - $this->courseid, $map[$key], $verbosescales); + $newgrades = $testobject->test_map_user_data_with_value( + 'default', + $testarray[0][6], + $this->columns, + $map, + $key, + $this->courseid, + $map[$key], + $verbosescales, + $linenumber + ); $this->assertEquals($testarray[0][6], $newgrades[0]->finalgrade); }