Moodle
  1. Moodle
  2. MDL-33982

Insert/edit embedded media dialog no longer displays a preview

    Details

    • Testing Instructions:
      Hide

      Test Pre-requisites:

      • YouTube repository is enabled.

      Test Steps:

      1. Navigate to any TinyMCE form element e.g. a forum post.
      2. Using the embed multimedia button use file picker to select a video from the YouTube repository.
      3. Preview the embedded media in the "Insert/edit embedded media" dialog.
      4. Make sure the media can be fully previewed in the "Insert/edit embedded media" dialog.
      Show
      Test Pre-requisites: YouTube repository is enabled. Test Steps: Navigate to any TinyMCE form element e.g. a forum post. Using the embed multimedia button use file picker to select a video from the YouTube repository. Preview the embedded media in the "Insert/edit embedded media" dialog. Make sure the media can be fully previewed in the "Insert/edit embedded media" dialog.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Rank:
      42094

      Description

      When a user adds embedded media to a tinymce field the media is supposed to be previewed in the "Insert/edit embedded media" dialog. This worked in 2.2 and does not in 2.3.

      To Recreate:
      Test Pre-requisites:

      • YouTube repository is enabled.
      • Multimedia filtering is turned on.

      Test Steps:

      1. Navigate to any tiny MCE form element e.g. a forum post.
      2. Using the embed multimedia button use file picker to select a video from the YouTube repository.
      3. Preview the embedded media in the "Insert/edit embedded media" dialog.

      Expected result:

      • The media can be fully previewed in the "Insert/edit embedded media" dialog.

      Actual result:

      • The preview field in the "Insert/edit embedded media" dialog is blank.

      Please see attached screen dumps for expected behaviour.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I was able to replicate this and I can confirm that it is a regression since 2.3.

          Show
          Michael de Raadt added a comment - I was able to replicate this and I can confirm that it is a regression since 2.3.
          Hide
          Dan Poltawski added a comment -

          This could be files things, or media embedding MDL-29624 or tinymce! The possibilities are endless!

          Show
          Dan Poltawski added a comment - This could be files things, or media embedding MDL-29624 or tinymce! The possibilities are endless!
          Hide
          Frédéric Massart added a comment -

          Sprint planning.
          Estimated development difficulty: S

          Show
          Frédéric Massart added a comment - Sprint planning. Estimated development difficulty: S
          Hide
          Frédéric Massart added a comment -

          To reviewers/integrators. I have added a new parameter so that I could nicely clean both URL and path parameters without having to hack around. I'm not entirely convinced on the JS part though, but well... All yours!

          Show
          Frédéric Massart added a comment - To reviewers/integrators. I have added a new parameter so that I could nicely clean both URL and path parameters without having to hack around. I'm not entirely convinced on the JS part though, but well... All yours!
          Hide
          Adrian Greeve added a comment -

          Hi Frédéric,

          The code looks good to me. I went through the testing instructions and after flushing the cache in both the browser and the site I was able to see a preview of the videos.

          Testers Please flush your browser to ensure that the changes take effect.

          Show
          Adrian Greeve added a comment - Hi Frédéric, The code looks good to me. I went through the testing instructions and after flushing the cache in both the browser and the site I was able to see a preview of the videos. Testers Please flush your browser to ensure that the changes take effect.
          Hide
          Frédéric Massart added a comment -

          Thanks Adrian. Pushing for integration!

          Show
          Frédéric Massart added a comment - Thanks Adrian. Pushing for integration!
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          The changes look good to me but could you please do a couple of things for me so that this can be integrated.

          1. Produce a custom-tinymce branch as described in lib/editor/tinymce/readme_moodle.txt
          2. Get a +1 from Petr regarding these changes. He is the maintainer for our tinymce integration.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, The changes look good to me but could you please do a couple of things for me so that this can be integrated. Produce a custom-tinymce branch as described in lib/editor/tinymce/readme_moodle.txt Get a +1 from Petr regarding these changes. He is the maintainer for our tinymce integration. Cheers Sam
          Hide
          Frédéric Massart added a comment -

          Thanks Sam. Addind Petr as watcher.
          Petr, could you have a look at my branch and tell me what you think about those changes? I will create the branches for custom_tinymce right after it.
          Cheers!

          Show
          Frédéric Massart added a comment - Thanks Sam. Addind Petr as watcher. Petr, could you have a look at my branch and tell me what you think about those changes? I will create the branches for custom_tinymce right after it. Cheers!
          Hide
          Sam Hemelryk added a comment -

          Hi Fred

          I'm reopening this sorry.

          I've just spoken to Petr (the end of his day crosses over with the start of mine).
          He pointed out a couple of important things:

          1. This has to land in our tinymce repo before it laands in Moodle. Could you please produce a tinymce branch and point him towards that.

          2. Passing the url as is being presently done is a security issue. Petr mentioned base encoding, double encoding, host stripping, but concluded that given we are interested in passing external links so probably base encoding is the only option.

          3. It must use sesskey, also flowing on from the above point he picked up that if we are going to start doing this we had better include and validate sesskey as well, again in the interests of security.

          If you have any other questions or when you're ready for this to me looked at pm Petr.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred I'm reopening this sorry. I've just spoken to Petr (the end of his day crosses over with the start of mine). He pointed out a couple of important things: 1. This has to land in our tinymce repo before it laands in Moodle. Could you please produce a tinymce branch and point him towards that. 2. Passing the url as is being presently done is a security issue. Petr mentioned base encoding, double encoding, host stripping, but concluded that given we are interested in passing external links so probably base encoding is the only option. 3. It must use sesskey, also flowing on from the above point he picked up that if we are going to start doing this we had better include and validate sesskey as well, again in the interests of security. If you have any other questions or when you're ready for this to me looked at pm Petr. Cheers Sam
          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
          Frédéric Massart added a comment -

          Thanks Sam, Petr,

          I have updated the branches to reflect the changes. I don't know where to send the information for the custom-tinymce branch, so here it is:

          Repo: git://github.com/FMCorz/custom-tinymce.git
          Branch: MDL-33982-tinymce-3.5.4.1
          Diff: https://github.com/FMCorz/custom-tinymce/compare/MOODLE_23_3.5.4.1...MDL-33982-tinymce-3.5.4.1
          

          Not sure what the procedure is when dealing with TinyMCE updates, please let me know if you need something else.

          Cheers!

          Show
          Frédéric Massart added a comment - Thanks Sam, Petr, I have updated the branches to reflect the changes. I don't know where to send the information for the custom-tinymce branch, so here it is: Repo: git://github.com/FMCorz/custom-tinymce.git Branch: MDL-33982-tinymce-3.5.4.1 Diff: https://github.com/FMCorz/custom-tinymce/compare/MOODLE_23_3.5.4.1...MDL-33982-tinymce-3.5.4.1 Not sure what the procedure is when dealing with TinyMCE updates, please let me know if you need something else. Cheers!
          Hide
          Frédéric Massart added a comment -

          Sam, regarding our discussion yesterday about the security, I think we're good to go now.

          • The path and URL parameter are cleaned as they should be
          • The script now uses sesskey, which makes it pretty hard to exploit
          • The preview only works if the corresponding media renderer is enabled site wide
          • The SWF preview is disabled as we force it as not 'trusted'. (config page says "Files with extension *.swf. For security reasons this format is only embedded within trusted text.")
          Show
          Frédéric Massart added a comment - Sam, regarding our discussion yesterday about the security, I think we're good to go now. The path and URL parameter are cleaned as they should be The script now uses sesskey, which makes it pretty hard to exploit The preview only works if the corresponding media renderer is enabled site wide The SWF preview is disabled as we force it as not 'trusted'. (config page says "Files with extension *.swf. For security reasons this format is only embedded within trusted text." )
          Hide
          Aparup Banerjee 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
          Aparup Banerjee 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
          Dan Poltawski added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
          Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Petr Škoda added a comment -

          Hi, I do not think it is enough to urlencode, anyway I discovered some other issues when working on new TinyMCE integration in master so I decided to improve the moodlemedia plugin a bit - see MDL-33041.

          Show
          Petr Škoda added a comment - Hi, I do not think it is enough to urlencode, anyway I discovered some other issues when working on new TinyMCE integration in master so I decided to improve the moodlemedia plugin a bit - see MDL-33041 .
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          I've discussed this with Petr, he has addressed this issue in MDL-33041 and informs me that this solution would need yet more work.
          For that reason I am reopening this issue now and think that we should wait for MDL-33041 to land before confirming this issue has been addressed and then closing it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, I've discussed this with Petr, he has addressed this issue in MDL-33041 and informs me that this solution would need yet more work. For that reason I am reopening this issue now and think that we should wait for MDL-33041 to land before confirming this issue has been addressed and then closing it. Cheers Sam
          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
          Akin Delamarre added a comment -

          While this is still being worked on I would like to add something to the discussion.

          I'm creating a video repository for Kaltura. All videos are stored on the Kaltura server, the purpose of the plug-in is to allow the user to browse videos that belong to their Kaltura account. None of the Kaltura videos are physically stored in Moodle, the are just links to videos. I use a custom filter plug-in to convert these links to embed markup so that they videos playback for the user when displayed on a page.

          I'm having some difficulty updating my repository to Moodle 2.3 spec. Kaltura, similar to Youtube, does not include the file name extension in the link to the video and as a result I'm running into problems in the preview dialog.

          In pre Moodle 2.3 you did the same thing and gave youtube core treatment in order for it to display properly (details in MDL-33390). Luckily I was able to add a workaround to have the Kaltura videos display properly. The workaround was to add a /v/flash#video_name to the end of the URL to instruct Moodle to create the proper embed markup.

          With Moodle 2.3 this system has changed with the introduction of using an iframe in the preview dialog and relying on lib/medialib.php to determine what to do when a URL or file extension matches the criteria (similar to how the filter plug-ins work). Again Youtube gets core treatment in this area.

          This causes a problem with other video repository plug-in similar to Youtube. There is no way for my plug-in to specify to Moodle which embed markup to use when previewing the video; and it's even more difficult because with Moodle 2.3 it is looking for specific file extensions.

          I urge you to please consider allowing repository plug-ins to do the following

          • Override a class method that determines how to display the video
            • Allow developers to write the embed markup that is to display in the preview dialog
          Show
          Akin Delamarre added a comment - While this is still being worked on I would like to add something to the discussion. I'm creating a video repository for Kaltura. All videos are stored on the Kaltura server, the purpose of the plug-in is to allow the user to browse videos that belong to their Kaltura account. None of the Kaltura videos are physically stored in Moodle, the are just links to videos. I use a custom filter plug-in to convert these links to embed markup so that they videos playback for the user when displayed on a page. I'm having some difficulty updating my repository to Moodle 2.3 spec. Kaltura, similar to Youtube, does not include the file name extension in the link to the video and as a result I'm running into problems in the preview dialog. In pre Moodle 2.3 you did the same thing and gave youtube core treatment in order for it to display properly (details in MDL-33390 ). Luckily I was able to add a workaround to have the Kaltura videos display properly. The workaround was to add a /v/flash#video_name to the end of the URL to instruct Moodle to create the proper embed markup. With Moodle 2.3 this system has changed with the introduction of using an iframe in the preview dialog and relying on lib/medialib.php to determine what to do when a URL or file extension matches the criteria (similar to how the filter plug-ins work). Again Youtube gets core treatment in this area. This causes a problem with other video repository plug-in similar to Youtube. There is no way for my plug-in to specify to Moodle which embed markup to use when previewing the video; and it's even more difficult because with Moodle 2.3 it is looking for specific file extensions. I urge you to please consider allowing repository plug-ins to do the following Override a class method that determines how to display the video Allow developers to write the embed markup that is to display in the preview dialog
          Hide
          Frédéric Massart added a comment -

          Closing this issue as it has been fixed in MDL-33041.

          Akin, thank you for your input here. Unfortunately this issue has been fixed in another one. May I suggest you to create an improvement issue with your suggestion?

          Thank you!

          Show
          Frédéric Massart added a comment - Closing this issue as it has been fixed in MDL-33041 . Akin, thank you for your input here. Unfortunately this issue has been fixed in another one. May I suggest you to create an improvement issue with your suggestion? Thank you!
          Hide
          Akin Delamarre added a comment -

          Hello Frederic,

          The issue you referenced as having fixed this issue does not fix the issue for Moodle 2.3. This bug has been reported for Moodle 2.3 and but [MDL-33041[ applies a fix for Moodle 2.4.

          What is going to happen to the previews dialog for Moodle 2.3 and where is it going to be fixed?

          I will create new 'improvement' task for my comments.

          Thanks.

          Show
          Akin Delamarre added a comment - Hello Frederic, The issue you referenced as having fixed this issue does not fix the issue for Moodle 2.3. This bug has been reported for Moodle 2.3 and but [ MDL-33041 [ applies a fix for Moodle 2.4. What is going to happen to the previews dialog for Moodle 2.3 and where is it going to be fixed? I will create new 'improvement' task for my comments. Thanks.
          Hide
          Frédéric Massart added a comment -

          Reopening as this issue still occurs on 2.3

          Show
          Frédéric Massart added a comment - Reopening as this issue still occurs on 2.3
          Hide
          Frédéric Massart added a comment -

          Here is a patch to fix the 2.3 branch. Please note that the patch is a custom pick from MDL-33041 (1edb79824c3011cb61670f070a108d524e73e1a7). Also, there is no need to create a branch for our custom TinyMCE, as we are not going to use any separate TinyMCE repo any more. I have decided to keep the name of the parameter to 'path' even though it now gets a URL.

          @ Integrators: 2.3 only!

          Show
          Frédéric Massart added a comment - Here is a patch to fix the 2.3 branch. Please note that the patch is a custom pick from MDL-33041 (1edb79824c3011cb61670f070a108d524e73e1a7). Also, there is no need to create a branch for our custom TinyMCE, as we are not going to use any separate TinyMCE repo any more. I have decided to keep the name of the parameter to 'path' even though it now gets a URL. @ Integrators: 2.3 only!
          Hide
          Adrian Greeve added a comment -

          Hi Fred,

          I notice that the encode64() function doesn't have a doc block. I know that none of the other functions in there have a doc block either, and that the function is pretty self explanatory, but we should probably add it anyway.

          My +1 for integration.

          Show
          Adrian Greeve added a comment - Hi Fred, I notice that the encode64() function doesn't have a doc block. I know that none of the other functions in there have a doc block either, and that the function is pretty self explanatory, but we should probably add it anyway. My +1 for integration.
          Hide
          Frédéric Massart added a comment -

          Thanks for your review Adrian. I actually copied/pasted this function from master (commit 1edb79824c3011cb61670f070a108d524e73e1a7) where the doc block was not present either. As this patch will only be integrated in 2.3 I am pushing for integration. Cheers!

          Show
          Frédéric Massart added a comment - Thanks for your review Adrian. I actually copied/pasted this function from master (commit 1edb79824c3011cb61670f070a108d524e73e1a7) where the doc block was not present either. As this patch will only be integrated in 2.3 I am pushing for integration. Cheers!
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Dan Poltawski added a comment -

          Hi Fred,

          Not looked at the code yet, but reading your comment, I am not sure you are correct:

          "Also, there is no need to create a branch for our custom TinyMCE, as we are not going to use any separate TinyMCE repo any more. "

          In 2.3, we still need to manage changes on that branch, even if not in future dev branches. For example, we have in the past done minor upgrades to tinymce.

          Show
          Dan Poltawski added a comment - Hi Fred, Not looked at the code yet, but reading your comment, I am not sure you are correct: "Also, there is no need to create a branch for our custom TinyMCE, as we are not going to use any separate TinyMCE repo any more. " In 2.3, we still need to manage changes on that branch, even if not in future dev branches. For example, we have in the past done minor upgrades to tinymce.
          Hide
          Petr Škoda added a comment -

          Hi Dan, we have discussed the TinyMCE updates last week during developer meeting, the result is anybody is free to update TinyMCE bugs directly in current stable branches. No backports of TinyMCE are planned, if really necessary the person who is going to import it needs to look for any modifications are reapply them.

          The new rule for master is to not modify upstream TinyMCE at all, only the custom plugins or if really necessary there is a new monkey patching section in module.js

          Show
          Petr Škoda added a comment - Hi Dan, we have discussed the TinyMCE updates last week during developer meeting, the result is anybody is free to update TinyMCE bugs directly in current stable branches. No backports of TinyMCE are planned, if really necessary the person who is going to import it needs to look for any modifications are reapply them. The new rule for master is to not modify upstream TinyMCE at all, only the custom plugins or if really necessary there is a new monkey patching section in module.js
          Hide
          Dan Poltawski added a comment -

          Thanks for clarifying Petr, fred said he'd discussed it with you too.

          Show
          Dan Poltawski added a comment - Thanks for clarifying Petr, fred said he'd discussed it with you too.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks

          Show
          Dan Poltawski added a comment - Integrated, thanks
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Thanks for fixing.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Thanks for fixing. Test passed.
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!
          Hide
          Akin Delamarre added a comment -

          Thanks very much everyone.

          Show
          Akin Delamarre added a comment - Thanks very much everyone.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: