Fix #921: Truncate latlng to 30 chars #926
Conversation
organize/models.py
Outdated
| @@ -129,7 +129,8 @@ def __str__(self): | |||
|
|
|||
| def save(self, *args, **kwargs): | |||
| if not self.latlng: | |||
| self.latlng = get_coordinates_for_city(self.city, self.get_country_display()) | |||
| latlng_coord = get_coordinates_for_city(self.city, self.get_country_display()) | |||
| self.latlng = latlng_coord[:30] # Truncate to 30 characters to match the database field limit | |||
There was a problem hiding this comment.
If we get values like this (-74.80111590000001, -74.80111590000001) of which each value is 19 characters, we would truncate this and have the first value remain 19 characters while the second value becomes -74.80111 is 9 characters long. It would be better to format the values in the get_coordinates_for_city()and limit the number of decimals places for each so that they are of equal length, making consideration for the possible-in the values. The function is here https://github.com/DjangoGirls/djangogirls/blob/main/core/utils.py#L18 incore/utils.py`.
Thanks for your work on this.
There was a problem hiding this comment.
Thank you very much for the review and letting me know @amakarudze
I Got it. Let me work on this and I will get back soon.
There was a problem hiding this comment.
I have made those changes. Please let me know if any changes are required.
There was a problem hiding this comment.
Thanks, sorry for not getting back to you earlier on. Just have one more comment below.
There was a problem hiding this comment.
One more change, can you truncate to 7 decimal places, tests are failing because the values in the tests are set to 7 decimal places while your changes truncate to 6 decimal places. Truncating to 7 decimal places should fix this. Also use double quotes for the f string to address ruff issues.
There was a problem hiding this comment.
Yes sure. Will do. Thank you
core/utils.py
Outdated
| return f'{data["lat"]}, {data["lon"]}' | ||
| formatted_lat = "{:.6f}".format(float(data["lat"])) | ||
| formatted_lon = "{:.6f}".format(float(data["lon"])) | ||
| return f'{formatted_lat}, {formatted_lon}' |
There was a problem hiding this comment.
The ruff build is failing because of the single quotes here. May you please address this so we can merge this pull request. Sorry for taking so much time to review again.
There was a problem hiding this comment.
Hi @amakarudze ,
Thank you for your review. No problem let me fix this and I will update the code soon.
There was a problem hiding this comment.
You're welcome. Thanks for your work on this.
This is done. Let me know if anything else is required here. |
|
@amakarudze I think we also need to adjust test_utils.py to match new changes. |
Yeah @dostogircse171 , true I just noticed it has 8 decimal places, so removing 1 each should work. Sorry about that. |
@amakarudze I can see inside test_utils.py file same function |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #926 +/- ##
==========================================
+ Coverage 80.28% 81.02% +0.74%
==========================================
Files 95 95
Lines 2845 2862 +17
Branches 273 269 -4
==========================================
+ Hits 2284 2319 +35
+ Misses 503 492 -11
+ Partials 58 51 -7
... and 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@dostogircse171 i have no idea, will have to look into it first before making any changes. Thanks for your work on this, I have merged the PR and some of my issues will go away because of this change! |
@amakarudze Thank you very much. It's a pleasure to work on this task. Is there any other task you can recommend me to pick next? |
This pull request fixes #921 the DataError encountered when saving
latlngvalues longer than 30 characters.It modifies the
savemethod of theEventApplicationmodel to truncate thelatlngvalue to 30 characters, ensuring data integrity without altering the database schema.