Moodle
  1. Moodle
  2. MDL-32464

Multianswer question import format is broken in Moodle 2.2 and master

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      39348

      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 !!

        Activity

        Hide
        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
        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
        Pierre Pichet added a comment -

        However the bug appears as described in 2,2.

        Show
        Pierre Pichet added a comment - However the bug appears as described in 2,2.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Rajesh Taneja added a comment -

        Works Great,

        Thanks for fixing this Tim.

        Show
        Rajesh Taneja added a comment - Works Great, Thanks for fixing this Tim.
        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

          People

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

            Dates

            • Created:
              Updated:
              Resolved: