Skip to content

fix: remove unneccerasy conversion of recurrence IDs to UTC #124

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 4, 2025
Merged

fix: remove unneccerasy conversion of recurrence IDs to UTC #124

merged 4 commits into from
Feb 4, 2025

Conversation

abe-101
Copy link
Member

@abe-101 abe-101 commented Feb 4, 2025 •
edited by niccokunzmann
Loading

  • Add failing test to ensure calendar merger accepts both date and datetime recurrence IDs in iCal events
  • Test demonstrates current issue where date-only recurrence IDs break calendar merging
  • Will be followed by fix to properly handle both types of recurrence IDs

Part of bug #123
Closes #126

Description of change

Pull-Request Checklist

  • Code is up-to-date with the main branch
  • This pull request follows the contributing guidelines.
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions outlined in the conventional commit spec, such as "fix(api): prevent racing of requests".
  • If pre-commit.ci is failing, try pre-commit run -a for further information.
  • If CI / test is failing, try uv run pytest for further information.

@abe-101
test: verify calendar merger handles date and datetime recurrence IDs
023462b 
- Add failing test to ensure calendar merger accepts both date and datetime
  recurrence IDs in iCal events
- Test demonstrates current issue where date-only recurrence IDs break
  calendar merging
- Will be followed by fix to properly handle both types of recurrence IDs

Part of bug #123
@abe-101 abe-101 linked an issue Feb 4, 2025 that may be closed by this pull request
AttributeError when processing iCloud calendar with DATE-only RECURRENCE-ID #123
Closed
2 tasks
@abe-101 abe-101 requested a review from niccokunzmann February 4, 2025 00:57
@abe-101 abe-101 added the bug Something isn't working label Feb 4, 2025
@abe-101
Copy link
Member Author

abe-101 commented Feb 4, 2025

TODO: fix the issue so the test don't fail :)

niccokunzmann
niccokunzmann reviewed Feb 4, 2025
View reviewed changes
tests/test_merge_calendars_with_date_recurrence_id.py
def test_merge_calendars_with_date_recurrence_id(merge: Any) -> None:
"""Test that merging calendars works with both date and datetime recurrence IDs."""
merged = merge(["calendar_with_date_recurrence_id.ics"])
events = list(merged.walk("VEVENT"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the latest icalendar, you can do merged.events

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know

tests/test_merge_calendars_with_date_recurrence_id.py
for event in recurrence_events:
recurrence_id = event["RECURRENCE-ID"].dt
# Should be either a date or datetime
assert isinstance(recurrence_id, (date, datetime)), (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, here one could test if a certain recurrence id is included. The fix could also just delete the event or skip it - would the test then still succeed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason we want to convert the recurrence ID to UTC?
If yes, then we should test if the recurrence ID is a datetime object, and only then converted to UTC. Otherwise, leave it as a date.

That's how I think we should fix the issue

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason we want to convert the recurrence ID to UTC?

I did that for recurring ical events because I wanted to match them (some are without timezone, some with, some use date although they should use datetime). It might not be needed at all for this use case - I would say it is probably worth not converting to UTC and waiting for an issue to arise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rare case is: microsoft outlook (date recurrence id) gets accepted by another client, this converts the id and then sends it back and now they do not match. But that is not of concern here, I think.

(merging calendars <= accepting invites)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree
I don't think this is a concern we need to focus on yet

That bring us to the solution of just removing the conversion to UTC, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

less code better code :)

@abe-101 abe-101 mentioned this pull request Feb 4, 2025
Closed
@codecov Codecov
Copy link

codecov bot commented Feb 4, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/mergecal/calendar_merger.py 84.84% <100.00%> (-1.27%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@abe-101 abe-101 changed the title test: verify calendar merger handles date and datetime recurrence IDs fix: remove unneccerasy conversion of recurrence IDs to UTC Feb 4, 2025
@abe-101 abe-101 force-pushed the 123-attributeerror-when-processing-icloud-calendar-with-date-only-recurrence-id branch from c29df98 to bde3ff3 Compare February 4, 2025 11:05
@abe-101 abe-101 merged commit 78b0e2e into main Feb 4, 2025
17 checks passed
@abe-101 abe-101 deleted the 123-attributeerror-when-processing-icloud-calendar-with-date-only-recurrence-id branch February 4, 2025 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@niccokunzmann niccokunzmann niccokunzmann left review comments

Assignees

@niccokunzmann niccokunzmann

@abe-101 abe-101

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

wich lock file? AttributeError when processing iCloud calendar with DATE-only RECURRENCE-ID

2 participants

@abe-101 @niccokunzmann