Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26672

A value equal to 0 is meaningful within the context of the AICC HACP Mastery Score business logic

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.11, 2.0.2
    • Fix Version/s: 2.0.3
    • Component/s: SCORM
    • Labels:
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      On making a deep analysis on a significant (for us ) number of records about an unexpected SCO status found in the mdl_scorm_scoes_track of a medium-sized Moodle installation with plenty of AICC HACP based packages, we found a flaw in the business logic related with the evaluation of the status by the Mastery Score mechanism.

      Shortly, the failure is due to a bad, in this case, usage of PHP empty() which excludes 0 from being a valid value.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            matteo Matteo Scaramuccia added a comment -

            Find below the patch we're thinking to test. Basically:

            • removed the emtpy() calls using a different sanity check strategy trying the usage of is_numeric(), already used in checking a score;
            • improved the Mastery_Score initilization in replying to a GetParam command.

            Index: aicc.php
            ===================================================================
            RCS file: /cvsroot/moodle/moodle/mod/scorm/aicc.php,v
            retrieving revision 1.21.2.6
            diff -u -r1.21.2.6 aicc.php
            --- aicc.php	6 May 2010 00:13:54 -0000	1.21.2.6
            +++ aicc.php	3 Mar 2011 14:14:42 -0000
            @@ -77,7 +77,7 @@
                                     if ($sco = scorm_get_sco($scoid)) {
                                         $userdata->course_id = $sco->identifier;
                                         $userdata->datafromlms = isset($sco->datafromlms)?$sco->datafromlms:'';
            -							$userdata->mastery_score = isset($sco->mastery_score)?$sco->mastery_score:'';
            +                            $userdata->mastery_score = isset($sco->mastery_score) && is_numeric($sco->mastery_score)?trim($sco->mastery_score):'';
                                         $userdata->max_time_allowed = isset($sco->max_time_allowed)?$sco->max_time_allowed:'';
                                         $userdata->time_limit_action = isset($sco->time_limit_action)?$sco->time_limit_action:'';
                                         echo "error=0\r\nerror_text=Successful\r\naicc_data=";
            @@ -260,11 +260,10 @@
                                             $id = scorm_insert_track($USER->id, $scorm->id, $sco->id, $attempt, 'cmi.core.lesson_status', 'browsed');
                                         }
                                         if ($mode == 'normal') {
            -
                                             if ($sco = scorm_get_sco($scoid)) {
            -                                    if (!empty($sco->mastery_score)) {
            -                                        if (!empty($score)) {
            -                                            if ($score >= $sco->mastery_score) {
            +                                    if (isset($sco->mastery_score) && is_numeric($sco->mastery_score)) {
            +                                        if ($score != '') { // $score is correctly initialized w/ an empty string, see above
            +                                            if ($score >= trim($sco->mastery_score)) {
                                                             $lessonstatus = 'passed';
                                                         } else {
                                                             $lessonstatus = 'failed';

            Note: is_numeric() is effective only in conjunction with trim(), see below.

            <?php
            header('Content-Type: text/plain');
             
            $values = array(
                ' 75',
                '75 ',
                '75',
                'aaa',
                'Llll',
                '111',
                '111L',
                ''
            );
             
            echo 'PHP v'.phpversion()."\n";
            $i = 0;
            foreach ($values as $value) {
                $i++;
                echo "#$i,\t'$value'\t=> ".(is_numeric($value)?'is_num':'not_is_num')."\n";
            }

            which outputs:

            PHP v5.2.14
            #1,	' 75'	=> is_num	<===== Here is where trim() is required
            #2,	'75 '	=> not_is_num
            #3,	'75'	=> is_num
            #4,	'aaa'	=> not_is_num
            #5,	'Llll'	=> not_is_num
            #6,	'111'	=> is_num
            #7,	'111L'	=> not_is_num
            #8,	''	=> not_is_num

            Show
            matteo Matteo Scaramuccia added a comment - Find below the patch we're thinking to test. Basically: removed the emtpy() calls using a different sanity check strategy trying the usage of is_numeric() , already used in checking a score; improved the Mastery_Score initilization in replying to a GetParam command. Index: aicc.php =================================================================== RCS file: /cvsroot/moodle/moodle/mod/scorm/aicc.php,v retrieving revision 1.21.2.6 diff -u -r1.21.2.6 aicc.php --- aicc.php 6 May 2010 00:13:54 -0000 1.21.2.6 +++ aicc.php 3 Mar 2011 14:14:42 -0000 @@ -77,7 +77,7 @@ if ($sco = scorm_get_sco($scoid)) { $userdata->course_id = $sco->identifier; $userdata->datafromlms = isset($sco->datafromlms)?$sco->datafromlms:''; - $userdata->mastery_score = isset($sco->mastery_score)?$sco->mastery_score:''; + $userdata->mastery_score = isset($sco->mastery_score) && is_numeric($sco->mastery_score)?trim($sco->mastery_score):''; $userdata->max_time_allowed = isset($sco->max_time_allowed)?$sco->max_time_allowed:''; $userdata->time_limit_action = isset($sco->time_limit_action)?$sco->time_limit_action:''; echo "error=0\r\nerror_text=Successful\r\naicc_data="; @@ -260,11 +260,10 @@ $id = scorm_insert_track($USER->id, $scorm->id, $sco->id, $attempt, 'cmi.core.lesson_status', 'browsed'); } if ($mode == 'normal') { - if ($sco = scorm_get_sco($scoid)) { - if (!empty($sco->mastery_score)) { - if (!empty($score)) { - if ($score >= $sco->mastery_score) { + if (isset($sco->mastery_score) && is_numeric($sco->mastery_score)) { + if ($score != '') { // $score is correctly initialized w/ an empty string, see above + if ($score >= trim($sco->mastery_score)) { $lessonstatus = 'passed'; } else { $lessonstatus = 'failed'; Note: is_numeric() is effective only in conjunction with trim() , see below. <?php header('Content-Type: text/plain');   $values = array( ' 75', '75 ', '75', 'aaa', 'Llll', '111', '111L', '' );   echo 'PHP v'.phpversion()."\n"; $i = 0; foreach ($values as $value) { $i++; echo "#$i,\t'$value'\t=> ".(is_numeric($value)?'is_num':'not_is_num')."\n"; } which outputs: PHP v5.2.14 #1, ' 75' => is_num <===== Here is where trim() is required #2, '75 ' => not_is_num #3, '75' => is_num #4, 'aaa' => not_is_num #5, 'Llll' => not_is_num #6, '111' => is_num #7, '111L' => not_is_num #8, '' => not_is_num
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Matteo - all makes sense to me. Will try to create a pull request early next week for this.

            Show
            danmarsden Dan Marsden added a comment - thanks Matteo - all makes sense to me. Will try to create a pull request early next week for this.
            Hide
            tsala Helen Foster added a comment -

            Thanks Matteo and Dan.

            Adding a fix version of stable backlog as part of our triaging process.

            Show
            tsala Helen Foster added a comment - Thanks Matteo and Dan. Adding a fix version of stable backlog as part of our triaging process.
            Hide
            danmarsden Dan Marsden added a comment -

            pull request has been submitted - thanks Matteo!

            Show
            danmarsden Dan Marsden added a comment - pull request has been submitted - thanks Matteo!
            Hide
            matteo Matteo Scaramuccia added a comment -

            I'm glad I could help

            Show
            matteo Matteo Scaramuccia added a comment - I'm glad I could help
            Hide
            tsala Helen Foster added a comment -

            This issue is fixed in the latest 2.0.2+. Thanks everyone.

            Show
            tsala Helen Foster added a comment - This issue is fixed in the latest 2.0.2+. Thanks everyone.

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/May/11