Moodle
  1. Moodle
  2. MDL-39283

GIFT and XML question export - do no write any data to the export for unsupported qtypes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a question category containing one multianswer question, and one true-false question.
      2. Export that category in GIFT format.
      3. Verify that the export file contains only the T/F question and only a comment line begining with double slash for the multianswer question
        no line with <div class="notifyproblem"> HTML tag should be present at the begining of the file.
      4. Verify that importing back the file in another category only import a T/F question with no warning or error message.

      testing Moodle XML export is a little more difficult as all core question types are supported by XML export, so we will remove XML export for multianswer questions to be able to do the test.

      1. edit the question/format/xml/format.php file and remove or comment lines 1255-1259
                    case 'multianswer':
                        foreach ($question->options->questions as $index => $subq) {
                            $expout = preg_replace('~{#' . $index . '}~', $subq->questiontext, $expout);
                        }
                        break;
        
      2. use the same question category that for testing Gift export with one multianswer question, and one true-false question.
      3. Export that category in Moodle XML format.
      4. Verify that there is no line with <div class="notifyproblem"> HTML tag present at the begining of the file and that the export file contains only the T/F question
      5. Verify that importing back the file in another category only import a T/F question with no warning or error message.
      Show
      Create a question category containing one multianswer question, and one true-false question. Export that category in GIFT format. Verify that the export file contains only the T/F question and only a comment line begining with double slash for the multianswer question no line with <div class="notifyproblem"> HTML tag should be present at the begining of the file. Verify that importing back the file in another category only import a T/F question with no warning or error message. testing Moodle XML export is a little more difficult as all core question types are supported by XML export, so we will remove XML export for multianswer questions to be able to do the test. edit the question/format/xml/format.php file and remove or comment lines 1255-1259 case 'multianswer': foreach ($question->options->questions as $index => $subq) { $expout = preg_replace('~{#' . $index . '}~', $subq->questiontext, $expout); } break ; use the same question category that for testing Gift export with one multianswer question, and one true-false question. Export that category in Moodle XML format. Verify that there is no line with <div class="notifyproblem"> HTML tag present at the begining of the file and that the export file contains only the T/F question Verify that importing back the file in another category only import a T/F question with no warning or error message.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      49902

      Description

      To reproduce:
      1. Create a question category containing one multianswer question, and one true-false question.
      2. Export that category in GIFT format.

      Expected result: an export file containing only the T/F question.

      Actual results: export file containing the T/F question, and a broken representation of the multianswer question.

      See also https://tracker.moodle.org/browse/MDLQA-5340.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          If you don't want to work on this Jean-Michel, please reassign it back to me.

          Show
          Tim Hunt added a comment - If you don't want to work on this Jean-Michel, please reassign it back to me.
          Hide
          Jean-Michel Vedrine added a comment -

          I am on holidays so I have all the needed time !
          as I noted in MDLQA-5340 XML export is also writing notifications to the export file when an unsupported question type is found.
          Is there any way to write a warning on screen without writing anything to the export file ?

          Show
          Jean-Michel Vedrine added a comment - I am on holidays so I have all the needed time ! as I noted in MDLQA-5340 XML export is also writing notifications to the export file when an unsupported question type is found. Is there any way to write a warning on screen without writing anything to the export file ?
          Hide
          Tim Hunt added a comment - - edited

          Moving the discussino from MDLQA-5340. Jean-Michel wrote:

          In fact all export formats suffer from the same problem: if there are questions from an unsupported question type the file produced is broken. To test, install a third party question type XXXXXX that don't have an export_to_xml function, create a question of this type and try to export the category using Moodle XML and the beginning of the produced file will include <div class="notifyproblem">Question type XXXXXX is not supported by XML export</div> just before the <?xml version="1.0" encoding="UTF-8"?>

          This is something that got screwed up as part of the conversion to the new file API in 2.0.

          In Moodle 1.x we used to write the export file to a temporary folder while outputting a HTML page with warning messages. Then we would trigger the download of the temporary file.

          Now, we have to generate the export file as it is being downloaded. Therefore, we have no channel to use for errors.

          It is not easy to go back to the old way of doing things while still using the file API. The only option would be to run through the expoort process twice. Once to generat the warnings as part of the question/export.php script and display them; and a second time when the export files is being downloaded. That is not good for performance

          The simplest fix for now is to make the download silently skip the questions is cannot cope with, and then file a separate issue for displaying appropriate warnings.

          Show
          Tim Hunt added a comment - - edited Moving the discussino from MDLQA-5340 . Jean-Michel wrote: In fact all export formats suffer from the same problem: if there are questions from an unsupported question type the file produced is broken. To test, install a third party question type XXXXXX that don't have an export_to_xml function, create a question of this type and try to export the category using Moodle XML and the beginning of the produced file will include <div class="notifyproblem">Question type XXXXXX is not supported by XML export</div> just before the <?xml version="1.0" encoding="UTF-8"?> This is something that got screwed up as part of the conversion to the new file API in 2.0. In Moodle 1.x we used to write the export file to a temporary folder while outputting a HTML page with warning messages. Then we would trigger the download of the temporary file. Now, we have to generate the export file as it is being downloaded. Therefore, we have no channel to use for errors. It is not easy to go back to the old way of doing things while still using the file API. The only option would be to run through the expoort process twice. Once to generat the warnings as part of the question/export.php script and display them; and a second time when the export files is being downloaded. That is not good for performance The simplest fix for now is to make the download silently skip the questions is cannot cope with, and then file a separate issue for displaying appropriate warnings.
          Hide
          Jean-Michel Vedrine added a comment -

          I will create a github branch suppressing all notifications from output for Gift and Moodle XML. The only other format supporting export is XHTML and I already suppressed this problem in XHTML when I removed hardcoded strings from qformat plugins . Unfortunately when I spotted this problem in the XHTML output I didn't realized other format were doing the same thing

          Show
          Jean-Michel Vedrine added a comment - I will create a github branch suppressing all notifications from output for Gift and Moodle XML. The only other format supporting export is XHTML and I already suppressed this problem in XHTML when I removed hardcoded strings from qformat plugins . Unfortunately when I spotted this problem in the XHTML output I didn't realized other format were doing the same thing
          Hide
          Jean-Michel Vedrine added a comment -

          Oops, I realize my testing instructions are wrong, because to test XML export, you need some question from a qtype not supported by XML export, so multianswer can't do the job, I need to find another way to test it.

          Show
          Jean-Michel Vedrine added a comment - Oops, I realize my testing instructions are wrong, because to test XML export, you need some question from a qtype not supported by XML export, so multianswer can't do the job, I need to find another way to test it.
          Hide
          Jean-Michel Vedrine added a comment -

          I see 2 ways to test XML export:

          • install a third party qtype without support for XML export
          • edit question/format/xml/format.php commenting some lines so that one of the core qtypes is not handled and verify all is OK, for instance if we comment lines 1255-1259, multianswer would not be handled any more and we could use the same test that is used for Gift, but that seems like a trick to me !
          Show
          Jean-Michel Vedrine added a comment - I see 2 ways to test XML export: install a third party qtype without support for XML export edit question/format/xml/format.php commenting some lines so that one of the core qtypes is not handled and verify all is OK, for instance if we comment lines 1255-1259, multianswer would not be handled any more and we could use the same test that is used for Gift, but that seems like a trick to me !
          Hide
          Jean-Michel Vedrine added a comment -

          While we think about testing, I will backport to stable branches.

          Show
          Jean-Michel Vedrine added a comment - While we think about testing, I will backport to stable branches.
          Hide
          Tim Hunt added a comment -

          In the past, I have written testing instructions that say "please install this plugin ...".

          Show
          Tim Hunt added a comment - In the past, I have written testing instructions that say "please install this plugin ...".
          Hide
          Jean-Michel Vedrine added a comment -

          Well to tell the truth I somewhat cheated when I tested this: all my qtypes are good qtypes that include XML import/export so what I did was that I edited multichoiceset (all or nothing) to comment the export_to_xml function, so maybe a solution would be to attach to this issue

          • my modified multichoiceset qtype
          • an xml file cointaining a T/F and a multichoiceset question

          Then testing would tell

          • install the modified multichoiceset questiontype
          • inport the attached xml file
          • try to export the category using moodle XML format and verify it only contains the T/F question and nothing for the multichoiceset question.
          Show
          Jean-Michel Vedrine added a comment - Well to tell the truth I somewhat cheated when I tested this: all my qtypes are good qtypes that include XML import/export so what I did was that I edited multichoiceset (all or nothing) to comment the export_to_xml function, so maybe a solution would be to attach to this issue my modified multichoiceset qtype an xml file cointaining a T/F and a multichoiceset question Then testing would tell install the modified multichoiceset questiontype inport the attached xml file try to export the category using moodle XML format and verify it only contains the T/F question and nothing for the multichoiceset question.
          Hide
          Tim Hunt added a comment -

          In that case, editing format_xml in the way you suggest might be the easiest way to test.

          Show
          Tim Hunt added a comment - In that case, editing format_xml in the way you suggest might be the easiest way to test.
          Hide
          Jean-Michel Vedrine added a comment -

          In fact my fix don't completely fix the problem for XML export (fortunately I did the test at the same time I wrote the testing instructions this is how I spotted the problem)

          • the fix prevent writing the notification about unsupported question type in the export file
          • but all parts of the unsupported question not specific to that question type are still written to the export file: generic informations output by lines 1140-1156, hints and question tags output by lines 1421-1438
          • so the export file still contains a broken question !

          It is to late for trying to fix this problem now, so I will try to have an idea tomorrow.
          At the same time, I don't want to block MDLQA-5340 for too long.

          Show
          Jean-Michel Vedrine added a comment - In fact my fix don't completely fix the problem for XML export (fortunately I did the test at the same time I wrote the testing instructions this is how I spotted the problem) the fix prevent writing the notification about unsupported question type in the export file but all parts of the unsupported question not specific to that question type are still written to the export file: generic informations output by lines 1140-1156, hints and question tags output by lines 1421-1438 so the export file still contains a broken question ! It is to late for trying to fix this problem now, so I will try to have an idea tomorrow. At the same time, I don't want to block MDLQA-5340 for too long.
          Hide
          Jean-Michel Vedrine added a comment -

          A short term fix, no very elegant, could be to add a boolean $invalidquestion set to false at the start of writequestion and set it to true when an unhandled question is found, then at the end of writequestion return an empty string rather than $expout when $invalidquestion is set.
          A long term fix would be to add an error data structure to the qformat class so that errors and errors messages could be added to it during export (or import). This would also solve the problem to display appropriate warnings at the end of export process as we can't display them before that.
          Better to wait your comments and advices before doing anything.

          Show
          Jean-Michel Vedrine added a comment - A short term fix, no very elegant, could be to add a boolean $invalidquestion set to false at the start of writequestion and set it to true when an unhandled question is found, then at the end of writequestion return an empty string rather than $expout when $invalidquestion is set. A long term fix would be to add an error data structure to the qformat class so that errors and errors messages could be added to it during export (or import). This would also solve the problem to display appropriate warnings at the end of export process as we can't display them before that. Better to wait your comments and advices before doing anything.
          Hide
          Tim Hunt added a comment -

          Your short-term fix sounds good. Thanks.

          Show
          Tim Hunt added a comment - Your short-term fix sounds good. Thanks.
          Hide
          Tim Hunt added a comment -

          I created MDL-39301 to remind us to do a long-term fix one day. If you have any insight, please add it there, but I don't expect to fix that soon.

          Show
          Tim Hunt added a comment - I created MDL-39301 to remind us to do a long-term fix one day. If you have any insight, please add it there, but I don't expect to fix that soon.
          Hide
          Jean-Michel Vedrine added a comment -

          I think this is ready for peer-review, and I tried the testing instructions, this is now working as expected.

          Show
          Jean-Michel Vedrine added a comment - I think this is ready for peer-review, and I tried the testing instructions, this is now working as expected.
          Hide
          Tim Hunt added a comment -

          Great. Submitting for integration.

          Show
          Tim Hunt added a comment - Great. Submitting for integration.
          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
          Dan Poltawski added a comment -

          I tested on all branches during integration and looked good. Just waiting for CI server now

          Show
          Dan Poltawski added a comment - I tested on all branches during integration and looked good. Just waiting for CI server now
          Hide
          Dan Poltawski added a comment -

          Passing this preemptively because its looking OK

          Show
          Dan Poltawski added a comment - Passing this preemptively because its looking OK
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: