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

Issue 17 filter date timezone comparison #18

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alexschwantes
Copy link

References #17

This pull request may cause a conflict with the changes in #16 ...

…o UTC to fix an issue where it wasn't identifying the time of the configured event correctly.
…ted end minutes was 0.

Now the end date is are correctly calculated when ending at midnight and compared as dates rather than as minutes of the day.
@alexschwantes
Copy link
Author

Also worth highlighting is that this fix is only for the schedule filter only. I haven't looked at the schedule planner which may also experience the same issues from #10

@hardillb
Copy link
Collaborator

hardillb commented Jan 5, 2017

I'm testing these changes and I'm getting the wrong output.

The times are all out by 1 hour

evtStart:  Thu Jan 05 2017 15:00:00 GMT+0100 (CET)
evtEnd:  Thu Jan 05 2017 16:00:00 GMT+0100 (CET)
now:  Thu Jan 05 2017 14:22:26 GMT+0100 (CET)

The period marked in the node red is 14:00 to 15:00

I'll keep playing

@hardillb
Copy link
Collaborator

hardillb commented Jan 5, 2017

I've been having a play on my branch and I think I have a fix for this that works. Can you test the head from here: https://github.com/hardillb/node-red-contrib-simple-weekly-scheduler

Fixed issue where events ending at midnight weren't recognised as such.
Fixing detection of midnight for the event end for sure this time :)
@alexschwantes
Copy link
Author

Hi, I tested your code but it still fails test 1 and 2 from the issue that I raised.
This is due to the day comparison still comparing in UTC day var day = now.getUTCDay();. If I am in UTC +2 timezone, then it will fail for any event that starts before 2am. This gets exacerbated if you are in a UTC+10 timezone...

I also found out what was wrong with my solution and updated the branch. The following check failed because getDay() was comparing the day of the stored event, which is stored as a date in 2009. So the day comparison worked only by luck on the day that I was testing it..

if(evtEnd.getDay() > day) {
    midnight = 1;
}

I've updated it so that it correctly determines if the event ends at midnight, regardless of the actual date/day.

if(evtEnd.getHours() +  evtEnd.getMinutes() == 0) {

This seems to be working now for me. Are you able to give that a test?

@hardillb
Copy link
Collaborator

I can't remember where we left this? can we check what the current state is and if I should work out how to merge it?

@solariz
Copy link

solariz commented Sep 14, 2017

sad that this wasn't merged early- have the same problem and can confirm there is a TZ problem.

@hardillb
Copy link
Collaborator

@solariz feel free to see if you can clean this up so it doesn't conflict anymore

@k3nz3l
Copy link

k3nz3l commented Apr 4, 2019

Hi hardillb, I am experiencing the same issue in version 0.0.14. After changing to daylight savings time, my schedules don't work anymore. Figured out that they are off by 2h now.
Is there any estimate on when this issue will be fixed? I'd be happy to help if necessary.

@hardillb
Copy link
Collaborator

hardillb commented Apr 4, 2019

@k3nz3l this node (and all others in this org) were written under contract for Tyrrell Systems.

That contract has long since expired, so it is up to them as to if they want to improve/maintain them.

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

Successfully merging this pull request may close these issues.

4 participants