gilesknap / gphotos-sync

Google Photos and Albums backup with Google Photos Library API
Apache License 2.0
1.97k stars 161 forks source link

Incorrect timezone #435

Closed deathtenk closed 1 year ago

deathtenk commented 1 year ago

for issue: https://github.com/gilesknap/gphotos-sync/issues/413 , basically all this does is get the system timezone offset in seconds from time.timezone and subtracts it from expiry.timestamp. Also accounts for DST.

gilesknap commented 1 year ago

Thanks @deathtenk this looks plausible but seems awfully manual, I'm wondering if there is a library function to do this for us?

@juzna please could you comment on this?

deathtenk commented 1 year ago

yea I only did it this way because it doesn't add a dependency and as a clojure developer I try to make it a habit of trying not to muck up dependency trees. My research on pythons core time and datetime stuff revealed its fairly limited in terms of what it can do at a high level. However I'm sure someone's written a library that would account for more edge cases (perhaps pytz?).

gilesknap commented 1 year ago

Yes it looks like pytz does this kind of thing but as you say it feels odd to bring in another dependency for such a small thing.

I also find it strange that InstalledAppFlow is essentially giving us a UTC when the token requires local. It seems like the auth library is setting us up to fail. Or I still have not fully understood what is happening here.

@juzna you say that the auth library is doing the right thing here so do you think that @deathtenk 's fix is the way to go?

gilesknap commented 1 year ago

@deathtenk I'm going to run some tests with your changes. As I'm in British Summer Time right now your fix is relevant to me (in the winter GMT=UTC so the bug would not show up for me).

juzna commented 1 year ago

Hi, if you look at python documentation of the datetime.timestamp function used, it says exactly what to do:

There is no method to obtain the POSIX timestamp directly from a naive datetime instance representing UTC time. If your application uses this convention and your system timezone is not set to UTC, you can obtain the POSIX timestamp by supplying tzinfo=timezone.utc: timestamp = dt.replace(tzinfo=timezone.utc).timestamp()

I didn't try to validate it with this library, but I think it should work.

deathtenk commented 1 year ago

@gilesknap tested locally and this works. It's a more managed solution then the manual offset I was doing before, go ahead and test it on your end.

gilesknap commented 1 year ago

Thanks both will run some tests here in BST.

gilesknap commented 1 year ago

The test failures are due to httpbin.org being down. Not related to the code changes.

gilesknap commented 1 year ago

Testing looks good. merging...

Thanks @deathtenk and @juzna