Moodle
  1. Moodle
  2. MDL-21305

Mastery Score: wrong condition in the main flow (status vs credit)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • 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:
      26825

      Description

      Mastery Score is currently linked to the "completed" status.
      While this should be a practice of good sense, it's an assumption over the way courses implement learning strategies quite opposite with the AICC (AICC - CMI Guidelines for interoperability) / SCORM (v1.2: The SCORM Run-Time Environment) specs, where the conditional branch is evaluated over the value of core.credit, being "credit".

      The current code will prevent the Mastery Score related behaviour to work as expected with those tracked activities where the status of a SCO is "incomplete" and everything inside a SCO is managed by a final test whose final status is managed only through the Mastery Score.

      mod/scorm/aicc.php:
      ...
      if ($mode == 'normal') {
      if ($lessonstatus == 'completed') {
      if ($sco = scorm_get_sco($scoid)) {
      if (!empty($sco->mastery_score)) {
      if (!empty($score)) {
      if ($score >= $sco->mastery_score)

      { $lessonstatus = 'passed'; }

      else

      { $lessonstatus = 'failed'; }

      }
      }
      $id = scorm_insert_track($USER->id, $scorm->id, $sco->id, $attempt, 'cmi.core.lesson_status', $lessonstatus);
      }
      }
      }
      ...

      "$lessonstatus == 'completed'" should be replaced with "true" since "normal" mode is just set when core.credit is "credit". It could be optionally sophisticated accessing the core.credit data.

        Activity

        Hide
        Dan Marsden added a comment -

        Hi Matteo - are you saying that this is an issue with SCORM 1.2 objects as well? - or just AICC objects?

        would replacing this line:
        if ($lessonstatus == 'completed') {
        with this line:
        if (isset($lessonstatus) && $lessonstatus) {

        do what you'd expect here for AICC?

        We haven't run Moodle against the AICC tests for a long time - is this something that you might be able to help with as well?

        thanks,

        Dan

        Show
        Dan Marsden added a comment - Hi Matteo - are you saying that this is an issue with SCORM 1.2 objects as well? - or just AICC objects? would replacing this line: if ($lessonstatus == 'completed') { with this line: if (isset($lessonstatus) && $lessonstatus) { do what you'd expect here for AICC? We haven't run Moodle against the AICC tests for a long time - is this something that you might be able to help with as well? thanks, Dan
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        we're currently testing the issue using something brutal as below:

        @@ -261,7 +261,7 @@
                                         $id = scorm_insert_track($USER->id, $scorm->id, $sco->id, $attempt, 'cmi.core.lesson_status', 'browsed');
                                     }
                                     if ($mode == 'normal') {
        -                                if ($lessonstatus == 'completed') {
        +                                if (true) { //($lessonstatus == 'completed') {
         									if ($sco = scorm_get_sco($scoid)) {
                                                 if (!empty($sco->mastery_score)) {
                                                     if (!empty($score)) {
        

        since it seems we cannot easily access at core.credit, there in the code .

        In other terms your fix proposal is correct since it looks at the availability of a status whatever it is, even if not required (i.e. the checks should be just on credit and score); the only possible issue is when the item doesn't set any status at all: the LMS is supposed to assign at least not attempted so the fix should be fine also in such situation, pending to verify if Moodle set the status before entering in the Mastery Score logic section.

        An alternative could be the one below (brutal + comment for the brutality ), just to avoid the conditional branch removal, also for nice diff/history viewing:

        -                                if ($lessonstatus == 'completed') {
        +                                if (true) { // It is supposed that checking mode equal to normal is the same as checking core.credit equal to credit
        
        Show
        Matteo Scaramuccia added a comment - Hi Dan, we're currently testing the issue using something brutal as below: @@ -261,7 +261,7 @@ $id = scorm_insert_track($USER->id, $scorm->id, $sco->id, $attempt, 'cmi.core.lesson_status', 'browsed'); } if ($mode == 'normal') { - if ($lessonstatus == 'completed') { + if ( true ) { //($lessonstatus == 'completed') { if ($sco = scorm_get_sco($scoid)) { if (!empty($sco->mastery_score)) { if (!empty($score)) { since it seems we cannot easily access at core.credit , there in the code . In other terms your fix proposal is correct since it looks at the availability of a status whatever it is, even if not required (i.e. the checks should be just on credit and score ); the only possible issue is when the item doesn't set any status at all: the LMS is supposed to assign at least not attempted so the fix should be fine also in such situation, pending to verify if Moodle set the status before entering in the Mastery Score logic section. An alternative could be the one below ( brutal + comment for the brutality ), just to avoid the conditional branch removal, also for nice diff/history viewing: - if ($lessonstatus == 'completed') { + if ( true ) { // It is supposed that checking mode equal to normal is the same as checking core.credit equal to credit
        Hide
        Dan Marsden added a comment -

        fix now in 19_STABLE and HEAD - thanks for the report and fix!

        Show
        Dan Marsden added a comment - fix now in 19_STABLE and HEAD - thanks for the report and fix!
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        the same logic must be applied on API based LOs, both SCORM 1.2, mod/scorm/datamodels/scorm_12.js.php, and AICC, mod/scorm/datamodels/aicc.js.php, see MDL-21306.
        We're running a modified version (almost according with the bugs opened by me) so it's difficult right now to give you just the differences but I can provided you a unified diff including other than the things related with this bug.

        Show
        Matteo Scaramuccia added a comment - Hi Dan, the same logic must be applied on API based LOs, both SCORM 1.2, mod/scorm/datamodels/scorm_12.js.php , and AICC, mod/scorm/datamodels/aicc.js.php , see MDL-21306 . We're running a modified version (almost according with the bugs opened by me) so it's difficult right now to give you just the differences but I can provided you a unified diff including other than the things related with this bug.
        Hide
        Dan Marsden added a comment -

        oops - good point!

        Show
        Dan Marsden added a comment - oops - good point!
        Hide
        Dan Marsden added a comment -

        Hi Matteo,

        can this one be closed now that the patch for MDL-21306 has been committed or is there something else I've missed here?

        thanks!

        Show
        Dan Marsden added a comment - Hi Matteo, can this one be closed now that the patch for MDL-21306 has been committed or is there something else I've missed here? thanks!
        Hide
        Dan Marsden added a comment -

        pretty sure this is finished - let me know if not and I'll reopen.

        Show
        Dan Marsden added a comment - pretty sure this is finished - let me know if not and I'll reopen.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: