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

IMS content packages with external links don't display

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      1/ Modify some imscp file to contain external http or https links
      2/ Upload it to moodle as new mod_imscp resource
      3/ Verify the links are clickable

      Show
      1/ Modify some imscp file to contain external http or https links 2/ Upload it to moodle as new mod_imscp resource 3/ Verify the links are clickable
    • Workaround:
      Hide

      Modify mod/imscp code according to attached diff file

      Show
      Modify mod/imscp code according to attached diff file
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w24_MDL-32480_m26_imscp

      Description

      IMS Content Packages can either incorporate resources as files within the Content Package, or just provide links to external repositories (http://www.imsglobal.org/content/packaging/cpv1p2pd2/imscp_primerv1p2pd2.html#Xaa1747408).

      In Moodle 2, IMS Content Packages of the latter kind don't display the remote pages (i.e. try to display them as if they were local pages).

        Gliffy Diagrams

        1. MDL-32480_proposed_fix.diff
          0.9 kB
          Nicolas Dunand

          Activity

          Hide
          monidu Nicolas Dunand added a comment -

          It seems the XML parser handles this correctly but later the part of the code building the HTML markup assumes the resources are all local links.

          Proposed fix attached.

          Show
          monidu Nicolas Dunand added a comment - It seems the XML parser handles this correctly but later the part of the code building the HTML markup assumes the resources are all local links. Proposed fix attached.
          Hide
          monidu Nicolas Dunand added a comment -

          Hi,
          Sorry if this is not the right way to proceed, but I have a fix for this issue and would like to have it reviewed.

          Show
          monidu Nicolas Dunand added a comment - Hi, Sorry if this is not the right way to proceed, but I have a fix for this issue and would like to have it reviewed.
          Hide
          skodak Petr Skoda added a comment -

          Hello, I suppose it could look for https links too, there is a also a minor whitespace issue before the else keyword.

          otherwise my +1

          Show
          skodak Petr Skoda added a comment - Hello, I suppose it could look for https links too, there is a also a minor whitespace issue before the else keyword. otherwise my +1
          Hide
          skodak Petr Skoda added a comment -

          hello, could you please put the patch into some git repository in a branch on top of current master?

          Show
          skodak Petr Skoda added a comment - hello, could you please put the patch into some git repository in a branch on top of current master?
          Hide
          monidu Nicolas Dunand added a comment -
          Show
          monidu Nicolas Dunand added a comment - Hello Petr, Here : https://github.com/ndunand/moodle/tree/MDL-32480
          Hide
          skodak Petr Skoda added a comment -

          Thanks Nicolas for the branch, I have tweaked the commit message a bit (see http://docs.moodle.org/dev/Commit_cheat_sheet) and added one more commit that adds https:// support too.

          Could you please attach some small test file here? I would have a lot during testing...

          Show
          skodak Petr Skoda added a comment - Thanks Nicolas for the branch, I have tweaked the commit message a bit (see http://docs.moodle.org/dev/Commit_cheat_sheet ) and added one more commit that adds https:// support too. Could you please attach some small test file here? I would have a lot during testing...
          Hide
          monidu Nicolas Dunand added a comment -

          A small IMS content package to test, containing links to an external Web site. Content is from a geology course, but I'm afraid it's in French.

          Show
          monidu Nicolas Dunand added a comment - A small IMS content package to test, containing links to an external Web site. Content is from a geology course, but I'm afraid it's in French.
          Hide
          skodak Petr Skoda added a comment -

          thanks!

          Show
          skodak Petr Skoda added a comment - thanks!
          Hide
          poltawski Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks guys! this has been integrated now.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks guys! this has been integrated now.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for fixing this Petr,

          I tried it with attached IMS package and I can see external contents being displayed.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Petr, I tried it with attached IMS package and I can see external contents being displayed.
          Hide
          marina Marina Glancy added a comment -

          Thanks for your awesome work! This has now become a part of Moodle.

          Closing as fixed!

          Show
          marina Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                8/Jul/13