Moodle
  1. Moodle
  2. MDL-38489

SCORM player re-directs user to course page on form submit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.2
    • Fix Version/s: 2.6
    • Component/s: SCORM
    • Testing Instructions:
      Hide

      To reproduce the issue:
      1. Create a course and edit course settings:
      2. Set 'Format' to 'Topics format'
      3. Set 'Course layout' to 'Show one section per page'
      4. Add a SCORM activity to 'Topic 1' and edit settings:
      5. Set 'Display package' to 'New window'
      6. Save and return to course, navigate to the created SCORM activity in 'Topic 1'
      7. Launch the SCORM activity, a new window with the SCORM content should pop up.
      8. The SCORM launch page is redirected back to course overview after SCORM content is launched; Correct behavior would be to redirect to the topic view which contains the SCORM activity.

      With patch applied, it should look like (sectionid has been added):
      http://moodleroot/course/view.php?id=2&sectionid=4&sesskey=xxxxxxxxxx

      Show
      To reproduce the issue: 1. Create a course and edit course settings: 2. Set 'Format' to 'Topics format' 3. Set 'Course layout' to 'Show one section per page' 4. Add a SCORM activity to 'Topic 1' and edit settings: 5. Set 'Display package' to 'New window' 6. Save and return to course, navigate to the created SCORM activity in 'Topic 1' 7. Launch the SCORM activity, a new window with the SCORM content should pop up. 8. The SCORM launch page is redirected back to course overview after SCORM content is launched; Correct behavior would be to redirect to the topic view which contains the SCORM activity. With patch applied, it should look like (sectionid has been added): http://moodleroot/course/view.php?id=2&sectionid=4&sesskey=xxxxxxxxxx
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      master_MDL-38489
    • Rank:
      48479

      Description

      When a SCORM package is set to open in a pop-up window, it leads the user to a page with an 'Open' button. This button uses Javascript to open the SCORM package in a pop-up window, then returns to the course page.

      In mod/scorm/view.js:

      scormform.onsubmit = function()

      {window.open('', 'Popup', poptions); this.target='Popup'; parent.window.location = course_url;}

      ;

      This is great, unless you're in a course format (e.g. Topics) with 'show one section per page' enabled. When this happens, you're taken to the course's overview page rather than the page for the topic you were just looking at.

      In these circumstances, especially on a course with a lot of stuff in each topic, it would be nice to return to the page for the topic you had open rather than the overview page for the course.

      For example, instead of returning to:

      my-moodle.tld/course/view.php?id=1234

      We should return to:

      my-moodle.tld/course/view.php?id=1234&section=5

      1. MDL-38489.patch
        2 kB
        Vinnie Monaco
      2. MDL-38489v2.patch
        3 kB
        Vinnie Monaco

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          thanks for the report - patches are welcome

          Show
          Dan Marsden added a comment - thanks for the report - patches are welcome
          Hide
          Vinnie Monaco added a comment - - edited

          This should fix it, patch uploaded. Or see here: https://github.com/vmonaco/moodle/compare/MOODLE_24_STABLE...MDL-38489

          Show
          Vinnie Monaco added a comment - - edited This should fix it, patch uploaded. Or see here: https://github.com/vmonaco/moodle/compare/MOODLE_24_STABLE...MDL-38489
          Hide
          Vinnie Monaco added a comment -

          Actually, use the MDL-38489v2.patch. I added a check for the course format display, only redirect back to the section if we're in a course with one section per page.

          Show
          Vinnie Monaco added a comment - Actually, use the MDL-38489 v2.patch. I added a check for the course format display, only redirect back to the section if we're in a course with one section per page.
          Hide
          Dan Marsden added a comment -

          Thanks Vinnie - took a quick look and it looks good - bouncing up for peer review (which I may do myself later this week unless someone beats me to it)

          Show
          Dan Marsden added a comment - Thanks Vinnie - took a quick look and it looks good - bouncing up for peer review (which I may do myself later this week unless someone beats me to it)
          Hide
          Dan Marsden added a comment -

          adding Matteo FYI - (Vinnie is a GSOC applicant this year but not for a SCORM project)

          Show
          Dan Marsden added a comment - adding Matteo FYI - (Vinnie is a GSOC applicant this year but not for a SCORM project)
          Hide
          Andrew Davis added a comment -

          This issue is missing some testing instructions. Please describe how a human tester can verify that this fix has been correctly integrated and is working correctly.

          Show
          Andrew Davis added a comment - This issue is missing some testing instructions. Please describe how a human tester can verify that this fix has been correctly integrated and is working correctly.
          Hide
          Vinnie Monaco added a comment -

          To reproduce the issue:
          1. Create a course and edit course settings:
          2. Set 'Format' to 'Topics format'
          3. Set 'Course layout' to 'Show one section per page'
          4. Add a SCORM activity to 'Topic 1' and edit settings:
          5. Set 'Display package' to 'New window'
          6. Save and return to course, navigate to the created SCORM activity in 'Topic 1'
          7. Launch the SCORM activity, a new window with the SCORM content should pop up.
          8. The SCORM launch page is redirected back to course overview after SCORM content is launched; Correct behavior would be to redirect to the topic view which contains the SCORM activity.

          After launching the SCORM content, the redirected URL looks like:
          http://moodleroot/course/view.php?id=2&&sesskey=xxxxxxxxxx

          With patch applied, it should look like (sectionid has been added):
          http://moodleroot/course/view.php?id=2&sectionid=4&sesskey=xxxxxxxxxx

          Show
          Vinnie Monaco added a comment - To reproduce the issue: 1. Create a course and edit course settings: 2. Set 'Format' to 'Topics format' 3. Set 'Course layout' to 'Show one section per page' 4. Add a SCORM activity to 'Topic 1' and edit settings: 5. Set 'Display package' to 'New window' 6. Save and return to course, navigate to the created SCORM activity in 'Topic 1' 7. Launch the SCORM activity, a new window with the SCORM content should pop up. 8. The SCORM launch page is redirected back to course overview after SCORM content is launched; Correct behavior would be to redirect to the topic view which contains the SCORM activity. After launching the SCORM content, the redirected URL looks like: http://moodleroot/course/view.php?id=2&&sesskey=xxxxxxxxxx With patch applied, it should look like (sectionid has been added): http://moodleroot/course/view.php?id=2&sectionid=4&sesskey=xxxxxxxxxx
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Dan and Vinnie,
          The patch looks nice. A few minor things:-

          • Is this behaviour correct:-
          1. I am in, something like course/view.php?id=2
          2. I launch a scorm in a section with above settings
          3. I am redirected to something like course/view.php?id=2&sectionid=4&sesskey=xxxxxxxxxx
          • It might be worth using the apis to get the course option rather than doing a sql query. Something like
            $course = course_get_format($cm->course)->get_course();
            

            This will return all course format options along with the course details.

          • You might want to port it to other branches.
          • It is not a strict rule, but adding component name to commit messages is encouraged.
            Rest looks great.
            Thanks
          Show
          Ankit Agarwal added a comment - - edited Hi Dan and Vinnie, The patch looks nice. A few minor things:- Is this behaviour correct:- I am in, something like course/view.php?id=2 I launch a scorm in a section with above settings I am redirected to something like course/view.php?id=2&sectionid=4&sesskey=xxxxxxxxxx It might be worth using the apis to get the course option rather than doing a sql query. Something like $course = course_get_format($cm->course)->get_course(); This will return all course format options along with the course details. You might want to port it to other branches. It is not a strict rule, but adding component name to commit messages is encouraged. Rest looks great. Thanks
          Hide
          Vinnie Monaco added a comment -

          Hi Ankit

          Re. the behavior, I believe that is correct. You must enter the section before launching a scorm, so the url would look like course/view.php?id=2&sectionid=4 and then redirect back to that page after the scorm launch page is done opening the content in a new window.

          I'll replace the query with the course function and try to get this in a few more branches.

          Thanks for letting me know about the commits.

          Show
          Vinnie Monaco added a comment - Hi Ankit Re. the behavior, I believe that is correct. You must enter the section before launching a scorm, so the url would look like course/view.php?id=2&sectionid=4 and then redirect back to that page after the scorm launch page is done opening the content in a new window. I'll replace the query with the course function and try to get this in a few more branches. Thanks for letting me know about the commits.
          Hide
          Dan Marsden added a comment -

          updated Vinnies patch to use course_get_format - thanks for your work on this Vinnie.

          Show
          Dan Marsden added a comment - updated Vinnies patch to use course_get_format - thanks for your work on this Vinnie.
          Hide
          Ankit Agarwal added a comment -

          Thanks guys for working this.

          Patch looks perfect.

          +1 for integration.

          Show
          Ankit Agarwal added a comment - Thanks guys for working this. Patch looks perfect. +1 for integration.
          Hide
          Dan Marsden added a comment -

          thanks for the help with the reviews this week Ankit.

          Show
          Dan Marsden added a comment - thanks for the help with the reviews this week Ankit.
          Hide
          Dan Marsden added a comment -

          MDL-28579 and MDL-39910 must be integrated before this one (this branch contains those patches as well.)

          Show
          Dan Marsden added a comment - MDL-28579 and MDL-39910 must be integrated before this one (this branch contains those patches as well.)
          Hide
          Dan Marsden added a comment -

          rebased - no longer reliant on MDL-39910 (but still reliant on MDL-28579) - thanks.

          Show
          Dan Marsden added a comment - rebased - no longer reliant on MDL-39910 (but still reliant on MDL-28579 ) - thanks.
          Hide
          Dan Marsden added a comment -

          rebased and no longer reliant on MDL-28579 -

          Show
          Dan Marsden added a comment - rebased and no longer reliant on MDL-28579 -
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          I'm reopening this because I think that you should be using course_get_url() to get the return url rather than working it out yourself.

          cheers,
          Dan

          Show
          Dan Poltawski added a comment - Hi Guys, I'm reopening this because I think that you should be using course_get_url() to get the return url rather than working it out yourself. cheers, Dan
          Hide
          CiBoT added a comment -

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

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

          fair enough - updated patch that does it that way.

          Show
          Dan Marsden added a comment - fair enough - updated patch that does it that way.
          Hide
          Sam Hemelryk added a comment -

          Thanks Dan this has been integrated now. Just noting I added a commit to require course/lib.php at the top of view.php as that is required to ensure course_get_url is available.

          Show
          Sam Hemelryk added a comment - Thanks Dan this has been integrated now. Just noting I added a commit to require course/lib.php at the top of view.php as that is required to ensure course_get_url is available.
          Hide
          David Monllaó added a comment -

          Sorry Dan, is failing on #8, the main window redirection does not work, seems like section value is not correct.

          Debug info:
          Error code: sectionnotexist
          Stack trace:
          
              line 319 of /lib/modinfolib.php: moodle_exception thrown
              line 105 of /course/view.php: call to course_modinfo->get_section_info()
          
          Show
          David Monllaó added a comment - Sorry Dan, is failing on #8, the main window redirection does not work, seems like section value is not correct. Debug info: Error code: sectionnotexist Stack trace: line 319 of /lib/modinfolib.php: moodle_exception thrown line 105 of /course/view.php: call to course_modinfo->get_section_info()
          Hide
          Dan Marsden added a comment -

          grr - obviously too many iterations on the code meaning it wasn't tested well enough... I can look at this tomorrow morning - if that's too late feel free to reject and pull out of integration this week - Thanks David.

          Show
          Dan Marsden added a comment - grr - obviously too many iterations on the code meaning it wasn't tested well enough... I can look at this tomorrow morning - if that's too late feel free to reject and pull out of integration this week - Thanks David.
          Hide
          David Monllaó added a comment -

          Hi Dan,

          I don't have any problem in testing this again tomorrow, feel free to send the fix, if Sam Hemelryk agrees to integrate it I will test it again

          Show
          David Monllaó added a comment - Hi Dan, I don't have any problem in testing this again tomorrow, feel free to send the fix, if Sam Hemelryk agrees to integrate it I will test it again
          Hide
          Sam Hemelryk added a comment -

          Let me know how you get along today Dan

          Show
          Sam Hemelryk added a comment - Let me know how you get along today Dan
          Hide
          Dan Marsden added a comment -

          just pushed a commit that seems to fix this - thanks Sam/David

          Show
          Dan Marsden added a comment - just pushed a commit that seems to fix this - thanks Sam/David
          Hide
          Sam Hemelryk added a comment -

          Yarrrr thanks matey, yer patch 'as been hoist'd and shes read-aye unce more!

          Show
          Sam Hemelryk added a comment - Yarrrr thanks matey, yer patch 'as been hoist'd and shes read-aye unce more!
          Hide
          David Monllaó added a comment -

          Thank Dan and Sam, it passes now

          Show
          David Monllaó added a comment - Thank Dan and Sam, it passes now
          Hide
          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
          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:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: