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

Add a trashbin for calendars and calendar objects #26083

Merged
merged 2 commits into from
May 31, 2021

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Mar 12, 2021

This adds the calendar part of #1662.

Todo

  • Db structure to mark calendar as deleted
  • Db structure to mark calendar object as deleted
  • URI conflict handling
  • Exclude deleted calendars from allprop requests list them, but give them a distinct resourcetype
  • Exclude deleted calendar objects from requests
  • Soft delete calendars only, keep their objects and shares
  • Soft delete objects only, keep their properties
  • Add a dedicated calendars_trashbin root to list deleted trashbins
  • Expose deleted-at as DAV property
  • Add a retention background job
  • Handle calendar restore, possibly like https://github.com/nextcloud/server/blob/master/apps/files_trashbin/lib/Sabre/RestoreFolder.php
  • Handle object restore
  • Etag propagation on delete/restore
  • Handling of shared claendars

API

List deleted objects and their contents

REPORT /remote.php/dav/calendars/admin/trashbin/objects/

<x1:calendar-query xmlns:x1="urn:ietf:params:xml:ns:caldav">
  <x0:prop xmlns:x0="DAV:">
    <x0:getcontenttype/>
    <x0:getetag/>
    <x0:resourcetype/>
    <x0:displayname/>
    <x0:owner/>
    <x0:resourcetype/>
    <x0:sync-token/>
    <x0:current-user-privilege-set/>
    <x0:getcontenttype/>
    <x0:getetag/>
    <x0:resourcetype/>
    <x1:calendar-data/>
  </x0:prop>
  <x1:filter>
    <x1:comp-filter name="VCALENDAR">
      <x1:comp-filter name="VEVENT">
      </x1:comp-filter>
    </x1:comp-filter>
  </x1:filter>
</x1:calendar-query>

note: filter is mandatory as per caldav specs (sabre enforces) but they are ignored. you can send anything as filter criteria …

Restore a calendar

MOVE /remote.php/dav/calendars/admin/my-deleted-cal

Destination: /remote.php/dav/calendars/admin/trashbin/restore/my-deleted-cal

Restore a calendar object (event)

MOVE /remote.php/dav/calendars/admin/trashbin/objects/73.ics

Destination: /remote.php/dav/calendars/admin/trashbin/restore/73.ics

What I tested

Nextcloud calendar

  • Deleted calendar is hidden from list
  • Deleted calendar object is not rendered
  • Restored calendar object is rendered again

Thunderbird

  • Deleted calendar is hidden from list
  • Deleted calendar object is not rendered
  • Restored calendar object is rendered again

Evolution

  • Deleted calendar is hidden from list
  • Deleted calendar object is not rendered
  • Restored calendar object is rendered again

DAVx5

  • Delete calendar collection (tick delete date from server)
  • Deleted calendar is hidden from list
  • Restore calendar is shown again after calendars refresh
  • Deleted calendar object is not rendered
  • Restored calendar object is rendered again

'length' => 4,
'unsigned' => true,
]);

Copy link
Member

Choose a reason for hiding this comment

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

No index for calendarobjects?

apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
config/config.sample.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/CalendarTrashbinHome.php Outdated Show resolved Hide resolved
@ChristophWurst

This comment has been minimized.

@tcitworld

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@rullzer

This comment has been minimized.

@ChristophWurst

This comment has been minimized.

@tcitworld
Copy link
Member

tcitworld commented Mar 19, 2021

Do we even need to use RestoreFolder at all? Can't you just move the object to it's original collection directly?

/me knows nothing about why it's used in files_trashbin or files_versions.

@ChristophWurst
Copy link
Member Author

Do we even need to use RestoreFolder at all? Can't you just move the object to it's original collection directly?

I tried that first but then we run into the issue that the URI is identical. Which brings us back to the question whether URIs should be changed when a calendar object is deleted …

@tcitworld
Copy link
Member

tcitworld commented Apr 15, 2021

  • Check if calendar subscriptions are handled correctly

@ChristophWurst
Copy link
Member Author

@tcitworld now that the trashbin was moved into the user's calendar home, couldn't that mean that there is a potential conflict between the new trashbin collection and a user's calendar with the same URI?

@tcitworld
Copy link
Member

Indeed, we need to reserve that URI just like the birthday calendar.

@ChristophWurst
Copy link
Member Author

Check if calendar subscriptions are handled correctly

I have to check the details but right now I think subscriptions are just not going into the trashbin. They are deleted like before.

@ChristophWurst
Copy link
Member Author

If I'm not mistaken then my todo list is done, yet I'm not fully convinced of the design.

The solution with the changed resource type for deleted calendars and that clients still see but ignore the node is solid. But hiding the objects from listing but giving them away when you directly access them (with the -deleted suffix) is strange. I'll dive deeper to find out whether it's not somehow possible to have objects in the calendar that will be ignored by clients like the deleted calendars.

@ChristophWurst
Copy link
Member Author

I'll dive deeper to find out whether it's not somehow possible to have objects in the calendar that will be ignored by clients like the deleted calendars.

The RFC says a calendar can only contain calendar object resources. In that sense it's hard to have anything in there that isn't an event. I'll therefore keep the logic where we move the object to a suffix'ed URI and add a flag so the object is excluded from ordinary calendar queries/listings.

