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 Master Branch:
      MDL-38239-master

      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

        Gliffy Diagrams

          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: