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

Start New Attempt option is ignored if SCORM is set to appear in a popup

    Details

    • Testing Instructions:
      Hide

      1. Create new course.

      2. Enrol test learner.

      3. Create new SCORM activity.

      4. Ensure 'Force new attempt' is set to 'No' in SCORM settings.

      5. Set scorm to appear in New Window

      6. Log in as a test learner and complete SCORM (pass or fail, it doesn't matter).

      7. Go back to SCORM once completed and select 'Start new attempt' checkbox and Normal mode.

      8. Ensure that a new attempt is made and SCORM should open a new popup in "Normal" mode and the progress should be tracked.

      Show
      1. Create new course. 2. Enrol test learner. 3. Create new SCORM activity. 4. Ensure 'Force new attempt' is set to 'No' in SCORM settings. 5. Set scorm to appear in New Window 6. Log in as a test learner and complete SCORM (pass or fail, it doesn't matter). 7. Go back to SCORM once completed and select 'Start new attempt' checkbox and Normal mode. 8. Ensure that a new attempt is made and SCORM should open a new popup in "Normal" mode and the progress should be tracked.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
    • Pull Master Branch:

      Description

      If a previously-attempted scorm is set to appear in a popup, then even if the user selects Normal mode and checks the "Start new attempt" checkbox, the scorm will always open in Review mode and the attempt is not recorded.

      It works properly when the scorm is opened in the current window.

      TO REPRODUCE:

      1. Create new course.

      2. Enrol test learner.

      3. Create new SCORM activity.

      4. Ensure 'Force new attempt' is set to 'No' in SCORM settings.

      5. Set scorm to appear in New Window

      6. Log in as a test learner and complete SCORM (pass or fail, it doesn't matter).

      7. Go back to SCORM once completed and select 'Start new attempt' checkbox and Normal mode.

      8. SCORM is actually opened in 'Review mode' and attempt is not recorded.

      REASON:
      mod/scorm/view.js

      The url for the popup window is set on line 11 (2.6)

      var launch_url = M.cfg.wwwroot+"/mod/scorm/player.php?a="scorm"&currentorg="currentorg"&scoid="sco"&sesskey="M.cfg.sesskey"&display=popup";

      Then there is a YUI event handler that triggers when the submit button is pressed:

      Y.on('submit', function(e) {
      winobj = window.open(launch_url, 'Popup', poptions);

      The issue of course is that the mode and newattempt options on the form are not being considered and added if appropriate to the popup URL.

      SUGGESTED FIX:

      I added a function to the view.js file to do just that:

      var check_launchurl = function() {
      // Check the current state of the mode and newattempt options if they exist.
      var mode = Y.one('#n');
      if (mode && mode.get('checked'))

      { launch_url += '&mode=normal'; }

      var newattempt = Y.one('#n');
      if (newattempt && newattempt.get('checked'))

      { launch_url += '&newattempt=on'; }

      }

      Then called this function on the two places where popups can be launched

      if (launch == true) {
      check_launchurl();
      winobj = window.open(launch_url,'Popup', poptions);

      Y.on('submit', function(e) {
      check_launchurl();
      winobj = window.open(launch_url, 'Popup', poptions);

      And now popups launch correctly and accept new attempts.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              ciaran.irvine@totaralms.com Ciaran Irvine added a comment -

              May also be an issue in 2.7, I've not tested it there yet

              Show
              ciaran.irvine@totaralms.com Ciaran Irvine added a comment - May also be an issue in 2.7, I've not tested it there yet
              Hide
              ciaran.irvine@totaralms.com Ciaran Irvine added a comment -

              Typo in the suggested fix, the YUI call to get the newattempt node should be

              var newattempt = Y.one('#a');

              Show
              ciaran.irvine@totaralms.com Ciaran Irvine added a comment - Typo in the suggested fix, the YUI call to get the newattempt node should be var newattempt = Y.one('#a');
              Hide
              nobelium vignesh p added a comment -

              Thanks Ciaran Irvine, Your fix looks great. I have made a patch out of your fix

              Dan - Can you please take a look at the patch. If it's ok, I will cherry pick it for v2.5 and v2.6.

              Show
              nobelium vignesh p added a comment - Thanks Ciaran Irvine, Your fix looks great. I have made a patch out of your fix Dan - Can you please take a look at the patch. If it's ok, I will cherry pick it for v2.5 and v2.6.
              Hide
              danmarsden Dan Marsden added a comment -

              cool - looks ok but I haven't tested it yet - throwing up for peer review to remind me to look further. Would also be good if you could copy the reproduction info into the testing instructions field if you get a chance!

              thanks,

              Show
              danmarsden Dan Marsden added a comment - cool - looks ok but I haven't tested it yet - throwing up for peer review to remind me to look further. Would also be good if you could copy the reproduction info into the testing instructions field if you get a chance! thanks,
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-46236 using repository: git://github.com/nobelium/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-46236 using repository: git://github.com/nobelium/moodle.git master (branch: MDL-46236 | CI Job ) Testing instructions are missing. More information about this report
              Hide
              danmarsden Dan Marsden added a comment -

              looks good - pushing up for integration.

              Show
              danmarsden Dan Marsden added a comment - looks good - pushing up for integration.
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated to master, 27 and 26 - thanks Ciaran/Vignesh/Dan

              Show
              poltawski Dan Poltawski added a comment - Integrated to master, 27 and 26 - thanks Ciaran/Vignesh/Dan
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for fixing this Ciaran and Vignesh,

              Works as expected, passing...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Ciaran and Vignesh, Works as expected, passing...
              Hide
              rajeshtaneja Rajesh Taneja added a comment - - edited

              Sorry guys,

              I just looked at the scorm report and I am seeing multiple entries with empty user information. Attaching screenshot for reference.

              • 1st attempt populates user columns
              • 2nd attempt onwards user columns are empty. (IMHO They should not be included at first place.)

              Failing this for now, so it can be looked at.

              Show
              rajeshtaneja Rajesh Taneja added a comment - - edited Sorry guys, I just looked at the scorm report and I am seeing multiple entries with empty user information. Attaching screenshot for reference. 1st attempt populates user columns 2nd attempt onwards user columns are empty. (IMHO They should not be included at first place.) Failing this for now, so it can be looked at.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Rajesh - not sure why you're failing on that.... I presume you haven't seen the reports with multiple attempts before? - I haven't liked that display either but it's been like that since 1.9 I think and I vaguely remember seeing similar tables in other plugins in the past too. Feel free to create a new tracker issue and include a mockup to show how you think the report should display multiple attempts from a single user...

              Show
              danmarsden Dan Marsden added a comment - Hi Rajesh - not sure why you're failing on that.... I presume you haven't seen the reports with multiple attempts before? - I haven't liked that display either but it's been like that since 1.9 I think and I vaguely remember seeing similar tables in other plugins in the past too. Feel free to create a new tracker issue and include a mockup to show how you think the report should display multiple attempts from a single user...
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Aha.. By Bad..

              Thanks for the update Dan, Sorry I haven't seen that report before with multiple attempts.

              I assumed that was not correct. Surely will create an issue as follow-up.

              Re-opening this for testing.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Aha.. By Bad.. Thanks for the update Dan, Sorry I haven't seen that report before with multiple attempts. I assumed that was not correct. Surely will create an issue as follow-up. Re-opening this for testing.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Sorry for the noise guys,

              As Dan mentioned, report should appear like that, so passing it...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Sorry for the noise guys, As Dan mentioned, report should appear like that, so passing it...
              Hide
              danmarsden Dan Marsden added a comment -

              thanks - I've never really liked the way it does that - I see the quiz reports display the user firstname/lastname and e-mail in subsequent attempts but exclude the profile image - that looks slightly better than the way we do it in the SCORM reports at the moment.

              Show
              danmarsden Dan Marsden added a comment - thanks - I've never really liked the way it does that - I see the quiz reports display the user firstname/lastname and e-mail in subsequent attempts but exclude the profile image - that looks slightly better than the way we do it in the SCORM reports at the moment.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your contribution - this change is now part of Moodle!

              True knowledge exists in knowing that you know nothing.
              – Socrates

              Show
              poltawski Dan Poltawski added a comment - Thanks for your contribution - this change is now part of Moodle! True knowledge exists in knowing that you know nothing. – Socrates
              Hide
              lersheng Hansen Ler added a comment -

              Guys,

              There is a bug for a new Scorm object which prevents "New Window" from coming up and "Disable Preview mode" > Yes.
              The problem comes from this code:

              var mode = Y.one('#scormviewform input[name=mode]:checked').get('value');

              where it is undefined.

              To fix it, I have added a check to find the control before getting the value.

              var modeinputexist = Y.one('#scormviewform input[name=mode]:checked');
              if (modeinputexist)

              { var mode = Y.one('#scormviewform input[name=mode]:checked').get('value'); if (mode) launch_url += '&mode=' + (mode ? mode : 'normal'); }

              For people who encounter "New Window" problem...

              Show
              lersheng Hansen Ler added a comment - Guys, There is a bug for a new Scorm object which prevents "New Window" from coming up and "Disable Preview mode" > Yes. The problem comes from this code: var mode = Y.one('#scormviewform input [name=mode] :checked').get('value'); where it is undefined. To fix it, I have added a check to find the control before getting the value. var modeinputexist = Y.one('#scormviewform input [name=mode] :checked'); if (modeinputexist) { var mode = Y.one('#scormviewform input[name=mode]:checked').get('value'); if (mode) launch_url += '&mode=' + (mode ? mode : 'normal'); } For people who encounter "New Window" problem...
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Hansen - can you please report a new bug in the tracker with those details including information on the moodle version you are using?
              thanks,

              Show
              danmarsden Dan Marsden added a comment - Hi Hansen - can you please report a new bug in the tracker with those details including information on the moodle version you are using? thanks,
              Hide
              lersheng Hansen Ler added a comment -

              Done. The new issue is here:
              https://tracker.moodle.org/browse/MDL-46760

              Part of the sub-task.

              Show
              lersheng Hansen Ler added a comment - Done. The new issue is here: https://tracker.moodle.org/browse/MDL-46760 Part of the sub-task.
              Hide
              danmarsden Dan Marsden added a comment -

              thanks - I've shifted it into a bug by itself - sub-tasks are usually used when you have a big chunk of work that you want to separate out into different chunks.

              Show
              danmarsden Dan Marsden added a comment - thanks - I've shifted it into a bug by itself - sub-tasks are usually used when you have a big chunk of work that you want to separate out into different chunks.
              Hide
              sergeyandro Sergey Vidusov added a comment -

              Note that if "Force new attempt" is set to "Yes" in the activity settings, newattempt variable fix won't work, because it moves into the hidden input. I had to add the following in order to make the popup window work:

              diff --git a/mod/scorm/view.js b/mod/scorm/view.js
              index 900f3e5..2eb6667 100644
              — a/mod/scorm/view.js
              +++ b/mod/scorm/view.js
              @@ -76,6 +76,12 @@ M.mod_scormform.init = function(Y) {

              var newattempt = Y.one('#scormviewform #a');
              launch_url += (newattempt && newattempt.get('checked') ? '&newattempt=on' : '');
              + //BEGIN CLIENT-SPECIFIC CHANGE: RETAIN
              + var newattempt_input = Y.one('#scormviewform input[name=newattempt]');
              + if (newattempt_input)

              { + launch_url += '&newattempt=' + newattempt_input.get('value'); + }

              + //END CLIENT-SPECIFIC CHANGE: RETAIN
              }

              if (launch == true) {

              Show
              sergeyandro Sergey Vidusov added a comment - Note that if "Force new attempt" is set to "Yes" in the activity settings, newattempt variable fix won't work, because it moves into the hidden input. I had to add the following in order to make the popup window work: diff --git a/mod/scorm/view.js b/mod/scorm/view.js index 900f3e5..2eb6667 100644 — a/mod/scorm/view.js +++ b/mod/scorm/view.js @@ -76,6 +76,12 @@ M.mod_scormform.init = function(Y) { var newattempt = Y.one('#scormviewform #a'); launch_url += (newattempt && newattempt.get('checked') ? '&newattempt=on' : ''); + //BEGIN CLIENT-SPECIFIC CHANGE: RETAIN + var newattempt_input = Y.one('#scormviewform input [name=newattempt] '); + if (newattempt_input) { + launch_url += '&newattempt=' + newattempt_input.get('value'); + } + //END CLIENT-SPECIFIC CHANGE: RETAIN } if (launch == true) { –

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    8/Sep/14