GioBonvi / GoogleContactsEventsNotifier

Receive automatic email notifications before your Google Contacts birthday and other events!
MIT License
219 stars 50 forks source link

Time zone issue #79

Closed mile1206 closed 6 years ago

mile1206 commented 7 years ago

Steps to reproduce

What action or series of actions is the cause of the issue?

  1. I live in NewZealand, many of my friends are in Europe which is why I want to be reminded one day prior at 5 pm and on their birthday on 5 pm;
  2. action 2;
  3. action 3;

Expected behavior

What should happen?

Lets say Birthday is 5. Oct. so I should get a notification on 4. Oct at 5pm NZ time and on 5. Oct 5pm NZ time

Current behavior

What happens instead?

I get a notification on 3. Oct and on 4. Oct. The birthday is shown to me to be on 4. Oct instead of 5. Oct.

Context

Extended description

Try to describe the problem in the most complete way you can add images, error messages, hypothesis and observations regarding the problem here.

Possible solution

There is probably an error in the calculation between time zones.

GioBonvi commented 7 years ago

Hello @mile1206, thanks for reporting this. Can I ask you some more questions?

Thank you for your time.

mile1206 commented 7 years ago

Hi Giorgio,

I am sorry I might have not been clear.

The notifications arrive at the correct date and time but (!) the date of the birthday - which is the variable formattedDate in your script, I believe - is incorrect.

I believe the error arises because of back and forth calculations of time zones.

One example (with my settings):

var myTimeZone = 'Pacific/Auckland';
var notificationHour = 19 ;
var anticipateDays = [0];

The actual birthday could be 04/10/2017, but I would get a notification on the 03/10/2017 at around 19:15 and the notification would say "Birthday today (03/10/2017)".

I hope that helps.

Cheers Michael

GioBonvi commented 7 years ago

Thank you, now I understand!

The problem seems to be that the date is formatted using your local timezone, not the birthday one.

I'll look into this and report here my findings, but as far as I can remember we have two options:

  1. Format the date using your local time zone (myTimeZone)
  2. Format the date using your Google Calendar time zone
  3. Introduce a new time zone variable which can be set to any value and use it to format the date instead (this would add some complexity, so I'd like to discuss with the other collaborators, but there will will talk about this if it is needed)

By the way: what is the time zone of your Google Calendar account? You can see this in "Settings" in Google Calendar.

EDIT:
Upon a cursory inspection it looks like formattedDate (which is the date displayed near the "in X days" text) is formatted using the calendar time zone... This would leave us with option 3 or with a possible modification of option 1. I'll explain what I mean after your answer to my previous question.

mile1206 commented 7 years ago

No worries. My calendar timezone is Auckland/NZ.

The actual problem that I have is the following: NZ is way ahead of all European time zones and I would like to be reminded by the script for birthdays in NZ as well as in Europe.

So, these are the min. requirements: For European birthdays: Ideally on the same day (the birthday) at ~19:00 NZ time (which is in their morning) and on the following day at ~8:00 in the morning NZ time (which is their evening). For NZ birthdays: On the same day at ~8:00 NZ time.

That way I should never forget any birthday. To do this I would prefer to be reminded several times regardless of where that person lives (I don't mind to be reminded too often).

Maybe the best solution is to have two scripts running. One with these values:

var myTimeZone = 'Pacific/Auckland'; var notificationHour = 8 ; var anticipateDays = [0];

That should cover the NZ birthdays just fine (and gives me a hint that soon there will be a bd in Europe)

and the second script:

var myTimeZone = 'Europe/Berlin'; var notificationHour = 8 ; var anticipateDays = [0];

That should cover the European birthdays just fine (and give me a second chance to congratulate a bd in NZ)

All of this however needs the formattedDate issue to be soved first.

I found a brute force solution by adding one day to the formattedDate, because it seemed to be one day off:

formattedDate = Utilities.formatDate(new Date(now.getTime() + timeInterval + 24 60 60 * 1000), calendarTimeZone, _('dd-MM-yyyy'));

thanks Michael

mile1206 commented 7 years ago

Another issue: I tried to do exactly what I decribed above with two scripts running at the same time. Unfortunately for the second script I wasn't able to load "Advanced Google Services". It wouldn't be able to load correctly. Would you know why?

GioBonvi commented 7 years ago

I've never tried running more copies of the same script at the same time and while I cannot pinpoint anything that would prevent this from working there could be some unknown compatibility issues.

My suggestion to solve this problem is to introduce the possibility to have multiple notifications hours on the same script (exactly like you can have multiple daysBefore). It should be both easy to implement for us and to set up for the users.

Paging @baatochan and @rowanthorpe to get an opinion on this.

rowanthorpe commented 7 years ago

(FWIW: @GioBonvi I remember at some point fixing some stale documentation which still referred to "number of hours before" rather than "number of days before" and if I remember correctly from scrolling through old git-logs it was a leftover from an earlier version when the setting/processing was for "hours before" anyway).

In my opinion a less confusing system than either of:

is to just allow additional handling of entries of string type (with optional suffix in the string) in the config-array, so that this:

var notificationHour = 6;
var anticipateDays = [0, 1, 7];

could be equivalently expressed (in a silly example) by:

var defaultNotificationHour = 6;
var anticipateLength = [ '0d', '18h', 7];

where the 18h explicitly specifies hour (1 day = 24 hours before, minus 6 hours for the equivalent of notificationHour=6, means 18 hours before), and '0d' = '0' = 0 (the 'd' suffix being redundant as it is the default unit), and both the 0 and 7 (days) default to "at 6am on that day" as per defaultNotificationHour. A syntactic alternative to bare-number/string could be bare-number/2-element array (which would remove the need for the "split to val/unit, convert string-val to integer-val" steps), e.g.:

var defaultNotificationHour = 6;
var anticipateLength = [
  [0, 'd'],
  [18, 'h'],
  7,
  [12, null]
];

where, as in the case of the [12, null], null means "default" which means 'd'.

The way this could be handled in the code is, rather than the following trigger logic:

Wake up at notificationHour each day and send a notification if there is something to send for this day = anticipateDays

we have this logic:

Wake up every hour, and if there is a match for anticipateLength (day/hour) then send a notification

and as long as we keep the 'wake up and see if there is an anticipateLength match' logic as lightweight as possible (early exit on no match) and don't hammer the API unnecessarily, that shouldn't become problematic to run every hour. I think we are already pretty close to optimal WRT "early exiting", and "no unnecessary churn".

Edit: of course this approach would mean that a default like [0, 1, 7] would work just fine, as before, for those who don't have sophisticated requirements.

GioBonvi commented 7 years ago

I actual feel like your suggestion is a little bit too complicated for most users (since I imagine this feature would be used by a very small part of the total). I think we should implement it in some other way.

I've not thought a lot about this but what about a {numberOfDays: [hours]} structure? Something like:

{
  0: [6, 20],
  1: [6],
  7: [20]
}

This would mean receiving two emails on the same day (at 6 and at 20), one the day before (at 6) and one a week before (at 20).

It should be easy to integrate in the code: wake up every hour, build an array with the keys corresponding to arrays containing the current hour: the obtained array would be in the same format as the current version one and if it was empty it would mean that for that hour there is nothing left to do.

GioBonvi commented 7 years ago

By the way, @mile1206, rethinking about your original issues I've just realised that it should not happen at all since 6am in Europe translates to late evening of the same day in Australia, while I previously thought that the problem was caused by a change in the day between the two time zones.

I'll have to think about it some more...

rowanthorpe commented 7 years ago

@GioBonvi

What we both suggested is semantically effectively the same (requires exactly the same functionality once the config-syntax has been massaged into usable vars - waking up every hour, etc). Therefore the differences are syntactic (in "how the config-var should look"). The differences between the 3 suggestions so far (which each have different pros/cons) are below. There are many other variations possible (I suggest another further down, which is a bit of a hybrid of our other suggestions, and now I re-read it, I think may be a better suggestion, if readability and flexibility are higher priority than backwards-compatibility):


  1. My first suggestion:

    1. is still an array - can be visually backwards compatible for those who don't want to tweak it (e.g. [0, 1, 7])
    2. can be readable as single-line in its simple form
    3. allows optional specification of day/hour, defaulting to day if not specified
    4. is free-form in terms of order of day and/or hour
    5. allows any of the following within the array
      1. positive integer
      2. string with suffix
      3. string without suffix

    This means it:

    • (+) only requires a "change" from the users who need it, others can copy-paste (or sed) existing setting
    • (=/+) is only less concise for those who need to change it (require day or hour)
    • (+) is more flexible (require day or hour)
    • (-) is less explicit (hour can be overridden, less intuitive for non-technical people)
    • (=/-) is as readable as before, except for those who need the new format, where it can become noticeably less readable
    • (-) needs string-manipulation and sanity-checking
  2. My second suggestion:

    1. is an array-of-arrays rather than an array, with mandatory day-integer-key and optional hour(s)-array-value (null means default) - is not visually backwards-compatible with the previous format
    2. needs to be multiline to be readable
    3. allows optional specification of day/hour, defaulting to day if null
    4. allows positive integer as the first element & string-or-null as the second element of each sub-array
    5. is free-form in terms of order of day and/or hour

    This means it:

    • (-) requires a "change" from the user, rather than just copy-paste (or sed) existing setting
    • (-) is less concise (require day or hour-per-line)
    • (+) is more flexible (require day or hour)
    • (-) is less explicit (hour can be overridden, less intuitive for non-technical people)
    • (-) is noticeably less readable than before
    • (=/-) needs no string manipulation, and minimal sanity-checking on second element
  3. Your suggestion:

    1. is now a hash-of-arrays rather than an array, with mandatory day-integer-key and hour(s)-array-value - is not visually backwards-compatible with the previous format
    2. needs to be multiline to be readable
    3. requires explicit specification of day and hour(s) for each notification(s)
    4. requires different notifications on the same day to be grouped together
    5. only allows positive integers within the object & each sub-array

    This means it:

    • (-) requires a "change" from the user, rather than just copy-paste (or sed) existing setting
    • (-/=) is slightly less concise (require day-and-hour(s)-per-day, but group multiple notifications per-day per-line)
    • (-) is less flexible (require day & hour)
    • (+) is more explicit (no need for default hour, more intuitive for non-technical people)
    • (=/-) is slightly less readable than before
    • (=) needs no string-manipulation, and about the same sanity-checking as before is required

Another variant could be:

var defaultNotificationHour = 6;
var anticipateLength = [
  { day: 0, hour: 6 },       // means "06:00" on the day of the event
  { day: 2, hour: null },    // null 'hour' means default (06:00)
  { day: 3 },                // no 'hour' means default (06:00)
  { hour: 50 },              // no 'day' means count the hour as "how many hours before
                             // the event" instead of "hour of day"
  { day: 10, hour: [8, 20] } // array means at all specified hours on that day
];

and the default setting (that would match the existing one) for that variant would be:

var defaultNotificationHour = 6;
var anticipateLength = [
  { day: 0 },
  { day: 1 },
  { day: 7 }
];
mile1206 commented 7 years ago

Hi again!

Sorry to be pushy. Has there been any progress in this issue? Appreciate your help.

Cheers Michael

GioBonvi commented 7 years ago

Hello @mile1206, sorry, I complete forgot to update you.

Regarding the original bug I failed to identify the root cause of the date shift (I am still running some tests, but they proceed very slowly ajd without any conclusively finding)

Regarding the multiple hours/dates setting we still have not settled on a definitive answer.

Today or tomorrow at least a new version (v4.0.0) should be released (I'll notify you by posting a message here). I don't think updating to it will solve your problem, but at least that way we will be on the same page and an eventual solution can be directly pushed to the development branch.

Thank you for your patience

baatochan commented 7 years ago

@mile1206 In terms of the issue:

could you paste this function at the end of the script:

function x (){
  var xxx = new Date();
  log.add(xxx + ' ' + xxx.getTimezoneOffset());
}

save script, run it (as you run notifStart during configuration, but now the name is x) and paste the content of the log here (to get the log you press ctrl + enter)?

EDIT: As @GioBonvi suggested it's better to run this function:

function x () {
  var xxx = new Date();
  log.add(xxx + ' ' + xxx.getTimezoneOffset());
  Logger.log('Session timezone: ' + Session.getScriptTimeZone());
}

and it would be great to check the value of script time zone which can be checked in File > Project properties > Info > Time Zone

baatochan commented 7 years ago

Few notes why I suggested that.

I think that the problem may be in line R1927. While the trigger is set to correct time zone in this line we just get new Date() and AFAIK it returns a date in client time zone. However, in our case, the 'client' is the Google server which runs this script, not the user. It's possible that the closest Google server to New Zeland is located in the USA. And in that case when the script is run at 6 AM New Zeland time of X day, new Date() would return 10 AM of X-1 day in the Las Vegas time zone (even if the script takes Europe time zone it would generate problem as 6 AM in New Zeland is 8 PM in Europe). And while using that Date for our script to run it would return events of X-1 day.

That explanation would be correct with the symptoms that @mile1206 gets.

And if that's a case we have to convert this date to client time zone. It doesn't happen to us because AFAIK we're all from Europe and (at least for me in Poland) the closest Google server is located in UTC+2 time zone which corresponds to the time zone in Poland.

However, I'm not 100% sure if that's a case because when I used VPN to connect to Google (from the USA and Japan) it always returned me UTC+2 time zone.

GioBonvi commented 7 years ago

@baatochan

You might be onto something: the VPN thing might not work if the server is chosen based on the location you set in your Google Account (e.g. Google knows you are from Poland not because you are connecting from Poland, but because you have specified so in your account details.

@mile1206

Please follow @baatochan advice, but add this line Logger.log('Session timezone: ' + Session.getScriptTimeZone());

Resulting in:

function x () {
  var xxx = new Date();
  log.add(xxx + ' ' + xxx.getTimezoneOffset());
  Logger.log('Session timezone: ' + Session.getScriptTimeZone());
}

Lastly could you please perform the following:

Thank you

baatochan commented 7 years ago

I thought about that (that Google knows I'm from Poland because I set it) but I don't understand why it would choose Europe server when I connect from a different part of the world. The only explanation I see is that Google chooses my time for the Date() class and not the server time which would be clever but then it would not explain @mile1206 issue.

Or there is even simpler explanation - all google scripts works on servers that use Europe time because of reasons (don't know then why they don't use just UTC time then).

Anyway, there is no need in trying to guess that - it's best to just wait for @mile1206 answer.

mile1206 commented 7 years ago

I added function x and when I run it I get this error:

ReferenceError: "log" is not defined. (line 644, file "Code")Dismiss

mile1206 commented 7 years ago

I followed the fake test date routine and this is the log:

[17-10-16 15:40:24:677 PDT] Testing.
[17-10-16 15:40:24:701 PDT] Test date: Tue Oct 03 2017 20:48:00 GMT-0700 (PDT)
[17-10-16 15:40:24:702 PDT] Starting run of Google Birthday Notifier version 2.1.3.
[17-10-16 15:40:24:703 PDT] Date used: Tue Oct 03 2017 20:48:00 GMT-0700 (PDT)
[17-10-16 15:40:24:707 PDT] Checking birthdays from 2017-10-05T03:48:00Z to 2017-10-05T03:48:01Z
[17-10-16 15:40:24:824 PDT] Found 1 birthdays in this time range.
[17-10-16 15:40:24:826 PDT] Contact #0
[17-10-16 15:40:24:828 PDT] Has full name
[17-10-16 15:40:28:500 PDT] Has birthday year.
[17-10-16 15:40:28:505 PDT] Has phones.
[17-10-16 15:40:28:512 PDT] Checking birthdays from 2017-10-06T03:48:00Z to 2017-10-06T03:48:01Z
[17-10-16 15:40:28:740 PDT] Found 0 birthdays in this time range.
[17-10-16 15:40:28:741 PDT] Sending email...
[17-10-16 15:40:28:779 PDT] Email sent.
baatochan commented 7 years ago

Oh, my bad, then run this:

function x () {
  var xxx = new Date();
  Logger.log(xxx + ' ' + xxx.getTimezoneOffset());
  Logger.log('Session timezone: ' + Session.getScriptTimeZone());
}

However from this test output it can be seen that the script uses PDT time zone.

Also, it would be great to check the value of script time zone which can be checked in File > Project properties > Info > Time Zone

GioBonvi commented 7 years ago

@mile1206, which date and hour did you use to run the test? (the exact vale you set fakeTestDate to)

mile1206 commented 7 years ago

the log to the x function:

[17-10-17 00:37:51:063 PDT] Tue Oct 17 2017 00:37:51 GMT-0700 (PDT) 420 [17-10-17 00:37:51:064 PDT] Session timezone: America/Los_Angeles

faketestdate was: var fakeTestDate = '2017/10/03 20:48:00';

baatochan commented 7 years ago

Could you provide us script time zone which can be set in File > Project properties, in the tab 'info' there is a field called 'Time Zone'? It looks like it's set to Los Angeles

mile1206 commented 7 years ago

yea its gmt -8 pacific time

baatochan commented 7 years ago

So the easiest way to solve you're issue is to change that to New Zealand time

mile1206 commented 7 years ago

i did put in auckland/pacific time though, in the customization part of the script

baatochan commented 7 years ago

Yes, but it looks like it's not obeyed everywhere. As for now just change it and it should be fine.

@GioBonvi I'm working at making the script working completely without taking this setting into account and I hope I'll finish it until today evening so it should be ready before v4.0.0 release.

mile1206 commented 7 years ago

okay thanks

baatochan commented 7 years ago

@GioBonvi, @rowanthorpe if you'll have time look at time-zone-issue branch in my fork.

My idea was that if the script uses wrong time zone then the date in the correct time zone has to be converted to the corresponding exact date (same day and hour) in that wrong script time zone. I think I was able to do it, but I don't know. After few testing, it looks like it works.

If the functions that I wrote are OK (and useful for us) then I can start to implementing usage of them in the real code (right now they are never used).

GioBonvi commented 7 years ago

Thanks, I'll look at it this evening

GioBonvi commented 7 years ago

I have looked into your code, but have not tested it: does it solve this issue? On which date could you apply the functions?

I ask this because on second thought I believe that the issue is originated by a date formatting issue instead of an actual date issue: let me explain why.

In javascript dates are fundamentally timestamps (seconds passed from the 1/1/1970): they do not take timezones into account (except when outputting the date as string).
For example 10/17/2012 20:00 (UTC) and 10/17/2012 21:00 (UTC+1) would be the same date in javascript (having timestamp 1350504000).

Now: I've checked and tested this: the script calls the trigger at the right time. If the user has set it to fire at 20:00 Pacific/Auckland on 17/10/2017 it will do so correctly, so new Date() will generate an object based on the timestamp 1508223600 both if the script time zone is set to Pacific/Auckland or Europe/Paris or GMT-8.

The timezone matters when outputting the date as string, because 1508223600 represents an instant in time, not a date. So 1508223600 in Pacific/Auckland is 17/10/2017 20:00, while in Europe/Paris it is 17/10/2017 09:00 and in America/Los angeles it is 17/10/2017 00:00.

This would let me think that the probelm is somewhere in the output, but I could not find it.

mile1206 commented 7 years ago

Just to let you know, the problem persists. I changed the time zone as batochan suggested but in the log files I still see Los Angeles time. Also when I delete the one additional day that I put in as a brute force solution, birthdays migrate to one day before they actually are, so from 4. October to 3. October.

I think there is a fundamental flaw in the time calculation somewhere.

GioBonvi commented 7 years ago

Yes, there probably is, but don't have enough time to put into this to find out the culprit right now. In fact I have lttile time to work on this project in general 😾

mile1206 commented 7 years ago

sorry, just pointing it out.

GioBonvi commented 7 years ago

Pease excuse me for being rude: I was just angry with myself, because I don't have the time to do anything I'd like to do in these days...
In fact I thibk that once v4.0.0 is released I am taking some days off: hopefully @baatochan, @rowanthorpe or some kind stranger is able to sort this out.

Sorry again

baatochan commented 7 years ago

@mile1206 what is the log of the x function after changing what I suggested? Is it the same?

mile1206 commented 6 years ago

it is: [17-10-19 01:22:47:317 PDT] Thu Oct 19 2017 21:22:47 GMT+1300 (NZDT) -780 [17-10-19 01:22:47:319 PDT] Session timezone: Pacific/Auckland

but that didn't solve the issue unfortunately.

baatochan commented 6 years ago

@mile1206 I'm really sorry for leaving you without the answer for so long. I've been very busy with my university stuff lately.

Does the problem persist or has it been resolved by changing this setting?

mile1206 commented 6 years ago

Hi! The code has a flaw in time zone calculation but I am unable to pinpoint it.

I have a - not very elegant - solution that I am happy with: 1) to have several times of the day I am running two scripts. 2) to make up for incorrect timing I added a day in the calculation of formattedDate as explained above as 'brute force'

formattedDate = Utilities.formatDate(new Date(now.getTime() + timeInterval + 24 60 60 * 1000), calendarTimeZone, _('dd-MM-yyyy'));

GioBonvi commented 6 years ago

Closing this as the fix has been merged to master.