Question import into lesson is broken

I'm fixing it.

Thanks Tim,
I completely forgot about lesson !!

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

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

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.

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 ?

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 ?

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

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.

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.

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.

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)

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

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.

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.

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

Integrated (22, 23 & master), thanks!

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

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

Thanks Eloy.

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

http://youtu.be/n64CdfDRnZY

Closing as fixed, ciao

@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.

Super, Jean-Michel!

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