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

Review / browse mode not passed correctly

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      make sure SCORM debugger is enabled
      Add the attached SCORM file to your course.
      Enter the SCORM in normal mode as a student and make sure you answer both questions correctly.
      Finish/exit the SCORM
      Return to the SCORM again and enter it but make sure "start new attempt" checkbox is not ticked and normal mode is selected.
      Review mode text will show in header of player.
      In the SCORM debugger select cmi.core.lesson_status and hit the LMSGetValue Button
      a line like this should appear in the debugger:

      Thu, 12 Sep 2013 22:58:02 GMT: LMSGetValue("cmi.core.lesson_mode") - review => 0

      make sure that line states "Review" and not "normal" or "preview" and no Red errors occur in the debugger window.

      Show
      make sure SCORM debugger is enabled Add the attached SCORM file to your course. Enter the SCORM in normal mode as a student and make sure you answer both questions correctly. Finish/exit the SCORM Return to the SCORM again and enter it but make sure "start new attempt" checkbox is not ticked and normal mode is selected. Review mode text will show in header of player. In the SCORM debugger select cmi.core.lesson_status and hit the LMSGetValue Button a line like this should appear in the debugger: Thu, 12 Sep 2013 22:58:02 GMT: LMSGetValue("cmi.core.lesson_mode") - review => 0 make sure that line states "Review" and not "normal" or "preview" and no Red errors occur in the debugger window.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      master_MDL-37524

      Description

      In mod/scorm/player.php line #136, the $mode ('normal', 'review', or 'browse') is an argument to scorm_get_toc:

      $result = scorm_get_toc($USER, $scorm, $cm->id, TOCJSLINK, $currentorg, $scoid, $mode, $attempt, true, true);

      However, $mode isn't actualy set until nine lines later:
      if (($trackdata->status == 'completed') || ($trackdata->status == 'passed') || ($trackdata->status == 'failed'))

      { $mode = 'review'; }

      By the time $mode is set, the loadSCO.php URL has already been stashed in the toc title attribute by scorm_get_toc, so it's too late. The mode=review isn't passed to loadSCO.php and cmi.mode isn't set correctly.

      The attached patch re-calls scorm_get_toc if $mode=review nine lines later.

      Also in the patch are two other minor and self-explanatory fixes for places $mode isn't passed.

        Gliffy Diagrams

        1. scorm_review_mode.patch
          2 kB
          Ray Morris

          Activity

          Hide
          danmarsden Dan Marsden added a comment -

          thanks - I'll try to take a look at this next week.

          Show
          danmarsden Dan Marsden added a comment - thanks - I'll try to take a look at this next week.
          Hide
          danmarsden Dan Marsden added a comment -

          Thanks Ray - I see what you mean, that code is a bit of a mess and really needs a bit of a rewrite as calling scorm_get_toc multiple times is a bit of a performance hit and although it fixes the issue for users I'd expect the integrators would reject the patch and tell me to do it properly and rewrite those functions

          bumping priority up a bit to make it higher in my list (although I don't usually pay much attention to the priority field) - I'm pretty busy in Feb/March but hopefully I'll get some time to look at this at some point soon.

          Show
          danmarsden Dan Marsden added a comment - Thanks Ray - I see what you mean, that code is a bit of a mess and really needs a bit of a rewrite as calling scorm_get_toc multiple times is a bit of a performance hit and although it fixes the issue for users I'd expect the integrators would reject the patch and tell me to do it properly and rewrite those functions bumping priority up a bit to make it higher in my list (although I don't usually pay much attention to the priority field) - I'm pretty busy in Feb/March but hopefully I'll get some time to look at this at some point soon.
          Hide
          raymor Ray Morris added a comment -

          Let me know if I can be of any assistance.
          What do you think about just doing the two trivial fixes for now, passing the "mode" argument in those two places?

          Show
          raymor Ray Morris added a comment - Let me know if I can be of any assistance. What do you think about just doing the two trivial fixes for now, passing the "mode" argument in those two places?
          Hide
          danmarsden Dan Marsden added a comment -

          I'm not sure if I'd sneak that past the integrators I'll think on it a bit.

          Show
          danmarsden Dan Marsden added a comment - I'm not sure if I'd sneak that past the integrators I'll think on it a bit.
          Hide
          danmarsden Dan Marsden added a comment -

          finally got round to taking a look - here's my first attempt, Ray any chance you might be able to take a look and test this?

          Show
          danmarsden Dan Marsden added a comment - finally got round to taking a look - here's my first attempt, Ray any chance you might be able to take a look and test this?
          Hide
          danmarsden Dan Marsden added a comment -

          hmm - no that won't work... back to the drawing board....

          Show
          danmarsden Dan Marsden added a comment - hmm - no that won't work... back to the drawing board....
          Hide
          danmarsden Dan Marsden added a comment -

          updated the patch and is now slightly better - seems to work but I think it's still a bit fragile - esp lines 75-85 in the patch - Matteo do you have some thoughts on how we could do this better?

          Show
          danmarsden Dan Marsden added a comment - updated the patch and is now slightly better - seems to work but I think it's still a bit fragile - esp lines 75-85 in the patch - Matteo do you have some thoughts on how we could do this better?
          Hide
          matteo Matteo Scaramuccia added a comment -

          Hi Dan,
          I'll look at it later, probably in the weekend.

          Show
          matteo Matteo Scaramuccia added a comment - Hi Dan, I'll look at it later, probably in the weekend.
          Hide
          raymor Ray Morris added a comment - - edited

          Should line 82 be "break" rather than "exit"? If so, does that suggest a testing issue?

          Also, where my patch passes review $mode in locallib.php/scorm_get_toc, is that not needed, or was it overlooked?

          Show
          raymor Ray Morris added a comment - - edited Should line 82 be "break" rather than "exit"? If so, does that suggest a testing issue? Also, where my patch passes review $mode in locallib.php/scorm_get_toc, is that not needed, or was it overlooked?
          Hide
          danmarsden Dan Marsden added a comment -

          heh - that should definitely have been a "break" - coding late fri night isn't the best time to do this stuff...Thanks for spotting that!

          the $mode only needs to be passed when the "user" sets the mode to "browse" - otherwise it's treated as "normal" unless we discover that their "last" attempt was complete (passed/completed/failed) - so I think the code in scorm_get_toc is still ok.

          Show
          danmarsden Dan Marsden added a comment - heh - that should definitely have been a "break" - coding late fri night isn't the best time to do this stuff...Thanks for spotting that! the $mode only needs to be passed when the "user" sets the mode to "browse" - otherwise it's treated as "normal" unless we discover that their "last" attempt was complete (passed/completed/failed) - so I think the code in scorm_get_toc is still ok.
          Hide
          matteo Matteo Scaramuccia added a comment -

          First rough (== w/o running the code) review:

          1. (Trivial) $newattempt=='on' should be $newattempt == 'on'
          2. $modepop is updated just when $mode is equal to browse and that var is never used: we should search for its history and optionally remove it definitively
          3. scorm_get_toc_object should be reviewed to let review, generally $mode, be always evaluated via module.js::node.title into the CMI Data model, initialized in loaddatamodel.php i.e. $modestr = '&mode='.$mode; out of the conditional block
          Show
          matteo Matteo Scaramuccia added a comment - First rough (== w/o running the code) review: (Trivial) $newattempt=='on' should be $newattempt == 'on' $modepop is updated just when $mode is equal to browse and that var is never used: we should search for its history and optionally remove it definitively scorm_get_toc_object should be reviewed to let review , generally $mode , be always evaluated via module.js::node.title into the CMI Data model, initialized in loaddatamodel.php i.e. $modestr = '&mode='.$mode; out of the conditional block
          Hide
          danmarsden Dan Marsden added a comment -

          1 - done
          2 - good point - missed that it was no longer used
          3 - bit more work involved there and it will conflict with Mayanks rewrite to using YUI3 - we could keep this in mind for future restructuring?

          Show
          danmarsden Dan Marsden added a comment - 1 - done 2 - good point - missed that it was no longer used 3 - bit more work involved there and it will conflict with Mayanks rewrite to using YUI3 - we could keep this in mind for future restructuring?
          Hide
          danmarsden Dan Marsden added a comment -

          bouncing up for peer review - still open to feedback on lines 75-85 if there's a better way to do that.

          Show
          danmarsden Dan Marsden added a comment - bouncing up for peer review - still open to feedback on lines 75-85 if there's a better way to do that.
          Hide
          fred Frédéric Massart added a comment - - edited

          Hi guys,

          thanks for fixing this, here are some comments:

          1. Previously it was possible to fall in the if ($mode != 'browse') after the if ($newattempt == 'on' ...). Can you confirm that in the case where $mode is normal because $newattempt is on, it is never relevant to switch to review mode?
          2. Can we consider placing the logic to check if the SCO has been completed in its dedicated function in lib.php? I guess it could also use scorm_get_tracks() as before, to prevent similar code duplication. Up to you.

          As you appear to be cleaning up some stuff around:

          1. $attemptstr is never used.
          2. $scoidpop is never used.
          3. $orgstr is never used.
          4. $scoidstr and $modestr seem to be constructing a URL. Can we consider using a moodle_url() here?
          5. strpos($_SERVER['HTTP_USER_AGENT'], 'MSIE 9') is not recommended, see check_browser_version() (moved to core_useragent in master)

          Cheers,
          Fred

          Show
          fred Frédéric Massart added a comment - - edited Hi guys, thanks for fixing this, here are some comments: Previously it was possible to fall in the if ($mode != 'browse') after the if ($newattempt == 'on' ...) . Can you confirm that in the case where $mode is normal because $newattempt is on, it is never relevant to switch to review mode? Can we consider placing the logic to check if the SCO has been completed in its dedicated function in lib.php? I guess it could also use scorm_get_tracks() as before, to prevent similar code duplication. Up to you. As you appear to be cleaning up some stuff around: $attemptstr is never used. $scoidpop is never used. $orgstr is never used. $scoidstr and $modestr seem to be constructing a URL. Can we consider using a moodle_url() here? strpos($_SERVER ['HTTP_USER_AGENT'] , 'MSIE 9') is not recommended, see check_browser_version() (moved to core_useragent in master) Cheers, Fred
          Hide
          danmarsden Dan Marsden added a comment -

          thanks fred:
          1. - yes - if newattempt is on, mode should always be normal - review mode is for reviewing an existing attempt so if new attempt is being generated then it shouldn't be reviewing.
          2. Makes sense - I'll do that as long as no-one asks me to implement a unit test on the new function before integration :-p

          attemptstr - gone.
          scoidpop = gone.
          orgstr - gone.
          scoid/modestr - I'd really like to stop using that and prevent SCORM from working at all if JS is disabled. - I might do this at some point.
          HTTP_USER_AGENT - I keep meaning to rip that out - see MDL-41577

          Show
          danmarsden Dan Marsden added a comment - thanks fred: 1. - yes - if newattempt is on, mode should always be normal - review mode is for reviewing an existing attempt so if new attempt is being generated then it shouldn't be reviewing. 2. Makes sense - I'll do that as long as no-one asks me to implement a unit test on the new function before integration :-p attemptstr - gone. scoidpop = gone. orgstr - gone. scoid/modestr - I'd really like to stop using that and prevent SCORM from working at all if JS is disabled. - I might do this at some point. HTTP_USER_AGENT - I keep meaning to rip that out - see MDL-41577
          Hide
          danmarsden Dan Marsden added a comment -

          bouncing updated code back up for review.

          Show
          danmarsden Dan Marsden added a comment - bouncing updated code back up for review.
          Hide
          fred Frédéric Massart added a comment -

          Hi Dan,

          It seems good, however you should either return $attempt or pass it by reference as it can be incremented. I'm not asking for unit tests, but I know integrators love them, and your code would look super awesome .

          This code also appears to be missing global $DB. I would highly suggest testing the backported branches before pushing this issue for integration.

          Many thanks!
          Fred

          Show
          fred Frédéric Massart added a comment - Hi Dan, It seems good, however you should either return $attempt or pass it by reference as it can be incremented. I'm not asking for unit tests, but I know integrators love them, and your code would look super awesome . This code also appears to be missing global $DB . I would highly suggest testing the backported branches before pushing this issue for integration. Many thanks! Fred
          Hide
          danmarsden Dan Marsden added a comment -

          grr - I really should be taking some more time on this.... thanks, looking now.

          Show
          danmarsden Dan Marsden added a comment - grr - I really should be taking some more time on this.... thanks, looking now.
          Hide
          danmarsden Dan Marsden added a comment -

          modified the commit in the master branch a bit - am testing this on stable branches now - would be good to get another sanity check on the updated code if you get a chance.

          Show
          danmarsden Dan Marsden added a comment - modified the commit in the master branch a bit - am testing this on stable branches now - would be good to get another sanity check on the updated code if you get a chance.
          Hide
          danmarsden Dan Marsden added a comment -

          found some more issues while testing - updated branches after testing.

          Show
          danmarsden Dan Marsden added a comment - found some more issues while testing - updated branches after testing.
          Hide
          danmarsden Dan Marsden added a comment -

          bouncing this up for integration but I expect I'll need to rebase/somehow merge with the other patches that will be in integration next week. Thanks Fred.

          Show
          danmarsden Dan Marsden added a comment - bouncing this up for integration but I expect I'll need to rebase/somehow merge with the other patches that will be in integration next week. Thanks Fred.
          Hide
          damyon Damyon Wiese added a comment -

          Thanks Dan,

          But I tested this and found that:

          A) This is only implemented for scorm 1.3 files - not 1.2. If this is correct it should be noted in the testing instructions.

          B) I tried several scorm 1.3 quizes etc from files I found on tracker bugs etc - but could not get any of my 1.3 samples to enter "Review mode" (it was working on on the 1.2 samples). Can you please attach a valid sample that supports this so I can test it?

          Show
          damyon Damyon Wiese added a comment - Thanks Dan, But I tested this and found that: A) This is only implemented for scorm 1.3 files - not 1.2. If this is correct it should be noted in the testing instructions. B) I tried several scorm 1.3 quizes etc from files I found on tracker bugs etc - but could not get any of my 1.3 samples to enter "Review mode" (it was working on on the 1.2 samples). Can you please attach a valid sample that supports this so I can test it?
          Hide
          danmarsden Dan Marsden added a comment -

          this is implemented generally - not sure why you think it's not implemented for 1.2 and is only implemented for 1.3 ? - are you looking at the right patch?

          1.3 isn't being developed any more so we don't need to test 1.3 (although patches shouldn't break support)

          I'll add a 1.2 package I was using to test this.

          Show
          danmarsden Dan Marsden added a comment - this is implemented generally - not sure why you think it's not implemented for 1.2 and is only implemented for 1.3 ? - are you looking at the right patch? 1.3 isn't being developed any more so we don't need to test 1.3 (although patches shouldn't break support) I'll add a 1.2 package I was using to test this.
          Hide
          danmarsden Dan Marsden added a comment -

          here's a 1.2 package to use during testing.

          Show
          danmarsden Dan Marsden added a comment - here's a 1.2 package to use during testing.
          Hide
          damyon Damyon Wiese added a comment -

          Sorry Dan failing this.

          Following the testing instructions exactly, I get: "LMSGetValue Returned:
          Error Code: 201"
          Error Description: Invalid argument error

          The full log from the attempt is here:

          Moodle SCORM 1.2 API Loaded, Activity: SCORM, SCO: Fruit_Quizzes
          Thu, 12 Sep 2013 02:56:40 GMT: LMSInitialize("", "") => 0
          Thu, 12 Sep 2013 02:56:40 GMT: LMSGetValue("cmi.core.lesson_status") - failed => 0
          Thu, 12 Sep 2013 02:56:40 GMT: LMSGetValue("cmi.suspend_data") - viewed=1|lastviewedslide= => 0
          Thu, 12 Sep 2013 02:57:18 GMT: LMSGetValue("cmi.mode") - => 201
          Thu, 12 Sep 2013 02:57:18 GMT: LMSGetErrorString("201", "Invalid argument error") => 0
          Thu, 12 Sep 2013 02:57:20 GMT: LMSGetErrorString("201", "Invalid argument error") => 0

          The reason I supposed it was only valid for 1.3 is because:

          grep -r cmi.mode *
          datamodels/debug.js.php:                                'cmi.mode',
          datamodels/scorm_13.js.php:        'cmi.mode':{'defaultvalue':'<?php echo $userdata->mode ?>', 'mod':'r'},
          datamodels/scorm_13.js.php:            if (cmi.mode == 'normal') {

          It is only in the 1.3 datamodel - not the 1.2 one.

          And in the debugger, the cmi.mode variable does not appear for 1.2 scorms, but it does for 1.3.

          Show
          damyon Damyon Wiese added a comment - Sorry Dan failing this. Following the testing instructions exactly, I get: "LMSGetValue Returned: Error Code: 201" Error Description: Invalid argument error The full log from the attempt is here: Moodle SCORM 1.2 API Loaded, Activity: SCORM, SCO: Fruit_Quizzes Thu, 12 Sep 2013 02:56:40 GMT: LMSInitialize("", "") => 0 Thu, 12 Sep 2013 02:56:40 GMT: LMSGetValue("cmi.core.lesson_status") - failed => 0 Thu, 12 Sep 2013 02:56:40 GMT: LMSGetValue("cmi.suspend_data") - viewed=1|lastviewedslide= => 0 Thu, 12 Sep 2013 02:57:18 GMT: LMSGetValue("cmi.mode") - => 201 Thu, 12 Sep 2013 02:57:18 GMT: LMSGetErrorString("201", "Invalid argument error") => 0 Thu, 12 Sep 2013 02:57:20 GMT: LMSGetErrorString("201", "Invalid argument error") => 0 The reason I supposed it was only valid for 1.3 is because: grep -r cmi.mode * datamodels/debug.js.php: 'cmi.mode', datamodels/scorm_13.js.php: 'cmi.mode':{'defaultvalue':'<?php echo $userdata->mode ?>', 'mod':'r'}, datamodels/scorm_13.js.php: if (cmi.mode == 'normal') { It is only in the 1.3 datamodel - not the 1.2 one. And in the debugger, the cmi.mode variable does not appear for 1.2 scorms, but it does for 1.3.
          Hide
          cibot CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          danmarsden Dan Marsden added a comment -

          ah - ok that's my dodgy test instructions then - 1.2 uses core.lesson_mode for the same thing. Will tidy up the testing instructions and re-submit Thanks.

          Show
          danmarsden Dan Marsden added a comment - ah - ok that's my dodgy test instructions then - 1.2 uses core.lesson_mode for the same thing. Will tidy up the testing instructions and re-submit Thanks.
          Hide
          danmarsden Dan Marsden added a comment -

          updated testing instructions and resubmitted - thanks Damyon.

          Show
          danmarsden Dan Marsden added a comment - updated testing instructions and resubmitted - thanks Damyon.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks for the touch-ups Dan this has been integrated again

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks for the touch-ups Dan this has been integrated again
          Hide
          andyjdavis Andrew Davis added a comment -

          This is the first time I've used the scorm debugger but it appears to be working as described. Passing.

          Show
          andyjdavis Andrew Davis added a comment - This is the first time I've used the scorm debugger but it appears to be working as described. Passing.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow.
          The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut.

          Thanks for the effort everyone, another successful weekly release has been rolled.
          Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP.

          Many thanks
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow. The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut. Thanks for the effort everyone, another successful weekly release has been rolled. Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP. Many thanks Sam

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Nov/13