Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38560 META: Images management in lesson questions
  3. MDL-38505

Permit lesson to import images in question text when importing questions

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3
    • Fix Version/s: 2.6
    • Component/s: Lesson
    • Testing Instructions:
      Hide
      • All tests should be done with debugging set to developer level
      • Import the files attached to this issue in a lesson (for *.xml files uses Moodle XML format and for *.zip files use Blackboard format)
      • Verify that no error, warning are displayed during import or after
      • Verify that each question has an image (or a sound for one of the questions) in the question text and can be edited with no problem
      • Do some attempts to the lesson as a student
      • As a teacher go to various areas of the lesson including preview, reports, statistics, grading essay (unfortunately "Email graded essays" feature is broken and can' be tested see MDL-39691)
      • If possible, test with your own questions files, but please note that only Moodle XML and BLackboard format support images and medias and that this issue is only about images in question texts, images in answers and feedbacks are not working at that stage (this will be the object of MDL-32870 and MDL-38540)
      Show
      All tests should be done with debugging set to developer level Import the files attached to this issue in a lesson (for *.xml files uses Moodle XML format and for *.zip files use Blackboard format) Verify that no error, warning are displayed during import or after Verify that each question has an image (or a sound for one of the questions) in the question text and can be edited with no problem Do some attempts to the lesson as a student As a teacher go to various areas of the lesson including preview, reports, statistics, grading essay (unfortunately "Email graded essays" feature is broken and can' be tested see MDL-39691 ) If possible, test with your own questions files, but please note that only Moodle XML and BLackboard format support images and medias and that this issue is only about images in question texts, images in answers and feedbacks are not working at that stage (this will be the object of MDL-32870 and MDL-38540 )
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:

      Description

      Objectif

      Lesson activity uses question bank import formats to import questions in lessons page.
      There are currently 2 imports format able to import images using the same mecanism (draft files): Blackboard and Moodle XML. In the future webct import format will use the same mecanism too.
      I think it would be quite easy to add a few lines of codes to lesson so that images are imported.
      I will submit a patch.

      Known limitation

      Unfortunately currently it is not possible to import images in lesson questions multichoice answers. But this is because of the limitations of the lesson code: currently when you create/edit a question in lesson, the filepicker is not available in the editor for multichoice answers. If in the future lesson is enhanced to support images in multichoice answers (I think there is already an issue in the tracker for that) it will be possible to also add support to import images in multichoice answers.

        Gliffy Diagrams

        1. multichoice.xml
          19 kB
          Jean-Michel Vedrine
        2. truefalse.xml
          3 kB
          Jean-Michel Vedrine

          Issue Links

            Activity

            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani,
            I created a branch for master on github.
            If you think this is OK, I can create Blackboard and Moodle XML testfiles including questions with images and write some testing instructions.
            I have classified this issue as "improvement" but it can also be seen as a bugfix as previously import of questions with images produced broken images.
            I let you decide if this should be changed to bugfix and if I should create branches for MOODLE_24_STABLE and MOODLE_23_STABLE or not ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, I created a branch for master on github. If you think this is OK, I can create Blackboard and Moodle XML testfiles including questions with images and write some testing instructions. I have classified this issue as "improvement" but it can also be seen as a bugfix as previously import of questions with images produced broken images. I let you decide if this should be changed to bugfix and if I should create branches for MOODLE_24_STABLE and MOODLE_23_STABLE or not ?
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for proposing that and sharing your solution.

            Show
            salvetore Michael de Raadt added a comment - Thanks for proposing that and sharing your solution.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Thanks Michael,
            Maybe I should explain my plan: I have an urgent need to import Moodle XML files with questions including images (both in question text and in multichoices answers and feedback) in lessons, so the first step was to fix this. I am now finishing a fix to MDL-32870 to add images support to multichoice answers and responses. The last step will be to create an issue so that images in questions answers and responses can be imported.
            As I am doing this for myself, better to create github branchs and submit my solutions in the tracker
            I hope this will be useful to other Moodle users and maybe integrated in a future Moodle version.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Thanks Michael, Maybe I should explain my plan: I have an urgent need to import Moodle XML files with questions including images (both in question text and in multichoices answers and feedback) in lessons, so the first step was to fix this. I am now finishing a fix to MDL-32870 to add images support to multichoice answers and responses. The last step will be to create an issue so that images in questions answers and responses can be imported. As I am doing this for myself, better to create github branchs and submit my solutions in the tracker I hope this will be useful to other Moodle users and maybe integrated in a future Moodle version.
            Hide
            rezeau Joseph Rézeau added a comment -

            Tested. Import of images in question TEXT does work. Now waiting for import of images in MCQ questions ANSWER and FEEDBACK (RESPONSE) fields as well.
            Joseph

            Show
            rezeau Joseph Rézeau added a comment - Tested. Import of images in question TEXT does work. Now waiting for import of images in MCQ questions ANSWER and FEEDBACK (RESPONSE) fields as well. Joseph
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani,
            I just rebased this issue. Would you accept that I assign this issue to me and put you as peer reviewer, even if you can only review this after Moodle 2.5 release (I understand all core developers are very busy now )

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, I just rebased this issue. Would you accept that I assign this issue to me and put you as peer reviewer, even if you can only review this after Moodle 2.5 release (I understand all core developers are very busy now )
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Jean-Michel,

            That would be alright to assign it to you and make me the peer reviewer.

            Thank you for understanding.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Jean-Michel, That would be alright to assign it to you and make me the peer reviewer. Thank you for understanding.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani,
            Now that the 2.5 and master branchs are separated seems to me like a good period to look at this.So I rebased on latest weekly and put this on peer review.
            This is the first of 3 chained tracker issue to add complete image management in lesson questions.
            This tracker issue deals with importing images in question text if the import format support it (currently only blackboard_six and xml import formats support images in core but I have another contributed one in plugin directory: gift with media files).
            The change is in fact rather simple because of the way all the formats now import images: they are imported as draftfiles so the code to manage them in in lesson is rather short.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, Now that the 2.5 and master branchs are separated seems to me like a good period to look at this.So I rebased on latest weekly and put this on peer review. This is the first of 3 chained tracker issue to add complete image management in lesson questions. This tracker issue deals with importing images in question text if the import format support it (currently only blackboard_six and xml import formats support images in core but I have another contributed one in plugin directory: gift with media files). The change is in fact rather simple because of the way all the formats now import images: they are imported as draftfiles so the code to manage them in in lesson is rather short.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Jean-Michel,

            Thank you for working on this issue providing patch.

            Your patch looks great, however I can't really test the functionality for this issue due to the following error: 'Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()'. I'm aware this is not related to your patch.
            The abouve issue might be fixed through this week integration. So in the meantime, I will look at the other sub-task issues.

            Also, it would be great if you could provide some sample files and testing instructions.

            Note: I tried this on master branch.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Jean-Michel, Thank you for working on this issue providing patch. Your patch looks great, however I can't really test the functionality for this issue due to the following error: 'Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()'. I'm aware this is not related to your patch. The abouve issue might be fixed through this week integration. So in the meantime, I will look at the other sub-task issues. Also, it would be great if you could provide some sample files and testing instructions. Note: I tried this on master branch.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani,
            Thanks for reviewing this.
            I will rebase and do some tests.
            We had similar debugging warnings in the quiz and question bank components. they are currently worked on in MDL-39507, so if hey are not fixed by this weekly release I could use a similar method.
            I will also attach some sample files.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, Thanks for reviewing this. I will rebase and do some tests. We had similar debugging warnings in the quiz and question bank components. they are currently worked on in MDL-39507 , so if hey are not fixed by this weekly release I could use a similar method. I will also attach some sample files.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani,
            I have rebased this patch and added a little fix to the format_question_text function in mod/lesson/format.php to prevent the 'Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()' problem. The solution is similar to what we have done in MDL-39507 (Tim's idea ) so I think it is good.
            I will upload some sample files with questions including images in the question text and add testing instructions.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, I have rebased this patch and added a little fix to the format_question_text function in mod/lesson/format.php to prevent the 'Before calling format_text(), the content must be processed with file_rewrite_pluginfile_urls()' problem. The solution is similar to what we have done in MDL-39507 (Tim's idea ) so I think it is good. I will upload some sample files with questions including images in the question text and add testing instructions.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            2 sample files in Moodle XML format one is a true false question with an image (Moodle logo) in the question text and the other is a multichoice question with a sound.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - 2 sample files in Moodle XML format one is a true false question with an image (Moodle logo) in the question text and the other is a multichoice question with a sound.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            A sample Blackboard file (sample_with_images.zip) with 3 multichoice questions, each one has an image in the question text. Rather hard to get right if like me you have no chemical knowledge

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited A sample Blackboard file (sample_with_images.zip) with 3 multichoice questions, each one has an image in the question text. Rather hard to get right if like me you have no chemical knowledge
            Hide
            rwijaya Rossiani Wijaya added a comment - - edited

            Hi Jean,

            Thank you for the sample files. I'm able to use both of the xml files, however the Blackboard file produce the following error:

            Warning: DOMDocument::load(): Namespace prefix bb for title on resource is not defined in /stable/master/temp/bbquiz_import/1375855596/imsmanifest.xml, line: 7 
            in /stable/master/question/format/blackboard_six/format.php on line 106 Fatal error: Call to undefined method qformat_blackboard_six::error() in /stable/master/question/format/blackboard_six/format.php on line 147 

            Comments for patch:

            • mod/lesson/format.php line 422: the array for array('subdirs' => false, 'maxfiles' => -1, 'maxbytes' => 0) could be set as null because those are the default value for options.
            • mod/lesson/format.php line 622: I think it is better to use '' instead of 'http://example.com/'

            The rest of the patch looks good.

            Show
            rwijaya Rossiani Wijaya added a comment - - edited Hi Jean, Thank you for the sample files. I'm able to use both of the xml files, however the Blackboard file produce the following error: Warning: DOMDocument::load(): Namespace prefix bb for title on resource is not defined in /stable/master/temp/bbquiz_import/1375855596/imsmanifest.xml, line: 7 in /stable/master/question/format/blackboard_six/format.php on line 106 Fatal error: Call to undefined method qformat_blackboard_six::error() in /stable/master/question/format/blackboard_six/format.php on line 147 Comments for patch: mod/lesson/format.php line 422: the array for array('subdirs' => false, 'maxfiles' => -1, 'maxbytes' => 0) could be set as null because those are the default value for options. mod/lesson/format.php line 622: I think it is better to use '' instead of 'http://example.com/' The rest of the patch looks good.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Hello Rossiani,
            Sorry I prepared a file with images only in question text but I made a mistake and the sample_with_images.zip file was incorrect. So I think the problem is only in the sample file and not in my code I will re-upload a correct file
            But this helped to discover a problem: contrary to the question bank qformat_default class, the lesson qformat_default class don't define an error method so if here is an error during import we get a fatal error
            Fatal error: Call to undefined method qformat_blackboard_six::error() in C:\wamp\www\moodle_head\question\format\blackboard_six\format.php on line ...
            I will create a tracker issue and propose a patch.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Hello Rossiani, Sorry I prepared a file with images only in question text but I made a mistake and the sample_with_images.zip file was incorrect. So I think the problem is only in the sample file and not in my code I will re-upload a correct file But this helped to discover a problem: contrary to the question bank qformat_default class, the lesson qformat_default class don't define an error method so if here is an error during import we get a fatal error Fatal error: Call to undefined method qformat_blackboard_six::error() in C:\wamp\www\moodle_head\question\format\blackboard_six\format.php on line ... I will create a tracker issue and propose a patch.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            I have created MDL-41063 for tthe missing error function.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - I have created MDL-41063 for tthe missing error function.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Uploading the correct Sample Test wih images.ZIP blackboard file. You see, stupid me, I mixed names and uploaded a bogus sample_with_images.zip file by mistake !! The positive thing is that this helped to discover MDL-41063.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Uploading the correct Sample Test wih images.ZIP blackboard file. You see, stupid me, I mixed names and uploaded a bogus sample_with_images.zip file by mistake !! The positive thing is that this helped to discover MDL-41063 .
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Hello Rossiani,
            Thanks a lot for your review
            As I said, the idea to use 'http://example.com' was Tim's one, not mine but I do support it: as we are calling html_to_text after that to clean the text, better to be sure we have syntactically correct urls so that they are caught and removed. Don't you think so ? If not, could you discuss this with Tim so that the same code is used in lesson and in question bank ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Hello Rossiani, Thanks a lot for your review As I said, the idea to use 'http://example.com' was Tim's one, not mine but I do support it: as we are calling html_to_text after that to clean the text, better to be sure we have syntactically correct urls so that they are caught and removed. Don't you think so ? If not, could you discuss this with Tim so that the same code is used in lesson and in question bank ?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani, I have rebased and taken your comments into account. For line 622 I added the same comment as Tim for MDL-39507 after your discussion.
            I have run codechecker and verified all 3 sample files are imported cleanly.
            The only problem I see, but totally unrelated to this issue, is MDL-41070 on editors in collapsed sections. Tim says it will be fixed by MDL-40668. I hope so, but have not verified that.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, I have rebased and taken your comments into account. For line 622 I added the same comment as Tim for MDL-39507 after your discussion. I have run codechecker and verified all 3 sample files are imported cleanly. The only problem I see, but totally unrelated to this issue, is MDL-41070 on editors in collapsed sections. Tim says it will be fixed by MDL-40668 . I hope so, but have not verified that.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Requesting a new peer review when you can find some time.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Requesting a new peer review when you can find some time.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Jean-michel,

            The patch looks great and I tested all 3 sample files and it works as expected.

            Will you be able to push this for integration review?

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Jean-michel, The patch looks great and I tested all 3 sample files and it works as expected. Will you be able to push this for integration review?
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani,
            Thanks a lot for your review. I am no allowed to push something for integration review. Can you do it ?

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, Thanks a lot for your review. I am no allowed to push something for integration review. Can you do it ?
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Sure Jean no problem.

            Pushing for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Sure Jean no problem. Pushing for integration review.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Jean-Michel, i've integrated this to master.

            Show
            poltawski Dan Poltawski added a comment - Thanks Jean-Michel, i've integrated this to master.
            Hide
            markn Mark Nelson added a comment -

            Hi Jean, thanks for your work! Works as expected, passing.

            Show
            markn Mark Nelson added a comment - Hi Jean, thanks for your work! Works as expected, passing.
            Hide
            damyon Damyon Wiese added a comment -

            There was a young man named McGee
            Who thought squashing bugs was easy
            He tried it one day
            And to his dismay
            The bug guts made his keyboard all greasy

            Thanks!

            This has issue has been fixed and released in Moodle.

            Show
            damyon Damyon Wiese added a comment - There was a young man named McGee Who thought squashing bugs was easy He tried it one day And to his dismay The bug guts made his keyboard all greasy Thanks! This has issue has been fixed and released in Moodle.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Hello Rossiani, Dan, Mark and Damyon,
            Thanks a lot.
            I will now work to finish MDL-32870, the next issue in the MDL-38560 META.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Hello Rossiani, Dan, Mark and Damyon, Thanks a lot. I will now work to finish MDL-32870 , the next issue in the MDL-38560 META.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13