SocialPass / socialpass

Hosting the next generation of events
https://registry.socialpass.io
Other
1 stars 0 forks source link

Fully supported localized event locations #204

Closed crypto-rizzo closed 2 years ago

crypto-rizzo commented 2 years ago

Leftover from #202

EDIT: OK here is updated list of fields

Additionally, the location field should be replaced with localized_address_display

devjoaov commented 2 years ago

About set timezone based on selected location I tested getting the timezone using the lat/long values provided by gmaps api and timezonefinder python package. It seems to work fine.

@crypto-rizzo could you resume what is "Add localized fields as ordered list"?

I added formatted_address to places API, but what we should do with the values? Insert into address_1 and address_2?

crypto-rizzo commented 2 years ago

@devjoaov Our location schema in the Event model closely mirrors the Eventbrite API in the pictures attached.

The goal of this issue was to populate the rest of location fields with corresponding Google Maps API data

Screen Shot 2022-08-18 at 3 46 25 PM Screen Shot 2022-08-18 at 3 46 20 PM
devjoaov commented 2 years ago

Thanks @crypto-rizzo.

devjoaov commented 2 years ago

Hey guys @crypto-rizzo @hubertokf, I want open two discussion for this issue.

FIRST DISCUSSION

It is really make sense store _localized_multi_line_address_display_ values in database? It is just a combination of other fields (adresses, region and etc). I thought about just create a property for Event model called _localized_multi_line_addressdisplay that will works like _fullembed property of Ticket model (beeing just a combination of other fields).

SECOND DISCUSSION

The other discussion is about which package we should use to get timezone lookup based on latitude and longitude values. In first approach, I tested timezonefinder (https://github.com/jannikmi/timezonefinder), a python package that uses Timezone Boundary Builder data. This package works well and is being updated (the last release was 4 days ago) but the performance is not so good. In some tests took 2-3 seconds to find the timezone lookup. Another thing I took into account was how we are saving the localized fields. All the localized fields are being saved in _eventform template. Using the timezonefinder we force that just one field (timezone) is defined into .py code.

I decided search for a timezone lookup javascript lib and I found https://github.com/darkskyapp/tz-lookup-oss/. With it it is possible to import the code in the template and use its methods. This lib also worked fine and is very fast. The problem is that this lib is no longer being updated or maintained for a few years. It solves the problem of to keep the definition of the localized fields in the template.

Resume:

timezonefinder (python package)

Pros:

  1. is being updated
  2. few issues on github and none related to lookup error

Cons:

  1. it's a little slow
  2. the code to save the timezone is outside event_form template code

tz-lookup-oss (javascript lib)

Pros:

  1. is really fast
  2. we can import into event_form template and just use document.getElementById("id_timezone").value = tzlookup(lat, long);

Cons:

  1. It is no longer being maintained
  2. have one issue of incorrect timezone (can be a warning)
crypto-rizzo commented 2 years ago

Thanks for this @devjoaov. My feedback below:

  1. I'm OK with leaving out the multiline display for now. I wouldn't worry about the @property either. Doesn't seem like a big priority for now.
  2. Thanks for deep dive on this. I hadn't thought of using frontend for this. Personally, I would vote to leave out timezone lookup until we setup geodjango, which should be its own separate issue. This is official part of django library and solves a few different issues we have (including timezone lookup in this case).
devjoaov commented 2 years ago

I agree about both @crypto-rizzo. As there is not a perfect solution for timezone lookup with python or js packages, I will search how to make a good solution with geodjango for timezone lookup.