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

penalty and Hints donot work with XML export for Cloze questions

    Details

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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              ppichet 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 {noformat}

              case 'multianswer':
              $acount = 1;
              foreach ($question->options->questions as $subquestion) {
              $thispattern = "

              {#".$acount."}

              ";
              $thisreplace = $subquestion->questiontext;
              $expout = preg_replace("$thispattern", $thisreplace, $expout);
              $acount++;
              }
              break;

               

              Show
              ppichet 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 {noformat} case 'multianswer': $acount = 1; foreach ($question->options->questions as $subquestion) { $thispattern = " {#".$acount."} "; $thisreplace = $subquestion->questiontext; $expout = preg_replace(" $thispattern ", $thisreplace, $expout); $acount++; } break;  
              Hide
              timhunt Tim Hunt added a comment -

              Now ready to integrate.

              Show
              timhunt Tim Hunt added a comment - Now ready to integrate.
              Hide
              timhunt 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
              timhunt 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
              ppichet 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
              ppichet 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
              ppichet 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
              ppichet 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
              stronk7 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
              stronk7 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
              timhunt 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
              timhunt 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated, thanks!

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

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

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - While reviewing and integrating export / import and unittests have been checked, so passing this. Thanks!
              Hide
              ppichet 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
              ppichet 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

              Show
              stronk7 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:
                    Fix Release Date:
                    10/Oct/11