From 338fd396b7f1c2b925e6219ba790c76ff025b490 Mon Sep 17 00:00:00 2001
From: Jonathan Champ <jonathan_champ@ncsu.edu>
Date: Thu, 20 Mar 2014 11:04:41 -0400
Subject: [PATCH] MDL-29572 Grade Import: Avoid creating invalid temporary
 rows; Provide feedback on error

---
 grade/import/csv/index.php | 65 ++++++++++++++++++++------------------
 grade/import/lib.php       | 27 ++++++++++++----
 lang/en/grades.php         |  5 +++
 3 files changed, 59 insertions(+), 38 deletions(-)

diff --git a/grade/import/csv/index.php b/grade/import/csv/index.php
index 3a931a2de30..72fc851c457 100644
--- a/grade/import/csv/index.php
+++ b/grade/import/csv/index.php
@@ -187,6 +187,25 @@ if ($formdata = $mform2->get_data()) {
     @set_time_limit(0);
     raise_memory_limit(MEMORY_EXTRA);
 
+    $userfields = 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',
+        ),
+    );
+
     $csvimport->init();
 
     $newgradeitems = array(); // temporary array to keep track of what new headers are processed
@@ -225,39 +244,23 @@ if ($formdata = $mform2->get_data()) {
             }
 
             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 (empty($value) || !$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 (empty($value)) {
+                        continue 3;
+                    }
+
+                    if (!$user = $DB->get_record('user', array($userfields[$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 = $userfields[$t0]['label'];
+                        $usermappingerrorobj->value = $value;
+                        echo $OUTPUT->notification(get_string('usermappingerror', 'grades', $usermappingerrorobj));
+                        unset($usermappingerrorobj);
                         $status = false;
                         break 3;
                     }
@@ -383,7 +386,7 @@ if ($formdata = $mform2->get_data()) {
             // 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;
         }
 
@@ -391,7 +394,7 @@ if ($formdata = $mform2->get_data()) {
             // 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;
         }
 
diff --git a/grade/import/lib.php b/grade/import/lib.php
index 5a61de4c996..f8d8e5777cb 100644
--- a/grade/import/lib.php
+++ b/grade/import/lib.php
@@ -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,11 @@ 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))) {
+            $gradeimportparams = array('newgradeitem' => $newitem->id, 'importcode' => $importcode, 'importer' => $USER->id);
+            if ($grades = $DB->get_records('grade_import_values', $gradeimportparams)) {
                 /// 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,7 +106,8 @@ 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))) {
+            $gradeimportparams = array('itemid' => $itemid, 'importcode' => $importcode, 'importer' => $USER->id);
+            if ($grades = $DB->get_records('grade_import_values', $gradeimportparams)) {
 
                 // make the grades array for update_grade
                 foreach ($grades as $grade) {
@@ -112,7 +115,11 @@ function grade_import_commit($courseid, $importcode, $importfeedback=true, $verb
                         $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;
                     }
                 }
@@ -120,11 +127,17 @@ function grade_import_commit($courseid, $importcode, $importfeedback=true, $verb
                 $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;
         }
     }
 
diff --git a/lang/en/grades.php b/lang/en/grades.php
index 005f649a247..1c01301486f 100644
--- a/lang/en/grades.php
+++ b/lang/en/grades.php
@@ -180,6 +180,7 @@ $string['errornocategorisedid'] = 'Could not get an uncategorised id!';
 $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}';
@@ -253,6 +254,7 @@ $string['gradehelp'] = 'Grade help';
 $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';
@@ -661,6 +663,9 @@ $string['usepercent'] = 'Use percent';
 $string['user'] = 'User';
 $string['usergrade'] = 'User {$a->fullname} ({$a->useridnumber}) on item {$a->gradeidnumber}';
 $string['userid'] = 'User ID';
+$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';