Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect time zone based on IP address #36

Open
frederikcreemers opened this issue May 8, 2017 · 12 comments
Open

Detect time zone based on IP address #36

frederikcreemers opened this issue May 8, 2017 · 12 comments

Comments

@frederikcreemers
Copy link

frederikcreemers commented May 8, 2017

There's an API called ip-api.com which will give you the location and time zone based on the user's IP.

If we use this on the server side, we can get the time zone in the first request. This does come at a performance cost, because network requests take time, so this should definitely be a setting. Also, the free tier only allows you to make 150 requests/minute, which might be too few for high-traffic apps, although you'd need to have a lot of unique visitors per minute, since we're only making one api request per user session.

This would also solve the time zone change detection problem (#25). We could detect a change in the user's ip, and make another request to the time zone service.

If this sounds like a good idea, I'm willing to submit a pull request.

@adamcharnock
Copy link
Owner

This sounds like a good idea to me, happy to receive a pull request. I suggest it be an optional feature given the dependency on an external service.

@SehoNoh
Copy link

SehoNoh commented Sep 12, 2018

@adamcharnock Hi Adam, Can I do this if there is no one to solve this?

@adamcharnock
Copy link
Owner

Hey @NohSeho, that would be great, please go ahead.

I suggest two was to implement this, and I'll be happy with either:

1. Simple setting

A simple setting such as TZ_DETECT_USE_IP_API=True|False which simply enables the lookups. Probably disabled by default.

2. A pluggable system

A setting such as TZ_DETECT_API="tz_detect.detectors.ipapi.IpApi". The IpApi would be something like this:

class TimezoneDetectionDetector(object):  # Base class in tz_detect/apis/__init__.py

    def get_timezone_offset():
        raise NotImplementedError()

# In tz_detect/apis/ipapi.py
class IpApi(TimezoneDetectionDetector):
    
    def get_timezone_offset(self, request):
        offset_minutes = ...
        return offset_minutes

tz_detect would then import the class given in the string (see django.utils.importlib)

The benefit of this system is it allows others to create custom detectors, and also allows us to provide multiple implementations.

The downside is that it is more of a faff to write.


Definitely feel free to choose option 1 should you prefer, it can always be refactored into option 2 at a later date should the need arise.

Also, tagging @bashu in case of any additional comments.

@SehoNoh
Copy link

SehoNoh commented Sep 12, 2018

Oh, you already design the system. That's cool. but I don't know how I spend my time for this. Please take your time.

@adamcharnock
Copy link
Owner

My apologies @NohSeho, these were only my initial thoughts in the last 20 minutes, my intention was to start a discussion. I find discussing the implementation in advance can save time. I should have been clearer about this.

Equally, if you just want to get started on an implementation then I am very happy for you to do so, and I'll look over whatever you come up with.

I still don't have time to work on this, so contributions are still very welcome.

@SehoNoh
Copy link

SehoNoh commented Sep 13, 2018

No problem. but I just want to clarify one. I saw above @adamcharnock you and @bigblind 's comments and the ip-api.com website, that means, Fully using ip-api.com needs a pricing and we are going to implement that API just for an option, not required. right?

@adamcharnock
Copy link
Owner

Hi @NohSeho, correct. I think it should be optional and (probably) not enabled by default.

@SehoNoh
Copy link

SehoNoh commented Sep 13, 2018

Great, I got it.

@gotexis
Copy link

gotexis commented Sep 23, 2018

@NohSeho Can't wait for your idea implemented! :)

@SehoNoh
Copy link

SehoNoh commented Oct 3, 2018

@gotexis Calm down :)
I will do this in this month.

@denis-trofimov
Copy link

denis-trofimov commented Dec 11, 2018

I vote against this whole idea. In fact all Russia use anonymizer proxy to bypass the Telegram ban. User would be located in Central Europe instead of Ural. Also true for all Internet access controlling countries.
This feature should not be required, only optional.

@adamcharnock
Copy link
Owner

You make an excellent point @denis-trofimov, thank you.

I therefore stand by my original comment that this should be an optional feature. This is on the grounds that:

  1. It adds a dependency on an external service
  2. It is not suitable for all users for the reasons @denis-trofimov gives

My above comment shows a couple of ways in which to achieve this.

Just to be clear: I think this feature is very valuable and I encourage anyone to implement it. Even valuable features need to be optional sometimes :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants