From ddcfda87d6f3cc4dfec9947a15bee84b880a76a6 Mon Sep 17 00:00:00 2001 From: Charles Severance Date: Tue, 15 Nov 2011 12:46:48 -0500 Subject: [PATCH] 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. --- mod/lti/locallib.php | 34 ++++++++++++++++------------------ mod/lti/service.php | 23 ++++++++++++++++++----- mod/lti/servicelib.php | 12 +++++++++++- 3 files changed, 45 insertions(+), 24 deletions(-) diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php index a3fdecbd41a..03b257b0350 100644 --- 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 9099e105111..e78522d692d 100644 --- 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 0dde18cb760..969212ab481 100644 --- 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'); } } +