[Fix] Calendar-Module: Event start time correction based on time zone #2634
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2634 +/- ##
===========================================
+ Coverage 70.47% 71.01% +0.54%
===========================================
Files 16 16
Lines 823 828 +5
===========================================
+ Hits 580 588 +8
+ Misses 243 240 -3
Continue to review full report at Codecov.
|
| @@ -296,38 +307,42 @@ const CalendarUtils = { | |||
| // Loop through the set of date entries to see which recurrences should be added to our event list. | |||
| for (let d in dates) { | |||
| let date = dates[d]; | |||
| // FIXME: We now use node-ical instead of ical.js, but method stays the same. | |||
There was a problem hiding this comment.
Thats alittle unclear, what should be fixed here?
There was a problem hiding this comment.
The comment says that we use(d) ical.js to parse the ical-file, which was changed in v 2.15. if I remember correctly.
Currently we are using node-ical, such that the comment is out of date but the method is still valid.
Maybe there is more elegant way using a built-in method, since the date variable represents a date object - but I have to double check this.
In both cases the comment has to be changed or removed but since this does not affect the issue and just served as a reminder.
Currently I'm reviewing the whole calendar module, making simplifications and refactoring such that I will have a second look on it :)
There was a problem hiding this comment.
So basically the comment should just be changed to say "node-ical" instead of "ical" , right?
There was a problem hiding this comment.
Yes, as well as line 313 (I don't know how to mark or link this line), since it also talks about ical.js.
Can you do this or do I have to redo the PR?
For further PR the whole procedure might change, since we could maybe use a built-in function - but this is another topic.
There was a problem hiding this comment.
You can just add another commit with the changes to your branch and push it. The PR will get updated automatically.
There was a problem hiding this comment.
Ah, perfect. Thank you very much!
I'm going to update the PR in a few seconds :)
Done.
PR fixes #2632.
The time zone correction was only applied if the date hour of the event is smaller than the actual time zone offset.
In my case the date hour was equal the time zone offset but since the javascript date object is able to handle this case (and maybe also the case where the time zone offset is bigger than the date hour) this check is not necessary any longer and also prevents the start date correction.
Done.
Done.
Note: Sometimes the development moves very fast. It is highly
recommended that you update your branch of
developbefore creating apull request to send us your changes. This makes everyone's lives
easier (including yours) and helps us out on the development team.
Done.