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

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

Linking to a similar issue.

I just added a patch.

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?

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

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

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

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.

$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!

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.

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

No problem. Submitting for integration.

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

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

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!

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

This looks fine to me, submitting back to integration.

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

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).

Phew. Seems to be working as described. Passing.

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

Regards, Damyon