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

          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