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
    • Rank:
      46977

      Description

      Potential for locale issues.

        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: