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
            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
            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
            Adrian Greeve added a comment -

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

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

            Thanks!

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

            Rebased and ready to go.

            Show
            Adrian Greeve added a comment - Rebased and ready to go.
            Hide
            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
            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 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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            Eloy Lafuente (stronk7) added a comment -

            Integrated (master only), thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            Dan Poltawski added a comment -

            Based on fred comment, sending back to testing.

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

            As discussed above, passing this.

            Show
            Mark Nelson added a comment - As discussed above, passing this.
            Hide
            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
            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: