Corrected timezone handling for recurring events #231
Conversation
There was a problem hiding this comment.
First of all, thank you for your submitted contribution. However, please be more verbose in what exactly you are trying to fix/change and explain why exactly are you doing a certain change.
Unfortunately, you changes are not self-explaining, thus I cannot fully follow or identify each individual change. So please explain in some more details each individual change/fix.
In addition, please take note that some unit test is failing with you changes incorporated. Thus, please check and correct that as well.
| @@ -577,7 +591,9 @@ module.exports = { | |||
| // If the original date has a TZID, add it | |||
| if (curr.start.tz) { | |||
| const tz = getTimeZone(curr.start.tz); | |||
| rule += `;DTSTART;TZID=${tz}:${curr.start.toISOString().replace(/[-:]/g, '')}`; | |||
| const tzoffset = moment.tz(tz).utcOffset() * -60000; | |||
There was a problem hiding this comment.
Why -60000 is required here? This smells like a modification for DST/non-DST which might not be valid in other time zones. Please explain.
There was a problem hiding this comment.
In fact, the minus in -60000 compensates with the minus in Date(curr.start - tzoffset) in the next line in order to avoid a plus to be interpreted as implicit string concatenation. The factor 60000 is for conversion from minutes to milliseconds.
| @@ -138,9 +138,19 @@ function getTimeZone(value) { | |||
| // And offset is still present | |||
| if (tz && tz.startsWith('(')) { | |||
| // Extract just the offset | |||
| const regex = /[+|-]\d*:\d*/; | |||
| const offsetcomps = tz.match(/([+|-]\d+):?(\d+)/); | |||
There was a problem hiding this comment.
Please explain why exactly you changed that regex and what the added lines following this change try to fix/change.
There was a problem hiding this comment.
I try to map indications like (GMT +02:00) to matching IANA etcetera zones, if available. The advantage of those IANA zone names is that they can be processed by moment-timezone.
| if (tz && tz.startsWith('(')) { | ||
| // Extract just the offset | ||
| const regex = /[+|-]\d*:\d*/; | ||
| offset = tz.match(regex); | ||
| const offsetcomps = tz.match(/([+|-]\d+):?(\d+)/); |
There was a problem hiding this comment.
same here, why is this regex change required?
|
Thank you for your review and detailed feedback!
Indeed, one of the tests is failing, which wasn't the case, when I committed this (before Oct 30). This could be a DST problem. I will look into it. |
|
Just wanted to know is this PR going to get merge soon ? |
|
Bump |
|
This feels like a critical bug... not sure why this hasn't been figured out yet. I've put up an alternative solution in #329, which hopefully will be easier to review? |
|
This issue has been resvoled via #329 |
... and fixed some apparent bugs along the way.
This includes new tests. All existing tests succeed.