@tcitworld
Copy link
Member

tcitworld commented Apr 22, 2021

  • Test what happens for invites on deleted calendar objects
  • Test what happens for reminders on deleted calendar objects

@ChristophWurst
Copy link
Member Author

Test what happens for invites on deleted calendar objects

Haven't done that yet but I assume anything that we do immediately now could be done either still immediately or eventually when the object is dropped. So the question is more what is the expected behavior. As this is a feature that isn't specified by an official *dav RFC, I supposed we could only look into other non-dav solutions to compare how they handle it.

@ChristophWurst

This comment has been minimized.

ChristophWurst added a commit that referenced this pull request Apr 23, 2021
The etag is only set for files, but it's also possible that in edge
cases the copy destination node can't be found. In that case we don't
need to set any etag.

Required for #26083

Signed-off-by: Christoph Wurst <[email protected]>
@ChristophWurst ChristophWurst force-pushed the feature/dav-calendar-and-object-trashbin branch from 809b014 to b6b56ec Compare April 23, 2021 10:05
@MorrisJobke MorrisJobke mentioned this pull request May 20, 2021
@ChristophWurst

This comment has been minimized.

@ChristophWurst ChristophWurst force-pushed the feature/dav-calendar-and-object-trashbin branch from 8b28ff5 to ee3ba56 Compare May 20, 2021 17:04
@ChristophWurst
Copy link
Member Author

Test what happens for invites on deleted calendar objects

I tested this.

I meant what happens when invites are sent, then the event is deleted, and an invite is answered after that. Since we change the calendar object uid we should be fine, but we should check it anyway.

Well depends on what we define as fine 😉. The recipient can still confirm their attendance. But that attendance is on an event that doesn't exist anymore for the organizer.

I don't know what is the right thing to do here to be honest. Keeping it like it is seems a bit meh, but also not allowing the recipient to confirm when the organizer later restores is meh as the confirmation is lost :/

@ChristophWurst
Copy link
Member Author

ChristophWurst commented May 20, 2021

I don't know what is the right thing to do here to be honest. Keeping it like it is seems a bit meh, but also not allowing the recipient to confirm when the organizer later restores is meh as the confirmation is lost :/

So what if we do this: the links works, the event is updated, but the recipient also gets a little warning that the event they just confirmed/rejected is actually deleted.

This is also a bit strange but at least we do keep their attendance information and the recipient is aware that the event won't take place.

On a related note I think this topic isn't quite solved in general. I noticed that we send out cancellation emails when an event is deleted, but not when a whole calendar is deleted. This feels a bit inconsistent.

@ChristophWurst ChristophWurst force-pushed the feature/dav-calendar-and-object-trashbin branch from ee3ba56 to 97f042a Compare May 20, 2021 17:39
@ChristophWurst ChristophWurst marked this pull request as ready for review May 20, 2021 17:43
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 20, 2021
@tcitworld
Copy link
Member

So what if we do this: the links works, the event is updated, but the recipient also gets a little warning that the event they just confirmed/rejected is actually deleted.

Yup. Check with ITip if there's a special status for this as well.

I noticed that we send out cancellation emails when an event is deleted, but not when a whole calendar is deleted. This feels a bit inconsistent.

That could trigger a lot of emails being sent if there's many future events with attendees in the calendar. Would be great if specs or CalConnect gave advice on this.

@tcitworld
Copy link
Member

Is it worth it to have a repair step to rename existing calendars which URI is exactly trashbin?

@tcitworld
Copy link
Member

It would be nice to be able to filter on the deleted-calendar property (or at least deleted-at) so that the trashbin UI can get only the deleted calendars instead of getting all of the user's calendars and do the filtering afterwards.

@MorrisJobke MorrisJobke mentioned this pull request May 26, 2021
98 tasks
@ChristophWurst ChristophWurst force-pushed the feature/dav-calendar-and-object-trashbin branch from 0492643 to 2759fde Compare May 26, 2021 13:37
@ChristophWurst
Copy link
Member Author

It would be nice to be able to filter on the deleted-calendar property (or at least deleted-at) so that the trashbin UI can get only the deleted calendars instead of getting all of the user's calendars and do the filtering afterwards.

Possibly. But we can also possibly do the filtering and splitting from the current listing.

@ChristophWurst
Copy link
Member Author

Failing CI seems unrelated. Three of three failed due to a timeout.

@ChristophWurst
Copy link
Member Author

This works well with nextcloud/calendar#3118. I'll merge this PR here when CI happy. Let's rebase just to be sure.

@ChristophWurst ChristophWurst force-pushed the feature/dav-calendar-and-object-trashbin branch from 2759fde to 421f17a Compare May 28, 2021 20:45
ChristophWurst and others added 2 commits May 31, 2021 07:49
It's needed in ReminderService::onCalendarObjectCreate()

Signed-off-by: Thomas Citharel <[email protected]>
@ChristophWurst ChristophWurst force-pushed the feature/dav-calendar-and-object-trashbin branch from 421f17a to 8a73b18 Compare May 31, 2021 05:49
@ChristophWurst ChristophWurst merged commit 9f70c6c into master May 31, 2021
@ChristophWurst ChristophWurst deleted the feature/dav-calendar-and-object-trashbin branch May 31, 2021 06:29
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants