From d381fb77765f9293fa6936b9943f02863c18bfb8 Mon Sep 17 00:00:00 2001 From: Gilles-Philippe Leblanc Date: Thu, 9 May 2013 11:35:38 -0400 Subject: [PATCH] MDL-38239 gradebook: Importing grades from a .txt file with commas for decimals causes an error --- grade/import/csv/index.php | 25 ++++---- lib/moodlelib.php | 12 +++- lib/tests/moodlelib_test.php | 111 +++++++++++++++++++++++++++++++---- 3 files changed, 123 insertions(+), 25 deletions(-) diff --git a/grade/import/csv/index.php b/grade/import/csv/index.php index 36784b375df..f09a79a92f9 100644 --- a/grade/import/csv/index.php +++ b/grade/import/csv/index.php @@ -351,16 +351,21 @@ if ($formdata = $mform2->get_data()) { $newgrade->finalgrade = $value; } else { if ($value === '' or $value == '-') { - $value = null; // no grade - - } else if (!is_numeric($value)) { - // non numeric grade value supplied, possibly mapped wrong column - echo "
t0 is $t0"; - echo "
grade is $value"; - $status = false; - import_cleanup($importcode); - echo $OUTPUT->notification(get_string('badgrade', 'grades')); - break 3; + $value = null; // No grade. + } else { + // If the value has a local decimal or can correctly be unformatted, do it. + $validvalue = unformat_float($value, true); + if ($validvalue !== false) { + $value = $validvalue; + } else { + // Non numeric grade value supplied, possibly mapped wrong column. + echo "
t0 is $t0"; + echo "
grade is $value"; + $status = false; + import_cleanup($importcode); + echo $OUTPUT->notification(get_string('badgrade', 'grades')); + break 3; + } } $newgrade->finalgrade = $value; } diff --git a/lib/moodlelib.php b/lib/moodlelib.php index a2c508fec0f..3f94e3ffa92 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -9669,9 +9669,10 @@ function format_float($float, $decimalpoints=1, $localized=true, $stripzeros=fal * Do NOT try to do any math operations before this conversion on any user submitted floats! * * @param string $locale_float locale aware float representation - * @return float + * @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) { +function unformat_float($locale_float, $strict = false) { $locale_float = trim($locale_float); if ($locale_float == '') { @@ -9679,8 +9680,13 @@ function unformat_float($locale_float) { } $locale_float = str_replace(' ', '', $locale_float); // no spaces - those might be used as thousand separators + $locale_float = str_replace(get_string('decsep', 'langconfig'), '.', $locale_float); - return (float)str_replace(get_string('decsep', 'langconfig'), '.', $locale_float); + if ($strict && !is_numeric($locale_float)) { + return false; + } + + return (float)$locale_float; } /** diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index abf25602c99..cedb716c5bd 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -85,6 +85,29 @@ class moodlelib_testcase extends advanced_testcase { ) ); + /** + * Define a local decimal separator. + * + * It is not possible to directly change the result of get_string in + * a unit test. Instead, we create a language pack for language 'xx' in + * dataroot and make langconfig.php with the string we need to change. + * The example separator used here is 'X'; on PHP 5.3 and before this + * must be a single byte character due to PHP bug/limitation in + * number_format, so you can't use UTF-8 characters. + * + * @global type $SESSION + * @global type $CFG + */ + protected function define_local_decimal_separator() { + global $SESSION, $CFG; + + $SESSION->lang = 'xx'; + $langconfig = "dataroot . '/lang/xx'; + check_dir_exists($langfolder); + file_put_contents($langfolder . '/langconfig.php', $langconfig); + } + function test_cleanremoteaddr() { //IPv4 $this->assertEquals(cleanremoteaddr('1023.121.234.1'), null); @@ -2122,7 +2145,6 @@ class moodlelib_testcase extends advanced_testcase { * Test localised float formatting. */ public function test_format_float() { - global $SESSION, $CFG; // Special case for null $this->assertEquals('', format_float(null)); @@ -2138,17 +2160,8 @@ class moodlelib_testcase extends advanced_testcase { $this->assertEquals('5.43', format_float(5.43, 5, true, true)); $this->assertEquals('5', format_float(5.0001, 3, true, true)); - // It is not possible to directly change the result of get_string in - // a unit test. Instead, we create a language pack for language 'xx' in - // dataroot and make langconfig.php with the string we need to change. - // The example separator used here is 'X'; on PHP 5.3 and before this - // must be a single byte character due to PHP bug/limitation in - // number_format, so you can't use UTF-8 characters. - $SESSION->lang = 'xx'; - $langconfig = "dataroot . '/lang/xx'; - check_dir_exists($langfolder); - file_put_contents($langfolder . '/langconfig.php', $langconfig); + // Tests with a localised decimal separator. + $this->define_local_decimal_separator(); // Localisation on (default) $this->assertEquals('5X43000', format_float(5.43, 5)); @@ -2159,6 +2172,80 @@ class moodlelib_testcase extends advanced_testcase { $this->assertEquals('5.43', format_float(5.43, 5, false, true)); } + /** + * Test localised float unformatting. + */ + public function test_unformat_float() { + + // Tests without the localised decimal separator. + + // Special case for null, empty or white spaces only strings. + $this->assertEquals(null, unformat_float(null)); + $this->assertEquals(null, unformat_float('')); + $this->assertEquals(null, unformat_float(' ')); + + // Regular use. + $this->assertEquals(5.4, unformat_float('5.4')); + $this->assertEquals(5.4, unformat_float('5.4', true)); + + // No decimal. + $this->assertEquals(5.0, unformat_float('5')); + + // Custom number of decimal. + $this->assertEquals(5.43267, unformat_float('5.43267')); + + // Empty decimal. + $this->assertEquals(100.0, unformat_float('100.00')); + + // With the thousand separator. + $this->assertEquals(1000.0, unformat_float('1 000')); + $this->assertEquals(1000.32, unformat_float('1 000.32')); + + // Negative number. + $this->assertEquals(-100.0, unformat_float('-100')); + + // Wrong value. + $this->assertEquals(0.0, unformat_float('Wrong value')); + // Wrong value in strict mode. + $this->assertFalse(unformat_float('Wrong value', true)); + + // Combining options. + $this->assertEquals(-1023.862567, unformat_float(' -1 023.862567 ')); + + // Bad decimal separator (should crop the decimal). + $this->assertEquals(50.0, unformat_float('50,57')); + // Bad decimal separator in strict mode (should return false). + $this->assertFalse(unformat_float('50,57', true)); + + // Tests with a localised decimal separator. + $this->define_local_decimal_separator(); + + // We repeat the tests above but with the current decimal separator. + + // Regular use without and with the localised separator. + $this->assertEquals (5.4, unformat_float('5.4')); + $this->assertEquals (5.4, unformat_float('5X4')); + + // Custom number of decimal. + $this->assertEquals (5.43267, unformat_float('5X43267')); + + // Empty decimal. + $this->assertEquals (100.0, unformat_float('100X00')); + + // With the thousand separator. + $this->assertEquals (1000.32, unformat_float('1 000X32')); + + // Bad different separator (should crop the decimal). + $this->assertEquals (50.0, unformat_float('50Y57')); + // Bad different separator in strict mode (should return false). + $this->assertFalse (unformat_float('50Y57', true)); + + // Combining options. + $this->assertEquals (-1023.862567, unformat_float(' -1 023X862567 ')); + // Combining options in strict mode. + $this->assertEquals (-1023.862567, unformat_float(' -1 023X862567 ', true)); + } + /** * Test deleting of users. */