Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37356

Database activity not using format_float/unformat_float for entries in latitude/longitude fields.

    Details

    • Testing Instructions:
      Hide

      Before you start

      • You should have an appropriate language pack installed such as French or Russian.

      Testing - Manual entry

      1. Create a database module.
      2. Add a Latitude/longitude field.
      3. Save the default templates.
      4. Switch your language to one that uses commas as a decimal delimiter, such as French.
      5. Add an entry.
        • Try adding a location with data such as (-31,9468 115,8714)
        • Try a location with a positive latitude and a negative longitude (49,242175 -123,11383)
        • Try with lat or long with a period mark (-32.0053 115,8934)
        • Try with both with a period mark (-31.9422 115.8261)
        • Verify that the location data is displayed appropriately. (With commas in the chosen language and with a period mark when in English)

      Testing - csv import

      1. Create a csv file for the database activity, possibly export a file and alter that.
      2. Create entries with the above variations.
      3. Make sure that you are importing in a different language to English.
        • Verify that the location data is displayed appropriately. (With commas in the chosen language and with a period mark when in English)
      Show
      Before you start You should have an appropriate language pack installed such as French or Russian. Testing - Manual entry Create a database module. Add a Latitude/longitude field. Save the default templates. Switch your language to one that uses commas as a decimal delimiter, such as French. Add an entry. Try adding a location with data such as (-31,9468 115,8714) Try a location with a positive latitude and a negative longitude (49,242175 -123,11383) Try with lat or long with a period mark (-32.0053 115,8934) Try with both with a period mark (-31.9422 115.8261) Verify that the location data is displayed appropriately. (With commas in the chosen language and with a period mark when in English) Testing - csv import Create a csv file for the database activity, possibly export a file and alter that. Create entries with the above variations. Make sure that you are importing in a different language to English. Verify that the location data is displayed appropriately. (With commas in the chosen language and with a period mark when in English)
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-37356-master

      Description

      Potential for locale issues.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred Frédéric Massart added a comment - - edited

            Hi Adrian,

            your patch looks spot on! Could you just comment on the following points:

            • Would it be worth having some testing instructions where the latitude is a positive, and where the longitude is negative?
            • Did you notice the white space on line 125?
            • I was curious about the use of sprintf in export_text_value(), could you comment here and/or in the code to explain why we don't need to format the float here?
            • I'd like to see a comment in update_content which explains why you unformat the data.

            Other than that, it looks perfect!

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - - edited Hi Adrian, your patch looks spot on! Could you just comment on the following points: Would it be worth having some testing instructions where the latitude is a positive, and where the longitude is negative? Did you notice the white space on line 125? I was curious about the use of sprintf in export_text_value(), could you comment here and/or in the code to explain why we don't need to format the float here? I'd like to see a comment in update_content which explains why you unformat the data. Other than that, it looks perfect! Cheers, Fred
            Hide
            abgreeve Adrian Greeve added a comment -

            Thanks for the peer review and spotting those mistakes.
            Submitting for integration.

            Show
            abgreeve Adrian Greeve added a comment - Thanks for the peer review and spotting those mistakes. Submitting for integration.
            Hide
            damyon 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.

            Thanks!

            Show
            damyon 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. Thanks!
            Hide
            abgreeve Adrian Greeve added a comment -

            Rebased and ready to go.

            Show
            abgreeve Adrian Greeve added a comment - Rebased and ready to go.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Uhm, I'm a bit worried about this change (that looks perfect) and how things will work after it (more changes needed).

            If I'm not wrong, after current change people will get heterogeneous results here and there (on export, when searching, linking...). While old contents follow the stable (and imperfect, agree) rule of "being shown the way they were introduced", after the change people will start to get mixed results.

            And the only solution I can imagine is to try to fix all existing latlong contents in DB. So I think that:

            1) Some conversion - un-format - (via upgrade step) of stored latlongs is required to normalize them to use the dot decsep.

            2) On search we should be also un-formatting, in order to get all the entries matching (because of 1)

            3) On export, IMO it's better to apply formatting, but I'm not strong about that. Feel free to decide.

            4) Any import code (restore included) must ensure proper (un-formatted) insertion.

            5) On linking to external service... it depends of which decsep the service supports.

            So I'm reopening this because it seems to need some more work. For sure, only for 2.5dev.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Uhm, I'm a bit worried about this change (that looks perfect) and how things will work after it (more changes needed). If I'm not wrong, after current change people will get heterogeneous results here and there (on export, when searching, linking...). While old contents follow the stable (and imperfect, agree) rule of "being shown the way they were introduced", after the change people will start to get mixed results. And the only solution I can imagine is to try to fix all existing latlong contents in DB. So I think that: 1) Some conversion - un-format - (via upgrade step) of stored latlongs is required to normalize them to use the dot decsep. 2) On search we should be also un-formatting, in order to get all the entries matching (because of 1) 3) On export, IMO it's better to apply formatting, but I'm not strong about that. Feel free to decide. 4) Any import code (restore included) must ensure proper (un-formatted) insertion. 5) On linking to external service... it depends of which decsep the service supports. So I'm reopening this because it seems to need some more work. For sure, only for 2.5dev. Ciao
            Hide
            cibot CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            abgreeve Adrian Greeve added a comment - - edited

            Hello Eloy,

            Thanks for raising these concerns. It made me think a bit more about my solution.

            Excuse me if I have this wrong, but your main concern is that old entries in the database will not be displayed correctly due to them being entered in a region specific way?

            With some testing I have found that currently any entry that does not conform to the period delimiter will not enter into the database correctly i.e. -23,8764 will be stored in the database as -23. This data is already corrupted and hopefully spotted by the user who entered it in. Either way there is no way or need to 'fix' this information.

            So with this in mind -

            1. A conversion using an upgrade script is unnecessary.
            2. I had forgotten about the search. I have changed this so that the select box shows region formatted coordinates.
            3. I personally would prefer to leave this the way it is. If the file was exported with the data formatted and then restored in a moodle instance that was not set to the same sort of region then there would be import problems.
            4. As all the data is already in the correct format, there is no need to change anything here.
            5. All links to external services still function exactly as they did before.

            I think the main point that I am trying to make here is that only figures shown to the user are formatted for the region. All functioning figures remain in the same format as before.

            Thanks.

            Show
            abgreeve Adrian Greeve added a comment - - edited Hello Eloy, Thanks for raising these concerns. It made me think a bit more about my solution. Excuse me if I have this wrong, but your main concern is that old entries in the database will not be displayed correctly due to them being entered in a region specific way? With some testing I have found that currently any entry that does not conform to the period delimiter will not enter into the database correctly i.e. -23,8764 will be stored in the database as -23. This data is already corrupted and hopefully spotted by the user who entered it in. Either way there is no way or need to 'fix' this information. So with this in mind - A conversion using an upgrade script is unnecessary. I had forgotten about the search. I have changed this so that the select box shows region formatted coordinates. I personally would prefer to leave this the way it is. If the file was exported with the data formatted and then restored in a moodle instance that was not set to the same sort of region then there would be import problems. As all the data is already in the correct format, there is no need to change anything here. All links to external services still function exactly as they did before. I think the main point that I am trying to make here is that only figures shown to the user are formatted for the region. All functioning figures remain in the same format as before. Thanks.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Aha, so database has always "forced" decimal point, no matter the lang used. Super, that makes things really easier. Looking...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Aha, so database has always "forced" decimal point, no matter the lang used. Super, that makes things really easier. Looking...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Oh, but import entries can break that, grrr. I just exported a database, changed some "points" by "commas" and imported them back. Result = commas in the DB. See, for example, the generated, googlemaps link:

            http://maps.google.com/maps?q=42,2800,+-2.2700&iwloc=A&hl=en (and it should be my city).

            Anyway, I think that can be moved to another issue... and sort it out there so ignoring here...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Oh, but import entries can break that, grrr. I just exported a database, changed some "points" by "commas" and imported them back. Result = commas in the DB. See, for example, the generated, googlemaps link: http://maps.google.com/maps?q=42,2800,+-2.2700&iwloc=A&hl=en (and it should be my city). Anyway, I think that can be moved to another issue... and sort it out there so ignoring here...
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Uhm, also, note how negatives are handled. I introduced 42,2800 -2.2700 (thinking that the negative (West) would be converted into proper positive (East). But it ended being stored in DB as negative... the activity shows it without the sign, but exports contain the sign (see link above).

            Again, unrelated to this, but needing fix in another issue.

            Ciao

            Note this is BEFORE your patch. But means that we can have both negatives and commas in the DB.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Uhm, also, note how negatives are handled. I introduced 42,2800 -2.2700 (thinking that the negative (West) would be converted into proper positive (East). But it ended being stored in DB as negative... the activity shows it without the sign, but exports contain the sign (see link above). Again, unrelated to this, but needing fix in another issue. Ciao Note this is BEFORE your patch. But means that we can have both negatives and commas in the DB.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Ah, forget my last comment, I did not realize that it was, correctly, changing the -2.2700 E to 2.2700 W (on display). So it seems that the handling of negatives is being done ok.

            Oki, so:

            1) We need to ensure, on import, that conversion to decimal points happen always.
            2) Perhaps that should lead to an upgrade, to normalize existing decimal commas.
            3) Restore may find also decimal commas, it also needs handling.

            Would you create a new issue to fix all those remaining point/comma problems? I'm going to integrate this right now, so the 1-2-3 can be done on top of this. => Done, see below.

            Ciao

            Edited: I've created MDL-38217 to get the remaining point/comma problems under control.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Ah, forget my last comment, I did not realize that it was, correctly, changing the -2.2700 E to 2.2700 W (on display). So it seems that the handling of negatives is being done ok. Oki, so: 1) We need to ensure, on import, that conversion to decimal points happen always. 2) Perhaps that should lead to an upgrade, to normalize existing decimal commas. 3) Restore may find also decimal commas, it also needs handling. Would you create a new issue to fix all those remaining point/comma problems? I'm going to integrate this right now, so the 1-2-3 can be done on top of this. => Done, see below. Ciao Edited: I've created MDL-38217 to get the remaining point/comma problems under control.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (master only), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
            Hide
            markn Mark Nelson added a comment -

            It works as expected when following the provided testing instructions.

            However, if we add an entry using English with the values "49,242175 -123,11383" they are rendered as "49.0000°N, 123.0000°W", instead of "49.2422°N, 123.1138°W" when adding the entry in French. Is this expected? Shouldn't we handle both cases? If it is, please pass this test and ignore my comment.

            Show
            markn Mark Nelson added a comment - It works as expected when following the provided testing instructions. However, if we add an entry using English with the values "49,242175 -123,11383" they are rendered as "49.0000°N, 123.0000°W", instead of "49.2422°N, 123.1138°W" when adding the entry in French. Is this expected? Shouldn't we handle both cases? If it is, please pass this test and ignore my comment.
            Hide
            abgreeve Adrian Greeve added a comment -

            I personally think that it works fine this way, but I'm open to suggestions about the desired functionality. Perhaps we could get a few opinions about the idea.

            Show
            abgreeve Adrian Greeve added a comment - I personally think that it works fine this way, but I'm open to suggestions about the desired functionality. Perhaps we could get a few opinions about the idea.
            Hide
            fred Frédéric Massart added a comment -

            I personally think it's safer to use the current user locale, instead of trying to get possible decimal delimiters from "who knows where" and creating unlimited possible parsing options.

            Show
            fred Frédéric Massart added a comment - I personally think it's safer to use the current user locale, instead of trying to get possible decimal delimiters from "who knows where" and creating unlimited possible parsing options.
            Hide
            poltawski Dan Poltawski added a comment -

            Its hard to make a judgement on this because i'm a crazy englishman who only ever uses .

            1. Format_float is doing exactly what it should, being strict. This is used for grading by teachers all the time. It should not allow both, thats the whole point.
            2. Is there something about coordinates which makes them special and have interchangeable seperators? If there is then we shouldn't have added it this change, since surely users would know to swap it?

            But i'm willing to be corrected by someone who actually uses a different separator.

            Show
            poltawski Dan Poltawski added a comment - Its hard to make a judgement on this because i'm a crazy englishman who only ever uses . Format_float is doing exactly what it should, being strict. This is used for grading by teachers all the time. It should not allow both, thats the whole point. Is there something about coordinates which makes them special and have interchangeable seperators? If there is then we shouldn't have added it this change, since surely users would know to swap it? But i'm willing to be corrected by someone who actually uses a different separator.
            Hide
            poltawski Dan Poltawski added a comment -

            Based on fred comment, sending back to testing.

            Show
            poltawski Dan Poltawski added a comment - Based on fred comment, sending back to testing.
            Hide
            markn Mark Nelson added a comment -

            As discussed above, passing this.

            Show
            markn Mark Nelson added a comment - As discussed above, passing this.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Because

            A
            MARVELOUS
            A       U
            Z  YOU  P
            I  ARE  E
            N  PPL  R
            G       B
              TNKS! 
            

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13