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

Fix handling of modified recurrences with lower sequence number than their base event #45

Merged

Conversation

dstolpmann
Copy link
Collaborator

Hi,
the current implementation assumes that a modified recurrence has a higher sequence number than its base event. However, this is not always the case, as modifying the base event after creating a modified recurrence does only increase the sequence number of the base event.

This pull request fixes this behavior by also using the presents of the sequence ID field to determine if an event replaces another one. I am not sure if this behavior is according to the specification, but this topic seems to be highly controversial as stated in this article: https://icalevents.com/4437-correct-handling-of-uid-recurrence-id-sequence/

Best regards,
dstolpmann

@niccokunzmann
Copy link
Owner

Since you did the research, I am the more happy to merge this.

With the assumption, you are right. I did not know how to handle this.

Thanks for adding an example and tests!
I wonder: The default case should already be accounted for in other places, I believe. So, there is no test necessary, right?

@niccokunzmann niccokunzmann merged commit eacfafd into niccokunzmann:master Sep 24, 2020
@niccokunzmann
Copy link
Owner

Following the release process in the README file, this change can be documented in the changelog. Would you like to create a PR for it, mentioning Issue and to your liking your username in a kind of consistent format with that what was noted down in the changelog before?

@dstolpmann
Copy link
Collaborator Author

Sure! Should I also increase the version number to v0.1.20b?

@niccokunzmann
Copy link
Owner

@dstolpmann yes!

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

2 participants