Moodle
  1. Moodle
  2. MDL-38880

Imported Cloze questions lose embedded images or other media

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      Test that embedded images are not lost during import of multianswer (cloze) questions

      In a course go to Question bank -> Import and import the question/format/xml/test/fixtures/multianswer.xml file selecting Moodle XML format for import.
      Verify that there is no error or warning during import and that a single multianswer (cloze) question is imported with 2 images (Arizona and California flags) in the question text and a small Moodle logo image in the general feedback.

      Test that old Moodle 1.9 image tag is imported correctly

      Import the question/format/xml/test/fixtures/sample_questions_with_old_image_tag.xml file selecting Moodle XML format for import.
      Verify that there is no error or warning during import and that 2 questions are imported: a multichoice and a multianswer (cloze) question. Verify that both questions have a small Moodle logo at the end of the question text (Of course for the multianswer question, end of question text means end of question and for multichoice it means before the stems but that is the expected result ).

      Show
      Test that embedded images are not lost during import of multianswer (cloze) questions In a course go to Question bank -> Import and import the question/format/xml/test/fixtures/multianswer.xml file selecting Moodle XML format for import. Verify that there is no error or warning during import and that a single multianswer (cloze) question is imported with 2 images (Arizona and California flags) in the question text and a small Moodle logo image in the general feedback. Test that old Moodle 1.9 image tag is imported correctly Import the question/format/xml/test/fixtures/sample_questions_with_old_image_tag.xml file selecting Moodle XML format for import. Verify that there is no error or warning during import and that 2 questions are imported: a multichoice and a multianswer (cloze) question. Verify that both questions have a small Moodle logo at the end of the question text (Of course for the multianswer question, end of question text means end of question and for multichoice it means before the stems but that is the expected result ).
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      48975

      Description

      This appears to be a return of MDL-32865.
      If a Cloze question is created with embedded media, exported, and re-imported, the media is lost.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Please try to be more precise. It is impossible to fix a bug if I do not know what the problem is.

          When you say "exported, and re-imported" which method of importing and exporting are you using?

          Can you attach an example file that leads to broken images when you import it?

          Can you write complete steps to reproduce?

          Show
          Tim Hunt added a comment - Please try to be more precise. It is impossible to fix a bug if I do not know what the problem is. When you say "exported, and re-imported" which method of importing and exporting are you using? Can you attach an example file that leads to broken images when you import it? Can you write complete steps to reproduce?
          Hide
          Paul Wray added a comment -

          To reproduce:
          1. Log in to a course as editing teacher
          1. Go to question bank
          2. Select an empty category or create an empty category
          4. Click 'create new question'
          4. Select question type 'embedded answers (Cloze)
          5. Edit the question eg {:SA:=blah} and insert an image
          6. Select question bank/Export
          7. Select file format: Moodle XML
          8. Select category in which you placed the question created above
          9. Click 'Export questions to file'
          10. Note the path of the exported file
          11. Return to question bank and delete the question
          12. Select question bank/Import
          13. Select Moodle XML format
          14. Click Choose file...
          15. Use the file picker to select the xml file exported in step 10.
          15. Click import...continue
          16. Preview the import question. One sees an image placeholder, but the original image is lost.

          The images seem to be correctly embedded in the exported XML file, but are not imported.
          The attached exported file I just created on demo.moodle.net, as you suggested.
          I don't know the version of this instance, but it fails there too so I presume its 2.3 or 2.4.
          The bug is not limited to images but affect any embedded media.

          Show
          Paul Wray added a comment - To reproduce: 1. Log in to a course as editing teacher 1. Go to question bank 2. Select an empty category or create an empty category 4. Click 'create new question' 4. Select question type 'embedded answers (Cloze) 5. Edit the question eg {:SA:=blah} and insert an image 6. Select question bank/Export 7. Select file format: Moodle XML 8. Select category in which you placed the question created above 9. Click 'Export questions to file' 10. Note the path of the exported file 11. Return to question bank and delete the question 12. Select question bank/Import 13. Select Moodle XML format 14. Click Choose file... 15. Use the file picker to select the xml file exported in step 10. 15. Click import...continue 16. Preview the import question. One sees an image placeholder, but the original image is lost. The images seem to be correctly embedded in the exported XML file, but are not imported. The attached exported file I just created on demo.moodle.net, as you suggested. I don't know the version of this instance, but it fails there too so I presume its 2.3 or 2.4. The bug is not limited to images but affect any embedded media.
          Hide
          Paul Wray added a comment -

          Sorry, I said it appears to be a return of issue MDL-26511, I meant MDL-32865.
          I have modified the description.

          Show
          Paul Wray added a comment - Sorry, I said it appears to be a return of issue MDL-26511 , I meant MDL-32865 . I have modified the description.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello,
          with debugging on I get
          Warning: mysqli::real_escape_string() expects parameter 1 to be string, array given in C:\wamp\www\moodle_head\lib\dml\mysqli_native_moodle_database.php on line 868
          when I try to import Paul's xml file. Maybe a regression from something else. I will study it.

          Show
          Jean-Michel Vedrine added a comment - Hello, with debugging on I get Warning: mysqli::real_escape_string() expects parameter 1 to be string, array given in C:\wamp\www\moodle_head\lib\dml\mysqli_native_moodle_database.php on line 868 when I try to import Paul's xml file. Maybe a regression from something else. I will study it.
          Hide
          Jean-Michel Vedrine added a comment -

          The problem is that when file_save_draft_area_files is called line 400 of format.php $question->questiontextitemid is an array of objects (object has content, encoding and name properties) !! certainly not what is to be expected
          Tim do you remember where a change to this could have been made in cloze code recently ?

          Show
          Jean-Michel Vedrine added a comment - The problem is that when file_save_draft_area_files is called line 400 of format.php $question->questiontextitemid is an array of objects (object has content, encoding and name properties) !! certainly not what is to be expected Tim do you remember where a change to this could have been made in cloze code recently ?
          Hide
          Jean-Michel Vedrine added a comment -

          I have added Pierre as a watcher.
          Pierre, you know a lot about image management in cloze.
          Do you see where a change to questiontextitemid could have been made recently ?

          Show
          Jean-Michel Vedrine added a comment - I have added Pierre as a watcher. Pierre, you know a lot about image management in cloze. Do you see where a change to questiontextitemid could have been made recently ?
          Hide
          Jean-Michel Vedrine added a comment -

          Solved ! this is a regression from MDL-37313
          I forgot to change one import_files to import_files_as draft in question/format/xml/format.php line 466

          Show
          Jean-Michel Vedrine added a comment - Solved ! this is a regression from MDL-37313 I forgot to change one import_files to import_files_as draft in question/format/xml/format.php line 466
          Hide
          Jean-Michel Vedrine added a comment -

          linking to issue that caused this regression

          Show
          Jean-Michel Vedrine added a comment - linking to issue that caused this regression
          Hide
          Jean-Michel Vedrine added a comment -

          This is weird it seems I completely forgot to upgrade part of import_multianswer while working on MDL-37313 and as testing didn't include any cloze question this was completely missed during integration. I think that additionally to fixing this I must include a sample of cloze question in question/format/xml/tests/fixtures to prevent future similar issues !

          Show
          Jean-Michel Vedrine added a comment - This is weird it seems I completely forgot to upgrade part of import_multianswer while working on MDL-37313 and as testing didn't include any cloze question this was completely missed during integration. I think that additionally to fixing this I must include a sample of cloze question in question/format/xml/tests/fixtures to prevent future similar issues !
          Hide
          Tim Hunt added a comment -

          Thanks for working on this so quickly Jean-Michel. I agree with everything you have said so far.

          Show
          Tim Hunt added a comment - Thanks for working on this so quickly Jean-Michel. I agree with everything you have said so far.
          Hide
          Jean-Michel Vedrine added a comment -

          Created a master branch with fix and 2 new sample files in fixtures.
          One is to verify old image tag is correctly imported (both in a non multianswer and in a multianswer question because the code is not the same).
          The other one is to verify that multianswer questions with embedded images are correctly imported.

          Show
          Jean-Michel Vedrine added a comment - Created a master branch with fix and 2 new sample files in fixtures. One is to verify old image tag is correctly imported (both in a non multianswer and in a multianswer question because the code is not the same). The other one is to verify that multianswer questions with embedded images are correctly imported.
          Hide
          Jean-Michel Vedrine added a comment -

          Paul,
          Are you able to test if my fix solve your problem (you don't need the new files in question/format/xml/tests/fixtures, you only need the changes I made to question/format/xml/format.php) ?
          I already tested that your file questions-CF101-Default for CF101-20130404-1836.xml is correctly imported with the fix, but testing on other files you may have would be a confirmation.

          Show
          Jean-Michel Vedrine added a comment - Paul, Are you able to test if my fix solve your problem (you don't need the new files in question/format/xml/tests/fixtures, you only need the changes I made to question/format/xml/format.php) ? I already tested that your file questions-CF101-Default for CF101-20130404-1836.xml is correctly imported with the fix, but testing on other files you may have would be a confirmation.
          Hide
          Paul Wray added a comment -

          Thanks very much for your rapid response Jean-Michel!

          We are not set up as Moodle developers but I assume we can test V2.3.4 by grabbing the 3 modified source files from your diff? To test v2.4 I assume we need to wait for this fix to be propagated?

          Show
          Paul Wray added a comment - Thanks very much for your rapid response Jean-Michel! We are not set up as Moodle developers but I assume we can test V2.3.4 by grabbing the 3 modified source files from your diff? To test v2.4 I assume we need to wait for this fix to be propagated?
          Hide
          Paul Wray added a comment -

          We inserted your modified file:
          moodle /question/format/xml/format.php

          into our installations:

          Moodle 2.3.4+ (Build: 20130222) and
          Moodle 2.4.1+ (Build: 20130222)

          The issue seems to be fixed in each case.

          Thank you!

          Show
          Paul Wray added a comment - We inserted your modified file: moodle /question/format/xml/format.php into our installations: Moodle 2.3.4+ (Build: 20130222) and Moodle 2.4.1+ (Build: 20130222) The issue seems to be fixed in each case. Thank you!
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Paul,
          Submitting for peer review.

          Show
          Jean-Michel Vedrine added a comment - Thanks Paul, Submitting for peer review.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          Can look at this ? I will do other branchs and write testing instructions during the weekend.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, Can look at this ? I will do other branchs and write testing instructions during the weekend.
          Hide
          Tim Hunt added a comment -

          +1 patch looks great.

          Once you have backported, and done testing instructions, I will submit for integration.

          Show
          Tim Hunt added a comment - +1 patch looks great. Once you have backported, and done testing instructions, I will submit for integration.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I think this is ready for integration.
          Just a note: currently xml files produced when exporting questions don't have a final end of line (this can be seen on the diff pages) when I use such files as samples, I don't know if I should add a final end of line to make them conform to coding style or let them as they are so that they are "real" sample files.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I think this is ready for integration. Just a note: currently xml files produced when exporting questions don't have a final end of line (this can be seen on the diff pages) when I use such files as samples, I don't know if I should add a final end of line to make them conform to coding style or let them as they are so that they are "real" sample files.
          Hide
          Tim Hunt added a comment -

          Submitting for integration.

          I asked myself the same question about the example files when I peer reviewed this the first time, and I could not decide on the answer. I think what you have done (real example files) is good.

          Show
          Tim Hunt added a comment - Submitting for integration. I asked myself the same question about the example files when I peer reviewed this the first time, and I could not decide on the answer. I think what you have done (real example files) is good.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 24 and 23 - thanks Jean-Michel!

          Show
          Dan Poltawski added a comment - Integrated to master, 24 and 23 - thanks Jean-Michel!
          Hide
          Paul Wray added a comment -

          Thanks again to all.

          Show
          Paul Wray added a comment - Thanks again to all.
          Hide
          Mark Nelson added a comment -

          Thanks again Jean for your contribution. Works as expected, passing.

          Show
          Mark Nelson added a comment - Thanks again Jean for your contribution. Works as expected, passing.
          Hide
          Dan Poltawski added a comment -

          Blooming Marvelous! It's time for a knees up - your changes are upstream!

          Thanks for making Moodle better!

          Toodle pip

          Show
          Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

            People

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

              Dates

              • Created:
                Updated:
                Resolved: