MDL-20534

I ran the software through the certification and caught a few nits:

The error return is 'failure' not 'error'
The spec says that it needs to return 'failure' for out of range or non-numeric grades
The result score needs a language tag, hard-coded as 'en'
Setting a grade multiplied by 100 but reading the grade did not divide by 100
All those are now fixed with this patch as well as this bit of cruft:

I removed the "extension service url" as it is not implemented in service.php

Feel free to review and adjust - probably the one place you might want to refactor
is that I put code to catch out-of-range-and non-numeric in
lti_parse_grade_replace_message and threw an exception on error and then caught
it in service.php and sent back the 'failure' message. Feel free to refactor a
bit if you see this done in a cleaner manner.

diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php
index a3fdecb..03b257b 100644 (file)
--- a/mod/lti/locallib.php
+++ b/mod/lti/locallib.php
@@ -253,14 +253,18 @@ function lti_build_request($instance, $typeconfig, $course) {
     if ( isset($placementsecret) &&
          ( $typeconfig['acceptgrades'] == LTI_SETTING_ALWAYS ||
          ( $typeconfig['acceptgrades'] == LTI_SETTING_DELEGATE && $instance->instructorchoiceacceptgrades == LTI_SETTING_ALWAYS ) ) ) {
-        $requestparams['lis_result_sourcedid'] = $sourcedid;
+        $requestparams["lis_result_sourcedid"] = $sourcedid;
 
-        $serviceurl = $CFG->wwwroot . '/mod/lti/service.php';
+        /* $serviceurl = $CFG->wwwroot . '/mod/lti/service.php';
         if ($typeconfig['forcessl'] == '1') {
             $serviceurl = lti_ensure_url_is_https($serviceurl);
         }
 
-        $requestparams['ext_ims_lis_basic_outcome_url'] = $serviceurl;
+        $requestparams["ext_ims_lis_basic_outcome_url"] = $serviceurl; */
+
+        //Add outcome service URL
+        $url = new moodle_url('/mod/lti/service.php');
+        $requestparams['lis_outcome_service_url'] = $url->out();
     }
 
     /*if ( isset($placementsecret) &&
@@ -273,20 +277,16 @@ function lti_build_request($instance, $typeconfig, $course) {
     // Send user's name and email data if appropriate
     if ( $typeconfig['sendname'] == LTI_SETTING_ALWAYS ||
          ( $typeconfig['sendname'] == LTI_SETTING_DELEGATE && $instance->instructorchoicesendname == LTI_SETTING_ALWAYS ) ) {
-        $requestparams['lis_person_name_given'] =  $USER->firstname;
-        $requestparams['lis_person_name_family'] =  $USER->lastname;
-        $requestparams['lis_person_name_full'] =  $USER->firstname." ".$USER->lastname;
+        $requestparams["lis_person_name_given"] =  $USER->firstname;
+        $requestparams["lis_person_name_family"] =  $USER->lastname;
+        $requestparams["lis_person_name_full"] =  $USER->firstname." ".$USER->lastname;
     }
 
     if ( $typeconfig['sendemailaddr'] == LTI_SETTING_ALWAYS ||
          ( $typeconfig['sendemailaddr'] == LTI_SETTING_DELEGATE && $instance->instructorchoicesendemailaddr == LTI_SETTING_ALWAYS ) ) {
-        $requestparams['lis_person_contact_email_primary'] = $USER->email;
+        $requestparams["lis_person_contact_email_primary"] = $USER->email;
     }
 
-    //Add outcome service URL
-    $url = new moodle_url('/mod/lti/service.php');
-    $requestparams['lis_outcome_service_url'] = $url->out();
-
     // Concatenate the custom parameters from the administrator and the instructor
     // Instructor parameters are only taken into consideration if the administrator
     // has giver permission
@@ -313,20 +313,18 @@ function lti_build_request($instance, $typeconfig, $course) {
     }
 
     // Make sure we let the tool know what LMS they are being called from
-    $requestparams['ext_lms'] = 'moodle-2';
-    $requestparams['tool_consumer_info_product_family_code'] = 'moodle';
-    $requestparams['tool_consumer_info_version'] = strval($CFG->version);
+    $requestparams["ext_lms"] = "moodle-2";
 
     // Add oauth_callback to be compliant with the 1.0A spec
-    $requestparams['oauth_callback'] = 'about:blank';
+    $requestparams["oauth_callback"] = "about:blank";
 
     //The submit button needs to be part of the signature as it gets posted with the form.
     //This needs to be here to support launching without javascript.
     $submittext = get_string('press_to_submit', 'lti');
-    $requestparams['ext_submit'] = $submittext;
+    $requestparams["ext_submit"] = $submittext;
 
-    $requestparams['lti_version'] = 'LTI-1p0';
-    $requestparams['lti_message_type'] = 'basic-lti-launch-request';
+    $requestparams["lti_version"] = "LTI-1p0";
+    $requestparams["lti_message_type"] = "basic-lti-launch-request";
     /* Suppress this for now - Chuck
     if ( $orgdesc ) $requestparams["tool_consumer_instance_description"] = $orgdesc;
     */

diff --git a/mod/lti/service.php b/mod/lti/service.php
index 9099e10..e78522d 100644 (file)
--- a/mod/lti/service.php
+++ b/mod/lti/service.php
@@ -61,7 +61,18 @@ foreach ($body->children() as $child) {
 
 switch ($messagetype) {
     case 'replaceResultRequest':
-        $parsed = lti_parse_grade_replace_message($xml);
+        try {
+            $parsed = lti_parse_grade_replace_message($xml);
+        } catch (Exception $e) {
+            $responsexml = lti_get_response_xml(
+                'failure',
+                $e->getMessage(),
+                uniqid(),
+                'replaceResultResponse');
+
+            echo $responsexml->asXML();
+            break;
+        }
 
         $ltiinstance = $DB->get_record('lti', array('id' => $parsed->instanceid));
 
@@ -70,7 +81,7 @@ switch ($messagetype) {
         $gradestatus = lti_update_grade($ltiinstance, $parsed->userid, $parsed->launchid, $parsed->gradeval);
 
         $responsexml = lti_get_response_xml(
-                $gradestatus ? 'success' : 'error',
+                $gradestatus ? 'success' : 'failure',
                 'Grade replace response',
                 $parsed->messageid,
                 'replaceResultResponse'
@@ -94,14 +105,16 @@ switch ($messagetype) {
         $grade = lti_read_grade($ltiinstance, $parsed->userid);
 
         $responsexml = lti_get_response_xml(
-                isset($grade) ? 'success' : 'error',
+                isset($grade) ? 'success' : 'failure',
                 'Result read',
                 $parsed->messageid,
                 'readResultResponse'
         );
 
         $node = $responsexml->imsx_POXBody->readResultResponse;
-        $node->addChild('result')->addChild('resultScore')->addChild('textString', isset($grade) ? $grade : '');
+        $node = $node->addChild('result')->addChild('resultScore');
+        $node->addChild('language', 'en');
+        $node->addChild('textString', isset($grade) ? $grade : '');
 
         echo $responsexml->asXML();
 
@@ -117,7 +130,7 @@ switch ($messagetype) {
         $gradestatus = lti_delete_grade($ltiinstance, $parsed->userid);
 
         $responsexml = lti_get_response_xml(
-                $gradestatus ? 'success' : 'error',
+                $gradestatus ? 'success' : 'failure',
                 'Grade delete request',
                 $parsed->messageid,
                 'deleteResultResponse'

diff --git a/mod/lti/servicelib.php b/mod/lti/servicelib.php
index 0dde18c..969212a 100644 (file)
--- a/mod/lti/servicelib.php
+++ b/mod/lti/servicelib.php
@@ -66,7 +66,15 @@ function lti_parse_grade_replace_message($xml) {
     $resultjson = json_decode((string)$node);
 
     $node = $xml->imsx_POXBody->replaceResultRequest->resultRecord->result->resultScore->textString;
-    $grade = floatval((string)$node);
+
+    $score = (string) $node;
+    if ( ! is_numeric($score) ) {
+        throw new Exception('Score must be numeric');
+    }
+    $grade = floatval($score);
+    if ( $grade < 0.0 || $grade > 1.0 ) {
+        throw new Exception('Score not between 0.0 and 1.0');
+    }
 
     $parsed = new stdClass();
     $parsed->gradeval = $grade * 100;
@@ -163,6 +171,7 @@ function lti_read_grade($ltiinstance, $userid) {
     if (isset($grades) && isset($grades->items[0]) && is_array($grades->items[0]->grades)) {
         foreach ($grades->items[0]->grades as $agrade) {
             $grade = $agrade->grade;
+            $grade = $grade / 100.0;
             break;
         }
     }
@@ -211,3 +220,4 @@ function lti_verify_sourcedid($ltiinstance, $parsed) {
         throw new Exception('SourcedId hash not valid');
     }
 }
+