Moodle
  1. Moodle
  2. MDL-39363

Investigate why pass/fail status is not set for a grade of 0

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.7, 2.4.4, 2.5
    • Fix Version/s: 2.3.8, 2.4.5, 2.5.1
    • Component/s: SCORM
    • Labels:
    • Rank:
      49996

      Description

      As Pointed out on MDLQA-5699 , this is what is happening:-
      When score is 50 Scorm sends in:-
      cmi.core.lesson_status = completed
      cmi.core.score_raw = 50
      And the status is stored as failed in moodle

      When score is 100 Scorm sends in:-
      cmi.core.lesson_status = completed
      cmi.core.score_raw = 100
      And the status is stored as passed in moodle

      When score is 0 Scorm sends in:-
      cmi.core.lesson_status = completed
      cmi.core.score_raw = 0
      And the status is stored as completed in moodle, which should be failed because of Mastery Score is equal to 80 (see imsmanifest.xml, <adlcp:masteryscore>80</adlcp:masteryscore>)

      Am not sure why this is happening, or if it actually is an issue, but is worth investigating.

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          I'd need full SCORM debugger logs before I looked at this - also unlikely I'll have time before final 2.5 release to do this as a volunteer - so feel free to work on it yourself.

          Show
          Dan Marsden added a comment - I'd need full SCORM debugger logs before I looked at this - also unlikely I'll have time before final 2.5 release to do this as a volunteer - so feel free to work on it yourself.
          Hide
          Ankit Agarwal added a comment -

          Attaching the log file.
          This was a real pain to debug and find. The problem seems to be in StoreData() api, which is doing a cmi.core.score.raw != '' check, which is false for cmi.core.score.raw = 0 since in Js 0 == '' returns true.

          Show
          Ankit Agarwal added a comment - Attaching the log file. This was a real pain to debug and find. The problem seems to be in StoreData() api, which is doing a cmi.core.score.raw != '' check, which is false for cmi.core.score.raw = 0 since in Js 0 == '' returns true.
          Hide
          Ankit Agarwal added a comment -

          Hi Dan,
          Would you happen to have time to review this?

          Show
          Ankit Agarwal added a comment - Hi Dan, Would you happen to have time to review this?
          Hide
          Dan Marsden added a comment -

          Not this week - we need to run this through the adl tests before integration too

          Show
          Dan Marsden added a comment - Not this week - we need to run this through the adl tests before integration too
          Hide
          Dan Marsden added a comment -

          adding Matteo here too - if Matteo reviews and gives a +1 before I do I'm happy for this to be integrated.

          Show
          Dan Marsden added a comment - adding Matteo here too - if Matteo reviews and gives a +1 before I do I'm happy for this to be integrated.
          Hide
          Dan Marsden added a comment -

          No pressure Matteo - I'll take a look at this next week sometime anyway

          Show
          Dan Marsden added a comment - No pressure Matteo - I'll take a look at this next week sometime anyway
          Hide
          Ankit Agarwal added a comment -

          removing mdlqa as this issue is not a strict blocker for the linked QA.
          Thanks

          Show
          Ankit Agarwal added a comment - removing mdlqa as this issue is not a strict blocker for the linked QA. Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Ankit,

          Patch looks good to me, should this be back-ported?

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Ankit, Patch looks good to me, should this be back-ported?
          Hide
          Dan Marsden added a comment -

          Possibly - this also needs to be checked against the adl 1.2 yests and a +1 from me or Matteo before integration I can't do this until next week

          Show
          Dan Marsden added a comment - Possibly - this also needs to be checked against the adl 1.2 yests and a +1 from me or Matteo before integration I can't do this until next week
          Hide
          Michael de Raadt added a comment -

          I'm shifting this off the "Must fix for 2.5" backlog. It appears to affect current and past versions and it's not going to be wrapped up in time for release.

          Show
          Michael de Raadt added a comment - I'm shifting this off the "Must fix for 2.5" backlog. It appears to affect current and past versions and it's not going to be wrapped up in time for release.
          Hide
          Matteo Scaramuccia added a comment -

          Hi All,
          this recalled me MDL-26672: at that time I didn't check for SCORM too.

          The reason behind the issue is well explained and fixed according with https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/Comparison_Operators, given a 0 Score, being the first operand type a Number, the second - an empty String - gets converted to a Number as well: unfortunately for the implemented logics, Number('') evaluates to 0.
          From a SCORM spec POV you could argue that Mastery Score SHOULD never be equal to 0, being meaningless from a pedagogical POV, but it has been defined as a CMIDecimal i.e. 0.0 included.
          There are no other places where a CMIDecimal has been compared with != or ==.

          These above are the reasons for my +1: well done Ankit!

          Show
          Matteo Scaramuccia added a comment - Hi All, this recalled me MDL-26672 : at that time I didn't check for SCORM too. The reason behind the issue is well explained and fixed according with https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/Comparison_Operators , given a 0 Score, being the first operand type a Number, the second - an empty String - gets converted to a Number as well: unfortunately for the implemented logics, Number( '' ) evaluates to 0 . From a SCORM spec POV you could argue that Mastery Score SHOULD never be equal to 0 , being meaningless from a pedagogical POV, but it has been defined as a CMIDecimal i.e. 0.0 included. There are no other places where a CMIDecimal has been compared with != or == . These above are the reasons for my +1: well done Ankit!
          Hide
          Dan Marsden added a comment -

          bouncing up for integration based on Matteo's review. - thanks Matteo.

          Show
          Dan Marsden added a comment - bouncing up for integration based on Matteo's review. - thanks Matteo.
          Hide
          Ankit Agarwal added a comment -

          Thanks guys,
          I have updated the stable branches and added the instruction in testing to run adl tests as suggested by Dan.

          Show
          Ankit Agarwal added a comment - Thanks guys, I have updated the stable branches and added the instruction in testing to run adl tests as suggested by Dan.
          Hide
          Damyon Wiese added a comment -

          Thanks - I tested this - and (maybe my testing is wrong) - but if I do the scorm twice and get the answers wrong both times, it gets marked as complete in the navigation at the end of the second attempt.

          Let me know if you think this is a separate issue.

          Show
          Damyon Wiese added a comment - Thanks - I tested this - and (maybe my testing is wrong) - but if I do the scorm twice and get the answers wrong both times, it gets marked as complete in the navigation at the end of the second attempt. Let me know if you think this is a separate issue.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Damyon,
          you should look at the top of the activity, searching for "Review mode": when you complete an attempt the logics behind the Mastery Score are disabled as per spec so no magics appears in the Status in the same "closed" (read: whenever the item send a score or a completion status) attempt: try a new attempt instead.

          In case, I'll look at your issue my evening (I suppose to be pat of the game when I gave my +1).

          Show
          Matteo Scaramuccia added a comment - Hi Damyon, you should look at the top of the activity, searching for "Review mode": when you complete an attempt the logics behind the Mastery Score are disabled as per spec so no magics appears in the Status in the same "closed" (read: whenever the item send a score or a completion status) attempt: try a new attempt instead. In case, I'll look at your issue my evening (I suppose to be pat of the game when I gave my +1).
          Hide
          Ankit Agarwal added a comment -

          Hi Damyon,
          I was able to replicate the issue you mentioned. But as Matteo pointed out if the mastery score is disabled, it makes sense for this to be happening. In any case doesn't look related to current patch.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Damyon, I was able to replicate the issue you mentioned. But as Matteo pointed out if the mastery score is disabled, it makes sense for this to be happening. In any case doesn't look related to current patch. Thanks
          Hide
          Damyon Wiese added a comment -

          Thanks Matteo and Ankit,

          Integrated to 23, 24, 25 and master.

          I raised a new issue for the bug I found above.

          Show
          Damyon Wiese added a comment - Thanks Matteo and Ankit, Integrated to 23, 24, 25 and master. I raised a new issue for the bug I found above.
          Hide
          Matteo Scaramuccia added a comment -

          For the record: MDL-39747.

          Show
          Matteo Scaramuccia added a comment - For the record: MDL-39747 .
          Hide
          Frédéric Massart added a comment -

          Passing.

          • ADL were run on 2.3, 2.4 and master
          • Steps 1 to 4 were done on 2.3, 2.4 and 2.5
          Show
          Frédéric Massart added a comment - Passing. ADL were run on 2.3, 2.4 and master Steps 1 to 4 were done on 2.3, 2.4 and 2.5
          Hide
          Damyon Wiese added a comment -

          Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone.

          Closing as Fixed!

          Show
          Damyon Wiese added a comment - Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone. Closing as Fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: