Moodle
  1. Moodle
  2. MDL-21306

Redundant/bugged code on aicc.js and scorm_12.js?

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.7
    • Fix Version/s: 1.9.10
    • Component/s: SCORM
    • Labels:
      None
    • Environment:
      LAMP, PHP 5.2.11
    • Database:
      MySQL
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      26821

      Description

      While attempting to analize MDL-21305 issue we've found issues in the way the code around Mastery Score management has been duplicated client side (see at the end of this description):
      1. [OK] credit is correclty involved in the conditional branching
      2. [OK] "not attempted" => "completed" only in SCORM 1.2: the SCORM specs clearly write about this behaviour. It would be advisable to do the same with AICC, since SCORM has been derived from AICC using both Data Model and API but AICC spec doesnt' mention anything about this extra behaviour
      3. [KO] scorm_12.js adds the extra/wrong conditional branch using the status value, with no check about the availability of score
      4. [KO] aicc.js.php always set the status to "completed", regardless the intial status

      Fix proposal:
      a. alignment of the code on both files (maybe also scorm_13.js.php);
      b. move the Mastery Score management server side where there is already a bugged code (MDL-21305), keeping here just the status management in case of asset (not attempted)
      c. remove everything and let the server side code do such things.

      mod/scorm/datamodel/aicc.js.php:
      ...
      function StoreData(data,storetotaltime) {
      if (storetotaltime) {
      if (cmi.core.lesson_mode == 'normal') {
      if (cmi.core.credit == 'credit') {
      cmi.core.lesson_status = 'completed';
      if (cmi.student_data.mastery_score != '') {
      if (cmi.core.score.raw >= cmi.student_data.mastery_score)

      { cmi.core.lesson_status = 'passed'; }

      else

      { cmi.core.lesson_status = 'failed'; }

      }
      }
      }
      ...

      mod/scorm/datamodels/scorm_12.js.php:
      ...
      function StoreData(data,storetotaltime) {
      if (storetotaltime) {
      if (cmi.core.lesson_status == 'not attempted')

      { cmi.core.lesson_status = 'completed'; }

      if (cmi.core.lesson_mode == 'normal') {
      if (cmi.core.credit == 'credit') {
      if (cmi.core.lesson_status == 'completed') {
      if (cmi.student_data.mastery_score != '' && cmi.core.score.raw != '') {
      if (parseFloat(cmi.core.score.raw) >= parseFloat(cmi.student_data.mastery_score))

      { cmi.core.lesson_status = 'passed'; }

      else

      { cmi.core.lesson_status = 'failed'; }

      }
      }
      }
      }
      ...

        Activity

        Hide
        Dan Marsden added a comment -

        Hi Matteo,

        thanks for the bug reports - you've obviously spent a lot of time tracking these down!

        I'll try to have a closer look later this week! - In future it would be really helpful if you created patch files for your suggested changes - it makes it a lot easier to see what is proposed and to apply the changes.
        see here for more details on creating a patch:
        http://docs.moodle.org/en/Development:How_to_create_a_patch

        thanks!

        Show
        Dan Marsden added a comment - Hi Matteo, thanks for the bug reports - you've obviously spent a lot of time tracking these down! I'll try to have a closer look later this week! - In future it would be really helpful if you created patch files for your suggested changes - it makes it a lot easier to see what is proposed and to apply the changes. see here for more details on creating a patch: http://docs.moodle.org/en/Development:How_to_create_a_patch thanks!
        Hide
        Matteo Scaramuccia added a comment -

        OK, I'll do.

        BTW, for the scope of this bug I've done an error proposing (b) or (c) as a possible solution (i.e. transferring any Mastery Score logic server side) since I misunderstood "a bit" the different way of managing AICC HACP versus AICC/SCORM API based tracking in the Moodle code architecture. Sorry for that.

        Show
        Matteo Scaramuccia added a comment - OK, I'll do. BTW, for the scope of this bug I've done an error proposing (b) or (c) as a possible solution (i.e. transferring any Mastery Score logic server side) since I misunderstood "a bit" the different way of managing AICC HACP versus AICC/SCORM API based tracking in the Moodle code architecture. Sorry for that.
        Hide
        Matteo Scaramuccia added a comment -

        Find below the patch proposal:

        datamodels/aicc.js.php, according with §2.1.5 and §2.1.6 from spec (cmi001v4.pdf):

        @@ -497,8 +497,7 @@
                 if (storetotaltime) {
                     if (cmi.core.lesson_mode == 'normal') {
                         if (cmi.core.credit == 'credit') {
        -                    cmi.core.lesson_status = 'completed';
        -                    if (cmi.student_data.mastery_score != '') {
        +                    if (cmi.student_data.mastery_score != '' && cmi.core.score.raw != '') {
                                 if (cmi.core.score.raw >= cmi.student_data.mastery_score) {
                                     cmi.core.lesson_status = 'passed';
                                 } else {
        @@ -508,7 +507,7 @@
                         }
                     }
                     if (cmi.core.lesson_mode == 'browse') {
        -                if (datamodel['cmi.core.lesson_status'].defaultvalue == '') {
        +                if (datamodel['cmi.core.lesson_status'].defaultvalue == '' && cmi.core.lesson_status == 'not attempted') {
                             cmi.core.lesson_status = 'browsed';
                         }
                     }
        

        datamodels/scorm_12.js.php, according with ppgg 3-24, 3-25 from spec (SCORM_1.2_RunTimeEnv.pdf):

        @@ -561,13 +568,11 @@
                     }
                     if (cmi.core.lesson_mode == 'normal') {
                         if (cmi.core.credit == 'credit') {
        -                    if (cmi.core.lesson_status == 'completed') {
        -                        if (cmi.student_data.mastery_score != '' && cmi.core.score.raw != '') {
        -                            if (parseFloat(cmi.core.score.raw) >= parseFloat(cmi.student_data.mastery_score)) {
        -                                cmi.core.lesson_status = 'passed';
        -                            } else {
        -                                cmi.core.lesson_status = 'failed';
        -                            }
        +                    if (cmi.student_data.mastery_score != '' && cmi.core.score.raw != '') {
        +                        if (parseFloat(cmi.core.score.raw) >= parseFloat(cmi.student_data.mastery_score)) {
        +                            cmi.core.lesson_status = 'passed';
        +                        } else {
        +                            cmi.core.lesson_status = 'failed';
                                 }
                             }
                         }
        
        Show
        Matteo Scaramuccia added a comment - Find below the patch proposal: datamodels/aicc.js.php , according with §2.1.5 and §2.1.6 from spec ( cmi001v4.pdf ): @@ -497,8 +497,7 @@ if (storetotaltime) { if (cmi.core.lesson_mode == 'normal') { if (cmi.core.credit == 'credit') { - cmi.core.lesson_status = 'completed'; - if (cmi.student_data.mastery_score != '') { + if (cmi.student_data.mastery_score != '' && cmi.core.score.raw != '') { if (cmi.core.score.raw >= cmi.student_data.mastery_score) { cmi.core.lesson_status = 'passed'; } else { @@ -508,7 +507,7 @@ } } if (cmi.core.lesson_mode == 'browse') { - if (datamodel['cmi.core.lesson_status'].defaultvalue == '') { + if (datamodel['cmi.core.lesson_status'].defaultvalue == '' && cmi.core.lesson_status == 'not attempted') { cmi.core.lesson_status = 'browsed'; } } datamodels/scorm_12.js.php , according with ppgg 3-24, 3-25 from spec ( SCORM_1.2_RunTimeEnv.pdf ): @@ -561,13 +568,11 @@ } if (cmi.core.lesson_mode == 'normal') { if (cmi.core.credit == 'credit') { - if (cmi.core.lesson_status == 'completed') { - if (cmi.student_data.mastery_score != '' && cmi.core.score.raw != '') { - if (parseFloat(cmi.core.score.raw) >= parseFloat(cmi.student_data.mastery_score)) { - cmi.core.lesson_status = 'passed'; - } else { - cmi.core.lesson_status = 'failed'; - } + if (cmi.student_data.mastery_score != '' && cmi.core.score.raw != '') { + if (parseFloat(cmi.core.score.raw) >= parseFloat(cmi.student_data.mastery_score)) { + cmi.core.lesson_status = 'passed'; + } else { + cmi.core.lesson_status = 'failed'; } } }
        Hide
        Dan Marsden added a comment -

        Thanks Matteo, fix now in 1.9Stable and HEAD - can you please QA the commit to make sure I haven't missed anything?

        thanks!

        Show
        Dan Marsden added a comment - Thanks Matteo, fix now in 1.9Stable and HEAD - can you please QA the commit to make sure I haven't missed anything? thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: