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.

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

fix(dav): Add occ command to fix missing caldav sync tokens #44130

Merged
merged 1 commit into from
Mar 14, 2024
Merged

fix(dav): Add occ command to fix missing caldav sync tokens #44130

merged 1 commit into from
Mar 14, 2024

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Mar 11, 2024 •
edited
Loading

Summary

Our CalDAV backend keeps record of all modifications of the calendar so that clients can do a partial and efficient sync. Unfortunately there are cases where we lost that data: #44075.
This change attempts to restore a correct sync state by

  1. Tracking an addition for all existing events not necessary
  2. Tracking a change for all existing events
  3. Tracking a deletion for all events marked as deleted

TODO

  • Test with Thunderbird (me)
  • Test with Apple Calendar (@hamza221 )
  • Test with iPhone (@hamza221 )
  • Test with DAVx5 (me)

How to test

  1. Open Calendar and create a new testing calendar. Add an event "a" and an event "b".
  2. Set up a client for this one calendar.
  3. Synchronize the client.
  4. Close the client.
  5. Delete event b in the Calendar app.
  6. Create an event "c".
  7. Delete the four last rows of oc_calendarchange (two addition, a delete and another addition).
  8. Open the client.
  9. Synchronize the client.
  10. Note: the client doesn't pick up the changes. It shows a and b but should show a and c.
  11. Run occ dav:fix-missing-caldav-changes
  12. Synchronize the client.
  13. Note: the client picked up the changes. It now shows a and c.

Checklist

@ChristophWurst ChristophWurst added bug 2. developing Work in progress feature: dav feature: caldav Related to CalDAV internals labels Mar 11, 2024
@ChristophWurst ChristophWurst self-assigned this Mar 11, 2024
ChristophWurst
ChristophWurst commented Mar 11, 2024
View reviewed changes
apps/dav/lib/CalDAV/CalDavBackend.php
foreach ($uris as $uri) {
$this->addChange($subscriptionId, $uri, 3, self::CALENDAR_TYPE_SUBSCRIPTION);
}
$this->addChanges($subscriptionId, $uris, 3, self::CALENDAR_TYPE_SUBSCRIPTION);
Copy link
Member Author

Choose a reason for hiding this comment

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

this should also help with #40814 because there it eliminates increasing the sync token for every single change

@hamza221 hamza221 self-requested a review March 11, 2024 18:57
@hamza221
Copy link
Contributor

wrong hamza tagged haha, fixed

@ChristophWurst
Copy link
Member Author

There are a few small bugs. Working on them right now.

@ChristophWurst
Copy link
Member Author

Tested with Thunderbird. Seems fine now. Modifications propagate too 😌

@ChristophWurst ChristophWurst marked this pull request as ready for review March 11, 2024 19:21
@ChristophWurst
Copy link
Member Author

/backport to stable28

@ChristophWurst
Copy link
Member Author

/backport to stable27

@ChristophWurst
Copy link
Member Author

/backport to stable26

@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 11, 2024
@ChristophWurst ChristophWurst added this to the Nextcloud 29 milestone Mar 11, 2024
@hamza221
Copy link
Contributor

Tested on Mac ✅

@ChristophWurst
Copy link
Member Author

Tested with DAVx5. Works, but also doesn't seem to use the sync mechanism.

@Altahrim Altahrim mentioned this pull request Mar 12, 2024
Merged
miaulalala
miaulalala approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

Tested per instructions with Thunderbird, works as expected - the second sync doesn't pick up the changes but after running occ dav:fix-missing-caldav-changes the new event is show after another sync. Very nice 👏

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 12, 2024
@ChristophWurst
fix(dav): Add occ command to fix missing caldav sync tokens
f40f962 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the fix/dav/occ-fix-caldav-sync-tokens branch from bf9716c to f40f962 Compare March 12, 2024 11:44
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Mar 12, 2024
@ChristophWurst ChristophWurst mentioned this pull request Mar 12, 2024
Merged
@Altahrim Altahrim mentioned this pull request Mar 14, 2024
Merged
@ChristophWurst ChristophWurst merged commit d4ac4b8 into master Mar 14, 2024
161 checks passed
@ChristophWurst ChristophWurst deleted the fix/dav/occ-fix-caldav-sync-tokens branch March 14, 2024 13:05
This was referenced Mar 14, 2024
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamza221 hamza221 hamza221 approved these changes

@miaulalala miaulalala miaulalala approved these changes

@tcitworld tcitworld Awaiting requested review from tcitworld tcitworld is a code owner

Assignees

@ChristophWurst ChristophWurst

Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: caldav Related to CalDAV internals feature: dav pending documentation This pull request needs an associated documentation update
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.
3 participants
@ChristophWurst @hamza221 @miaulalala