Moodle
  1. Moodle
  2. MDL-33105

Numerical is_complete() could return a false on a good response

    Details

    • Type: Bug Bug
    • Status: Development in progress
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.1.6, 2.2.3, 2.3
    • Fix Version/s: 2.6.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      The new code allows users to use different numerical formats when creating numerical questions and when answering.
      The format allows to use indifferently , or . as decsep in all language ( i.e. all) where they a used.
      The thousandseps can be the one used in the language setting or space or , or .
      The code is set as follow
      // Dot . is mostly a decimal separator there a few exceptions where it is a thousand separator
      //If a . is present or there are multiple , (i.e. 2,456,789 ) assume
      //is a thousands separator and strip it or else assume it is a decimal
      // separator and change it to . if only one and it is , then change to .

      1.In summary the testing should include creating numerical questions answers with the number formats used in the phpunit test
      as in test_euro_style() as
      -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159 .
      Take care that when you create a numerical question either by using edit_numerical_form or as cloze numerical
      the number you write i.e. 10,000.1 WILL BE CONVERTED when saving the question in the STANDARD format i.e. 10000.1
      This flexibility in writing numbers apply for answers and tolerances values of numerical using
      edit_numerical_form or creating numerical using edit_multianswer
      and answering those questions in preview or quiz.

      LOAD THE EXAMPLES from the file
      Set your language to english default
      A. EDIT_NUMERICAL_FORM
      Notice that the answers and tolerance are written as PHP numerical
      Change the values using variants in decsep or thousand sep.
      Save the question and reedit.
      The numerical are converted back to standard PHP.

      Then start a preview of the question and open the reedit so that you can see the answers values and tolerance.

      Preview the question using diffferent response and check if the grading is OK in each case.
      Use the preview "fill the good response " , submit and look at the good response value.

      Verify in the database question_numerical if the tolerance values are stored correctly
      Create a copy and check that everything is OK.

      A. EDIT_MULTIANSWER_FORM

      Modify the values of answers and tolerance and use the
      DECODE AND VERIFY button to check if these values will not set errors.
      Save the variants and preview them using the "fill the good response " to see how these values are stored

      Use different way to write the answers and the tolerance.

      Verify in the database question_numerical if the tolerance values are stored correctly. i.e. 1e2 will be stored as 100

      Verify that the %100%number:tolerance#feedback format could tolerate one space
      between the number:tolerance as

      {1:NUMERICAL:~%100%1,00.1 : 1e2#feedback}

      Create a copy and check that everything is OK

      Testing language.

      Set your language to another where the decsep is , (i.e. français)
      Preview the questions using"fill the good response " , then submit.
      the fill in should ideally replace the decsep by a , in the the good response in the 2 step i.e fill and submit.
      This is true in multianswer but the fill in does not work correctly for numerical with a unit (see the comments below)

      Show
      The new code allows users to use different numerical formats when creating numerical questions and when answering. The format allows to use indifferently , or . as decsep in all language ( i.e. all) where they a used. The thousandseps can be the one used in the language setting or space or , or . The code is set as follow // Dot . is mostly a decimal separator there a few exceptions where it is a thousand separator //If a . is present or there are multiple , (i.e. 2,456,789 ) assume //is a thousands separator and strip it or else assume it is a decimal // separator and change it to . if only one and it is , then change to . 1.In summary the testing should include creating numerical questions answers with the number formats used in the phpunit test as in test_euro_style() as -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159 . Take care that when you create a numerical question either by using edit_numerical_form or as cloze numerical the number you write i.e. 10,000.1 WILL BE CONVERTED when saving the question in the STANDARD format i.e. 10000.1 This flexibility in writing numbers apply for answers and tolerances values of numerical using edit_numerical_form or creating numerical using edit_multianswer and answering those questions in preview or quiz. LOAD THE EXAMPLES from the file Set your language to english default A. EDIT_NUMERICAL_FORM Notice that the answers and tolerance are written as PHP numerical Change the values using variants in decsep or thousand sep. Save the question and reedit. The numerical are converted back to standard PHP. Then start a preview of the question and open the reedit so that you can see the answers values and tolerance. Preview the question using diffferent response and check if the grading is OK in each case. Use the preview "fill the good response " , submit and look at the good response value. Verify in the database question_numerical if the tolerance values are stored correctly Create a copy and check that everything is OK. A. EDIT_MULTIANSWER_FORM Modify the values of answers and tolerance and use the DECODE AND VERIFY button to check if these values will not set errors. Save the variants and preview them using the "fill the good response " to see how these values are stored Use different way to write the answers and the tolerance. Verify in the database question_numerical if the tolerance values are stored correctly. i.e. 1e2 will be stored as 100 Verify that the %100%number:tolerance#feedback format could tolerate one space between the number:tolerance as {1:NUMERICAL:~%100%1,00.1 : 1e2#feedback} Create a copy and check that everything is OK Testing language. Set your language to another where the decsep is , (i.e. français) Preview the questions using"fill the good response " , then submit. the fill in should ideally replace the decsep by a , in the the good response in the 2 step i.e fill and submit. This is true in multianswer but the fill in does not work correctly for numerical with a unit (see the comments below)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      40367

      Description

      The last step of is_complete_response(array $response) {
      is
      if ($this->ap->contains_thousands_seaparator($response['answer']))

      { return false; }

      This was valid in the first version of the 2,1 new engine apply_units($response, $separateunit = null)
      with its strict syntax analysis.
      However the apply_units() was retrofit to preceeding more error tolerant function() which can handle a , instead of a .
      as decimal point.
      So answering 10,25 for a 10.25 good answer response will generate a incomplete message although on submit and finish the 10,25 will
      grade as 100%.

        Issue Links

          Activity

          Hide
          Pierre Pichet added a comment -

          There are different ways to solve this problem.
          I think the most fruitful is to modify the grading process so that the strict apply unit new engine initial design is applied to the response.
          If the grade is not OK then the more flexible apply_units is applied.
          The is_complete_response() should reflect the grading process.

          We should go a step further with an additional grading option that will let the teacher choose between a strict grading as initially designed for the new engine and or the historical more flexible grading.

          Show
          Pierre Pichet added a comment - There are different ways to solve this problem. I think the most fruitful is to modify the grading process so that the strict apply unit new engine initial design is applied to the response. If the grade is not OK then the more flexible apply_units is applied. The is_complete_response() should reflect the grading process. We should go a step further with an additional grading option that will let the teacher choose between a strict grading as initially designed for the new engine and or the historical more flexible grading.
          Hide
          Pierre Pichet added a comment -

          An almost universal conversion of number formats with the unique restriction that if you use a thousand separator you MUST add the decsep could (or should ) be the default apply_units ()

          1234.5 1234,5 1 234.5 1,234.5 1.234,5 (deutch) 1(half space)234.5 1(half space)234,5 all grade good.

          I have a first working solution.
          More testing and building unit tests.

          This could be the default and on this we could add a more restrictive grading option using the original new engine code.

          Show
          Pierre Pichet added a comment - An almost universal conversion of number formats with the unique restriction that if you use a thousand separator you MUST add the decsep could (or should ) be the default apply_units () 1234.5 1234,5 1 234.5 1,234.5 1.234,5 (deutch) 1(half space)234.5 1(half space)234,5 all grade good. I have a first working solution. More testing and building unit tests. This could be the default and on this we could add a more restrictive grading option using the original new engine code.
          Hide
          Pierre Pichet added a comment -
          Show
          Pierre Pichet added a comment - A first version is available at https://github.com/ppichet/moodle/tree/MDL-33105 The compare at https://github.com/ppichet/moodle/compare/MDL-33105
          Hide
          Pierre Pichet added a comment -

          Tim,
          I did not ask for a peer review as the priority was to the 2,3 release.

          However you seem to be back from the mountains in great shape.

          I think that this grading option could be a good and simpler way to fulfill grading in the language number settings.

          This is a first sketch (i.e. without the unit tests).

          Show
          Pierre Pichet added a comment - Tim, I did not ask for a peer review as the priority was to the 2,3 release. However you seem to be back from the mountains in great shape. I think that this grading option could be a good and simpler way to fulfill grading in the language number settings. This is a first sketch (i.e. without the unit tests).
          Hide
          Tim Hunt added a comment -

          I am still in the mountains, just doing a few things around the edge of the MoodleMoot. Not sure if I will have time to look at this before I get back or not.

          Show
          Tim Hunt added a comment - I am still in the mountains, just doing a few things around the edge of the MoodleMoot. Not sure if I will have time to look at this before I get back or not.
          Hide
          Pierre Pichet added a comment -

          Stay in the mountains as long as you can

          This can wait as long as you want.

          There is no hurry.

          Show
          Pierre Pichet added a comment - Stay in the mountains as long as you can This can wait as long as you want. There is no hurry.
          Hide
          Tim Hunt added a comment -

          I just had a quick look while waiting for Martin's keynote to start.

          I am not convinced that this needs to be another option. Can't we just make this work by default? In what ways is the new code not back-wards compatible?

          Show
          Tim Hunt added a comment - I just had a quick look while waiting for Martin's keynote to start. I am not convinced that this needs to be another option. Can't we just make this work by default? In what ways is the new code not back-wards compatible?
          Hide
          Pierre Pichet added a comment -

          The actual and historical default is an "almost" universal conversion of numercial formats to PHP . decsep with no thousands sep numerical value.
          This is why you have converted back the apply_units() function and put aside your development with this somehow sad comment
          "* This method can be used for more locale-strict parsing of repsonses. At the

          • moment we don't use it, and instead use the more lax parsing in apply_units.
          • This is just a note that this funciton was used in the past, so if you are
          • intersted, look through version control history.
            "
            The proposal to a grade format penalty was put aside as this give a too complex interface. (MDL-27418_ppichet)

          I just found that it will be easy to offer the choice between a strict conversion or the historical flexible formats (i.e. the necessary default for numerical in cloze numericals).

          So I build this to explore the concept.

          I think that some teachers will like a strict number formating.

          So this is more a proposal than a solution to this bug and others related to numerical formats.

          Anyway, this is already on "I am about to disappear for two weeks"
          http://moodle.org/mod/forum/discuss.php?d=202890

          Show
          Pierre Pichet added a comment - The actual and historical default is an "almost" universal conversion of numercial formats to PHP . decsep with no thousands sep numerical value. This is why you have converted back the apply_units() function and put aside your development with this somehow sad comment "* This method can be used for more locale-strict parsing of repsonses. At the moment we don't use it, and instead use the more lax parsing in apply_units. This is just a note that this funciton was used in the past, so if you are intersted, look through version control history. " The proposal to a grade format penalty was put aside as this give a too complex interface. ( MDL-27418 _ppichet) I just found that it will be easy to offer the choice between a strict conversion or the historical flexible formats (i.e. the necessary default for numerical in cloze numericals). So I build this to explore the concept. I think that some teachers will like a strict number formating. So this is more a proposal than a solution to this bug and others related to numerical formats. Anyway, this is already on "I am about to disappear for two weeks" http://moodle.org/mod/forum/discuss.php?d=202890
          Hide
          Pierre Pichet added a comment -

          If we remove the last incomplete test related to the thousand separator we have a temporary solution for MDL-33744 that will solve the most current number formatting as a large number of languages use , as a decsep.
          This eliminate a warning that most often but not in all cases will not modify the grade.
          OR
          We eliminate this test only to cloze numerical which is not a good way to code multianswer...

          We could add the proposed solution for MDL-33105 later.

          Show
          Pierre Pichet added a comment - If we remove the last incomplete test related to the thousand separator we have a temporary solution for MDL-33744 that will solve the most current number formatting as a large number of languages use , as a decsep. This eliminate a warning that most often but not in all cases will not modify the grade. OR We eliminate this test only to cloze numerical which is not a good way to code multianswer... We could add the proposed solution for MDL-33105 later.
          Hide
          Pierre Pichet added a comment -

          There are two occurences of
          if ($this->ap->contains_thousands_seaparator($response['answer']))

          in question/type/numerical/question.php
          One in the function is_complete test() and one in the function get_validation_error(array $response)

          They are changed as comment in the "experimental" proposal see

          https://github.com/ppichet/moodle/compare/MDL-33105

          Show
          Pierre Pichet added a comment - There are two occurences of if ($this->ap->contains_thousands_seaparator($response ['answer'] )) in question/type/numerical/question.php One in the function is_complete test() and one in the function get_validation_error(array $response) They are changed as comment in the "experimental" proposal see https://github.com/ppichet/moodle/compare/MDL-33105
          Hide
          Tim Hunt added a comment -

          I finally got back to looking at this.

          I like your original proposal here: http://tracker.moodle.org/browse/MDL-33105?focusedCommentId=158563&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-158563 which does not require any question options.

          It seems to me that will just work in almost all cases.

          Therefore, I don't think we need the complexity of adding new options.

          Show
          Tim Hunt added a comment - I finally got back to looking at this. I like your original proposal here: http://tracker.moodle.org/browse/MDL-33105?focusedCommentId=158563&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-158563 which does not require any question options. It seems to me that will just work in almost all cases. Therefore, I don't think we need the complexity of adding new options.
          Hide
          Pierre Pichet added a comment -

          Unless there is a specific need, I put this on my todo list (back from Collioure ...)

          Show
          Pierre Pichet added a comment - Unless there is a specific need, I put this on my todo list (back from Collioure ...)
          Hide
          Pierre Pichet added a comment -

          So back to this as this is related to a "critical" MDL-33744 and it seems that I can put some time this week on moodle coding.
          So as you suggest, I will test the more flexible apply_unit() function as described in https://github.com/ppichet/moodle/compare/MDL-33105
          without the new question numberdecodingtype option.

          Show
          Pierre Pichet added a comment - So back to this as this is related to a "critical" MDL-33744 and it seems that I can put some time this week on moodle coding. So as you suggest, I will test the more flexible apply_unit() function as described in https://github.com/ppichet/moodle/compare/MDL-33105 without the new question numberdecodingtype option.
          Hide
          Pierre Pichet added a comment -

          The github has been updated to a simpler improved apply_units().
          The other (options) solution being stored (just in case) as https://github.com/ppichet/moodle/tree/MDL-33105_strictformat
          I am begining testing and building the appropriate testunits..

          Show
          Pierre Pichet added a comment - The github has been updated to a simpler improved apply_units(). The other (options) solution being stored (just in case) as https://github.com/ppichet/moodle/tree/MDL-33105_strictformat I am begining testing and building the appropriate testunits..
          Hide
          Pierre Pichet added a comment -

          As the teacher is not able to set for a strict format but want that the thousand sep could be used, this imply that
          for an ambiguous entry we choose the more general one.
          The use of . for thousand sep in deutsch language cannot be distinguished from the english decimal sep unless there is a , after the . as the fourth character i.e. 12.345,67 and furthermore that the entry in done when deutsch is set as the language.

          However 12.345.678 is unambiguous as 12,345,678 as 12 345 678

          So I go back to the drawing board

          Show
          Pierre Pichet added a comment - As the teacher is not able to set for a strict format but want that the thousand sep could be used, this imply that for an ambiguous entry we choose the more general one. The use of . for thousand sep in deutsch language cannot be distinguished from the english decimal sep unless there is a , after the . as the fourth character i.e. 12.345,67 and furthermore that the entry in done when deutsch is set as the language. However 12.345.678 is unambiguous as 12,345,678 as 12 345 678 So I go back to the drawing board
          Hide
          Pierre Pichet added a comment -

          Tim,
          When you built the new question engine, your first reaction was to build a strict format for numbers in your qtype_numerical_answer_processor().
          Because of the Moodle history, you put this handling aside.
          From other english Moodle users comments, your personal "strict" attitude seems quite normal.
          Other Moodle users from other countries express their need to have the possibility of grading their own number format.

          So I will build the "almost" universal numerical format reading without removing the possibility to a future number format grading.

          Incidently almost all languages use the indoeuropean numerical with the same numbers order i.e.

          "In Arabic multidigit numbers are written right-to-left, in increasing powers of ten. Thus, just as in the west, the units are in the rightmost position and greater powers of ten are to the left"

          http://en.wikipedia.org/wiki/List_of_numbers_in_various_languages#Indo-European_languages

          Show
          Pierre Pichet added a comment - Tim, When you built the new question engine, your first reaction was to build a strict format for numbers in your qtype_numerical_answer_processor(). Because of the Moodle history, you put this handling aside. From other english Moodle users comments, your personal "strict" attitude seems quite normal. Other Moodle users from other countries express their need to have the possibility of grading their own number format. So I will build the "almost" universal numerical format reading without removing the possibility to a future number format grading. Incidently almost all languages use the indoeuropean numerical with the same numbers order i.e. "In Arabic multidigit numbers are written right-to-left, in increasing powers of ten. Thus, just as in the west, the units are in the rightmost position and greater powers of ten are to the left" http://en.wikipedia.org/wiki/List_of_numbers_in_various_languages#Indo-European_languages
          Hide
          Tim Hunt added a comment -

          What I would most like to see is an update to https://github.com/ppichet/moodle/blob/71c238b5494dcd7431ee25893c8869a81e235490/question/type/numerical/tests/answerprocessor_test.php so it is easy to see at a glance lots of examples of how the new code will work.

          Show
          Tim Hunt added a comment - What I would most like to see is an update to https://github.com/ppichet/moodle/blob/71c238b5494dcd7431ee25893c8869a81e235490/question/type/numerical/tests/answerprocessor_test.php so it is easy to see at a glance lots of examples of how the new code will work.
          Hide
          Pierre Pichet added a comment -

          This is also my next step.
          I am completing the phpunit installation ( as usual the docs are not always up-to-date ) and I will give you the info as soon as possible.
          let's say that my "manual" tests
          10,3 10.3 10,300.0e-3 10.300,0e-3 10300.0e-3 10300,0e-3 ... give the same grade either in english, french and deustch set as language

          Show
          Pierre Pichet added a comment - This is also my next step. I am completing the phpunit installation ( as usual the docs are not always up-to-date ) and I will give you the info as soon as possible. let's say that my "manual" tests 10,3 10.3 10,300.0e-3 10.300,0e-3 10300.0e-3 10300,0e-3 ... give the same grade either in english, french and deustch set as language
          Hide
          Pierre Pichet added a comment -

          AS far as I can understand my PHPUnit installation problems seems to be related to having followed
          http://docs.moodle.org/dev/PHPUnit_installation_in_Windows#Install_PHPUnit
          and did not noticed
          http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_PEAR_.28not_recommended.29
          As I worked remotely on my desktop computer, more news next week...

          Show
          Pierre Pichet added a comment - AS far as I can understand my PHPUnit installation problems seems to be related to having followed http://docs.moodle.org/dev/PHPUnit_installation_in_Windows#Install_PHPUnit and did not noticed http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_PEAR_.28not_recommended.29 As I worked remotely on my desktop computer, more news next week...
          Hide
          Tim Hunt added a comment -

          Actually, you did not make a mistake. The Composer stuff is very new. It has only been there for about a week. It is much easier than the old way, which is why they changed.

          Show
          Tim Hunt added a comment - Actually, you did not make a mistake. The Composer stuff is very new. It has only been there for about a week. It is much easier than the old way, which is why they changed.
          Hide
          Pierre Pichet added a comment - - edited

          Just to tell that I don't give up to install PHPUnit but the learning curve is steep.
          As an example I just finally found what vendor means in
          "To execute all test suites from main configuration file execute the vendor/bin/phpunit script from your $CFG->dirroot directory."

          I have never went so far in the intricacies of the Apache-Windows commands.
          Finally the "vendor" was set by the installer to something as evident as
          C:\Documents and Settings\All Users\Application Data\Composer\

          Show
          Pierre Pichet added a comment - - edited Just to tell that I don't give up to install PHPUnit but the learning curve is steep. As an example I just finally found what vendor means in "To execute all test suites from main configuration file execute the vendor/bin/phpunit script from your $CFG->dirroot directory." I have never went so far in the intricacies of the Apache-Windows commands. Finally the "vendor" was set by the installer to something as evident as C:\Documents and Settings\All Users\Application Data\Composer\
          Hide
          Pierre Pichet added a comment -

          Tim,
          The Apache path seems to be Ok because if I go to my moodle dirroot as D:moodle2\server\moodle and type Composer
          I obtain a list of
          Usage:
          Options
          Available commands:
          etc.

          However from there I don't know how proceed to install PHPUnit from http://docs.moodle.org/dev/PHPUnit or http://docs.moodle.org/dev/PHPUnit_installation_in_Windows or the Composer docs.

          Show
          Pierre Pichet added a comment - Tim, The Apache path seems to be Ok because if I go to my moodle dirroot as D:moodle2\server\moodle and type Composer I obtain a list of Usage: Options Available commands: etc. However from there I don't know how proceed to install PHPUnit from http://docs.moodle.org/dev/PHPUnit or http://docs.moodle.org/dev/PHPUnit_installation_in_Windows or the Composer docs.
          Hide
          Tim Hunt added a comment -

          It should be as simple as http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_Composer

          Except that that refers to Unix commands like curl. Does http://getcomposer.org/doc/00-intro.md#installation-windows help?

          I am not at work (where I have a Windows computer) today. Tomorrow, I will try installing PHPUnit using Composer, and produce some clear documentation.

          Show
          Tim Hunt added a comment - It should be as simple as http://docs.moodle.org/dev/PHPUnit#Installation_of_PHPUnit_via_Composer Except that that refers to Unix commands like curl. Does http://getcomposer.org/doc/00-intro.md#installation-windows help? I am not at work (where I have a Windows computer) today. Tomorrow, I will try installing PHPUnit using Composer, and produce some clear documentation.
          Hide
          Pierre Pichet added a comment - - edited

          just find
          http://www.phpunit.de/manual/current/en/installation.html#installation.composer
          will try and give news.
          To summarize
          After experimenting with PEAR which effectively load somehow PHPUnit code that did not seem to work, I try the windows installer from http://getcomposer.org/doc/00-intro.md
          "Download and run Composer-Setup.exe, it will install the latest Composer version and set up your PATH so that you can just call composer from any directory in your command line."

          This install the composer in
          C:\Documents and Settings\All Users\Application Data\Composer\bin
          then I did
          C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer install
          Loading composer repositories with package information
          Installing dependencies
          Nothing to install or update
          Generating autoload files

          This create the directories vendor and composer i.e.
          C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\composer

          Probably bacause of the multiple installations, things are not working properly.
          I will reinstall everything clean once you have clarified the correct procedure.

          Show
          Pierre Pichet added a comment - - edited just find http://www.phpunit.de/manual/current/en/installation.html#installation.composer will try and give news. To summarize After experimenting with PEAR which effectively load somehow PHPUnit code that did not seem to work, I try the windows installer from http://getcomposer.org/doc/00-intro.md "Download and run Composer-Setup.exe, it will install the latest Composer version and set up your PATH so that you can just call composer from any directory in your command line." This install the composer in C:\Documents and Settings\All Users\Application Data\Composer\bin then I did C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer install Loading composer repositories with package information Installing dependencies Nothing to install or update Generating autoload files This create the directories vendor and composer i.e. C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\composer Probably bacause of the multiple installations, things are not working properly. I will reinstall everything clean once you have clarified the correct procedure.
          Hide
          Pierre Pichet added a comment -

          I miss the --dev in Install
          Was added 3 days ago in the doc.
          So I just try it

          C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer insta
          ll --dev
          Loading composer repositories with package information
          Installing dependencies
          Nothing to install or update
          Loading composer repositories with package information
          Installing dev dependencies

          • Installing symfony/yaml (v2.1.4)
            Downloading: 100%
          • Installing phpunit/php-text-template (1.1.4)
            Downloading: 100%
          • Installing phpunit/phpunit-mock-objects (1.2.2)
            Downloading: 100%
          • Installing phpunit/php-timer (1.0.4)
            Downloading: 100%
          • Installing phpunit/php-token-stream (1.1.5)
            Downloading: 100%
          • Installing phpunit/php-file-iterator (1.3.3)
            Downloading: 100%
          • Installing phpunit/php-code-coverage (1.2.7)
            Downloading: 100%
          • Installing phpunit/phpunit (3.7.10)
            Downloading: 100%
          • Installing phpunit/dbunit (1.2.1)
            Downloading: 100%

          phpunit/php-code-coverage suggests installing ext-xdebug (>=2.0.5)
          phpunit/phpunit suggests installing phpunit/php-invoker (>=1.1.0)
          Writing lock file
          Generating autoload files

          Continuing the testing later today

          Show
          Pierre Pichet added a comment - I miss the --dev in Install Was added 3 days ago in the doc. So I just try it C:\Documents and Settings\All Users\Application Data\Composer\bin>Composer insta ll --dev Loading composer repositories with package information Installing dependencies Nothing to install or update Loading composer repositories with package information Installing dev dependencies Installing symfony/yaml (v2.1.4) Downloading: 100% Installing phpunit/php-text-template (1.1.4) Downloading: 100% Installing phpunit/phpunit-mock-objects (1.2.2) Downloading: 100% Installing phpunit/php-timer (1.0.4) Downloading: 100% Installing phpunit/php-token-stream (1.1.5) Downloading: 100% Installing phpunit/php-file-iterator (1.3.3) Downloading: 100% Installing phpunit/php-code-coverage (1.2.7) Downloading: 100% Installing phpunit/phpunit (3.7.10) Downloading: 100% Installing phpunit/dbunit (1.2.1) Downloading: 100% phpunit/php-code-coverage suggests installing ext-xdebug (>=2.0.5) phpunit/phpunit suggests installing phpunit/php-invoker (>=1.1.0) Writing lock file Generating autoload files Continuing the testing later today
          Hide
          Tim Hunt added a comment -

          Show
          Tim Hunt added a comment -
          Hide
          Pierre Pichet added a comment - - edited

          Probably because of my weird installation (see the phpinfo (index1.php.htm)) I don't know how to call phpunit

          C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\phpunit\phpunit\phpunit.php

          Note that composer is recognized at the location of the http://132.208.141.198/moodle_w/ site i.e.
          D:/moodle2/server/moodle/moodle_w

          Show
          Pierre Pichet added a comment - - edited Probably because of my weird installation (see the phpinfo (index1.php.htm)) I don't know how to call phpunit C:\Documents and Settings\All Users\Application Data\Composer\bin\vendor\phpunit\phpunit\phpunit.php Note that composer is recognized at the location of the http://132.208.141.198/moodle_w/ site i.e. D:/moodle2/server/moodle/moodle_w
          Hide
          Pierre Pichet added a comment - - edited

          Tim,
          Its working ...

          Finally just get and put at the right place the correct instructions with the correct / or \ ...

          Si I passed the test

          Show
          Pierre Pichet added a comment - - edited Tim, Its working ... Finally just get and put at the right place the correct instructions with the correct / or \ ... Si I passed the test
          Hide
          Tim Hunt added a comment -

          Great! I am glad you could work it out. Sorry I was too busy to help.

          Show
          Tim Hunt added a comment - Great! I am glad you could work it out. Sorry I was too busy to help.
          Hide
          Pierre Pichet added a comment - - edited

          Tim,
          Do I understand well that the main phpunit test that I need to modify is

          public function test_euro_style() {
                  $ap = new qtype_numerical_answer_processor(array(), false, ',', ' ');
          
                  $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000'));
                  $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159'));
              }
          

          inside numerical/tests/answerprocessor_test.php
          I should also modify the is_complete_response(array $response) { test as we will drop the last step

          if ($this->ap->contains_thousands_seaparator($response['answer'])) {

          However this was apparently not tested

          public function test_is_complete_response() {
                  $question = test_question_maker::make_question('numerical');
          
                  $this->assertFalse($question->is_complete_response(array()));
                  $this->assertTrue($question->is_complete_response(array('answer' => '0')));
                  $this->assertTrue($question->is_complete_response(array('answer' => 0)));
                  $this->assertFalse($question->is_complete_response(array('answer' => 'test')));
              }
          
          Show
          Pierre Pichet added a comment - - edited Tim, Do I understand well that the main phpunit test that I need to modify is public function test_euro_style() { $ap = new qtype_numerical_answer_processor(array(), false, ',', ' '); $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000')); $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159')); } inside numerical/tests/answerprocessor_test.php I should also modify the is_complete_response(array $response) { test as we will drop the last step if ($this->ap->contains_thousands_seaparator($response ['answer'] )) { However this was apparently not tested public function test_is_complete_response() { $question = test_question_maker::make_question('numerical'); $this->assertFalse($question->is_complete_response(array())); $this->assertTrue($question->is_complete_response(array('answer' => '0'))); $this->assertTrue($question->is_complete_response(array('answer' => 0))); $this->assertFalse($question->is_complete_response(array('answer' => 'test'))); }
          Hide
          Tim Hunt added a comment -

          I think you are correct about which tests need to be modified.

          Show
          Tim Hunt added a comment - I think you are correct about which tests need to be modified.
          Hide
          Pierre Pichet added a comment -

          The following tests are OK

           
              public function test_euro_style() {
                  $ap = new qtype_numerical_answer_processor(array(), false, ',', ' ');
          
                  $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000'));
                  $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000'));
                  $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000'));
                  $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000'));
                  $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000,1'));
                  $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1.000,1'));
                  $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1,000.1'));
                  $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159'));
              }
          

          The space in 1 000 000 is officially not a regular space but a non-breaking space.
          http://en.wikipedia.org/wiki/Non-breaking_space
          However as the response will be typed in a regular input element, I don't think that the normal user will use the keys combination (...) that will be equivalent to this non-breaking space.

          Show
          Pierre Pichet added a comment - The following tests are OK public function test_euro_style() { $ap = new qtype_numerical_answer_processor(array(), false, ',', ' '); $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000')); $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000,1')); $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1.000,1')); $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1,000.1')); $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159')); } The space in 1 000 000 is officially not a regular space but a non-breaking space. http://en.wikipedia.org/wiki/Non-breaking_space However as the response will be typed in a regular input element, I don't think that the normal user will use the keys combination (...) that will be equivalent to this non-breaking space.
          Hide
          Pierre Pichet added a comment - - edited

          Adding a specific test_deutsch_style() function to test . as thousand separator and , as decsep.
          The logic used is as long as the deustch user use a single . as thousand separator ( i.e. 1.000,1) he MUST simultanously use the , decsep so the code can identify the . as a thousand separator.
          With larger numbers which necessitate more than 1 thousand separator (i.e 1 million 1.000.000 ) the decsep is not necessary.

          Show
          Pierre Pichet added a comment - - edited Adding a specific test_deutsch_style() function to test . as thousand separator and , as decsep. The logic used is as long as the deustch user use a single . as thousand separator ( i.e. 1.000,1) he MUST simultanously use the , decsep so the code can identify the . as a thousand separator. With larger numbers which necessitate more than 1 thousand separator (i.e 1 million 1.000.000 ) the decsep is not necessary.
          Hide
          Pierre Pichet added a comment -

          The phpunit tests seem OK.

          Show
          Pierre Pichet added a comment - The phpunit tests seem OK.
          Hide
          Pierre Pichet added a comment -

          This works correctly with numerical but not when editing with cloze although it is Ok when answering the question (response 1.000,1 is correctly interpreted as 1000.1 )
          So I will get back to the drawing board and take a look to the cloze edit form validation.
          Will also test calculateds.

          Show
          Pierre Pichet added a comment - This works correctly with numerical but not when editing with cloze although it is Ok when answering the question (response 1.000,1 is correctly interpreted as 1000.1 ) So I will get back to the drawing board and take a look to the cloze edit form validation. Will also test calculateds.
          Hide
          Pierre Pichet added a comment -

          The test done in cloze is the following.

          if ($subquestion->qtype == 'numerical' &&
                                                  !(is_numeric($trimmedanswer) || $trimmedanswer == '*')) {
                                              $this->_form->setElementError($prefix.'answer['.$key.']',
                                                      get_string('answermustbenumberorstar',
                                                              'qtype_numerical'));
                                          }
          

          this is another bug MDL-29691 so it should solved separately .

          Show
          Pierre Pichet added a comment - The test done in cloze is the following. if ($subquestion->qtype == 'numerical' && !(is_numeric($trimmedanswer) || $trimmedanswer == '*')) { $this->_form->setElementError($prefix.'answer['.$key.']', get_string('answermustbenumberorstar', 'qtype_numerical')); } this is another bug MDL-29691 so it should solved separately .
          Hide
          Pierre Pichet added a comment -

          For calculated simple the formula being pure php code does not tolerate thousandseps or decsep other than .
          The response however as for the cloze is interpreted with the new code so 1.000,1 is given the same 100% grade as 1000.1

          Things seems OK for further testing by someone else than me.

          Show
          Pierre Pichet added a comment - For calculated simple the formula being pure php code does not tolerate thousandseps or decsep other than . The response however as for the cloze is interpreted with the new code so 1.000,1 is given the same 100% grade as 1000.1 Things seems OK for further testing by someone else than me.
          Hide
          Pierre Pichet added a comment - - edited

          We should note that this solution is user-friendly in a specific way.
          When editing the question the user can write the answer as 1.000,1.
          The answer saved is the correct php integer format i.e. 1000.1
          The student can write 1.000,1 as answer but the correct answer in numerical shown will be 1000.1 or 1000,1 following the language setting decsep.
          However the correct answer is not decsep compliant in multianswer rendering.

          As the thousand seps were not allowed, they are not implemented in the correct answer rendering.

          Show
          Pierre Pichet added a comment - - edited We should note that this solution is user-friendly in a specific way. When editing the question the user can write the answer as 1.000,1. The answer saved is the correct php integer format i.e. 1000.1 The student can write 1.000,1 as answer but the correct answer in numerical shown will be 1000.1 or 1000,1 following the language setting decsep. However the correct answer is not decsep compliant in multianswer rendering. As the thousand seps were not allowed, they are not implemented in the correct answer rendering.
          Hide
          Pierre Pichet added a comment - - edited

          "However the correct answer is not decsep compliant in multianswer rendering."
          This is related to the not correct call in multianswer rendering

          if ($subq->qtype->name() == 'shortanswer') {
                      $correctanswer = $subq->get_matching_answer($subq->get_correct_response());
                  } else {
                      $correctanswer = $subq->get_correct_answer();
                  }
          

          in numerical renderer

            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);
                  if ($question->unitdisplay != qtype_numerical::UNITNONE) {
                      $response = $question->ap->add_unit($response);
                  }
          
                  return get_string('correctansweris', 'qtype_shortanswer', $response);
              }
          

          which cannot be used as is given

                  $feedbackpopup = $this->feedback_popup($subq, $matchinganswer->fraction,
                          $subq->format_text($matchinganswer->feedback, $matchinganswer->feedbackformat,
                                  $qa, 'question', 'answerfeedback', $matchinganswer->id),
                          s($correctanswer->answer), $options);
          
          

          The simplest solution will be to copy in multianswer the part of the code that write the decsep

                  $response = str_replace('.', $question->ap->get_point(), $answer->answer);
          

          I will try and give the result.

          Show
          Pierre Pichet added a comment - - edited "However the correct answer is not decsep compliant in multianswer rendering." This is related to the not correct call in multianswer rendering if ($subq->qtype->name() == 'shortanswer') { $correctanswer = $subq->get_matching_answer($subq->get_correct_response()); } else { $correctanswer = $subq->get_correct_answer(); } in numerical renderer 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); if ($question->unitdisplay != qtype_numerical::UNITNONE) { $response = $question->ap->add_unit($response); } return get_string('correctansweris', 'qtype_shortanswer', $response); } which cannot be used as is given $feedbackpopup = $this->feedback_popup($subq, $matchinganswer->fraction, $subq->format_text($matchinganswer->feedback, $matchinganswer->feedbackformat, $qa, 'question', 'answerfeedback', $matchinganswer->id), s($correctanswer->answer), $options); The simplest solution will be to copy in multianswer the part of the code that write the decsep $response = str_replace('.', $question->ap->get_point(), $answer->answer); I will try and give the result.
          Hide
          Pierre Pichet added a comment -

          If you agree with the logic, this is ready to go ...

          Show
          Pierre Pichet added a comment - If you agree with the logic, this is ready to go ...
          Hide
          Tim Hunt added a comment -

          You definitely need to run codechecker (https://github.com/moodlehq/moodle-local_codechecker). The integrators will reject the patch as it is.

          Apart from that, this change looks good.

          I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it.

          Show
          Tim Hunt added a comment - You definitely need to run codechecker ( https://github.com/moodlehq/moodle-local_codechecker ). The integrators will reject the patch as it is. Apart from that, this change looks good. I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Using commas as thousand separator is evil ! it should be punished by law In fact using anything other than nothing or space is evil !

          Show
          Jean-Michel Vedrine added a comment - - edited Using commas as thousand separator is evil ! it should be punished by law In fact using anything other than nothing or space is evil !
          Hide
          Tim Hunt added a comment -

          OK, so looking in our live database, wo have 211712 responses to numerical questions.

          (It seems that the most response is 5, given 7255 times.)

          I see about 115 responses that are like 445,000, where ',' is almost certainly being used as a thousand separator.

          I also see many more rows where people are writing decimal numbers with a ',' as decimal point. This includes some like '0,821' which, becuase of the leading 0, is clearly a decimal even though it has three digits after the ','.

          So, on the whole, I think this change is OK.

          Show
          Tim Hunt added a comment - OK, so looking in our live database, wo have 211712 responses to numerical questions. (It seems that the most response is 5, given 7255 times.) I see about 115 responses that are like 445,000, where ',' is almost certainly being used as a thousand separator. I also see many more rows where people are writing decimal numbers with a ',' as decimal point. This includes some like '0,821' which, becuase of the leading 0, is clearly a decimal even though it has three digits after the ','. So, on the whole, I think this change is OK.
          Hide
          Tim Hunt added a comment -

          SELECT qasd.value, COUNT(1) AS frequency
          FROM mdl_question_attempt_step_data qasd
          JOIN mdl_question_attempt_steps qas ON qas.id = qasd.attemptstepid
          JOIN mdl_question_attempts qa ON qa.id = qas.questionattemptid
          JOIN mdl_question q ON q.id = qa.questionid
          WHERE q.qtype = 'numerical'
          AND qasd.name = 'answer'
          GROUP BY qasd.value
          ORDER BY frequency DESC

          if you want to try this on your own database.

          Show
          Tim Hunt added a comment - SELECT qasd.value, COUNT(1) AS frequency FROM mdl_question_attempt_step_data qasd JOIN mdl_question_attempt_steps qas ON qas.id = qasd.attemptstepid JOIN mdl_question_attempts qa ON qa.id = qas.questionattemptid JOIN mdl_question q ON q.id = qa.questionid WHERE q.qtype = 'numerical' AND qasd.name = 'answer' GROUP BY qasd.value ORDER BY frequency DESC if you want to try this on your own database.
          Hide
          Pierre Pichet added a comment -

          I will install the code checker and work on this not until friday.

          Show
          Pierre Pichet added a comment - I will install the code checker and work on this not until friday.
          Hide
          Pierre Pichet added a comment -

          "I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it."

          The student will not be incited to write the thousand separator as the correct response will not show any thousand separator.
          The correct answer is the number as written by php following a math operation so without any formatting apart the decsep or the E .

          Show
          Pierre Pichet added a comment - "I still worry that an OU accounting student who types £1,000 will have that treated as £1, but that is the only case that is handled wrongly, and I cannot see any way around that, so I think we have to live with it." The student will not be incited to write the thousand separator as the correct response will not show any thousand separator. The correct answer is the number as written by php following a math operation so without any formatting apart the decsep or the E .
          Hide
          Pierre Pichet added a comment -

          I finally got some time and pass the three files on code checker.
          for tests/answerprocessor_test.phph I got only the warning
          ····public·function·parse_response($response)·{
          31: Useless method overriding detected

          I pass again the phpunit for the numerical answerprocessor_text and everything remains OK

          I push the new version on GitHub.

          So it seems ready to tests.

          As in this code we eliminate the incomplete answer test related to the use of thousands seps, it could be useful
          to push to master this one first before finishing MDL-29691 which will use the apply_units function in multianswer.

          In all cases, what is the next step ?

          Show
          Pierre Pichet added a comment - I finally got some time and pass the three files on code checker. for tests/answerprocessor_test.phph I got only the warning ····public·function·parse_response($response)·{ 31: Useless method overriding detected I pass again the phpunit for the numerical answerprocessor_text and everything remains OK I push the new version on GitHub. So it seems ready to tests. As in this code we eliminate the incomplete answer test related to the use of thousands seps, it could be useful to push to master this one first before finishing MDL-29691 which will use the apply_units function in multianswer. In all cases, what is the next step ?
          Hide
          Tim Hunt added a comment -

          The "Useless method overriding detected" message is a common false-positive from codechecker. Since it is only checking one file at a time, in cannot see that the overriding is useful: It changes protected in the base class to public.

          The next step is that I review the code. ... Done.

          Then, if I am happy, I submit it for integration.

          Ah! one more thing is missing before I can submit this for integration. You need to write some "Testing Instructions" to explain to someone how to test this feature. Can you do that?

          Show
          Tim Hunt added a comment - The "Useless method overriding detected" message is a common false-positive from codechecker. Since it is only checking one file at a time, in cannot see that the overriding is useful: It changes protected in the base class to public. The next step is that I review the code. ... Done. Then, if I am happy, I submit it for integration. Ah! one more thing is missing before I can submit this for integration. You need to write some "Testing Instructions" to explain to someone how to test this feature. Can you do that?
          Hide
          Pierre Pichet added a comment -

          I updated the testing instruction field.

          Show
          Pierre Pichet added a comment - I updated the testing instruction field.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Pierre Pichet added a comment -

          I rebased MDL-29691 although the process does not work easily and trying to rebase MDL-33105 these problems continue.
          I try it verbose and it stop somewhere in the process (create mode ...)

          Can I do something similar to rebase using cherry-pick ?

          Show
          Pierre Pichet added a comment - I rebased MDL-29691 although the process does not work easily and trying to rebase MDL-33105 these problems continue. I try it verbose and it stop somewhere in the process (create mode ...) Can I do something similar to rebase using cherry-pick ?
          Hide
          Pierre Pichet added a comment -

          I did a cherry-pick and this seems to work ?

          https://github.com/ppichet/moodle/compare/MDL-33105

          Show
          Pierre Pichet added a comment - I did a cherry-pick and this seems to work ? https://github.com/ppichet/moodle/compare/MDL-33105
          Hide
          Tim Hunt added a comment -

          That looks correct. Thanks. I don't know why rebase did not work.

          Show
          Tim Hunt added a comment - That looks correct. Thanks. I don't know why rebase did not work.
          Hide
          Pierre Pichet added a comment -

          If we look at the last version of GIT for developper.
          http://docs.moodle.org/dev/Git_for_developers#Applying_changes_from_one_branch_to_another
          Given that we put the modifications in a single commit, cherry-picking appeared to me equivalent to rebasing.
          I effectively applied the old master commit to the new master as I would apply it to another Moodle version.

          Show
          Pierre Pichet added a comment - If we look at the last version of GIT for developper. http://docs.moodle.org/dev/Git_for_developers#Applying_changes_from_one_branch_to_another Given that we put the modifications in a single commit, cherry-picking appeared to me equivalent to rebasing. I effectively applied the old master commit to the new master as I would apply it to another Moodle version.
          Hide
          Tim Hunt added a comment -

          Indeed. Cherry-pick and rebase are two different ways to specify very similar operations. (For a given situation, one is usually more natural than the other.)

          Show
          Tim Hunt added a comment - Indeed. Cherry-pick and rebase are two different ways to specify very similar operations. (For a given situation, one is usually more natural than the other.)
          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
          Eloy Lafuente (stronk7) added a comment -

          ping?

          Show
          Eloy Lafuente (stronk7) added a comment - ping?
          Hide
          Tim Hunt added a comment -

          Sorry, I thought I replied to this. This one should not be back-ported. It changes the behaviour in a few edge cases in a way that I don't think is suitable for stable branches.

          Show
          Tim Hunt added a comment - Sorry, I thought I replied to this. This one should not be back-ported. It changes the behaviour in a few edge cases in a way that I don't think is suitable for stable branches.
          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
          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 only. Thanks Pierre.

          Show
          Dan Poltawski added a comment - Integrated to master only. Thanks Pierre.
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Test passed.
          Hide
          Pierre Pichet added a comment -

          See my last this last comment on MDL-29691
          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.
          $value should be respanced by $response.
          This imply to test for a thousandsep which could be not , . or regular space like halfspace.
          so I need to add additional test.
          I suggest you remove MDL-33105 so I can correct it and add supplementary tests

          Show
          Pierre Pichet added a comment - See my last this last comment on MDL-29691 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. $value should be respanced by $response. This imply to test for a thousandsep which could be not , . or regular space like halfspace. so I need to add additional test. I suggest you remove MDL-33105 so I can correct it and add supplementary tests
          Hide
          Dan Poltawski added a comment -

          Setting this to failed.

          So, am I understanding correctly that you think this should be reverted?

          Show
          Dan Poltawski added a comment - Setting this to failed. So, am I understanding correctly that you think this should be reverted?
          Hide
          Dan Poltawski added a comment -

          OK, reading again it seems clear, so reverting this change and reopening.

          Show
          Dan Poltawski added a comment - OK, reading again it seems clear, so reverting this change and reopening.
          Hide
          Dan Poltawski added a comment -

          I've reverted this change now. Thanks.

          Show
          Dan Poltawski added a comment - I've reverted this change now. Thanks.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Pierre Pichet added a comment -

          Thanks, you understand correctly and sorry for my typing errors...

          Show
          Pierre Pichet added a comment - Thanks, you understand correctly and sorry for my typing errors...
          Hide
          Pierre Pichet added a comment -

          I correct the code and add a new test for unusual thousandsep.
          Unit test Ok, code checker OK.
          As MDL-29691 is now on master, tests should include use of numerical in multianswer questiontype.
          More later on this

          Show
          Pierre Pichet added a comment - I correct the code and add a new test for unusual thousandsep. Unit test Ok, code checker OK. As MDL-29691 is now on master, tests should include use of numerical in multianswer questiontype. More later on this
          Hide
          Tim Hunt added a comment -

          Let me know when you have updated the testing instructions, and think this is ready for integration.

          Show
          Tim Hunt added a comment - Let me know when you have updated the testing instructions, and think this is ready for integration.
          Hide
          Pierre Pichet added a comment -

          On testing I discover that multianswer does not decode correctly number like 1 000,1 although it is OK for 1000,1
          The tests were done on MDL-29691 with 1000,1 as the numerical does not accept thousandseps.
          So I need to set the multianswer decoding ( I will create a new bug) before adding MDL-33105.

          Show
          Pierre Pichet added a comment - On testing I discover that multianswer does not decode correctly number like 1 000,1 although it is OK for 1000,1 The tests were done on MDL-29691 with 1000,1 as the numerical does not accept thousandseps. So I need to set the multianswer decoding ( I will create a new bug) before adding MDL-33105 .
          Hide
          Pierre Pichet added a comment -

          If I change the following lines in multianswer/questiontype.php

          @@ -260,11 +260,11 @@ define('ANSWER_ALTERNATIVE_REGEX_ANSWER', 3);
           define('ANSWER_ALTERNATIVE_REGEX_FEEDBACK', 5);
           
           // NUMBER_FORMATED_ALTERNATIVE_ANSWER_REGEX is used
           // for identifying numerical answers in ANSWER_ALTERNATIVE_REGEX_ANSWER
           define('NUMBER_REGEX',
          -        '-?(([0-9]+[.,]?[0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)');
          +        '-?(([0-9]+[., ]?[., 0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)');
           define('NUMERICAL_ALTERNATIVE_REGEX',
                   '^(' . NUMBER_REGEX . ')(:' . NUMBER_REGEX . ')?$');
           
           // Parenthesis positions for NUMERICAL_FORMATED_ALTERNATIVE_ANSWER_REGEX
           define('NUMERICAL_CORRECT_ANSWER', 1);
          

          then the decoding will correctly locate the answer and the tolerance in
          1 234.56 :0.1 or 1,234.56 or ....
          so the following numerical $ap->apply_units will give to the answer the correct value.

          Effectively the ,.or space can be mixed in any combinations.
          As we want flexibility and we do not grade number format, this is coherent.

          You could probably suggest a better regexp.

          Once the regexp set, I will build the supplementary tests.

          Show
          Pierre Pichet added a comment - If I change the following lines in multianswer/questiontype.php @@ -260,11 +260,11 @@ define('ANSWER_ALTERNATIVE_REGEX_ANSWER', 3); define('ANSWER_ALTERNATIVE_REGEX_FEEDBACK', 5); // NUMBER_FORMATED_ALTERNATIVE_ANSWER_REGEX is used // for identifying numerical answers in ANSWER_ALTERNATIVE_REGEX_ANSWER define('NUMBER_REGEX', - '-?(([0-9]+[.,]?[0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)'); + '-?(([0-9]+[., ]?[., 0-9]*|[.,][0-9]+)([eE][-+]?[0-9]+)?)'); define('NUMERICAL_ALTERNATIVE_REGEX', '^(' . NUMBER_REGEX . ')(:' . NUMBER_REGEX . ')?$'); // Parenthesis positions for NUMERICAL_FORMATED_ALTERNATIVE_ANSWER_REGEX define('NUMERICAL_CORRECT_ANSWER', 1); then the decoding will correctly locate the answer and the tolerance in 1 234.56 :0.1 or 1,234.56 or .... so the following numerical $ap->apply_units will give to the answer the correct value. Effectively the ,.or space can be mixed in any combinations. As we want flexibility and we do not grade number format, this is coherent. You could probably suggest a better regexp. Once the regexp set, I will build the supplementary tests.
          Hide
          Tim Hunt added a comment -

          That is regex is probably OK.

          Show
          Tim Hunt added a comment - That is regex is probably OK.
          Hide
          Pierre Pichet added a comment - - edited

          On further testing I realize that preceeding apply_unit was very strict about unit and number formatting.

          I need to test further how the new code handle bad responses or bad answer definition in creating multianswer numerical questions.

          I mostly tested that it was able to handle new numerical formats..

          Show
          Pierre Pichet added a comment - - edited On further testing I realize that preceeding apply_unit was very strict about unit and number formatting. I need to test further how the new code handle bad responses or bad answer definition in creating multianswer numerical questions. I mostly tested that it was able to handle new numerical formats..
          Hide
          Pierre Pichet added a comment -

          I just updated to actual master.
          I minimize relations to other question types.
          Some code checker errors are solved in bugs.

          Do you plan to put this on 2.5.1 ?
          Unit tests are OK.

          Show
          Pierre Pichet added a comment - I just updated to actual master. I minimize relations to other question types. Some code checker errors are solved in bugs. Do you plan to put this on 2.5.1 ? Unit tests are OK.
          Hide
          Pierre Pichet added a comment -

          Tim,

          Closing for holidays June 15 - Aug 15 as I will not have access to nothing more than an Ipad in this period.

          This weekend I will rebase to this week moodle versions.

          To which version(s) should I rebase if you think that this is ready for testing and integrate.

          If something goes wrong, I could possibly correct it in the next week.

          Show
          Pierre Pichet added a comment - Tim, Closing for holidays June 15 - Aug 15 as I will not have access to nothing more than an Ipad in this period. This weekend I will rebase to this week moodle versions. To which version(s) should I rebase if you think that this is ready for testing and integrate. If something goes wrong, I could possibly correct it in the next week.
          Hide
          Tim Hunt added a comment -

          Sorry, Pierre. I have badly behind on peer review. I will look later this afternoon.

          Show
          Tim Hunt added a comment - Sorry, Pierre. I have badly behind on peer review. I will look later this afternoon.
          Hide
          Tim Hunt added a comment -

          1. Fundamentally the new code looks good, so it is nearly ready for integration.

          but,

          2. You need to run codechecker on your changes.

          3. You don't need to change to list($value, $unit, $mult) = $this->ap->apply_units($x); It is OK if the list(...) bit of a line of code like this is shorter than the array returned.

          4. I think the testing instructions above would be easier to follow if you gave more explicit instructions for creating and testing one questions. You can then put at the end "try some other variants as used in the phpunit test, like -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159"

          Show
          Tim Hunt added a comment - 1. Fundamentally the new code looks good, so it is nearly ready for integration. but, 2. You need to run codechecker on your changes. 3. You don't need to change to list($value, $unit, $mult) = $this->ap->apply_units($x); It is OK if the list(...) bit of a line of code like this is shorter than the array returned. 4. I think the testing instructions above would be easier to follow if you gave more explicit instructions for creating and testing one questions. You can then put at the end "try some other variants as used in the phpunit test, like -1 000 -1 000 000 -1.000.000 -1,000,000 -1000,100 -1.000,100 -1,000.100 3,14159"
          Hide
          Tim Hunt added a comment -

          Oops! I forgot to update the workflow state.

          Show
          Tim Hunt added a comment - Oops! I forgot to update the workflow state.
          Hide
          Pierre Pichet added a comment - - edited

          Tim,
          In edit_numerical the tolerance is not tested the same way as the answer

          if (!$this->is_valid_answer($trimmedanswer, $data)) {
                              $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer);
                          }
                          if ($data['fraction'][$key] == 1) {
                              $maxgrade = true;
                          }
                          if ($answer !== '*' && !is_numeric($data['tolerance'][$key])) {
                              $errors['answeroptions['.$key.']'] =
                                      get_string('xmustbenumeric', 'qtype_numerical',
                                          get_string('acceptederror', 'qtype_numerical'));
                          }
                      } else if ($data['fraction'][$key] != 0 ||
                              !html_is_blank($data['feedback'][$key]['text'])) {
                          $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer);
                          $answercount++;
                      }
          

          $repeatedoptions['tolerance']['type'] = PARAM_FLOAT;
          $repeatedoptions['answer']['type'] = PARAM_RAW;
          I modified the code so that tolerance value can be edited as the answer although without units...
          I will add some PHPunit tests and testing instructions.

          Show
          Pierre Pichet added a comment - - edited Tim, In edit_numerical the tolerance is not tested the same way as the answer if (!$this->is_valid_answer($trimmedanswer, $data)) { $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer); } if ($data['fraction'][$key] == 1) { $maxgrade = true; } if ($answer !== '*' && !is_numeric($data['tolerance'][$key])) { $errors['answeroptions['.$key.']'] = get_string('xmustbenumeric', 'qtype_numerical', get_string('acceptederror', 'qtype_numerical')); } } else if ($data['fraction'][$key] != 0 || !html_is_blank($data['feedback'][$key]['text'])) { $errors['answeroptions[' . $key . ']'] = $this->valid_answer_message($trimmedanswer); $answercount++; } $repeatedoptions ['tolerance'] ['type'] = PARAM_FLOAT; $repeatedoptions ['answer'] ['type'] = PARAM_RAW; I modified the code so that tolerance value can be edited as the answer although without units... I will add some PHPunit tests and testing instructions.
          Hide
          Pierre Pichet added a comment -

          As form_test.php

          public function test_is_valid_number() {
                  $this->assertTrue($this->form->is_valid_number('1,001'));
                  $this->assertTrue($this->form->is_valid_number('1.001'));
                  $this->assertTrue($this->form->is_valid_number('1'));
                  $this->assertTrue($this->form->is_valid_number('1,e8'));
                  $this->assertFalse($this->form->is_valid_number('1001 xxx'));
                  $this->assertTrue($this->form->is_valid_number('1.e8'));
              }
          

          is testing is_valid_number() where the main test are done no supplementary tests appear necessary.

          Show
          Pierre Pichet added a comment - As form_test.php public function test_is_valid_number() { $this->assertTrue($this->form->is_valid_number('1,001')); $this->assertTrue($this->form->is_valid_number('1.001')); $this->assertTrue($this->form->is_valid_number('1')); $this->assertTrue($this->form->is_valid_number('1,e8')); $this->assertFalse($this->form->is_valid_number('1001 xxx')); $this->assertTrue($this->form->is_valid_number('1.e8')); } is testing is_valid_number() where the main test are done no supplementary tests appear necessary.
          Hide
          Pierre Pichet added a comment -

          Looking back I realize that I forgot to add the modifications in edit_multianswer_form for handling tolerances that I have done 3 months ago.
          https://github.com/ppichet/moodle/tree/MDL-33105c
          I will merge these in the final proposition.

          P.S. Working after midnight has some incoveniences...

          Show
          Pierre Pichet added a comment - Looking back I realize that I forgot to add the modifications in edit_multianswer_form for handling tolerances that I have done 3 months ago. https://github.com/ppichet/moodle/tree/MDL-33105c I will merge these in the final proposition. P.S. Working after midnight has some incoveniences...
          Hide
          Pierre Pichet added a comment -

          Finally I add the same flexibity when writing tolerance values.
          Only master have been updated.
          I will complete the testing instructions tomorrow.

          Show
          Pierre Pichet added a comment - Finally I add the same flexibity when writing tolerance values. Only master have been updated. I will complete the testing instructions tomorrow.
          Hide
          Pierre Pichet added a comment -

          sample questions for testing

          Show
          Pierre Pichet added a comment - sample questions for testing
          Hide
          Pierre Pichet added a comment -

          new examples to use when testing.
          There are 2 versions of the numerical question

          a first one with a unit (cm).
          the "fill the correct response" 1000.1 cm will not change when setting another language as french
          a second one without unit (sans unité)
          the "fill the correct response" will be
          will be 1000.1 when setting to english
          and
          1000,1 when setting another language as french

          The problem when there are a unit will be set (if not already done) when "finishing" MDL-31680

          Show
          Pierre Pichet added a comment - new examples to use when testing. There are 2 versions of the numerical question a first one with a unit (cm). the "fill the correct response" 1000.1 cm will not change when setting another language as french a second one without unit (sans unité) the "fill the correct response" will be will be 1000.1 when setting to english and 1000,1 when setting another language as french The problem when there are a unit will be set (if not already done) when "finishing" MDL-31680
          Hide
          Pierre Pichet added a comment -

          moodle xml version

          Show
          Pierre Pichet added a comment - moodle xml version
          Hide
          Pierre Pichet added a comment -

          Everything is OK on master.

          Show
          Pierre Pichet added a comment - Everything is OK on master.
          Hide
          Tim Hunt added a comment -

          Great! We got there in the end. Thanks for your persistence Pierre. I am now submitting this for integration.

          Show
          Tim Hunt added a comment - Great! We got there in the end. Thanks for your persistence Pierre. I am now submitting this for integration.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 -

          I'm stopping this as i'd prefer to have Tim about when integrating this change.

          Show
          Dan Poltawski added a comment - I'm stopping this as i'd prefer to have Tim about when integrating this change.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks Pierre

          Show
          Dan Poltawski added a comment - Integrated to master, thanks Pierre
          Hide
          Frédéric Massart added a comment -

          Hi guys, sorry but I am failing this test for now.

          I found out that the thousands separator is not properly used in some cases. I was using the standard English language pack, but using the values:

          • 10,000
          • 200,000

          The comma is considered as a decimal separator, which should not be the case in en_US... though it is the right behaviour for fr_FR for example...

          Also, I noticed that the unit penalty and unit multiplier do not support the same number format as the answers. Isn't that a bit confusing? If I can use a comma as a decsep, which not there?

          I have to admit that I am not entirely convinced by the flexibility here. Of course I understand the purpose and I think that would be great if we could add any number in any format, but is that possible without randomising the behaviours?

          Not to mention that throughout Moodle we do not have this level of flexibility, and I think this could really confuse a teacher/student that can use a format in one place, but not in the other. From a user point of view there is no differentiation between a quiz or any other part of Moodle. How is the gradebook behaving by the way?

          I also found the code a bit confusing as a developer as, again, the flexibility is high and so the unit tests cases, though passing decsep and thousandsep to the answer processor, were not failing using other formats.

          Here is a patch adding some more tests to the unit tests:

          diff --git a/question/type/numerical/tests/answerprocessor_test.php b/question/type/numerical/tests/answerprocessor_test.php
          index 000df7a..cf88d46 100644
          --- a/question/type/numerical/tests/answerprocessor_test.php
          +++ b/question/type/numerical/tests/answerprocessor_test.php
          @@ -144,6 +144,8 @@ class qtype_numerical_answer_processor_test extends advanced_testcase {
                   $ap = new qtype_numerical_answer_processor(array(), false, ',', '.');
           
                   $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000'));
          +        $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10.000'));
          +        $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100.000'));
                   $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000'));
                   $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000'));
                   $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000,'));
          @@ -153,6 +155,20 @@ class qtype_numerical_answer_processor_test extends advanced_testcase {
                   $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159'));
                   $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159'));
               }
          +
          +    public function test_us_style() {
          +        $ap = new qtype_numerical_answer_processor(array(), false, '.', ',');
          +
          +        $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1000'));
          +        $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10,000'));
          +        $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100,000'));
          +        $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000'));
          +        $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000.'));
          +        $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000.100'));
          +        $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159'));
          +        $this->assertEquals(array(30141.59, '', null), $ap->apply_units('30,141.59'));
          +    }
          +
               public function test_unusual_thousandseps_style() {
                   // Test with ' as thousandsep as used for currency in Swizterland.
                   $ap = new qtype_numerical_answer_processor(array(), false, '.', "'");
          

          Thanks all!
          Fred

          Show
          Frédéric Massart added a comment - Hi guys, sorry but I am failing this test for now. I found out that the thousands separator is not properly used in some cases. I was using the standard English language pack, but using the values: 10,000 200,000 The comma is considered as a decimal separator, which should not be the case in en_US... though it is the right behaviour for fr_FR for example... Also, I noticed that the unit penalty and unit multiplier do not support the same number format as the answers. Isn't that a bit confusing? If I can use a comma as a decsep, which not there? I have to admit that I am not entirely convinced by the flexibility here. Of course I understand the purpose and I think that would be great if we could add any number in any format, but is that possible without randomising the behaviours? Not to mention that throughout Moodle we do not have this level of flexibility, and I think this could really confuse a teacher/student that can use a format in one place, but not in the other. From a user point of view there is no differentiation between a quiz or any other part of Moodle. How is the gradebook behaving by the way? I also found the code a bit confusing as a developer as, again, the flexibility is high and so the unit tests cases, though passing decsep and thousandsep to the answer processor, were not failing using other formats. Here is a patch adding some more tests to the unit tests: diff --git a/question/type/numerical/tests/answerprocessor_test.php b/question/type/numerical/tests/answerprocessor_test.php index 000df7a..cf88d46 100644 --- a/question/type/numerical/tests/answerprocessor_test.php +++ b/question/type/numerical/tests/answerprocessor_test.php @@ -144,6 +144,8 @@ class qtype_numerical_answer_processor_test extends advanced_testcase { $ap = new qtype_numerical_answer_processor(array(), false, ',', '.'); $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000')); + $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10.000')); + $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100.000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1 000 000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000')); $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1.000.000,')); @@ -153,6 +155,20 @@ class qtype_numerical_answer_processor_test extends advanced_testcase { $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159')); $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159')); } + + public function test_us_style() { + $ap = new qtype_numerical_answer_processor(array(), false, '.', ','); + + $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1000')); + $this->assertEquals(array(-10000, '', null), $ap->apply_units('-10,000')); + $this->assertEquals(array(-100000, '', null), $ap->apply_units('-100,000')); + $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000')); + $this->assertEquals(array(-1000000, '', null), $ap->apply_units('-1,000,000.')); + $this->assertEquals(array(-1000.1, '', null), $ap->apply_units('-1000.100')); + $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3.14159')); + $this->assertEquals(array(30141.59, '', null), $ap->apply_units('30,141.59')); + } + public function test_unusual_thousandseps_style() { // Test with ' as thousandsep as used for currency in Swizterland. $ap = new qtype_numerical_answer_processor(array(), false, '.', "'"); Thanks all! Fred
          Hide
          Tim Hunt added a comment -

          Fred, I agree "I am not entirely convinced by the flexibility here". However, how long have you been thinking about this issue? Some of us have been worrying about it for years.

          You may think that language packs provide a solution, but you are wrong. Pierre is a teacher who works in Montreal, where they speak/write a mixture of English and French (probably most people speak both), so there will be a mixture of people in any given class, and then can understand the questions, whether they are written in French or English, but will write numbers the way they write them.

          So, if you think you can convince us of a solution that is better than the one proposed here, then please tell us exactly what it is, with your evidence that it will work better. If you are just guessing, then welcome to the world we have been in for the last N years.

          Finally, unit penalty and unit multiplier should be dealt with in a different issue.

          Show
          Tim Hunt added a comment - Fred, I agree "I am not entirely convinced by the flexibility here". However, how long have you been thinking about this issue? Some of us have been worrying about it for years. You may think that language packs provide a solution, but you are wrong. Pierre is a teacher who works in Montreal, where they speak/write a mixture of English and French (probably most people speak both), so there will be a mixture of people in any given class, and then can understand the questions, whether they are written in French or English, but will write numbers the way they write them. So, if you think you can convince us of a solution that is better than the one proposed here, then please tell us exactly what it is, with your evidence that it will work better. If you are just guessing, then welcome to the world we have been in for the last N years. Finally, unit penalty and unit multiplier should be dealt with in a different issue.
          Hide
          Dan Poltawski added a comment -

          I think Fred raises a good point about the inconsistency about this flexibility as a general issue, something i'd not properly considered whilst integrating this issue.

          I'm interested why we think the numerical type is a special case which should be handled differently to the rest of Moodle? Is this inconsistency going to cause more problems, should it instead be handled by the core functionality.

          I don't know and I certainly haven't been considering it at all, but I would be interested in having the rationale outlined here. Is it just because its variability of student answers?

          Show
          Dan Poltawski added a comment - I think Fred raises a good point about the inconsistency about this flexibility as a general issue, something i'd not properly considered whilst integrating this issue. I'm interested why we think the numerical type is a special case which should be handled differently to the rest of Moodle? Is this inconsistency going to cause more problems, should it instead be handled by the core functionality. I don't know and I certainly haven't been considering it at all, but I would be interested in having the rationale outlined here. Is it just because its variability of student answers?
          Hide
          Tim Hunt added a comment -

          Dan, Fred, how much stuff do you want to read? As I say, we have been thinking/talking about it for years. There are several tracker issues, forum threads and previous commits, but I am guessing you don't want to read all that.

          I guess you also want a nice simple logical rationale, that I can give you to explain why this solution is right. Well, sorry, there is no solution that is always right. The best we can do is a messy compromise that pleases most of the people most of the time. Maximising the meaning of most. That is what we have here.

          Yes, processing student answers is a special case. We can expect Teachers to spend at least a little time learning the right way to create things in Moodle. Also, teachers get the change to preview their quiz and ensure it is working, and they have a chance to fix it if not. So, for teachers, the standard (though not consistently implemented) PARAM_RAW + unformat_float to parse user input is OK.

          For students, doing a quiz, the situation is different:

          • We want the students to be concentrating on answering the question asked, to be thinking about their learning, not to be thinking about the technicalities of using Moodle.
          • Students are going to be graded on what they typed, with no chance to fix problems. It is much more important that we correctly interpret their meaning (even though it some cases there is genuine ambiguity) rather than that we follow some simple, logical Moodle coding pattern (PARAM_RAW + unformat_float) that works less well.
          Show
          Tim Hunt added a comment - Dan, Fred, how much stuff do you want to read? As I say, we have been thinking/talking about it for years. There are several tracker issues, forum threads and previous commits, but I am guessing you don't want to read all that. I guess you also want a nice simple logical rationale, that I can give you to explain why this solution is right. Well, sorry, there is no solution that is always right. The best we can do is a messy compromise that pleases most of the people most of the time. Maximising the meaning of most. That is what we have here. Yes, processing student answers is a special case. We can expect Teachers to spend at least a little time learning the right way to create things in Moodle. Also, teachers get the change to preview their quiz and ensure it is working, and they have a chance to fix it if not. So, for teachers, the standard (though not consistently implemented) PARAM_RAW + unformat_float to parse user input is OK. For students, doing a quiz, the situation is different: We want the students to be concentrating on answering the question asked, to be thinking about their learning, not to be thinking about the technicalities of using Moodle. Students are going to be graded on what they typed, with no chance to fix problems. It is much more important that we correctly interpret their meaning (even though it some cases there is genuine ambiguity) rather than that we follow some simple, logical Moodle coding pattern (PARAM_RAW + unformat_float) that works less well.
          Hide
          Frédéric Massart added a comment -

          Tim, I am certainly new to this issue, and I am just expressing my thoughts. The reason why I failed this test is because it didn't behave as expected, not because "I am not convinced". The English vs. French is a very good point, and that's exactly where the problem was during my testing. I speak French and I've used commas as a decimal separator for years, but to come back to your example, what is the expected behaviour, in Montral, in a bilingual environment, in a flexible Moodle, when you enter:

          10,001

          • If I speak English, I probably expect: int 10001
          • If I speak French, I expect: float 10.001

          And I don't mention the surprise of a student who ends up failing his quiz because he mistakenly assumed that Moodle knew what he was doing.

          Perhaps the solution does not only reside in the language pack, but when you install Windows/Mac/Linux using a specific locale, you don't expect Excel and friends to guess what format you decided to use when filling in your document, you're expected to use the chosen locale.

          I understand the problematic, and I understand the attempt of solution, and surely it'll help users in Montreal, but it could also create loads of confusion in other environments.

          Perhaps we should work on a core solution rather than a specific numerical question answers, and I personally don't think the unit penalty and multiplier should be dealt with in a different issue. And if they should, then they should be tied together because having 2 different behaviours, especially on the same page, would definitely confuse users more than it will help them.

          What about the HTML5 input element for floats? What rules does it follow?

          Show
          Frédéric Massart added a comment - Tim, I am certainly new to this issue, and I am just expressing my thoughts. The reason why I failed this test is because it didn't behave as expected, not because "I am not convinced". The English vs. French is a very good point, and that's exactly where the problem was during my testing. I speak French and I've used commas as a decimal separator for years, but to come back to your example, what is the expected behaviour, in Montral, in a bilingual environment, in a flexible Moodle, when you enter: 10,001 If I speak English, I probably expect: int 10001 If I speak French, I expect: float 10.001 And I don't mention the surprise of a student who ends up failing his quiz because he mistakenly assumed that Moodle knew what he was doing. Perhaps the solution does not only reside in the language pack, but when you install Windows/Mac/Linux using a specific locale, you don't expect Excel and friends to guess what format you decided to use when filling in your document, you're expected to use the chosen locale. I understand the problematic, and I understand the attempt of solution, and surely it'll help users in Montreal, but it could also create loads of confusion in other environments. Perhaps we should work on a core solution rather than a specific numerical question answers , and I personally don't think the unit penalty and multiplier should be dealt with in a different issue. And if they should, then they should be tied together because having 2 different behaviours, especially on the same page, would definitely confuse users more than it will help them. What about the HTML5 input element for floats? What rules does it follow?
          Hide
          Tim Hunt added a comment -

          I looked at the spec for HTML number input, and it almost completely ignores this issue

          If you look at the data (e.g. review all answers given by all students to all numerical questions in the database of a large Moodle site) then you will find that students hardly ever type thousand separators. Therefore, it is much more important to get decimal handling right.

          Show
          Tim Hunt added a comment - I looked at the spec for HTML number input, and it almost completely ignores this issue If you look at the data (e.g. review all answers given by all students to all numerical questions in the database of a large Moodle site) then you will find that students hardly ever type thousand separators. Therefore, it is much more important to get decimal handling right.
          Hide
          Frédéric Massart added a comment -

          Maybe we should ignore the thousands separator entirely then, and focus on the decimal one, at least for the user input. What scares me is the automatic assumption, I think it's safer, even if annoying for a student, to throw out a validation error once we have some uncertainty (multiple commas, weird pattern, etc...).

          Show
          Frédéric Massart added a comment - Maybe we should ignore the thousands separator entirely then, and focus on the decimal one, at least for the user input. What scares me is the automatic assumption, I think it's safer, even if annoying for a student, to throw out a validation error once we have some uncertainty (multiple commas, weird pattern, etc...).
          Hide
          Damyon Wiese added a comment -

          What about using the HTTP Accept-Language header to get the decsep value from any installed language pack with a fall back to the current language pack setting?

          Show
          Damyon Wiese added a comment - What about using the HTTP Accept-Language header to get the decsep value from any installed language pack with a fall back to the current language pack setting?
          Hide
          Frédéric Massart added a comment -

          What if you have your Moodle in French/Chinese/English but you are forced to use your Uni's computers set to whatever locale? Thus sending a mistakenly wrong Accept-Language header. The behaviour is even less predictable for a user jumping from multiple Moodle instances, which won't all have the same locale installed.

          Show
          Frédéric Massart added a comment - What if you have your Moodle in French/Chinese/English but you are forced to use your Uni's computers set to whatever locale? Thus sending a mistakenly wrong Accept-Language header. The behaviour is even less predictable for a user jumping from multiple Moodle instances, which won't all have the same locale installed.
          Hide
          Tim Hunt added a comment -

          Moodle does use Accept-Language to set the user's current language, if you have turned on that admin option. I don't think any other part of Moodle should touch Accept-Language.

          Fred, "Maybe we should ignore the thousands separator entirely then" what exactly do you mean?

          Show
          Tim Hunt added a comment - Moodle does use Accept-Language to set the user's current language, if you have turned on that admin option. I don't think any other part of Moodle should touch Accept-Language. Fred, "Maybe we should ignore the thousands separator entirely then" what exactly do you mean?
          Hide
          Frédéric Massart added a comment -

          I mean that we should not try to guess what are the thousands separators. We could simply say that if there is

          • 1 comma == decimal separator
          • 1 dot == decimal separator
          • >= 1 dot or >= 1 comma == error

          Of course there could still be some confusion when entering figures using the thousands separator, but I would assume that currently all over Moodle we are not allowing users to enter any thousands separator anywhere. We could set a rule where thousands separators are never accepted, and enforce our users to learn not to use them ever. In fact, rather than being very flexible, we could be helpful... either by throwing validation warnings, or Javascript smart validation (Did you mean 10.000?).

          (Not sure I made myself clear :/)

          Show
          Frédéric Massart added a comment - I mean that we should not try to guess what are the thousands separators. We could simply say that if there is 1 comma == decimal separator 1 dot == decimal separator >= 1 dot or >= 1 comma == error Of course there could still be some confusion when entering figures using the thousands separator, but I would assume that currently all over Moodle we are not allowing users to enter any thousands separator anywhere. We could set a rule where thousands separators are never accepted, and enforce our users to learn not to use them ever. In fact, rather than being very flexible, we could be helpful ... either by throwing validation warnings, or Javascript smart validation (Did you mean 10.000?). (Not sure I made myself clear :/)
          Hide
          Jean-Michel Vedrine added a comment -

          I am sorry but I am completely lost by comments from Frederic, Dan or Daymon !
          Shouldn't this issue concentrate on the situation before and after the patch and also on the title of this issue Numerical is_complete() could return a false on a good response
          It seems to me the discussion is changing on a completely different subject about completely changing the rules for answers in numerical question type

          • If this discussion is to take place surely a new tracker issue should be better
          • If this discussion is to take place surely it should not be when the most knowledgeable person on the subject (Pierre) is on holidays
          • If this discussion is to take place surely the final decision must take care of not break thousands of questions and attempts around the world
          • If this discussion is to take place surely some teachers should be part of it not only developers

          Fortunately it happen I am a teacher
          To summarize most of my students are French, but some are English, German, Spanish and Chinese. Most of my course use French language but each student can choose their own language, but some are in English with no possibility to change the language.
          Imagine you are a Chinese student in one of my course with French content but Moodle interface switched to Chinese, how do you expect answers like 20.45 or 20,45 to be graded ?
          Imagine another Chinese student in the same course but with Moodle interface in French (because he is a good student and want to preactice French as much as he can), would you expect him to be graded differently for the same answers than the previous student ?

          I can continue forever.

          My +1,000 (or +1.000 or + 1 000 or + 1000,00 or +1000.00, or ...) no to make any too quick change without fully understanding the consequences.

          Show
          Jean-Michel Vedrine added a comment - I am sorry but I am completely lost by comments from Frederic, Dan or Daymon ! Shouldn't this issue concentrate on the situation before and after the patch and also on the title of this issue Numerical is_complete() could return a false on a good response It seems to me the discussion is changing on a completely different subject about completely changing the rules for answers in numerical question type If this discussion is to take place surely a new tracker issue should be better If this discussion is to take place surely it should not be when the most knowledgeable person on the subject (Pierre) is on holidays If this discussion is to take place surely the final decision must take care of not break thousands of questions and attempts around the world If this discussion is to take place surely some teachers should be part of it not only developers Fortunately it happen I am a teacher To summarize most of my students are French, but some are English, German, Spanish and Chinese. Most of my course use French language but each student can choose their own language, but some are in English with no possibility to change the language. Imagine you are a Chinese student in one of my course with French content but Moodle interface switched to Chinese, how do you expect answers like 20.45 or 20,45 to be graded ? Imagine another Chinese student in the same course but with Moodle interface in French (because he is a good student and want to preactice French as much as he can), would you expect him to be graded differently for the same answers than the previous student ? I can continue forever. My +1,000 (or +1.000 or + 1 000 or + 1000,00 or +1000.00, or ...) no to make any too quick change without fully understanding the consequences.
          Hide
          Jean-Michel Vedrine added a comment -

          I am completely with Fred on "we could be helpful... either by throwing validation warnings, or Javascript smart validation".
          In the formulas question type where answers can not only be numerical but also algebraic formulas, as soon as the content is illegal (not wrong or correct but illegal according to the parser) a small icon (a red triangular warning sign) appears. This is done using javascript and also as long as the answer's content is correctly parsed the script display how it understand the student answer. See the first screenshot on this page http://code.google.com/p/moodle-coordinate-question/

          Show
          Jean-Michel Vedrine added a comment - I am completely with Fred on "we could be helpful... either by throwing validation warnings, or Javascript smart validation". In the formulas question type where answers can not only be numerical but also algebraic formulas, as soon as the content is illegal (not wrong or correct but illegal according to the parser) a small icon (a red triangular warning sign) appears. This is done using javascript and also as long as the answer's content is correctly parsed the script display how it understand the student answer. See the first screenshot on this page http://code.google.com/p/moodle-coordinate-question/
          Hide
          Dan Poltawski added a comment -

          Is it feasible to do freds idea and enforce only a decimal separator and not allow a thousands separator?

          A few of us have talked about it here and quite like that idea?

          Show
          Dan Poltawski added a comment - Is it feasible to do freds idea and enforce only a decimal separator and not allow a thousands separator? A few of us have talked about it here and quite like that idea?
          Hide
          Tim Hunt added a comment -

          Given the complexity of this issue I feel that "enforce only a decimal separator and not allow a thousands separator" is rather a vague statement. I will happily review/comment on any patch that Fred cares to write, even if it is only a proof concept.

          Show
          Tim Hunt added a comment - Given the complexity of this issue I feel that "enforce only a decimal separator and not allow a thousands separator" is rather a vague statement. I will happily review/comment on any patch that Fred cares to write, even if it is only a proof concept.
          Hide
          Tim Hunt added a comment - - edited

          Actually, Fred should realise his idea will not work. The key example is 10,001 (or 10.001)

          As Fred says

          If I speak English, I probably expect: int 10001
          If I speak French, I expect: float 10.001

          (or vice versa).

          If we could tell what was a decimal separator, or a thousands separator, in the student's response, then this issue would not exist. The whole point is that we can't. Anyway, Fred's suggestion does not work. I wish it did.

          Show
          Tim Hunt added a comment - - edited Actually, Fred should realise his idea will not work. The key example is 10,001 (or 10.001) As Fred says If I speak English, I probably expect: int 10001 If I speak French, I expect: float 10.001 (or vice versa). If we could tell what was a decimal separator, or a thousands separator, in the student's response, then this issue would not exist. The whole point is that we can't. Anyway, Fred's suggestion does not work. I wish it did.
          Hide
          Dan Poltawski added a comment -

          Yeah, I was just coming to that conclusion myself. I see some of the issues is already covered in here MDL-31889.

          I'm not totally convinced here, but I defer to Tim's jugement as quiz maintainer.

          I think Fred's point about consistency is important. When students start marking each others work in workshop, can they continue to use this same flexibility in grade values (or in any other modules where students handle floating point numbers)? Is partial, inconsistent flexibility (over the whole of Moodle) an improvement?

          Show
          Dan Poltawski added a comment - Yeah, I was just coming to that conclusion myself. I see some of the issues is already covered in here MDL-31889 . I'm not totally convinced here, but I defer to Tim's jugement as quiz maintainer. I think Fred's point about consistency is important. When students start marking each others work in workshop, can they continue to use this same flexibility in grade values (or in any other modules where students handle floating point numbers)? Is partial, inconsistent flexibility (over the whole of Moodle) an improvement?
          Hide
          Frédéric Massart added a comment - - edited

          Don't mention my name as if I was on a Crusade, I am just trying to express some concerns about the usability of such a flexibility, I am not suggesting that "my" suggestions are appropriate. Neither that I can/want to provide a patch satisfying everyone...

          On a side note, what I meant by ignoring the thousand separator, was:

          • 1 comma == decimal separator
          • 1 dot == decimal separator
          • >= 1 dot or >= 1 comma == error

          And educating the users in the sense that any separator used would be considered as decimal one, except if when too ambiguous (multiple separators, too many of the same, ...). And not (as it is at the moment), a comma could be both (10,000.00 vs. 10,000).

          Anyway, case closed.

          Show
          Frédéric Massart added a comment - - edited Don't mention my name as if I was on a Crusade, I am just trying to express some concerns about the usability of such a flexibility, I am not suggesting that "my" suggestions are appropriate. Neither that I can/want to provide a patch satisfying everyone... On a side note, what I meant by ignoring the thousand separator, was: 1 comma == decimal separator 1 dot == decimal separator >= 1 dot or >= 1 comma == error And educating the users in the sense that any separator used would be considered as decimal one, except if when too ambiguous (multiple separators, too many of the same, ...). And not (as it is at the moment), a comma could be both (10,000.00 vs. 10,000). Anyway, case closed.
          Hide
          Frédéric Massart added a comment -

          I think we could at least add some failing unit test (commented out) to this issue, to document the problem.

          Show
          Frédéric Massart added a comment - I think we could at least add some failing unit test (commented out) to this issue, to document the problem.
          Hide
          Tim Hunt added a comment -

          I'm sorry Fred. I really should not react like that. In fact, just after I was rude to you, in the next tracker issue Petr made an unfriendly response to one of my peer review comments, and I did not like that, so I really should know better. We are all trying our best to make Moodle better.

          So anyway, the case your suggestion is about is 1,000,000.0, or 1.000.000,0. The point about that case is that it is not ambiguous.

          Also, I think I am getting confused between this issue, and MDL-31889, which is where this issue is addressed properly.

          Show
          Tim Hunt added a comment - I'm sorry Fred. I really should not react like that. In fact, just after I was rude to you, in the next tracker issue Petr made an unfriendly response to one of my peer review comments, and I did not like that, so I really should know better. We are all trying our best to make Moodle better. So anyway, the case your suggestion is about is 1,000,000.0, or 1.000.000,0. The point about that case is that it is not ambiguous. Also, I think I am getting confused between this issue, and MDL-31889 , which is where this issue is addressed properly.
          Hide
          Dan Poltawski added a comment -

          Just attaching this image to demonstrate one of the problems

          Show
          Dan Poltawski added a comment - Just attaching this image to demonstrate one of the problems
          Hide
          Dan Poltawski added a comment -

          And this is what happened before this week, feels like a regression to me:

          Show
          Dan Poltawski added a comment - And this is what happened before this week, feels like a regression to me:
          Hide
          Frédéric Massart added a comment -

          Attaching a Quiz demonstrating the issue I didn't appear to make clear in my previous comments.

          Show
          Frédéric Massart added a comment - Attaching a Quiz demonstrating the issue I didn't appear to make clear in my previous comments.
          Hide
          Tim Hunt added a comment -

          Yes, that is the one case that the new code handles less well than the old code. There are other examples where the reverse is true.

          Do you want to revert this until Pierre gets back from holiday? This is 2.6-only, so we have time.

          Show
          Tim Hunt added a comment - Yes, that is the one case that the new code handles less well than the old code. There are other examples where the reverse is true. Do you want to revert this until Pierre gets back from holiday? This is 2.6-only, so we have time.
          Hide
          Dan Poltawski added a comment -

          OK, i've reverted this now.

          I have to say, the more I think about it, the more I think its an unsolvable problem to interpret their meaning with both notations at once. Additionally it makes me wonder if that is really teaching bad practice (i'm sure such tolerance is allowed on a tax form).

          Would we not be better making it clear what the expected separator is, or something like that to avoid ambiguity?

          I'm asking more questions to this tricky issue..

          Show
          Dan Poltawski added a comment - OK, i've reverted this now. I have to say, the more I think about it, the more I think its an unsolvable problem to interpret their meaning with both notations at once. Additionally it makes me wonder if that is really teaching bad practice (i'm sure such tolerance is allowed on a tax form). Would we not be better making it clear what the expected separator is, or something like that to avoid ambiguity? I'm asking more questions to this tricky issue..
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Tim Hunt added a comment -

          Well, the one question type I know that gets this really right is STACK.

          Maths notation is even more ambiguous than just numbers (e.g. x(t+1), is that a function call or multiplication? what about 2x, or 2½?)

          The approach in STACK is when you types your response, there is some Ajax code saying "Your answer was interpreted as ..." with a nicely rendered equation. You can then either accept that, or cahnge your response if it was misinterpreted.

          Isn't that over-kill for a simple numerical question? Surely it is. But then will anything less do?

          Show
          Tim Hunt added a comment - Well, the one question type I know that gets this really right is STACK. Maths notation is even more ambiguous than just numbers (e.g. x(t+1), is that a function call or multiplication? what about 2x, or 2½?) The approach in STACK is when you types your response, there is some Ajax code saying "Your answer was interpreted as ..." with a nicely rendered equation. You can then either accept that, or cahnge your response if it was misinterpreted. Isn't that over-kill for a simple numerical question? Surely it is. But then will anything less do?
          Hide
          Pierre Pichet added a comment -

          Thanks everybody for their comments.
          A short comment from Paris...
          The rule is that thousandsep should not be used without the decsep although this rule can be relieved if there are more than a single thousandsep.

          This should be shown to the student (or teacher) in some way.

          With this rule we CAN show to the student the correct response in the language set.
          The correct response does not show the thousandsep that are only allowed for convenience on input.
          The numerical and calculated questions are not graded for the format used.

          More on return.
          P.S Living in a bilingual state, my students are allowed to set the moodle course to either french or english.

          Show
          Pierre Pichet added a comment - Thanks everybody for their comments. A short comment from Paris... The rule is that thousandsep should not be used without the decsep although this rule can be relieved if there are more than a single thousandsep. This should be shown to the student (or teacher) in some way. With this rule we CAN show to the student the correct response in the language set. The correct response does not show the thousandsep that are only allowed for convenience on input. The numerical and calculated questions are not graded for the format used. More on return. P.S Living in a bilingual state, my students are allowed to set the moodle course to either french or english.
          Hide
          Pierre Pichet added a comment - - edited

          Moodle 2.6 being solved this weekend, let's go back to this issue.
          We need to agree that the main use of the actual numerical questiontype is to grade the number value within a +- range and not the way the numerical value is written. This should be as much flexible as it could be without breaking the millions of already existing questions.
          So one thousand should continue to be expressed as 1000 1,000.00 1 000, 1.000,
          To remove the ambiguity we need to set a way that the user who want to use the thousand separator should ALWAYS put the decimal sep even if the code could detect the case when the numbrer is 1,000,000. or greater.
          Where do we put this rule ?

          1. in an help button alongside the question text as was done in Moodle 2.0. However this is not easy to use for numerical questions in Cloze questions
          2. as a specific comment that expressed in the good response text the numerical value of the student response as seen by moodle.
          3. or ?
          Show
          Pierre Pichet added a comment - - edited Moodle 2.6 being solved this weekend, let's go back to this issue. We need to agree that the main use of the actual numerical questiontype is to grade the number value within a +- range and not the way the numerical value is written. This should be as much flexible as it could be without breaking the millions of already existing questions. So one thousand should continue to be expressed as 1000 1,000.00 1 000, 1.000, To remove the ambiguity we need to set a way that the user who want to use the thousand separator should ALWAYS put the decimal sep even if the code could detect the case when the numbrer is 1,000,000. or greater. Where do we put this rule ? in an help button alongside the question text as was done in Moodle 2.0. However this is not easy to use for numerical questions in Cloze questions as a specific comment that expressed in the good response text the numerical value of the student response as seen by moodle. or ?
          Hide
          Pierre Pichet added a comment -

          If among your other (effectively more important) duties, you could suggest how to resolve the apparent conflict between

          • the historical flexibility of numerical question grading i.e. 1,000 as 1.000
            and
          • the initial reaction of english user to grade "correctly" a written answer of 1,000 as one thousand .
          Show
          Pierre Pichet added a comment - If among your other (effectively more important) duties, you could suggest how to resolve the apparent conflict between the historical flexibility of numerical question grading i.e. 1,000 as 1.000 and the initial reaction of english user to grade "correctly" a written answer of 1,000 as one thousand .
          Hide
          Tim Hunt added a comment -

          I think the key point is that, in cases where we can still interact with the student, we should not remove the old validation message (https://tracker.moodle.org/browse/MDL-33105?focusedCommentId=229715&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-229715) that encourages the student to input their answer with no thousands separator, and using the right decimal point character for the current language.

          It is only if they ingore that warning, and click Submit all and finish anyway, that we should then do our best to grade their response.

          Does that seem reasonable?

          Show
          Tim Hunt added a comment - I think the key point is that, in cases where we can still interact with the student, we should not remove the old validation message ( https://tracker.moodle.org/browse/MDL-33105?focusedCommentId=229715&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-229715 ) that encourages the student to input their answer with no thousands separator, and using the right decimal point character for the current language. It is only if they ingore that warning, and click Submit all and finish anyway, that we should then do our best to grade their response. Does that seem reasonable?
          Hide
          Pierre Pichet added a comment -

          Effectively not using is the simplest rule but people somehow want to use the thousand sep.

          I will try to figure all the most plausible combinations ( i.e. bilingual courses) and put the result in the developper docs to help us see a clearer picture.
          We could put more efforts in decoding the number as 1,23 or 1,2345 are clearly using , as a decimal point.

          Show
          Pierre Pichet added a comment - Effectively not using is the simplest rule but people somehow want to use the thousand sep. I will try to figure all the most plausible combinations ( i.e. bilingual courses) and put the result in the developper docs to help us see a clearer picture. We could put more efforts in decoding the number as 1,23 or 1,2345 are clearly using , as a decimal point.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated: