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

Better fixes for #3291 and the underlying exdate issues #3342

Merged
merged 15 commits into from Jan 27, 2024

Conversation

jkriegshauser
Copy link
Contributor

@jkriegshauser jkriegshauser commented Jan 9, 2024

  • Worked around several issues in the RRULE library that were causing deleted calender events to still show, some initial and recurring events to not show, and some event times to be off an hour. (one calendar test fails since today #3291)
  • Renamed variables in calendarfetcherutils.js to be more clear about use of moment and js's Date class.
  • Added calendar config option forceUseCurrentTime (default:false) which will ignore overridden Date.now in the config in order to keep some tests consistent.
  • Added several unit tests for crossing DST in different timezones with excluded events.

@rejas
Copy link
Collaborator

rejas commented Jan 9, 2024

so, whats this PR about actually ;-) ?

@jkriegshauser
Copy link
Contributor Author

so, whats this PR about actually ;-) ?

It’s a WIP for better fixes for #3291 and the underlying exdate issues. I’m still seeing breakthrough events that have been excluded showing up. Stay tuned…

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 9, 2024

@jkriegshauser note that we changed rrule processors this release,(dragged in from node-ical) and are seeing weird results. skipped dates (null returned where date should be)

@jkriegshauser
Copy link
Contributor Author

@jkriegshauser note that we changed rrule processors this release,(dragged in from node-ical) and are seeing weird results. skipped dates (null returned where date should be)

Odd, where is the null date being returned?

@sdetweil
Copy link
Collaborator

sdetweil commented Jan 9, 2024

from the rrule.between in calendarfetcherutils.js

					Log.debug(`Search for recurring events between: ${pastLocal} and ${futureLocal}`);
					let dates = rule.between(pastLocal, futureLocal, true, limitFunction);   // <---- here 
					Log.debug(`Title: ${event.summary}, with dates: ${JSON.stringify(dates)}`); // you can see them in the debug output here 
					dates = dates.filter((d) => {      // this filter added in this release to remove null entries which cause crash
						if (JSON.stringify(d) === "null") return false;
						else return true;
					});

from a user with problem

1704763166785-screenshot_20240108_191819_chrome-resized

@rejas rejas changed the title Draft (WIP) Vetter fixes for #3291 and the underlying exdate issues Jan 13, 2024
@rejas rejas changed the title (WIP) Vetter fixes for #3291 and the underlying exdate issues (WIP) Better fixes for #3291 and the underlying exdate issues Jan 13, 2024
@jkriegshauser
Copy link
Contributor Author

from a user with problem

@sdetweil thanks, can you provide a link to the forum comment? The URL in the image doesn't seem to take me anywhere.

@jkriegshauser jkriegshauser changed the title (WIP) Better fixes for #3291 and the underlying exdate issues Better fixes for #3291 and the underlying exdate issues Jan 20, 2024
@jkriegshauser jkriegshauser marked this pull request as ready for review January 20, 2024 05:17
@jkriegshauser
Copy link
Contributor Author

@rejas @sdetweil @khassel this is ready for review now. Sam, I kept the null filtering that you mentioned above, and I believe this will also address the problems mentioned in the forum post above.

@KristjanESPERANTO
Copy link
Contributor

Thank you for your work! In case we need to look at the PR again later: Can you write in the initial comment what was changed? I think the text from the changelog would suffice.

@khassel
Copy link
Collaborator

khassel commented Jan 22, 2024

Can you write in the initial comment what was changed? I think the text from the changelog would suffice.

I updated the description.

@jkriegshauser
Copy link
Contributor Author

jkriegshauser commented Jan 22, 2024 via email

@khassel
Copy link
Collaborator

khassel commented Jan 22, 2024

Ah I was just working on this.

Sorry, you can update again ...

I'm fine with this PR, can this be merged? @sdetweil @rejas

@jkriegshauser
Copy link
Contributor Author

Alright, should be gtg now

@jkriegshauser
Copy link
Contributor Author

jkriegshauser commented Jan 27, 2024

@rejas @sdetweil Can this be merged? I've been running it on two local instances for a week and had no issues. @khassel gave approval above. I just resolved conflicts

@rejas rejas merged commit 7f0b8e4 into MagicMirrorOrg:develop Jan 27, 2024
5 checks passed
@rejas
Copy link
Collaborator

rejas commented Jan 27, 2024

Sorry for delaying this longer than needed. Thanks for your work (and the insane amount of tests ;-)
Now only some doc updates are needed ;-)

@jkriegshauser
Copy link
Contributor Author

jkriegshauser commented Jan 27, 2024 via email

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.

None yet

5 participants