Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32464

Multianswer question import format is broken in Moodle 2.2 and master

    Details

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

      1. Try to import the file that you can find at question/format/multianswer/tests/fixtures/questions.multianswer.txt in the code-base. There should be no errors.

      2. Preview the question (it is the Match the following cities with the correct state... one from http://docs.moodle.org/23/en/Embedded_Answers_%28Cloze%29_question_type). Make sure it works properly.

      Show
      1. Try to import the file that you can find at question/format/multianswer/tests/fixtures/questions.multianswer.txt in the code-base. There should be no errors. 2. Preview the question (it is the Match the following cities with the correct state... one from http://docs.moodle.org/23/en/Embedded_Answers_%28Cloze%29_question_type ). Make sure it works properly.
    • 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:

      Description

      When you try to import a multianswer question using the multianswer format (not gift format, multianswer format) you get :
      Fatal error: Call to undefined function qtype_multianswer_extract_question() in xxxxxxxxxx\question\format\multianswer\format.php
      This is because the protected function readquestions($lines) { is missing a
      question_bank::get_qtype('multianswer');
      Apparently this import format is rarely used because nobody has reported that it is broken for months !!

        Gliffy Diagrams

          Activity

          Hide
          ppichet Pierre Pichet added a comment -

          I exported a cloze question using moodle xml format and reimported in master and did not have any problem.
          Using cloze specific format,things were OK but this format does not handle multilines cloze question.

          Show
          ppichet Pierre Pichet added a comment - I exported a cloze question using moodle xml format and reimported in master and did not have any problem. Using cloze specific format,things were OK but this format does not handle multilines cloze question.
          Hide
          ppichet Pierre Pichet added a comment -

          However the bug appears as described in 2,2.

          Show
          ppichet Pierre Pichet added a comment - However the bug appears as described in 2,2.
          Hide
          ppichet Pierre Pichet added a comment -

          Tim,
          On further tests the Cloze import does not work on master.
          I probably use the gift import
          I obtain (as in 2,2)
          Fatal error: Call to undefined function qtype_multianswer_extract_question() in D:\moodle2\server\moodle\M22\moodle\question\format\multianswer\format.php on line 53

          So I add
          require_once($CFG->dirroot . '/question/type/multianswer/questiontype.php'); in multianswer/format.php
          Questions import correctly

          As function qtype_multianswer_extract_question() is not a function internal to multianswer class but
          a static function inside the multianswer/questiontype.php file, do you think that this is the best way to solve this bug ?

          If so, I will prepare the gits.

          Show
          ppichet Pierre Pichet added a comment - Tim, On further tests the Cloze import does not work on master. I probably use the gift import I obtain (as in 2,2) Fatal error: Call to undefined function qtype_multianswer_extract_question() in D:\moodle2\server\moodle\M22\moodle\question\format\multianswer\format.php on line 53 So I add require_once($CFG->dirroot . '/question/type/multianswer/questiontype.php'); in multianswer/format.php Questions import correctly As function qtype_multianswer_extract_question() is not a function internal to multianswer class but a static function inside the multianswer/questiontype.php file, do you think that this is the best way to solve this bug ? If so, I will prepare the gits.
          Hide
          ppichet Pierre Pichet added a comment -

          By the way

          Strict Standards: Only variables should be passed by reference in D:\moodle2\server\moodle\moodle_v\question\type\multianswer\renderer.php on line 282

          $rightanswer = $subq->answers[$order[reset($subq->get_correct_response())]];

          What is your solution to this ?

          Show
          ppichet Pierre Pichet added a comment - By the way Strict Standards: Only variables should be passed by reference in D:\moodle2\server\moodle\moodle_v\question\type\multianswer\renderer.php on line 282 $rightanswer = $subq->answers[$order [reset($subq->get_correct_response())] ]; What is your solution to this ?
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Pierre,
          rather than adding require_once($CFG->dirroot . '/question/type/multianswer/questiontype.php');
          at the begining, you may consider adding
          question_bank::get_qtype('multianswer');
          in the readquestions method
          If you study the get_qtype method you will see it will have the desired effect and this is how it is done in the xml import format.
          But this is just my understanding so it's better to wait for Tim comment.

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Pierre, rather than adding require_once($CFG->dirroot . '/question/type/multianswer/questiontype.php'); at the begining, you may consider adding question_bank::get_qtype('multianswer'); in the readquestions method If you study the get_qtype method you will see it will have the desired effect and this is how it is done in the xml import format. But this is just my understanding so it's better to wait for Tim comment.
          Hide
          ppichet Pierre Pichet added a comment -

          However in the moodle xml this is done in

          public function import_multianswer($question) {
          question_bank::get_qtype('multianswer');

          not in the readquestions() function.

          But the code flow is different as multianswer import format is a unique question import.
          This could explain why it has not be used often.

          I just worry that using the more general question_bank::get_qtype will as a side effect by loading multianswer/questiontype.php, load the desired qtype_multianswer_extract_question function.

          But effectively the two solutions are equivalent and the question_bank::get_qtype() respect the new more general syntax.

          Show
          ppichet Pierre Pichet added a comment - However in the moodle xml this is done in public function import_multianswer($question) { question_bank::get_qtype('multianswer'); not in the readquestions() function. But the code flow is different as multianswer import format is a unique question import. This could explain why it has not be used often. I just worry that using the more general question_bank::get_qtype will as a side effect by loading multianswer/questiontype.php, load the desired qtype_multianswer_extract_question function. But effectively the two solutions are equivalent and the question_bank::get_qtype() respect the new more general syntax.
          Hide
          timhunt Tim Hunt added a comment -

          The fix was easy. Making a unit test took a little longer.

          Jean-Michel, are you able to peer-review this?

          Show
          timhunt Tim Hunt added a comment - The fix was easy. Making a unit test took a little longer. Jean-Michel, are you able to peer-review this?
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Hello Tim,
          I will be glad to peer-review.

          • I spotted a typo in the comment on line 43.
          • Is there a norm for the number of spaces between @subpackage, @copyright, @license and the following word ? It seems to vary from file to file in Moodle.
          • I am always unsure if FORMAT_MOODLE is a good choice for questiontext and generalfeedback. Of course it's the most compatible with previous behaviour, but users don't get the editor when they try to edit the question and they don't always know the trick (change, save, re-open).
          • Unless I miss something the changes to the questiontype regex seems unrelated to this issue.
            Has it something to do with other problems in multianswer qtype ?
            I am unsure if peer reviewing include trying the code myself, runing codechecker or phpunit tests ? if yes I will only be able to do it in a few hours as I am away form my development computer and posting this from another laptop.
          Show
          jmvedrine Jean-Michel Vedrine added a comment - Hello Tim, I will be glad to peer-review. I spotted a typo in the comment on line 43. Is there a norm for the number of spaces between @subpackage, @copyright, @license and the following word ? It seems to vary from file to file in Moodle. I am always unsure if FORMAT_MOODLE is a good choice for questiontext and generalfeedback. Of course it's the most compatible with previous behaviour, but users don't get the editor when they try to edit the question and they don't always know the trick (change, save, re-open). Unless I miss something the changes to the questiontype regex seems unrelated to this issue. Has it something to do with other problems in multianswer qtype ? I am unsure if peer reviewing include trying the code myself, runing codechecker or phpunit tests ? if yes I will only be able to do it in a few hours as I am away form my development computer and posting this from another laptop.
          Hide
          timhunt Tim Hunt added a comment -

          1. Oops! fixed.

          2. I like to make them line up like this, but that is personal preference.

          3. I think it is a good choice for import formats that are close to plain text.

          4. Actually, that was essential to make the unit tests pass. Note how, in the test input, the shortanswer question is split over several lines.

          Also, we should not have the tests subfolder in Moodle 2.2.

          Show
          timhunt Tim Hunt added a comment - 1. Oops! fixed. 2. I like to make them line up like this, but that is personal preference. 3. I think it is a good choice for import formats that are close to plain text. 4. Actually, that was essential to make the unit tests pass. Note how, in the test input, the shortanswer question is split over several lines. Also, we should not have the tests subfolder in Moodle 2.2.
          Hide
          jmvedrine Jean-Michel Vedrine added a comment -

          Oops I missed the test subfolder in Moodle 2.2
          Also I missed the problem of shortanswer split over several lines reading too quickly the sample question, so made wrong deductions not seeng why the s option could be used for.
          Not very good for a first peer-review. Sorry !

          Show
          jmvedrine Jean-Michel Vedrine added a comment - Oops I missed the test subfolder in Moodle 2.2 Also I missed the problem of shortanswer split over several lines reading too quickly the sample question, so made wrong deductions not seeng why the s option could be used for. Not very good for a first peer-review. Sorry !
          Hide
          nebgor Aparup Banerjee added a comment -

          Thanks for the patches and phpunit tests Tim
          Integrated into 23 and master. The fix is in 22 also.

          jftr Jean-Michel Vedrine, that was a very proper peer-review imo . It brought about the thoughts and concerns that were needed.

          Show
          nebgor Aparup Banerjee added a comment - Thanks for the patches and phpunit tests Tim Integrated into 23 and master. The fix is in 22 also. jftr Jean-Michel Vedrine, that was a very proper peer-review imo . It brought about the thoughts and concerns that were needed.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Works Great,

          Thanks for fixing this Tim.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Works Great, Thanks for fixing this Tim.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

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

          http://youtu.be/n64CdfDRnZY

          Closing as fixed, ciao

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Sep/12