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

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

    Details

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

      Description

      This is because question import/export bypasses save_question.

        Gliffy Diagrams

          Activity

          Hide
          timhunt 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
          timhunt 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
          ppichet 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
          ppichet 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
          timhunt 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
          timhunt 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
          ppichet 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
          ppichet 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
          howardsmiller Howard Miller added a comment -

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

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

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

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

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

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

          I know the feeling. Don't worry.

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

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

          Show
          howardsmiller Howard Miller added a comment - I'm probably just being stupid, but I really don't understand the problem - the 'length' of what?
          Hide
          ppichet 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
          ppichet 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
          howardsmiller 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
          howardsmiller 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
          ppichet 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
          ppichet 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
          timhunt 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
          timhunt 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
          ppichet 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
          ppichet 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
          ppichet 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
          ppichet 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
          howardsmiller 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
          howardsmiller 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
          timhunt 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
          timhunt 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
          howardsmiller 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
          howardsmiller 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
          timhunt 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
          timhunt 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
          howardsmiller Howard Miller added a comment -

          Cool - I'll sort it

          Show
          howardsmiller Howard Miller added a comment - Cool - I'll sort it
          Hide
          howardsmiller 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
          howardsmiller 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
          stronk7 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
          stronk7 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:
                Fix Release Date:
                15/May/08