Moodle
  1. Moodle
  2. MDL-35147

Question import into lesson is broken

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.1, 2.4
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Lesson
    • Labels:
    • Testing Instructions:
      Hide

      1. Try importing some questions into the lesson module.

      (You can find test files in question/format/.../tests/fixtures/...)

      Show
      1. Try importing some questions into the lesson module. (You can find test files in question/format/.../tests/fixtures/...)
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      43779

      Description

      The MDL-25492 changes broke lesson import, because lesson tries to re-use the question import classes in an insane way.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          I'm fixing it.

          Show
          Tim Hunt added a comment - I'm fixing it.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Tim,
          I completely forgot about lesson !!

          Show
          Jean-Michel Vedrine added a comment - Thanks Tim, I completely forgot about lesson !!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Not sure if lesson question import is covered by QA tests, so adding the label here to get that checked and filled, if necessary.

          Show
          Eloy Lafuente (stronk7) added a comment - Not sure if lesson question import is covered by QA tests, so adding the label here to get that checked and filled, if necessary.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Uhm, this is not good, I've tried importing some formats (examvies, aiken... and everything went ok).

          The I've tried bb6 ones and I get some warnings from the DB layer about passing arrays in individual params.

          Tracing down the problem, it seems that all text fields (questiontext, feedbacktrue, answer...) are being returned as arrays containing (text, format and files). And looking to code, it seems that all BUT questiontext is prepared to read the information in that way.

          So, importing any of the bb6 fixtures, I get one array for questiontext (note no matter the array, there is also one separated questiontextformat:

          ...
          public 'questiontext' => 
              array (size=2)
                'text' => string '<span style="font-size:12pt">What's between orange and green in the spectrum?</span>' (length=84)
                'format' => string '1' (length=1)
            public 'questiontextformat' => string '1' (length=1)
          ...
          

          While importing other formats I get no arrays for questiontext:

          public 'single' => int 1
            public 'questiontext' => string '42 is the Absolute Answer to everything.' (length=40)
            public 'questiontextformat' => string '1' (length=1)
            public 'questiontextfiles' => 
              array (size=0)
                empty
          

          Then lesson/format.php, around #384 does:

          $newpage->contents = $question->questiontext;
          ...
          ...
          $newpageid = $DB->insert_record("lesson_pages", $newpage);
          

          that obviously breaks because $question->questiontext is an array.

          Curiously, it seems that the other places are prepared to get the array and spread the correct DB columns. But not for questiontext, where it's assumed to be one single text and not one array.

          So the question is... where should this be fixed?

          1) In the qformat as far as it should not be providing arrays for questiontext?
          2) In lesson, making it accept the 2 structures (single values and array) for questiontext.
          3) Or perhaps this is caused because of the text_field() method in your patch? And it needs to be modified to prevent the "arrayze" of questiontext ?

          Awaiting for some insight... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Uhm, this is not good, I've tried importing some formats (examvies, aiken... and everything went ok). The I've tried bb6 ones and I get some warnings from the DB layer about passing arrays in individual params. Tracing down the problem, it seems that all text fields (questiontext, feedbacktrue, answer...) are being returned as arrays containing (text, format and files). And looking to code, it seems that all BUT questiontext is prepared to read the information in that way. So, importing any of the bb6 fixtures, I get one array for questiontext (note no matter the array, there is also one separated questiontextformat: ... public 'questiontext' => array (size=2) 'text' => string '<span style= "font-size:12pt" >What's between orange and green in the spectrum?</span>' (length=84) 'format' => string '1' (length=1) public 'questiontextformat' => string '1' (length=1) ... While importing other formats I get no arrays for questiontext: public 'single' => int 1 public 'questiontext' => string '42 is the Absolute Answer to everything.' (length=40) public 'questiontextformat' => string '1' (length=1) public 'questiontextfiles' => array (size=0) empty Then lesson/format.php, around #384 does: $newpage->contents = $question->questiontext; ... ... $newpageid = $DB->insert_record( "lesson_pages" , $newpage); that obviously breaks because $question->questiontext is an array. Curiously, it seems that the other places are prepared to get the array and spread the correct DB columns. But not for questiontext, where it's assumed to be one single text and not one array. So the question is... where should this be fixed? 1) In the qformat as far as it should not be providing arrays for questiontext? 2) In lesson, making it accept the 2 structures (single values and array) for questiontext. 3) Or perhaps this is caused because of the text_field() method in your patch? And it needs to be modified to prevent the "arrayze" of questiontext ? Awaiting for some insight... ciao
          Hide
          Tim Hunt added a comment -

          That's interesting. I tested one BB6 file and it worked with no errors.

          I would say, with this change, lesson is much less broken than without this change. Therefore, my suggestion is:

          1. Integrate this change this week.

          2. File a new issue for next week, to deal with the remaining problems.

          Oh, the only problem is, next week is 2.3.2, etc., and I am now on holiday, so remaining issues will have to be dealt with by the lesson maintainer, or I will have to do it tonight. Hmm.

          Show
          Tim Hunt added a comment - That's interesting. I tested one BB6 file and it worked with no errors. I would say, with this change, lesson is much less broken than without this change. Therefore, my suggestion is: 1. Integrate this change this week. 2. File a new issue for next week, to deal with the remaining problems. Oh, the only problem is, next week is 2.3.2, etc., and I am now on holiday, so remaining issues will have to be dealt with by the lesson maintainer, or I will have to do it tonight. Hmm.
          Hide
          Jean-Michel Vedrine added a comment -

          Import using Blackboard V6+ has never (even in 1.9 !) worked when you try to import a zip file this is because the format blackboard_six need the real filename of the file being imported (to copy and unzip it) and the class in lesson don't provide this info contrary to the one in question.
          This has not changed with the recent changes. I have created MDL-34757 to deal with this problem, but I would not call it a regression as it has never worked !
          So maybe Tim's try was with a dat file (as the ones in fixtures) and Eloy's try with a zip file ?

          Show
          Jean-Michel Vedrine added a comment - Import using Blackboard V6+ has never (even in 1.9 !) worked when you try to import a zip file this is because the format blackboard_six need the real filename of the file being imported (to copy and unzip it) and the class in lesson don't provide this info contrary to the one in question. This has not changed with the recent changes. I have created MDL-34757 to deal with this problem, but I would not call it a regression as it has never worked ! So maybe Tim's try was with a dat file (as the ones in fixtures) and Eloy's try with a zip file ?
          Hide
          Jean-Michel Vedrine added a comment -

          The decision to provide an array for questiontext was done so that we can pass the itemid element when draft filearea were created during import of images (of course I could have used a separate var). Same change was done for generalfeedback. I don't know if lesson use generalfeedback.
          Why not implement something similar to the changes made in question/format.php ?

          Show
          Jean-Michel Vedrine added a comment - The decision to provide an array for questiontext was done so that we can pass the itemid element when draft filearea were created during import of images (of course I could have used a separate var). Same change was done for generalfeedback. I don't know if lesson use generalfeedback. Why not implement something similar to the changes made in question/format.php ?
          Hide
          Jean-Michel Vedrine added a comment -

          I must admit I don't really understand why questiontext and generalfeedback are not managed in the same way as other fields.

          Show
          Jean-Michel Vedrine added a comment - I must admit I don't really understand why questiontext and generalfeedback are not managed in the same way as other fields.
          Hide
          Jean-Michel Vedrine added a comment -

          Tim Advice is good : with his patch lesson is working better than it was when we started fixing question formats : examview and blackboard are now working. Only blackboard_six is broken but it was always broken since Moodle 2.0.
          Then Tim goes on holiday (I think he really deserve it !)
          And I will fix the problem with lesson and blackboard_six. After all it's my code which "broke" it
          Of course as Tim will be unavailable, somebody else will need to review my fix so that it can be integrated in time for 2.3.2 and 2.2.5
          But that seems doable.

          Show
          Jean-Michel Vedrine added a comment - Tim Advice is good : with his patch lesson is working better than it was when we started fixing question formats : examview and blackboard are now working. Only blackboard_six is broken but it was always broken since Moodle 2.0. Then Tim goes on holiday (I think he really deserve it !) And I will fix the problem with lesson and blackboard_six. After all it's my code which "broke" it Of course as Tim will be unavailable, somebody else will need to review my fix so that it can be integrated in time for 2.3.2 and 2.2.5 But that seems doable.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi,

          I tried the 2 .dat files within bb_six tests/fixtures, using bb6 as importer.

          Also note that it's relatively easy to solve the prroblem with a quick and dirty patch:

          diff --git a/mod/lesson/format.php b/mod/lesson/format.php
          index 4d8faf1..7711540 100644
          --- a/mod/lesson/format.php
          +++ b/mod/lesson/format.php
          @@ -384,7 +384,10 @@ class qformat_default {
                   $newpage->title = "Page $count";
               }
               $newpage->contents = $question->questiontext;
          -
          +    if (is_array($question->questiontext)) {
          +        $newpage->contents = isset($question->questiontext['text']) ? $question->questiontext['text'] : '';
          +        $newpage->contentsformat = isset($question->questiontext['format']) ? $question->questiontext['format'] : '
          +    }
               // set up page links
               if ($pageid) {
                   // the new page follows on from this page
          

          But I don't think it's the correct solution. IMO the lesson importer should be receiving constant structures. And this seems a nasty exception.

          And worse, I don't know which impact can have within qbanks importer, or how the hell that importer is able to handle both strings and arrays.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi, I tried the 2 .dat files within bb_six tests/fixtures, using bb6 as importer. Also note that it's relatively easy to solve the prroblem with a quick and dirty patch: diff --git a/mod/lesson/format.php b/mod/lesson/format.php index 4d8faf1..7711540 100644 --- a/mod/lesson/format.php +++ b/mod/lesson/format.php @@ -384,7 +384,10 @@ class qformat_default { $newpage->title = "Page $count" ; } $newpage->contents = $question->questiontext; - + if (is_array($question->questiontext)) { + $newpage->contents = isset($question->questiontext['text']) ? $question->questiontext['text'] : ''; + $newpage->contentsformat = isset($question->questiontext['format']) ? $question->questiontext['format'] : ' + } // set up page links if ($pageid) { // the new page follows on from this page But I don't think it's the correct solution. IMO the lesson importer should be receiving constant structures. And this seems a nasty exception. And worse, I don't know which impact can have within qbanks importer, or how the hell that importer is able to handle both strings and arrays.
          Hide
          Tim Hunt added a comment -

          Jean-Michel, the weird way that questiontext and generalfeedback are handled is a mistake. It got introduced in Moodle 2.0, when question code was converted to the file API at the last minute, and since it did work (albeit strangely) and time was short, I did not reject it when I peer-reviewed Dongsheng's work.

          Eloy. yes, formats should receive constant format, but that format had to change in Moodle 2.0 because of the file changes. At the same time, formats should be backwards compatible if possible. Therefore, qtype format base class tried to handle both new and old structure.

          +1 for you dirty patch in addition to my changes.

          Show
          Tim Hunt added a comment - Jean-Michel, the weird way that questiontext and generalfeedback are handled is a mistake. It got introduced in Moodle 2.0, when question code was converted to the file API at the last minute, and since it did work (albeit strangely) and time was short, I did not reject it when I peer-reviewed Dongsheng's work. Eloy. yes, formats should receive constant format, but that format had to change in Moodle 2.0 because of the file changes. At the same time, formats should be backwards compatible if possible. Therefore, qtype format base class tried to handle both new and old structure. +1 for you dirty patch in addition to my changes.
          Hide
          Jean-Michel Vedrine added a comment -

          I know this seems somewhat of a trick and I already said to Tim in MDL-25492 comments that I don't like the code change I did in question/format.php lines 427 - 442 but I don't know another method to permit

          • not to modify existing formats
          • to use draftfiles area to import files rather than base64 encoding them so that they can be imported like the xml format
          • not be forced to duplicate all the functions to import images just for questiontext and generalfeedback
            I think the real solution is a complete rework of the import code (see Tim's ideas in some comments about the recent fixed issue on question formats)
          Show
          Jean-Michel Vedrine added a comment - I know this seems somewhat of a trick and I already said to Tim in MDL-25492 comments that I don't like the code change I did in question/format.php lines 427 - 442 but I don't know another method to permit not to modify existing formats to use draftfiles area to import files rather than base64 encoding them so that they can be imported like the xml format not be forced to duplicate all the functions to import images just for questiontext and generalfeedback I think the real solution is a complete rework of the import code (see Tim's ideas in some comments about the recent fixed issue on question formats)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, so this is the extra commit I'm proposing (on top of Tim's one):

          diff --git a/mod/lesson/format.php b/mod/lesson/format.php
          index 4d8faf1..2ad15e4 100644
          --- a/mod/lesson/format.php
          +++ b/mod/lesson/format.php
          @@ -384,7 +384,15 @@ class qformat_default {
                $newpage->title = "Page $count";
            }
            $newpage->contents = $question->questiontext;
          -
          + $newpage->contentsformat = $question->questiontextformat;
          +
          + // Sometimes, questiontext is not a simple text, but one array
          + // containing both text and format, so we need to support here
          + // that case with the following dirty patch. MDL-35147, MDL-34757
          + if (is_array($question->questiontext)) {
          +     $newpage->contents = isset($question->questiontext['text']) ? $question->questiontext['text'] : '';
          +     $newpage->contentsformat = isset($question->questiontext['format']) ? $question->questiontext['format'] : 1;
          + }
            // set up page links
            if ($pageid) {
                // the new page follows on from this page
          

          Once I get your +1 I'll try here all the fixture examples, and if all them are imported without error I'll integrate and pass this.

          So surely that renders MDL-34757 unnecesary. Fell free to close it or reuse it aiming to get constant structures from all importers (that is how it should work, ideally).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, so this is the extra commit I'm proposing (on top of Tim's one): diff --git a/mod/lesson/format.php b/mod/lesson/format.php index 4d8faf1..2ad15e4 100644 --- a/mod/lesson/format.php +++ b/mod/lesson/format.php @@ -384,7 +384,15 @@ class qformat_default { $newpage->title = "Page $count" ; } $newpage->contents = $question->questiontext; - + $newpage->contentsformat = $question->questiontextformat; + + // Sometimes, questiontext is not a simple text, but one array + // containing both text and format, so we need to support here + // that case with the following dirty patch. MDL-35147, MDL-34757 + if (is_array($question->questiontext)) { + $newpage->contents = isset($question->questiontext['text']) ? $question->questiontext['text'] : ''; + $newpage->contentsformat = isset($question->questiontext['format']) ? $question->questiontext['format'] : 1; + } // set up page links if ($pageid) { // the new page follows on from this page Once I get your +1 I'll try here all the fixture examples, and if all them are imported without error I'll integrate and pass this. So surely that renders MDL-34757 unnecesary. Fell free to close it or reuse it aiming to get constant structures from all importers (that is how it should work, ideally). Ciao
          Hide
          Tim Hunt added a comment -

          Is it safe to do $newpage->contentsformat outside the if?

          Inside the if, I don't like the hard-coded 1 for the format. Surely that should be FORMAT_MOODLE or FOMRAT_HTML.

          Apart from that +1.

          Show
          Tim Hunt added a comment - Is it safe to do $newpage->contentsformat outside the if? Inside the if, I don't like the hard-coded 1 for the format. Surely that should be FORMAT_MOODLE or FOMRAT_HTML. Apart from that +1.
          Hide
          Jean-Michel Vedrine added a comment -

          I don't think MDL-34757 should be closed: since years ago blackboard_six was conceived to import both .dat and .zip files. This is because "Blackboard files" are not all of the same types !
          But lesson can't import .zip files.
          Try to import a .zip file mentioned in MDL25492 testing instructions both in the question bank and in lesson :

          • in the question bank it works
          • in lesson it doesn't work.
            Beside this the fix is easy :just a few lines to add to lesson import code so that realfilename is known from the import format.
          Show
          Jean-Michel Vedrine added a comment - I don't think MDL-34757 should be closed: since years ago blackboard_six was conceived to import both .dat and .zip files. This is because "Blackboard files" are not all of the same types ! But lesson can't import .zip files. Try to import a .zip file mentioned in MDL25492 testing instructions both in the question bank and in lesson : in the question bank it works in lesson it doesn't work. Beside this the fix is easy :just a few lines to add to lesson import code so that realfilename is known from the import format.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          If you see http://tracker.moodle.org/browse/MDL-35147?focusedCommentId=176141&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-176141 ... you'll notice that $question->questiontextformat comes in the structure from importer too, no matter of the array-ized information. So all it does is to, by default load both questiontext and questiontextformat and later, if we have one array, override those values.

          Yes 1 is wrong, better defined FORMAT_HTML. The same used by text_field().

          Aha, oki, Jean-Michel, I'll leave MDL-34757 open.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - If you see http://tracker.moodle.org/browse/MDL-35147?focusedCommentId=176141&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-176141 ... you'll notice that $question->questiontextformat comes in the structure from importer too, no matter of the array-ized information. So all it does is to, by default load both questiontext and questiontextformat and later, if we have one array, override those values. Yes 1 is wrong, better defined FORMAT_HTML. The same used by text_field(). Aha, oki, Jean-Michel, I'll leave MDL-34757 open. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 & master), thanks!

          In all branches I've added the dirty hack to support both single and array questiontext.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks! In all branches I've added the dirty hack to support both single and array questiontext.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tested: aiken, gift, examview, bb & bb6. Under the 3 branches. Process ended ok.

          Show
          Eloy Lafuente (stronk7) added a comment - Tested: aiken, gift, examview, bb & bb6. Under the 3 branches. Process ended ok.
          Hide
          Tim Hunt added a comment -

          Thanks Eloy.

          Show
          Tim Hunt added a comment - Thanks Eloy.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm so proud...of you, many thanks!

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao
          Hide
          Jean-Michel Vedrine added a comment -

          @Eloy: Thanks
          @Tim: Thanks and happy holidays
          @Tim & Eloy: I think I see a way so that both question/format.php and mod/lesson/format.php receive constant structures from all importers including bb6 one (only with minimal changes).
          I will write and test some code and open a tracker issue if it works.

          Show
          Jean-Michel Vedrine added a comment - @Eloy: Thanks @Tim: Thanks and happy holidays @Tim & Eloy: I think I see a way so that both question/format.php and mod/lesson/format.php receive constant structures from all importers including bb6 one (only with minimal changes). I will write and test some code and open a tracker issue if it works.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Super, Jean-Michel!

          Show
          Eloy Lafuente (stronk7) added a comment - Super, Jean-Michel!
          Hide
          Tim Barker added a comment -

          Removed qa_test_required as we already have tests that cover this scenario.

          Show
          Tim Barker added a comment - Removed qa_test_required as we already have tests that cover this scenario.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: