Moodle
  1. Moodle
  2. MDL-29691

Dutch/French users cannot create numerical Cloze questions in Moodle 2,1

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.3.4, 2.4.1
    • Fix Version/s: 2.3.5, 2.4.2, 2.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      0. Set your Moodle to a lang pack that uses , as the decimal separator.(french, deutsch)
      1. Try to create a numerical question with question text "Question

      {1:NUMERICAL:=10,3:0}

      Numérique".
      2. Make sure there are no problems when you click 'Decode and verify question text'.
      The Answer should appears as 10,3
      3. Make sure that after saving the question (without errors), you can preview it and answer with a , decimal separator.
      Use also the Fill in correct response which should be 10,3
      Verify that the correct answer is shown as 10.3 .
      4. Switch your language back to English, and test that when previewing you can now correctly answer the question with response 10.3.
      Use also the Fill in correct response which should be 10.3
      Verify that the correct answer is shown as 10.3 .
      5. Edit the initial question,
      Use the decode and verify button
      The Answer should appears as 10,3
      Save as new question.
      Test that when previewing the new question you can correctly answer the question with response 10.3 or 10,3.
      Verify that the correct answer is shown as 10.3 .

      Show
      0. Set your Moodle to a lang pack that uses , as the decimal separator.(french, deutsch) 1. Try to create a numerical question with question text "Question {1:NUMERICAL:=10,3:0} Numérique". 2. Make sure there are no problems when you click 'Decode and verify question text'. The Answer should appears as 10,3 3. Make sure that after saving the question (without errors), you can preview it and answer with a , decimal separator. Use also the Fill in correct response which should be 10,3 Verify that the correct answer is shown as 10.3 . 4. Switch your language back to English, and test that when previewing you can now correctly answer the question with response 10.3. Use also the Fill in correct response which should be 10.3 Verify that the correct answer is shown as 10.3 . 5. Edit the initial question, Use the decode and verify button The Answer should appears as 10,3 Save as new question. Test that when previewing the new question you can correctly answer the question with response 10.3 or 10,3. Verify that the correct answer is shown as 10.3 .
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      19201

      Description

      The multianswer editing form does not like , as a decimal separator. See http://moodle.org/mod/forum/discuss.php?d=184301.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          More specific link to a post that points out the problematic code: http://moodle.org/mod/forum/discuss.php?d=184301#p816472

          Show
          Tim Hunt added a comment - More specific link to a post that points out the problematic code: http://moodle.org/mod/forum/discuss.php?d=184301#p816472
          Hide
          Pierre Pichet added a comment -

          Tim,
          You create in edit_numerical_form.php protected functions
          protected function validate_answers($data, $errors) {
          ...

          /**
               * Validate a particular answer.
               * @param string $answer an answer to validate. Known to be non-blank and already trimmed.
               * @param array $data the submitted data.
               * @return bool whether this is a valid answer.
               */
              protected function is_valid_answer($answer, $data) {
                  return $answer == '*' || $this->is_valid_number($answer);
              }
          
              /**
               * Validate that a string is a nubmer formatted correctly for the current locale.
               * @param string $x a string
               * @return bool whether $x is a number that the numerical question type can interpret.
               */
              protected function is_valid_number($x) {
                  if (is_null($this->ap)) {
                      $this->ap = new qtype_numerical_answer_processor(array());
                  }
          
                  list($value, $unit) = $this->ap->apply_units($x);
          
                  return !is_null($value) && !$unit;
              }
          

          These functions are necessary to cloze edit_multianswer_form decode and verify function .

          How do you suggest we proceed ?

          Show
          Pierre Pichet added a comment - Tim, You create in edit_numerical_form.php protected functions protected function validate_answers($data, $errors) { ... /** * Validate a particular answer. * @param string $answer an answer to validate. Known to be non-blank and already trimmed. * @param array $data the submitted data. * @return bool whether this is a valid answer. */ protected function is_valid_answer($answer, $data) { return $answer == '*' || $this->is_valid_number($answer); } /** * Validate that a string is a nubmer formatted correctly for the current locale. * @param string $x a string * @return bool whether $x is a number that the numerical question type can interpret. */ protected function is_valid_number($x) { if (is_null($this->ap)) { $this->ap = new qtype_numerical_answer_processor(array()); } list($value, $unit) = $this->ap->apply_units($x); return !is_null($value) && !$unit; } These functions are necessary to cloze edit_multianswer_form decode and verify function . How do you suggest we proceed ?
          Hide
          Tim Hunt added a comment -

          Just change the method to be public.

          Show
          Tim Hunt added a comment - Just change the method to be public.
          Hide
          Pierre Pichet added a comment -

          However how do I call those functions from the edit_multianswer_form ?
          after the include of edit_multianswer_form
          use something like
          class qtype_numerical_edit_form:is_valid_answer($trimmedanswer, $data) ?
          although
          edit_multianswer_form does not inherit from edit_multianswer_form ??

          This is not the same when edit_numerical_form uses
          new qtype_numerical_answer_processor(array());

          Show
          Pierre Pichet added a comment - However how do I call those functions from the edit_multianswer_form ? after the include of edit_multianswer_form use something like class qtype_numerical_edit_form:is_valid_answer($trimmedanswer, $data) ? although edit_multianswer_form does not inherit from edit_multianswer_form ?? This is not the same when edit_numerical_form uses new qtype_numerical_answer_processor(array());
          Hide
          Tim Hunt added a comment -

          Sorry, I am not thinking straight. I actually think it would not be too bad to duplicate these methods into multianswer. The real work is done in qtype_numerical_answer_processor.

          The other option would be to move them to some other helper class, that is used in both places, but that seems a little excessive.

          Show
          Tim Hunt added a comment - Sorry, I am not thinking straight. I actually think it would not be too bad to duplicate these methods into multianswer. The real work is done in qtype_numerical_answer_processor. The other option would be to move them to some other helper class, that is used in both places, but that seems a little excessive.
          Hide
          Pierre Pichet added a comment -

          Just copy was my implicit solution

          Show
          Pierre Pichet added a comment - Just copy was my implicit solution
          Hide
          Pierre Pichet added a comment - - edited

          First modifications on edit_multianswer allow me to save the question en français
          But this is just the first part.
          Grading is not OK...as 10,3 is stored as 10.3 ...
          back to work
          just to remember some problems on numerical
          The tolérance does not accept 0,1 as good value

          Show
          Pierre Pichet added a comment - - edited First modifications on edit_multianswer allow me to save the question en français But this is just the first part. Grading is not OK...as 10,3 is stored as 10.3 ... back to work just to remember some problems on numerical The tolérance does not accept 0,1 as good value
          Hide
          Pierre Pichet added a comment -

          As a summary of my post on the forum
          http://moodle.org/mod/forum/discuss.php?d=174387

          Cloze multianswer are created from a text following specific conventions.
          The Cloze multianswer text is also the text that is being used to export Cloze questions.

          The code has been maintained in each moodle version so that an old valid Cloze multianswer text is valid in all versions without any restrictions apart that the question could not read by the user if the language is too far from the one used in the installation.

          If we localized the Cloze multianswer text for the numerical answer as proposed here then we lost an important
          feature for cloze question type.

          The actual regular numerical question store the teacher's written answer as a numerical variable that PHP can read anywhere the same way.

          To give to the Cloze numerical question the same property we should write the numerical value so that PHP will always be able to read it correctly so is_numeric() is the test to do.

          For the teacher the rules are simple : use . as decimal separator , don't use thousands and the E format is allowed.

          How the question will be graded is the way regular numerical are graded.

          We could however in the future set new parameters in the Cloze format to handle response formatting but Cloze syntax appears already complex to many users...

          My proposal is to not modify the cloze numerical validation.

          Show
          Pierre Pichet added a comment - As a summary of my post on the forum http://moodle.org/mod/forum/discuss.php?d=174387 Cloze multianswer are created from a text following specific conventions. The Cloze multianswer text is also the text that is being used to export Cloze questions. The code has been maintained in each moodle version so that an old valid Cloze multianswer text is valid in all versions without any restrictions apart that the question could not read by the user if the language is too far from the one used in the installation. If we localized the Cloze multianswer text for the numerical answer as proposed here then we lost an important feature for cloze question type. The actual regular numerical question store the teacher's written answer as a numerical variable that PHP can read anywhere the same way. To give to the Cloze numerical question the same property we should write the numerical value so that PHP will always be able to read it correctly so is_numeric() is the test to do. For the teacher the rules are simple : use . as decimal separator , don't use thousands and the E format is allowed. How the question will be graded is the way regular numerical are graded. We could however in the future set new parameters in the Cloze format to handle response formatting but Cloze syntax appears already complex to many users... My proposal is to not modify the cloze numerical validation.
          Hide
          Pierre Pichet added a comment -

          Tim,
          As a more flexible apply_unit() is available (MDL-33105) we could apply it to Cloze and as you suggest copy
          the numerical answer validation code from edit_numerical_form to edit_multianswer_form.

          Show
          Pierre Pichet added a comment - Tim, As a more flexible apply_unit() is available ( MDL-33105 ) we could apply it to Cloze and as you suggest copy the numerical answer validation code from edit_numerical_form to edit_multianswer_form.
          Hide
          Pierre Pichet added a comment -

          A first version applied to actual master i.e. without MDL-33105.
          Already allows answers as 1000,2
          Some trimmings to do (in decode should the answer be shown as 1000,2 or 1000.2)
          I think that we should applied it after MDL-33105 is finished

          Show
          Pierre Pichet added a comment - A first version applied to actual master i.e. without MDL-33105 . Already allows answers as 1000,2 Some trimmings to do (in decode should the answer be shown as 1000,2 or 1000.2) I think that we should applied it after MDL-33105 is finished
          Hide
          Pierre Pichet added a comment -

          Given that I was on MDL-29691 I apply the correction necessary so that the multianswer good response have the correct decsep here.
          See MDL-33105
          https://tracker.moodle.org/browse/MDL-33105?focusedCommentId=197116&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-197116
          So MDL-29691 allow the use of apply-unit in multianswer and MDL-33105 improve the apply-unit.
          I let you handle this the best way.

          Show
          Pierre Pichet added a comment - Given that I was on MDL-29691 I apply the correction necessary so that the multianswer good response have the correct decsep here. See MDL-33105 https://tracker.moodle.org/browse/MDL-33105?focusedCommentId=197116&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-197116 So MDL-29691 allow the use of apply-unit in multianswer and MDL-33105 improve the apply-unit. I let you handle this the best way.
          Hide
          Pierre Pichet added a comment -

          Notice also that the multianswer decsep editing is corrected for the answer value in the actual proposal.
          I think we should also correct similarly for the tolerance value either in this bug or creating another one ?

          Show
          Pierre Pichet added a comment - Notice also that the multianswer decsep editing is corrected for the answer value in the actual proposal. I think we should also correct similarly for the tolerance value either in this bug or creating another one ?
          Hide
          Pierre Pichet added a comment - - edited

          A first version is available for comments
          Just spot some whitespaces and unnecessary multiple commits done when pushing on github.

          Show
          Pierre Pichet added a comment - - edited A first version is available for comments Just spot some whitespaces and unnecessary multiple commits done when pushing on github.
          Hide
          Pierre Pichet added a comment -

          If we solve the decsep handling by this bug and MDL-33105, this will solve other bugs related to either numerical handling or numerical in cloze.

          Show
          Pierre Pichet added a comment - If we solve the decsep handling by this bug and MDL-33105 , this will solve other bugs related to either numerical handling or numerical in cloze.
          Hide
          Tim Hunt added a comment -

          That looks pretty good, if it were cleaned up.

          Just one bit I spotted. Instead of

          $correctanswer->answer = str_replace('.', $subq->ap->get_point(), $correctanswer->answer);

          Would it be better to use the format_float function?

          Show
          Tim Hunt added a comment - That looks pretty good, if it were cleaned up. Just one bit I spotted. Instead of $correctanswer->answer = str_replace('.', $subq->ap->get_point(), $correctanswer->answer); Would it be better to use the format_float function?
          Hide
          Pierre Pichet added a comment - - edited

          This was copied from numerical/renderer.php

          public function correct_response(question_attempt $qa) {
                  $question = $qa->get_question();
                  $answer = $question->get_correct_answer();
                  if (!$answer) {
                      return '';
                  }
          
                  $response = str_replace('.', $question->ap->get_point(), $answer->answer);
          

          given that the answer is saved as text

          <FIELD NAME="answer" TYPE="text" LENGTH="small" NOTNULL="true" SEQUENCE="false" PREVIOUS="question" NEXT="answerformat"/>
          

          we just modify the decsep so it reflects the way the user define the answer.

          However on testing this does not appears to be true as the value stored is the value that outputs from apply_unit()
          which is the result of a multiplication.

           $answer->answer = $this->apply_unit($answerdata, $units,
                                  !empty($question->unitsleft));
          

          As the answer is already formatted by php, your solution in renderer was OK.

          This imply also that the actual code will not allow easily to create a strict numerical format grading.

          Show
          Pierre Pichet added a comment - - edited This was copied from numerical/renderer.php public function correct_response(question_attempt $qa) { $question = $qa->get_question(); $answer = $question->get_correct_answer(); if (!$answer) { return ''; } $response = str_replace('.', $question->ap->get_point(), $answer->answer); given that the answer is saved as text <FIELD NAME="answer" TYPE="text" LENGTH="small" NOTNULL="true" SEQUENCE="false" PREVIOUS="question" NEXT="answerformat"/> we just modify the decsep so it reflects the way the user define the answer. However on testing this does not appears to be true as the value stored is the value that outputs from apply_unit() which is the result of a multiplication. $answer->answer = $this->apply_unit($answerdata, $units, !empty($question->unitsleft)); As the answer is already formatted by php, your solution in renderer was OK. This imply also that the actual code will not allow easily to create a strict numerical format grading.
          Hide
          Tim Hunt added a comment -

          Ah yes. The problem with format_float is that it will change the number of decimal places shown.

          Show
          Tim Hunt added a comment - Ah yes. The problem with format_float is that it will change the number of decimal places shown.
          Hide
          Pierre Pichet added a comment -

          I suppress the white spaces and push to github using -f so the correction was put in the last step.

          Do I need to suppress the steps or this is useable as is?

          As you suggest to Jean-Michel in MDL-37313
          Then do git rebase -i master
          which is an interactive rebase.
          You should be able to merge the new third commit into the first commit say saying it is a 'fixup'.

          Show
          Pierre Pichet added a comment - I suppress the white spaces and push to github using -f so the correction was put in the last step. Do I need to suppress the steps or this is useable as is? As you suggest to Jean-Michel in MDL-37313 Then do git rebase -i master which is an interactive rebase. You should be able to merge the new third commit into the first commit say saying it is a 'fixup'.
          Hide
          Tim Hunt added a comment -

          Please can you rebase this to be a single commit.

          Show
          Tim Hunt added a comment - Please can you rebase this to be a single commit.
          Hide
          Pierre Pichet added a comment -

          The interactive rebase open a VIM editor on my windows installation, I edit the file (the last time I work on VIM is more then 20 years ago...) then the process went wrong.
          So I create a new branch MDL-29691_a
          I will install the code checker.

          Show
          Pierre Pichet added a comment - The interactive rebase open a VIM editor on my windows installation, I edit the file (the last time I work on VIM is more then 20 years ago...) then the process went wrong. So I create a new branch MDL-29691 _a I will install the code checker.
          Hide
          Pierre Pichet added a comment -

          The new version pass the codecheker test with some modifications to the comment syntax...
          so ready

          Show
          Pierre Pichet added a comment - The new version pass the codecheker test with some modifications to the comment syntax... so ready
          Hide
          Tim Hunt added a comment -

          That is looking very good. I can only think of one further change you should make. I think you should add a comment to

          protected $ap = null;

          like

          /** @var qtype_numerical_answer_processor used when validating numerical answers. */

          Show
          Tim Hunt added a comment - That is looking very good. I can only think of one further change you should make. I think you should add a comment to protected $ap = null; like /** @var qtype_numerical_answer_processor used when validating numerical answers. */
          Hide
          Pierre Pichet added a comment -

          /** @var qtype_numerical_answer_processor used when validating numerical answers. */
          added.
          So ready for testing

          Show
          Pierre Pichet added a comment - /** @var qtype_numerical_answer_processor used when validating numerical answers. */ added. So ready for testing
          Hide
          Tim Hunt added a comment -

          Thanks Pierre. I am submitting this for integration now.

          Show
          Tim Hunt added a comment - Thanks Pierre. I am submitting this for integration now.
          Hide
          Pierre Pichet added a comment -

          Should I rebase now to this week master version.
          The version used here and MDL-33105 is Moodle 2.5dev (Build: 20121230).

          Show
          Pierre Pichet added a comment - Should I rebase now to this week master version. The version used here and MDL-33105 is Moodle 2.5dev (Build: 20121230).
          Hide
          Tim Hunt added a comment -

          Ideally, yes.

          Show
          Tim Hunt added a comment - Ideally, yes.
          Hide
          Pierre Pichet added a comment -

          Rebase done

          Show
          Pierre Pichet added a comment - Rebase done
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, any reason this should go to 2.5 only? Or is it back-porable to 2.4 too?

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, any reason this should go to 2.5 only? Or is it back-porable to 2.4 too?
          Hide
          Tim Hunt added a comment -

          Oops, sorry. I thought this depended on MDL-33105, but I was wrong. This can and should be back-ported to all stable branches.

          Show
          Tim Hunt added a comment - Oops, sorry. I thought this depended on MDL-33105 , but I was wrong. This can and should be back-ported to all stable branches.
          Hide
          Pierre Pichet added a comment -

          I will do the necessary later today. (2.3.2, 2.4.2)

          Show
          Pierre Pichet added a comment - I will do the necessary later today. (2.3.2, 2.4.2)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          FYI: It's Wednesday and I'm moving all MY "integration in progress" issues (3) out from current integration with message:

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - FYI: It's Wednesday and I'm moving all MY "integration in progress" issues (3) out from current integration with message: The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Pierre Pichet added a comment -

          So I will wait for this week release before back-porting.

          Show
          Pierre Pichet added a comment - So I will wait for this week release before back-porting.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 24 and 23. Thanks!

          Show
          Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks!
          Hide
          Michael de Raadt added a comment -

          Test result: Unsure

          I encountered an error in Master only...

          Notice: Undefined variable: value in D:\xampp\htdocs\master_integration\question\type\numerical\questiontype.php on line 637
          

          ...that appeared after verifying a question and on subsequent pages after saving.

          The rest of the test worked fine and the error did not appear in other versions.

          I'm failing this test for now, but perhaps Tim can determine if the cause is this change or another.

          Show
          Michael de Raadt added a comment - Test result: Unsure I encountered an error in Master only... Notice: Undefined variable: value in D:\xampp\htdocs\master_integration\question\type\numerical\questiontype.php on line 637 ...that appeared after verifying a question and on subsequent pages after saving. The rest of the test worked fine and the error did not appear in other versions. I'm failing this test for now, but perhaps Tim can determine if the cause is this change or another.
          Hide
          Tim Hunt added a comment -

          I will look shortly.

          Show
          Tim Hunt added a comment - I will look shortly.
          Hide
          Pierre Pichet added a comment -

          This is related to MDL-33105 which have been applied to master before MDL-29691.

              public function apply_units($response, $separateunit = null) {
                  // Strip spaces (which may be thousands separators) and change other forms
                  // of writing e to e.
                  $response = str_replace(' ', '', $response);
                  // Strip thousand separators like half space.
                  if (!in_array($this->thousandssep, array(',', '.', ' '))) {
                      if (strpos($value, $this->thousandssep) !== false) {
                          $response = str_replace($this->thousandssep, '', $response);
                      }
                  }
          

          The code is not correct.
          I suggest you remove MDL-33105 so I can correct it and add supplementary tests

          Show
          Pierre Pichet added a comment - This is related to MDL-33105 which have been applied to master before MDL-29691 . public function apply_units($response, $separateunit = null) { // Strip spaces (which may be thousands separators) and change other forms // of writing e to e. $response = str_replace(' ', '', $response); // Strip thousand separators like half space. if (!in_array($this->thousandssep, array(',', '.', ' '))) { if (strpos($value, $this->thousandssep) !== false) { $response = str_replace($this->thousandssep, '', $response); } } The code is not correct. I suggest you remove MDL-33105 so I can correct it and add supplementary tests
          Hide
          Dan Poltawski added a comment -

          Thanks Piere, i've reverted MDL-33105 now.

          Michael, please could retest this on master? Thanks.

          Show
          Dan Poltawski added a comment - Thanks Piere, i've reverted MDL-33105 now. Michael, please could retest this on master? Thanks.
          Hide
          Michael de Raadt added a comment -

          Retesting...

          Show
          Michael de Raadt added a comment - Retesting...
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested in Master, 2.4 and 2.3.

          Without the changes from that other issue, I was not seeing any errors.

          Thanks for working on this.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested in Master, 2.4 and 2.3. Without the changes from that other issue, I was not seeing any errors. Thanks for working on this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: