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

      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

        Gliffy Diagrams

        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: