Permalink
Browse files

MDL-29572 Don't Create Millions of Invalid Temporary Rows when import…

…ing grades via CSV; Provide Feedback on error.
  • Loading branch information...
1 parent c4a12af commit 90563fb6e14f707a438486c6745fa6d8bff1aa53 Glenn Ansley committed Feb 8, 2012
Showing with 57 additions and 38 deletions.
  1. +34 −31 grade/import/csv/index.php
  2. +18 −7 grade/import/lib.php
  3. +5 −0 lang/en/grades.php
@@ -216,6 +216,25 @@
@set_time_limit(0);
raise_memory_limit(MEMORY_EXTRA);
+ $user_fields = array(
+ 'userid' => array(
+ 'field' => 'id',
+ 'label' => 'id',
+ ),
+ 'useridnumber' => array(
+ 'field' => 'idnumber',
+ 'label' => 'idnumber',
+ ),
+ 'useremail' => array(
+ 'field' => 'email',
+ 'label' => 'email address',
+ ),
+ 'username' => array(
+ 'field' => 'username',
+ 'label' => 'username',
+ ),
+ );
+
// we only operate if file is readable
if ($fp = fopen($filename, "r")) {
@@ -264,39 +283,23 @@
}
switch ($t0) {
- case 'userid': //
- if (!$user = $DB->get_record('user', array('id' => $value))) {
- // user not found, abort whole import
- import_cleanup($importcode);
- echo $OUTPUT->notification("user mapping error, could not find user with id \"$value\"");
- $status = false;
- break 3;
- }
- $studentid = $value;
- break;
+ case 'userid':
case 'useridnumber':
- if (!$user = $DB->get_record('user', array('idnumber' => $value))) {
- // user not found, abort whole import
- import_cleanup($importcode);
- echo $OUTPUT->notification("user mapping error, could not find user with idnumber \"$value\"");
- $status = false;
- break 3;
- }
- $studentid = $user->id;
- break;
case 'useremail':
- if (!$user = $DB->get_record('user', array('email' => $value))) {
- import_cleanup($importcode);
- echo $OUTPUT->notification("user mapping error, could not find user with email address \"$value\"");
- $status = false;
- break 3;
- }
- $studentid = $user->id;
- break;
case 'username':
- if (!$user = $DB->get_record('user', array('username' => $value))) {
+ // Skip invalid row with blank user field
+ if ($value === '') {
+ continue 3;
+ }
+
+ if (!$user = $DB->get_record('user', array($user_fields[$t0]['field'] => $value))) {
+ // user not found, abort whole import
import_cleanup($importcode);
- echo $OUTPUT->notification("user mapping error, could not find user with username \"$value\"");
+ $usermappingerrorobj = new stdClass();
+ $usermappingerrorobj->field = $user_fields[$t0]['label'];
+ $usermappingerrorobj->value = $value;
+ echo $OUTPUT->notification(get_string('usermappingerror', 'grades', $usermappingerrorobj));
+ unset($usermappingerrorobj);
$status = false;
break 3;
}
@@ -417,15 +420,15 @@
// user not found, abort whole import
$status = false;
import_cleanup($importcode);
- echo $OUTPUT->notification('user mapping error, could not find user!');
+ echo $OUTPUT->notification(get_string('usermappingerrorusernotfound', 'grades'));
break;
}
if ($separatemode and !groups_is_member($currentgroup, $studentid)) {
// not allowed to import into this group, abort
$status = false;
import_cleanup($importcode);
- echo $OUTPUT->notification('user not member of current group, can not update!');
+ echo $OUTPUT->notification(get_string('usermappingerrorcurrentgroup', 'grades'));
break;
}
View
@@ -45,6 +45,8 @@ function get_new_importcode() {
function grade_import_commit($courseid, $importcode, $importfeedback=true, $verbose=true) {
global $CFG, $USER, $DB, $OUTPUT;
+ $failed = false;
+ $executionerrors = false;
$commitstart = time(); // start time in case we need to roll back
$newitemids = array(); // array to hold new grade_item ids from grade_import_newitem table, mapping array
@@ -57,11 +59,10 @@ function grade_import_commit($courseid, $importcode, $importfeedback=true, $verb
// instances of the new grade_items created, cached
// in case grade_update fails, so that we can remove them
$instances = array();
- $failed = false;
foreach ($newitems as $newitem) {
// get all grades with this item
- if ($grades = $DB->get_records('grade_import_values', array('newgradeitem' => $newitem->id))) {
+ if ($grades = $DB->get_records('grade_import_values', array('newgradeitem' => $newitem->id, 'importcode' => $importcode, 'importer' => $USER->id))) {
/// create a new grade item for this - must use false as second param!
/// TODO: we need some bounds here too
$gradeitem = new grade_item(array('courseid'=>$courseid, 'itemtype'=>'manual', 'itemname'=>$newitem->itemname), false);
@@ -104,27 +105,37 @@ function grade_import_commit($courseid, $importcode, $importfeedback=true, $verb
return false;
}
// get all grades with this item
- if ($grades = $DB->get_records('grade_import_values', array('itemid' => $itemid))) {
+ if ($grades = $DB->get_records('grade_import_values', array('itemid' => $itemid, 'importcode' => $importcode, 'importer' => $USER->id))) {
// make the grades array for update_grade
foreach ($grades as $grade) {
if (!$importfeedback) {
$grade->feedback = false; // ignore it
}
if (!$gradeitem->update_final_grade($grade->userid, $grade->finalgrade, 'import', $grade->feedback)) {
- $failed = 1;
+ $errordata = new stdClass();
+ $errordata->itemname = $gradeitem->itemname;
+ $errordata->userid = $grade->userid;
+ $executionerrors[] = get_string('errorsettinggrade', 'grades', $errordata);
+ $failed = true;
break 2;
}
}
//$itemdetails -> idnumber = $gradeitem->idnumber;
$modifieditems[] = $itemid;
}
+ }
- if (!empty($failed)) {
- import_cleanup($importcode);
- return false;
+ if ($failed) {
+ if ($executionerrors && $verbose) {
+ echo $OUTPUT->notification(get_string('gradeimportfailed', 'grades'));
+ foreach ($executionerrors as $errorstr) {
+ echo $OUTPUT->notification($errorstr);
+ }
}
+ import_cleanup($importcode);
+ return false;
}
}
View
@@ -177,6 +177,7 @@
$string['errornocourse'] = 'Could not get course information';
$string['errorreprintheadersnonnumeric'] = 'Received non-numeric value for reprint-headers';
$string['errorsavegrade'] = 'Could not save grade, sorry.';
+$string['errorsettinggrade'] = 'Error saving "{$a->itemname}" grade for userid {$a->userid}';
$string['errorupdatinggradecategoryaggregateonlygraded'] = 'Error updating the "Aggregate only non-empty grades" setting of grade category ID {$a->id}';
$string['errorupdatinggradecategoryaggregateoutcomes'] = 'Error updating the "Include outcomes in aggregation" setting of grade category ID {$a->id}';
$string['errorupdatinggradecategoryaggregatesubcats'] = 'Error updating the "Aggregate including subcategories" setting of grade category ID {$a->id}';
@@ -241,6 +242,7 @@
$string['gradehistorylifetime'] = 'Grade history lifetime';
$string['gradehistorylifetime_help'] = 'This specifies the length of time you want to keep history of changes in grade related tables. It is recommended to keep it as long as possible. If you experience performance problems or have limited database space, try to set lower value.';
$string['gradeimport'] = 'Grade import';
+$string['gradeimportfailed'] = 'Grade Import failed during commit. Details:';
$string['gradeitem'] = 'Grade item';
$string['gradeitemaddusers'] = 'Exclude from grading';
$string['gradeitemadvanced'] = 'Advanced grade item options';
@@ -638,6 +640,9 @@
$string['usepercent'] = 'Use percent';
$string['user'] = 'User';
$string['usergrade'] = 'User {$a->fullname} ({$a->useridnumber}) on item {$a->gradeidnumber}';
+$string['usermappingerror'] = 'User mapping error: Could not find user with {$a->field} of "{$a->value}".';
+$string['usermappingerrorusernotfound'] = 'User mapping error. Could not find user.';
+$string['usermappingerrorcurrentgroup'] = 'User is not a member of current group.';
$string['userpreferences'] = 'User preferences';
$string['userenrolmentsuspended'] = 'User enrolment suspended';
$string['useweighted'] = 'Use weighted';

0 comments on commit 90563fb

Please sign in to comment.