Moodle
  1. Moodle
  2. MDL-29644

penalty and Hints donot work with XML export for Cloze questions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      1. Create a cloze question with Penalty and Hints
      2. Export the question as Moodle XML and verify if it has "<hint>" tags.
      3. Import and verify the imported question is an accurate copy of the exported one.

      4. Also run all the unit tests in /question.

      Show
      1. Create a cloze question with Penalty and Hints 2. Export the question as Moodle XML and verify if it has "<hint>" tags. 3. Import and verify the imported question is an accurate copy of the exported one. 4. Also run all the unit tests in /question.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      19481

      Description

      refer MDL-29216 for details.
      Create a cloze question with Penalty and Hints
      Try to export the question as MOODLE XML.
      Exported file doesn't contain any <hint> tags.

        Issue Links

          Activity

          Hide
          Pierre Pichet added a comment - - edited

          The problem is located in the format.php file
          the froeach was modifying the main $question
          foreach ($question->options->questions as $question) {
          replacing by something else like $subquestion solve the problem
          format.php line 1240

                      case 'multianswer':                                                        
                          $acount = 1;                                                           
                          foreach ($question->options->questions as $subquestion) {              
                              $thispattern = "{#".$acount."}";                                   
                              $thisreplace = $subquestion->questiontext;                         
                              $expout = preg_replace("~$thispattern~", $thisreplace, $expout);   
                              $acount++;                                                         
                          }                                                                      
                          break;                                                                 
          
          Show
          Pierre Pichet added a comment - - edited The problem is located in the format.php file the froeach was modifying the main $question foreach ($question->options->questions as $question) { replacing by something else like $subquestion solve the problem format.php line 1240 case 'multianswer': $acount = 1; foreach ($question->options->questions as $subquestion) { $thispattern = "{#".$acount."}"; $thisreplace = $subquestion->questiontext; $expout = preg_replace("~$thispattern~", $thisreplace, $expout); $acount++; } break;
          Hide
          Tim Hunt added a comment -

          Now ready to integrate.

          Show
          Tim Hunt added a comment - Now ready to integrate.
          Hide
          Tim Hunt added a comment -

          Pierre was right about the basic problem, but it was important to also create some unit tests so that we can easily verify that this is still working in future.

          Show
          Tim Hunt added a comment - Pierre was right about the basic problem, but it was important to also create some unit tests so that we can easily verify that this is still working in future.
          Hide
          Pierre Pichet added a comment - - edited

          Commit
          https://github.com/ppichet/moodle/commit/292819c3441f14002f1131435415a866e0d4f27a

          tested on import-export and hints are saved.

          Oups I am somehow late (lunch time...)

          Show
          Pierre Pichet added a comment - - edited Commit https://github.com/ppichet/moodle/commit/292819c3441f14002f1131435415a866e0d4f27a tested on import-export and hints are saved. Oups I am somehow late (lunch time...)
          Hide
          Pierre Pichet added a comment -

          "but it was important to also create some unit tests so that we can easily verify that this is still working in future."

          I agree completly to your statement and these tests that you take great care to build will
          effectively play an important role in Moodle actual and future stability.

          Show
          Pierre Pichet added a comment - "but it was important to also create some unit tests so that we can easily verify that this is still working in future." I agree completly to your statement and these tests that you take great care to build will effectively play an important role in Moodle actual and future stability.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Was going to integrate this when I detected one Q with penalty = 0.5 in xml being imported as penalty = 0.333 (in edit Q page).

          Show
          Eloy Lafuente (stronk7) added a comment - Was going to integrate this when I detected one Q with penalty = 0.5 in xml being imported as penalty = 0.333 (in edit Q page).
          Hide
          Tim Hunt added a comment -

          OK, commit amended so that:
          1. The unit test uses a non-default value of penalty,
          2. and then actually checks the imported value.
          3. Then the code was fixed so that it actually works.

          Hopefully this can be integrated now.

          Show
          Tim Hunt added a comment - OK, commit amended so that: 1. The unit test uses a non-default value of penalty, 2. and then actually checks the imported value. 3. Then the code was fixed so that it actually works. Hopefully this can be integrated now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          While reviewing and integrating export / import and unittests have been checked, so passing this. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - While reviewing and integrating export / import and unittests have been checked, so passing this. Thanks!
          Hide
          Pierre Pichet added a comment - - edited

          Tim
          Just as a proof that your work to build "some unit tests so that we can easily verify that this is still working in future" is the thing to do.

          I look at the history of those code lines.
          This brink us back to
          http://cvs.moodle.org/moodle/question/format/xml/format.php?revision=1.5.2.3&view=markup
          where the cloze case was added

          case MATCH:
                      foreach($question->options->subquestions as $subquestion) {
                          $expout .= "<subquestion>\n";
                          $expout .= $this->writetext( $subquestion->questiontext );
                          $expout .= "<answer>".$this->writetext( $subquestion->answertext )."</answer>\n";
                          $expout .= "</subquestion>\n";
                      }
                      break;
                  case DESCRIPTION:
                      // nothing more to do for theis type
                      break;
                  case MULTIANSWER:
                      $a_count=1;
                      foreach($question->options->questions as $question) {
                          $thispattern = addslashes("{#".$a_count."}");
                          $thisreplace = $question->questiontext;
                          $expout=ereg_replace($thispattern, $thisreplace, $expout );
                          $a_count++;
                      }
                  break;
                  default:
                      $expout .= "<!-- Question type is unknown or not supported (Type=$question->qtype) -->\n";
                  }
          
                  // close the question tag
                  $expout .= "</question>\n";
          
          

          The problem was not seen as the cloze code was the last questiontype before closing the output.
          The irony is that the preceeding case i.e. MATCH was correctly written.

          This bring us back to
          Revision 1.5.2.3 - (view) (download) (annotate) - [select for diffs]
          Fri Jun 2 17:44:13 2006 WST (5 years, 4 months ago) by thepurpleblob
          Changes since 1.5.2.2: +113 -18 lines
          Diff to previous 1.5.2.2 , to branch point 1.5
          Added CLOZE question support.
          Thanks to Joseph Rezeau for this one!

          However as things got more complex, new paramaters were added to all questions.
          The first were tags and they were added at the output end
          So

          default:
                      // try support by optional plugin
                      if (!$data = $this->try_exporting_using_qtypes( $question->qtype, $question )) {
                          echo $OUTPUT->notification( get_string( 'unsupportedexport','qformat_xml',$QTYPES[$question->qtype]->local_name() ) );
                      }
                      $expout .= $data;
                  }
          
                  // Write the question tags.
                  if (!empty($CFG->usetags)) {
                      require_once($CFG->dirroot.'/tag/lib.php');
                      $tags = tag_get_tags_array('question', $question->id);
                      if (!empty($tags)) {
                          $expout .= "    <tags>\n";
                          foreach ($tags as $tag) {
                              $expout .= "      <tag>" . $this->writetext($tag, 0, true) . "</tag>\n";
                          }
                          $expout .= "    </tags>\n";
                      }
                  }
          
                  // close the question tag
                  $expout .= "</question>\n";
          
                  return $expout;
              }
          

          you know well the author

          Revision 1.74 - (view) (download) (annotate) - [select for diffs]
          Fri Nov 19 20:33:48 2010 WST (10 months, 2 weeks ago) by tjhunt
          Branch: MAIN
          Changes since 1.73: +30 -11 lines
          Diff to previous 1.73
          question XML import/export MDL-18916 should include question tags.

          This go unnoticed as few peoples use tags for cloze if at all.
          Adding hints to cloze create the bug.

          The real lesson is that Moodle code is growing in complexity and that your test policy
          is the good thing to do.
          This policy is another aspect of your professionalism as a computer code expert.
          So continue

          Show
          Pierre Pichet added a comment - - edited Tim Just as a proof that your work to build "some unit tests so that we can easily verify that this is still working in future" is the thing to do. I look at the history of those code lines. This brink us back to http://cvs.moodle.org/moodle/question/format/xml/format.php?revision=1.5.2.3&view=markup where the cloze case was added case MATCH: foreach($question->options->subquestions as $subquestion) { $expout .= "<subquestion>\n"; $expout .= $this->writetext( $subquestion->questiontext ); $expout .= "<answer>".$this->writetext( $subquestion->answertext )."</answer>\n"; $expout .= "</subquestion>\n"; } break; case DESCRIPTION: // nothing more to do for theis type break; case MULTIANSWER: $a_count=1; foreach($question->options->questions as $question) { $thispattern = addslashes("{#".$a_count."}"); $thisreplace = $question->questiontext; $expout=ereg_replace($thispattern, $thisreplace, $expout ); $a_count++; } break; default: $expout .= "<!-- Question type is unknown or not supported (Type=$question->qtype) -->\n"; } // close the question tag $expout .= "</question>\n"; The problem was not seen as the cloze code was the last questiontype before closing the output. The irony is that the preceeding case i.e. MATCH was correctly written. This bring us back to Revision 1.5.2.3 - (view) (download) (annotate) - [select for diffs] Fri Jun 2 17:44:13 2006 WST (5 years, 4 months ago) by thepurpleblob Changes since 1.5.2.2: +113 -18 lines Diff to previous 1.5.2.2 , to branch point 1.5 Added CLOZE question support. Thanks to Joseph Rezeau for this one! However as things got more complex, new paramaters were added to all questions. The first were tags and they were added at the output end So default: // try support by optional plugin if (!$data = $this->try_exporting_using_qtypes( $question->qtype, $question )) { echo $OUTPUT->notification( get_string( 'unsupportedexport','qformat_xml',$QTYPES[$question->qtype]->local_name() ) ); } $expout .= $data; } // Write the question tags. if (!empty($CFG->usetags)) { require_once($CFG->dirroot.'/tag/lib.php'); $tags = tag_get_tags_array('question', $question->id); if (!empty($tags)) { $expout .= " <tags>\n"; foreach ($tags as $tag) { $expout .= " <tag>" . $this->writetext($tag, 0, true) . "</tag>\n"; } $expout .= " </tags>\n"; } } // close the question tag $expout .= "</question>\n"; return $expout; } you know well the author Revision 1.74 - (view) (download) (annotate) - [select for diffs] Fri Nov 19 20:33:48 2010 WST (10 months, 2 weeks ago) by tjhunt Branch: MAIN Changes since 1.73: +30 -11 lines Diff to previous 1.73 question XML import/export MDL-18916 should include question tags. This go unnoticed as few peoples use tags for cloze if at all. Adding hints to cloze create the bug. The real lesson is that Moodle code is growing in complexity and that your test policy is the good thing to do. This policy is another aspect of your professionalism as a computer code expert. So continue
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git repositories have been updated with your awesome changes, thanks! Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - git repositories have been updated with your awesome changes, thanks! Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: