Moodle
  1. Moodle
  2. MDL-38239

Importing grades from a .txt file with commas for decimals causes an error

    Details

    • Testing Instructions:
      Hide

      Prerequisites

      1. As a teacher, enter a course with multiples students
      2. Create manual grading element in "Grades"

      Testing instructions

      1. Run the unit tests for the moodlelib_testcase class:
        phpunit moodlelib_testcase lib/tests/moodlelib_test.php
        
      2. Turn the language to one who the decimal separator is different that the one in English (Ex. French using ",").
      3. Export an Excel file (also in French version, with point delimiter set with a comma)
      4. Go to the file and enter many different grades for students:
        1. Put "78.5"
        2. Put "78,5"
        3. Put "100,00" (Be sure Excel do not convert it to "100" if so, edit the text file manually to keep "100,00")
        4. Put "0"
        5. Put "100"
        6. Put "1 00" (Should tested with "1 000" because the space is the thousand separator but the max grade is by default 100)
        7. Put "-12,34" (Should be converted to 0 unless you put the minimum range for the grade under the -12.34)
        8. Put "-12.34" (Should be converted to 0 unless you put the minimum range for the grade under the -12.34)
      5. Save it as a text file (with tabulation as separators)
      6. Import this new file, with Windows-1252 as encoding and tab for separator
      7. Go to the Grader report page
      8. It should all import successfully
      9. Erase all the imported grades
      10. Go to the file, remove all grades and enter this grade for one student:
        1. Enter "Wrong value"
      11. Import this new file, with Windows-1252 as encoding and tab for separator
      12. This should display an error : "Supplied grade is invalid"
      13. Change the language back to English
      14. Go to the file and replace the grade entered by this one:
        1. Put "78,5"
      15. Import this new file, with Windows-1252 as encoding and tab for separator
      16. This should not working by display: "Supplied grade is invalid"
      17. Go to the file and replace the grade entered by this one:
        1. Put "78.5"
      18. Import this new file, with Windows-1252 as encoding and tab for separator
      19. This one should work
      Show
      Prerequisites As a teacher, enter a course with multiples students Create manual grading element in "Grades" Testing instructions Run the unit tests for the moodlelib_testcase class: phpunit moodlelib_testcase lib/tests/moodlelib_test.php Turn the language to one who the decimal separator is different that the one in English (Ex. French using ","). Export an Excel file (also in French version, with point delimiter set with a comma) Go to the file and enter many different grades for students: Put "78.5" Put "78,5" Put "100,00" (Be sure Excel do not convert it to "100" if so, edit the text file manually to keep "100,00") Put "0" Put "100" Put "1 00" (Should tested with "1 000" because the space is the thousand separator but the max grade is by default 100) Put "-12,34" (Should be converted to 0 unless you put the minimum range for the grade under the -12.34) Put "-12.34" (Should be converted to 0 unless you put the minimum range for the grade under the -12.34) Save it as a text file (with tabulation as separators) Import this new file, with Windows-1252 as encoding and tab for separator Go to the Grader report page It should all import successfully Erase all the imported grades Go to the file, remove all grades and enter this grade for one student: Enter "Wrong value" Import this new file, with Windows-1252 as encoding and tab for separator This should display an error : "Supplied grade is invalid" Change the language back to English Go to the file and replace the grade entered by this one: Put "78,5" Import this new file, with Windows-1252 as encoding and tab for separator This should not working by display: "Supplied grade is invalid" Go to the file and replace the grade entered by this one: Put "78.5" Import this new file, with Windows-1252 as encoding and tab for separator This one should work
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-38239-moodle24
    • Pull 2.5 Branch:
      MDL-38239-moodle25
    • Pull Master Branch:
      MDL-38239-master
    • Rank:
      48088

      Description

      When a teacher imports a .txt file with grades, if this one has commas instead of periods for decimals, an error appears: "Supplied grade is invalid".

      To reproduce:

      1. Create manual grading elements in "Grades"
      2. Export an Excel file
      3. Go to the file and enter grades for students
      4. Save it as a .txt (with tabulation as separators)
      5. Import this new file, with Windows-1252 as encoding and tab for separator

      Result: error will appear.

      This has been tested with 2.3.2 and 2.2.x

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Thank you for the bug report. If anyone has any additional details or even a possible code fix please post it here.

          Show
          Andrew Davis added a comment - Thank you for the bug report. If anyone has any additional details or even a possible code fix please post it here.
          Hide
          Helen Foster added a comment -

          Linking to a similar issue.

          Show
          Helen Foster added a comment - Linking to a similar issue.
          Hide
          Gilles-Philippe Leblanc added a comment -

          I just added a patch.

          Show
          Gilles-Philippe Leblanc added a comment - I just added a patch.
          Hide
          Adrian Greeve added a comment -

          Hello Gilles-Philippe,

          Overall the code looks good. I'm just a little puzzled by the following if statement:

          if (floor($unformatedvalue) != $unformatedvalue) {
          

          Why are values like "61,00" invalid when a value like "61.00" is fine?

          Show
          Adrian Greeve added a comment - Hello Gilles-Philippe, Overall the code looks good. I'm just a little puzzled by the following if statement: if (floor($unformatedvalue) != $unformatedvalue) { Why are values like "61,00" invalid when a value like "61.00" is fine?
          Hide
          Gilles-Philippe Leblanc added a comment -

          Here is the modification based on your comment. Te zero decimal values are now accepted. To be reviewed.

          Show
          Gilles-Philippe Leblanc added a comment - Here is the modification based on your comment. Te zero decimal values are now accepted. To be reviewed.
          Hide
          Gilles-Philippe Leblanc added a comment -

          Putting back to developing state, I'll do some Unit test for the functions.

          Show
          Gilles-Philippe Leblanc added a comment - Putting back to developing state, I'll do some Unit test for the functions.
          Hide
          Gilles-Philippe Leblanc added a comment -

          Here is the new patch. I have added PHPUnit tests for unformat_float(), unformat_value() and can_be_unformatted() functions. To be reviewed.

          Show
          Gilles-Philippe Leblanc added a comment - Here is the new patch. I have added PHPUnit tests for unformat_float(), unformat_value() and can_be_unformatted() functions. To be reviewed.
          Hide
          Adrian Greeve added a comment -

          Hello

          Thanks for the unit tests, the integrators love seeing these and it makes integrating code into core much easier.

          Just a few minor suggestions:

          Unit tests (lib/tests/moodlelib_test.php)

          $this->assertSame is used for comparing objects, using $this->assertEquals like test_format_float would be better.

          I would consider putting the section where you create a dummy language pack into a function so that you don't have to duplicate the code multiple times.

          • White space:
            line 2177
            line 2190
            line 2237
            line 2253
            line 2266
            line 2329
            line 2342

          Moodlelib (lib/moodlelib.php)

          I don't personally think that the function can_be_unformatted is really necessary. I think that the one line that you have in that function could be written in the main code. But this is only what I think and the integrators may feel different.

          Suggestions for improving the comments in the dock block:

          (unformat_value)
          Converts a locale specific value back to a standard PHP number value. This function is required to distinguish between a value that can be unformatted compared to a value that doesn't, because unformat_float() returns 0.0 if an invalid value is passed. If unformat_float() is used for an invalid grade value, it is better to not grade rather than assigning a value of 0.

          (can_be_unformatted)
          Determines whether a value with removed white spaces, or a value that contains a decimal separator used by the local language, is a valid number when unformatted.

          line 9690 needs a fullstop at the end of the sentence.
          line 9710 needs a fullstop at the end of the sentence.

          • White space:

          line 9689

          CSV Import (grade/import/csv/index.php)

          I'm getting ^M at the end of some of the code in grade/import/csv/index.php from around line 359 - 367. If you could remove those it would keep all of the code uniform.

          Thank you for your hard work on this issue.

          Show
          Adrian Greeve added a comment - Hello Thanks for the unit tests, the integrators love seeing these and it makes integrating code into core much easier. Just a few minor suggestions: Unit tests (lib/tests/moodlelib_test.php) $this->assertSame is used for comparing objects, using $this->assertEquals like test_format_float would be better. I would consider putting the section where you create a dummy language pack into a function so that you don't have to duplicate the code multiple times. White space: line 2177 line 2190 line 2237 line 2253 line 2266 line 2329 line 2342 Moodlelib (lib/moodlelib.php) I don't personally think that the function can_be_unformatted is really necessary. I think that the one line that you have in that function could be written in the main code. But this is only what I think and the integrators may feel different. Suggestions for improving the comments in the dock block: (unformat_value) Converts a locale specific value back to a standard PHP number value. This function is required to distinguish between a value that can be unformatted compared to a value that doesn't, because unformat_float() returns 0.0 if an invalid value is passed. If unformat_float() is used for an invalid grade value, it is better to not grade rather than assigning a value of 0. (can_be_unformatted) Determines whether a value with removed white spaces, or a value that contains a decimal separator used by the local language, is a valid number when unformatted. line 9690 needs a fullstop at the end of the sentence. line 9710 needs a fullstop at the end of the sentence. White space: line 9689 CSV Import (grade/import/csv/index.php) I'm getting ^M at the end of some of the code in grade/import/csv/index.php from around line 359 - 367. If you could remove those it would keep all of the code uniform. Thank you for your hard work on this issue.
          Hide
          Gilles-Philippe Leblanc added a comment -

          $this->assertSame is used for comparing objects, using $this->assertEquals like test_format_float would be better.

          I used assertSame() because I wanted to test the specific type of variable, not only the returned value:
          http://phpunit.de/manual/3.4/en/api.html#api.assert.assertSame

          But if you think its ok to only test with asserEquals, its ok for me too!

          I would consider putting the section where you create a dummy language pack into a function so that you don't have to duplicate the code multiple times.

          Done.

          White space:

          Done. My Netbeans codesniffer plugins do not seems to work anymore...

          I don't personally think that the function can_be_unformatted is really necessary. I think that the one line that you have in that function could be written in the main code. But this is only what I think and the integrators may feel different.

          I created it for clarity and because I think this improvement could be use also in the xml file import method but I agree that its a little overkill
          The method has been removed.

          Suggestions for improving the comments in the dock block:

          Done, other code style too.

          I'm getting ^M at the end of some of the code in grade/import/csv/index.php from around line 359 - 367. If you could remove those it would keep all of the code uniform.

          Done.

          Thank you for your hard work on this issue.

          My pleasure

          To be peer-reviewing... again!

          Show
          Gilles-Philippe Leblanc added a comment - $this->assertSame is used for comparing objects, using $this->assertEquals like test_format_float would be better. I used assertSame() because I wanted to test the specific type of variable, not only the returned value: http://phpunit.de/manual/3.4/en/api.html#api.assert.assertSame But if you think its ok to only test with asserEquals, its ok for me too! I would consider putting the section where you create a dummy language pack into a function so that you don't have to duplicate the code multiple times. Done. White space: Done. My Netbeans codesniffer plugins do not seems to work anymore... I don't personally think that the function can_be_unformatted is really necessary. I think that the one line that you have in that function could be written in the main code. But this is only what I think and the integrators may feel different. I created it for clarity and because I think this improvement could be use also in the xml file import method but I agree that its a little overkill The method has been removed. Suggestions for improving the comments in the dock block: Done, other code style too. I'm getting ^M at the end of some of the code in grade/import/csv/index.php from around line 359 - 367. If you could remove those it would keep all of the code uniform. Done. Thank you for your hard work on this issue. My pleasure To be peer-reviewing... again!
          Hide
          Adrian Greeve added a comment -

          This looks good to me,

          I did however notice that lib/tests/moodlelib_test.php has some whitespace on lines 91, 92, and 94 (Sorry!).
          Once you have removed those, please put forward for integration.

          Show
          Adrian Greeve added a comment - This looks good to me, I did however notice that lib/tests/moodlelib_test.php has some whitespace on lines 91, 92, and 94 (Sorry!). Once you have removed those, please put forward for integration.
          Hide
          Gilles-Philippe Leblanc added a comment - - edited

          The white spaces has been removed. Sorry but I cannot put it in integration directly.

          Show
          Gilles-Philippe Leblanc added a comment - - edited The white spaces has been removed. Sorry but I cannot put it in integration directly.
          Hide
          Adrian Greeve added a comment -

          No problem. Submitting for integration.

          Show
          Adrian Greeve added a comment - No problem. Submitting for integration.
          Hide
          Damyon Wiese added a comment -

          Thanks for working on this Gilles-Philippe,

          Some comments about the patch:

          I agree that the current unformat_float function needs improvement to differentiate between an invalid value and 0.0.

          I don't agree with adding a new function unformat_value - we don't need 2 functions with overlapping functionality in moodlelib.

          My suggestion would be to add an optional parameter to unformat_float like this:

          /**
           * Converts locale specific floating point/comma number back to standard PHP float value
           * Do NOT try to do any math operations before this conversion on any user submitted floats!
           *
           * @param string $locale_float locale aware float representation
           * @param bool $strict If true, then check the input and return false if it is not a valid number.
           * @return mixed float|bool - false or the parsed float.
           */
          function unformat_float($locale_float, $strict = false)
          

          Also - as this is a bug fix it will need backport branches (back to 23).

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for working on this Gilles-Philippe, Some comments about the patch: I agree that the current unformat_float function needs improvement to differentiate between an invalid value and 0.0. I don't agree with adding a new function unformat_value - we don't need 2 functions with overlapping functionality in moodlelib. My suggestion would be to add an optional parameter to unformat_float like this: /** * Converts locale specific floating point/comma number back to standard PHP float value * Do NOT try to do any math operations before this conversion on any user submitted floats! * * @param string $locale_float locale aware float representation * @param bool $strict If true , then check the input and return false if it is not a valid number. * @ return mixed float |bool - false or the parsed float . */ function unformat_float($locale_float, $strict = false ) Also - as this is a bug fix it will need backport branches (back to 23). Regards, Damyon
          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
          Gilles-Philippe Leblanc added a comment -

          Your idea seems great. I was not keen on the idea of ​​modifying a core function like that but if I have your approval ...
          I'm on it!

          Show
          Gilles-Philippe Leblanc added a comment - Your idea seems great. I was not keen on the idea of ​​modifying a core function like that but if I have your approval ... I'm on it!
          Hide
          Gilles-Philippe Leblanc added a comment -

          Here is the modification based on your comment. To be peer-reviewed.

          Show
          Gilles-Philippe Leblanc added a comment - Here is the modification based on your comment. To be peer-reviewed.
          Hide
          Adrian Greeve added a comment -

          This looks fine to me, submitting back to integration.

          Show
          Adrian Greeve added a comment - This looks fine to me, submitting back to integration.
          Hide
          Damyon Wiese 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
          Damyon Wiese 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 -

          Looks good, thanks Gilles-Philippe. I've integrated this to master, 25, 24 and 23.

          (I was a little concerend about the strange decinalsep langpack 'hack' we do until I saw its how the existing code functioned).

          Show
          Dan Poltawski added a comment - Looks good, thanks Gilles-Philippe. I've integrated this to master, 25, 24 and 23. (I was a little concerend about the strange decinalsep langpack 'hack' we do until I saw its how the existing code functioned).
          Hide
          Andrew Davis added a comment -

          Phew. Seems to be working as described. Passing.

          Show
          Andrew Davis added a comment - Phew. Seems to be working as described. Passing.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has now been sent upstream and will soon be downloaded by millions of Moodlers!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has now been sent upstream and will soon be downloaded by millions of Moodlers! Regards, Damyon

            People

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

              Dates

              • Created:
                Updated:
                Resolved: