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 Bug
    • Status: Closed
    • Priority: Minor 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

          Activity

          Hide
          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 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
          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
          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
          Helen Foster added a comment -

          Thanks Matteo and Dan.

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

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

          pull request has been submitted - thanks Matteo!

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

          I'm glad I could help

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

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

          Show
          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: