Moodle
  1. Moodle
  2. MDL-14406

Importing description items sets their 'length' to 1, not 0

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.6, 1.7.4, 1.8.5, 1.9
    • Fix Version/s: 1.9.1, 2.0
    • Component/s: Questions
    • Labels:
      None
    • Affected Branches:
      MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      31026

      Description

      This is because question import/export bypasses save_question.

        Activity

        Hide
        Tim Hunt added a comment -

        I think this patch fixes the problem in all branches 1.6->head. Howard, I would be really grateful if you could review this please, then I will check it in.

        Show
        Tim Hunt added a comment - I think this patch fixes the problem in all branches 1.6->head. Howard, I would be really grateful if you could review this please, then I will check it in.
        Hide
        Pierre Pichet added a comment -

        Sorry I don't understand your patch on weblib.php.

        "This is because question import/export bypasses save_question."
        On the long term this should be changed.
        A somewhat old suggestion...
        http://docs.moodle.org/en/index.php?title=Development:Question_import/export_formats&oldid=12134#Importing_or_exporting_questions

        Show
        Pierre Pichet added a comment - Sorry I don't understand your patch on weblib.php. "This is because question import/export bypasses save_question." On the long term this should be changed. A somewhat old suggestion... http://docs.moodle.org/en/index.php?title=Development:Question_import/export_formats&oldid=12134#Importing_or_exporting_questions
        Hide
        Tim Hunt added a comment -

        The change in weblib are something else that only got included in the patch by mistake. Ignore them.

        I know we need a long-term solution to this eventually, but for now I just want to fix this bug.

        Show
        Tim Hunt added a comment - The change in weblib are something else that only got included in the patch by mistake. Ignore them. I know we need a long-term solution to this eventually, but for now I just want to fix this bug.
        Hide
        Pierre Pichet added a comment -

        At first sight the patch are Ok.
        Should find time to test them on various versions in the weekend.

        Show
        Pierre Pichet added a comment - At first sight the patch are Ok. Should find time to test them on various versions in the weekend.
        Hide
        Howard Miller added a comment -

        Just back in the country - I'll have a look at this asap.

        Show
        Howard Miller added a comment - Just back in the country - I'll have a look at this asap.
        Hide
        Tim Hunt added a comment -

        Have you had time to look at this yet Howard? Thanks.

        Show
        Tim Hunt added a comment - Have you had time to look at this yet Howard? Thanks.
        Hide
        Howard Miller added a comment -

        Getting to it Sorry, a weeks holiday just translates as two weeks work when you get back. Sigh!!

        Show
        Howard Miller added a comment - Getting to it Sorry, a weeks holiday just translates as two weeks work when you get back. Sigh!!
        Hide
        Tim Hunt added a comment -

        I know the feeling. Don't worry.

        Show
        Tim Hunt added a comment - I know the feeling. Don't worry.
        Hide
        Howard Miller added a comment -

        I'm probably just being stupid, but I really don't understand the problem - the 'length' of what?

        Show
        Howard Miller added a comment - I'm probably just being stupid, but I really don't understand the problem - the 'length' of what?
        Hide
        Pierre Pichet added a comment -

        Surely not stupid.
        This is the question->length normally set to 1 for most question types but 0 for description as this is not really a question.

        Show
        Pierre Pichet added a comment - Surely not stupid. This is the question->length normally set to 1 for most question types but 0 for description as this is not really a question.
        Hide
        Howard Miller added a comment -

        I can't remember what question->length does. I'll take your word for it though. The fix looks fine to me if that is the case.

        BUT... I think there's some extra stuff (not related to this patch) about weblib in the patch file in that case.

        Show
        Howard Miller added a comment - I can't remember what question->length does. I'll take your word for it though. The fix looks fine to me if that is the case. BUT... I think there's some extra stuff (not related to this patch) about weblib in the patch file in that case.
        Hide
        Pierre Pichet added a comment -

        from question/type/questiontype.php
        /**
        501 * Returns the number of question numbers which are used by the question
        502 *
        503 * This function returns the number of question numbers to be assigned
        504 * to the question. Most question types will have length one; they will be
        505 * assigned one number. The 'description' type, however does not use up a
        506 * number and so has a length of zero. Other question types may wish to
        507 * handle a bundle of questions and hence return a number greater than one.
        508 * @return integer The number of question numbers which should be
        509 * assigned to the question.
        510 * @param object $question The question whose length is to be determined.
        511 * Question type specific information is included.
        512 */
        513 function actual_number_of_questions($question)

        { 514 // By default, each question is given one number 515 return 1; 516 }

        also
        function save_question($question, $form, $course) {
        233 global $USER;
        234 // This default implementation is suitable for most
        235 // question types.
        236
        237 // First, save the basic question itself
        238 $question->name = trim($form->name);
        239 $question->questiontext = trim($form->questiontext);
        240 $question->questiontextformat = $form->questiontextformat;
        241 $question->parent = isset($form->parent)? $form->parent : 0;
        242 $question->length = $this->actual_number_of_questions($question);
        243 $question->penalty = isset($form->penalty) ? $form->penalty : 0;
        244
        this is used in attempt.php to number the questions
        /// Print all the questions
        464 $number = quiz_first_questionnumber($attempt->layout, $pagelist);
        465 foreach ($pagequestions as $i)

        { 466 $options = quiz_get_renderoptions($quiz->review, $states[$i]); 467 // Print the question 468 print_question($questions[$i], $states[$i], $number, $quiz, $options); 469 save_question_session($questions[$i], $states[$i]); 470 $number += $questions[$i]->length; 471 }

        472

        "I think there's some extra stuff (not related to this patch) about weblib in the patch file in that case."
        Tim commented this in the upper part
        "The change in weblib are something else that only got included in the patch by mistake. Ignore them."

        Show
        Pierre Pichet added a comment - from question/type/questiontype.php /** 501 * Returns the number of question numbers which are used by the question 502 * 503 * This function returns the number of question numbers to be assigned 504 * to the question. Most question types will have length one; they will be 505 * assigned one number. The 'description' type, however does not use up a 506 * number and so has a length of zero. Other question types may wish to 507 * handle a bundle of questions and hence return a number greater than one. 508 * @return integer The number of question numbers which should be 509 * assigned to the question. 510 * @param object $question The question whose length is to be determined. 511 * Question type specific information is included. 512 */ 513 function actual_number_of_questions($question) { 514 // By default, each question is given one number 515 return 1; 516 } also function save_question($question, $form, $course) { 233 global $USER; 234 // This default implementation is suitable for most 235 // question types. 236 237 // First, save the basic question itself 238 $question->name = trim($form->name); 239 $question->questiontext = trim($form->questiontext); 240 $question->questiontextformat = $form->questiontextformat; 241 $question->parent = isset($form->parent)? $form->parent : 0; 242 $question->length = $this->actual_number_of_questions($question); 243 $question->penalty = isset($form->penalty) ? $form->penalty : 0; 244 this is used in attempt.php to number the questions /// Print all the questions 464 $number = quiz_first_questionnumber($attempt->layout, $pagelist); 465 foreach ($pagequestions as $i) { 466 $options = quiz_get_renderoptions($quiz->review, $states[$i]); 467 // Print the question 468 print_question($questions[$i], $states[$i], $number, $quiz, $options); 469 save_question_session($questions[$i], $states[$i]); 470 $number += $questions[$i]->length; 471 } 472 "I think there's some extra stuff (not related to this patch) about weblib in the patch file in that case." Tim commented this in the upper part "The change in weblib are something else that only got included in the patch by mistake. Ignore them."
        Hide
        Tim Hunt added a comment -

        The symptom we were seeing that lead me to make this change is that descriptions were getting question number, or at least, if you had question, description, question, then the questions would be numbered 1 and 3, if the description had been imported.

        Show
        Tim Hunt added a comment - The symptom we were seeing that lead me to make this change is that descriptions were getting question number, or at least, if you had question, description, question, then the questions would be numbered 1 and 3, if the description had been imported.
        Hide
        Pierre Pichet added a comment -

        Looking to the format code
        function defaultquestion()

        { global $CFG; $question = new stdClass(); $question->shuffleanswers = $CFG->quiz_shuffleanswers; $question->defaultgrade = 1; $question->image = ""; $question->usecase = 0; $question->multiplier = array(); $question->generalfeedback = ''; $question->correctfeedback = ''; $question->partiallycorrectfeedback = ''; $question->incorrectfeedback = ''; $question->answernumbering = 'abc'; $question->penalty = 0.1; // this option in case the questiontypes class wants // to know where the data came from $question->export_process = true; $question->import_process = true; return $question; }

        There is no default value for question->length.
        Should we set it to 1 and change for description as coded by Tim.
        I have to check if question->length has a default value of 1 in the database table.

        Show
        Pierre Pichet added a comment - Looking to the format code function defaultquestion() { global $CFG; $question = new stdClass(); $question->shuffleanswers = $CFG->quiz_shuffleanswers; $question->defaultgrade = 1; $question->image = ""; $question->usecase = 0; $question->multiplier = array(); $question->generalfeedback = ''; $question->correctfeedback = ''; $question->partiallycorrectfeedback = ''; $question->incorrectfeedback = ''; $question->answernumbering = 'abc'; $question->penalty = 0.1; // this option in case the questiontypes class wants // to know where the data came from $question->export_process = true; $question->import_process = true; return $question; } There is no default value for question->length. Should we set it to 1 and change for description as coded by Tim. I have to check if question->length has a default value of 1 in the database table.
        Hide
        Pierre Pichet added a comment -

        from the docs
        http://docs.moodle.org/en/Quiz_database_structure
        question
        length
        int(10) unsigned NOT NULL default '1',
        This defines how many question numbers are required for this question. It is generally set to "1", but the description questiontype, for example, sets it to "0", reflecting the fact that it doesn't have a question number.

        Show
        Pierre Pichet added a comment - from the docs http://docs.moodle.org/en/Quiz_database_structure question length int(10) unsigned NOT NULL default '1', This defines how many question numbers are required for this question. It is generally set to "1", but the description questiontype, for example, sets it to "0", reflecting the fact that it doesn't have a question number.
        Hide
        Howard Miller added a comment -

        that would be my vote then, put it in defaultquestion so it is good for all QTs. Now and future.

        Show
        Howard Miller added a comment - that would be my vote then, put it in defaultquestion so it is good for all QTs. Now and future.
        Hide
        Tim Hunt added a comment -

        Sorry, Howard, from that last remark, I don't understand whether you are expecting me to make some sort of change to my patch, or whether you are volunteering to do something.

        Perhaps I just did not get enough sleep last night - any change you could spell out your suggestion in words of one syllable

        Show
        Tim Hunt added a comment - Sorry, Howard, from that last remark, I don't understand whether you are expecting me to make some sort of change to my patch, or whether you are volunteering to do something. Perhaps I just did not get enough sleep last night - any change you could spell out your suggestion in words of one syllable
        Hide
        Howard Miller added a comment -

        Yes - unless I am missing something, can the default value of length go into the defualtvalues() function in format.php and that's it? That function should have default values for all possible fields in it in case the import format doesn't set it explicitly.

        I'll do it, sure

        Show
        Howard Miller added a comment - Yes - unless I am missing something, can the default value of length go into the defualtvalues() function in format.php and that's it? That function should have default values for all possible fields in it in case the import format doesn't set it explicitly. I'll do it, sure
        Hide
        Tim Hunt added a comment -

        Right, so we need a default value (1) in that function for most question types, but then we also need my patch which sets it to 0 for descriptions.

        Good. When you do your bit, will you check in the changes from my patch too? or would you rather I did that?

        Show
        Tim Hunt added a comment - Right, so we need a default value (1) in that function for most question types, but then we also need my patch which sets it to 0 for descriptions. Good. When you do your bit, will you check in the changes from my patch too? or would you rather I did that?
        Hide
        Howard Miller added a comment -

        Cool - I'll sort it

        Show
        Howard Miller added a comment - Cool - I'll sort it
        Hide
        Howard Miller added a comment -

        Added default value (1) for length field to defaultvalues() in format.php and then value of 0 to length field for description question type. This only applies to xml and gift import/export types.

        Show
        Howard Miller added a comment - Added default value (1) for length field to defaultvalues() in format.php and then value of 0 to length field for description question type. This only applies to xml and gift import/export types.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Changes in CVS look ok, so I'm closing this, guessing:

        1) It only has been applied to gift and xml formats because only those formats support description qtypes.
        2) In the commit to 19_STABLE, one new function has appeared: xmltostring() . That' isn't a problem.

        If any of my guesses are wrong... feel free to reopen and fix. TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Changes in CVS look ok, so I'm closing this, guessing: 1) It only has been applied to gift and xml formats because only those formats support description qtypes. 2) In the commit to 19_STABLE, one new function has appeared: xmltostring() . That' isn't a problem. If any of my guesses are wrong... feel free to reopen and fix. TIA and ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: