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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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:
    • Testing Instructions:
      Hide

      Testing difficulty:- Difficult (Long, complex setups needed)

      1. Using the scorm pack attached in MDLQA-5699 attempt a scorm activity as a student who has not attempted the scorm before.
      2. Set "display course structure on entry" to yes in settings
      3. Get a score of 0 by answering everything incorrectly.
      4. Make sure a cross is show in navigation and on scorm entry page against the SCO.
      5. Run the ADL 1.2 tests http://docs.moodle.org/dev/SCORM_Test_Harness#ADL_1.2_Tests
      Show
      Testing difficulty:- Difficult (Long, complex setups needed) Using the scorm pack attached in MDLQA-5699 attempt a scorm activity as a student who has not attempted the scorm before. Set "display course structure on entry" to yes in settings Get a score of 0 by answering everything incorrectly. Make sure a cross is show in navigation and on scorm entry page against the SCO. Run the ADL 1.2 tests http://docs.moodle.org/dev/SCORM_Test_Harness#ADL_1.2_Tests
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39363-master

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            danmarsden 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
            danmarsden 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_frenz 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_frenz 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_frenz Ankit Agarwal added a comment -

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

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

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

            Show
            danmarsden Dan Marsden added a comment - Not this week - we need to run this through the adl tests before integration too
            Hide
            danmarsden 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
            danmarsden 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
            danmarsden Dan Marsden added a comment -

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

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

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

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

            Thanks for fixing this Ankit,

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

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Ankit, Patch looks good to me, should this be back-ported?
            Hide
            danmarsden 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
            danmarsden 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
            salvetore 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
            salvetore 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 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 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
            danmarsden Dan Marsden added a comment -

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

            Show
            danmarsden Dan Marsden added a comment - bouncing up for integration based on Matteo's review. - thanks Matteo.
            Hide
            ankit_frenz 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_frenz 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 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 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 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 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_frenz 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_frenz 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 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 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 Matteo Scaramuccia added a comment -

            For the record: MDL-39747.

            Show
            matteo Matteo Scaramuccia added a comment - For the record: MDL-39747 .
            Hide
            fred 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
            fred 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 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 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:
                  Fix Release Date:
                  8/Jul/13