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

feat(dav): add regex to match Gnome and KDE calendar user-agents #45841

Conversation

Vivida1
Copy link
Contributor

@Vivida1 Vivida1 commented Jun 12, 2024

Summary

Original Pull Request by @msrn: #33729 which became stale.

Quote: "This pull requests adds Gnome Calendar/Evolution and KDE PIM/Akonadi calendars to the list of user-agents in WebCalCaching -plugin, and exposes subscribed calendars to these calendar clients.

User-agent of Gnome Calendar/Evolution
Evolution/3.40.3

KDE PIM/Akonadi user-agent
Mozilla/5.0 (X11; Linux x86_64) KIO/5.86 akonadi_davgroupware_resource_1/5.18.2 (21.08.2)"

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.

I agree with Barth #33729 (comment)

@miaulalala miaulalala requested a review from skjnldsv June 13, 2024 08:04
@msrn
Copy link
Contributor

msrn commented Jun 13, 2024

Thanks for opening the PR again! I have been busy so wasn't able to work on this. I originally envisioned that I would test the user agents again if they still work, because there might have been user agent changes on those calendar clients during these two years.

Also the way I see it. This PR doesn't resolve #17754, as that issue isn't about any particular CalDAV client.

@skjnldsv skjnldsv added this to the Nextcloud 30 milestone Jun 13, 2024
@skjnldsv skjnldsv added the 4. to release Ready to be released and/or waiting for tests to finish label Jun 13, 2024
@tcitworld
Copy link
Member

tcitworld commented Jun 13, 2024

Still conflicted with what I said on the topic in #33729 (review), but I don't mind much.

However, we might as well reverse the logic and include all clients but DAVx⁵, since it's the only one supporting subscription collection discovery (and others did not make progress on this topic).

A drawback that might get observed is that the refresh rate (long by default on NC side, certainly much shorter on client side) might be an issue for most people.

@skjnldsv
Copy link
Member

I'll let @Vivida1 decide 👍

@Vivida1
Copy link
Contributor Author

Vivida1 commented Jun 13, 2024

I think reversing the logic makes more sense from a maintainability standpoint.

@tcitworld, can you explain the refresh rate drawback?

I can prepare a commit but for that I need to find the DAVx⁵ user-agent first.

@msrn
Copy link
Contributor

msrn commented Jun 13, 2024

I think reversing the logic makes more sense from a maintainability standpoint.

@tcitworld, can you explain the refresh rate drawback?

I can prepare a commit but for that I need to find the DAVx⁵ user-agent first.

Not 100% sure, but it might be just DAVx5 as per
https://github.com/bitfireAT/davx5-ose/blob/1d7084b55533b7000fd049f3d97f92d6ee748a59/app/build.gradle.kts#L31

edit: Did a request and got this

"DAVx5/4.3.16.1-ose (2024/04/20; dav4jvm; okhttp/4.12.0) Android/14"

@tcitworld
Copy link
Member

tcitworld commented Jun 13, 2024

@tcitworld, can you explain the refresh rate drawback?

The Nextcloud server caches the subscriptions (stores data in the database) and exposes them as regular calendars. The default refresh rate for this is of one week, which is appropriate for things like public holiday calendars, but might not be for other things, for instance subscribing to a public calendar from another Nextcloud user, and expecting real-time sync.

This high refresh rate makes sense because on servers with a lot of users and subscriptions, the cron would keep wasting time refreshing calendars for nothing, and there could be a risk of the subscription source blocking the server's IP. For instance, as subscriptions can't be shared (or provided by the admin), if all 10 000 users from a company need to subscribe to the holiday calendar from their local government, that's 10 000 requests you need to make every X minutes/hours/days when the original calendar data only changes once or twice a year.

Nextcloud supports properties available for the subscription source to tell what's the appropriate refresh rate (X-PUBLISHED-TTL, REFRESH-INTERVAL), but in practice no-one provides them.

I think there's an issue about providing the setting to select an appropriate refreshment rate for each subscription to end-users in calendar, which would kinda solve most of the issue, though there's a risk users could abuse it, so a minimal rate setting for the admin would be nice as well. In any case, that's not the current state.

So on the other hand, clients have no issue with refreshing the local subscription every 15 minutes/hour/day or even each time the app is opened, as it's only doing it for a single user and from a dedicated IP address.

Therefore, people who would have been forced to manually add the subscription on their device before this change and will now just used the exposed subscriptions from the NC server may have the impression that subscriptions don't get updated when they expect it.

@miaulalala
Copy link
Contributor

@tcitworld, can you explain the refresh rate drawback?

The Nextcloud server caches the subscriptions (stores data in the database) and exposes them as regular calendars. The default refresh rate for this is of one week, which is appropriate for things like public holiday calendars, but might not be for other things, for instance subscribing to a public calendar from another Nextcloud user, and expecting real-time sync.

This high refresh rate makes sense because on servers with a lot of users and subscriptions, the cron would keep wasting time refreshing calendars for nothing, and there could be a risk of the subscription source blocking the server's IP. For instance, as subscriptions can't be shared (or provided by the admin), if all 10 000 users from a company need to subscribe to the holiday calendar from their local government, that's 10 000 requests you need to make every X minutes/hours/days when the original calendar data only changes once or twice a year.

Nextcloud supports properties available for the subscription source to tell what's the appropriate refresh rate (X-PUBLISHED-TTL, REFRESH-INTERVAL), but in practice no-one provides them.

I think there's an issue about providing the setting to select an appropriate refreshment rate for each subscription to end-users in calendar, which would kinda solve most of the issue, though there's a risk users could abuse it, so a minimal rate setting for the admin would be nice as well. In any case, that's not the current state.

So on the other hand, clients have no issue with refreshing the local subscription every 15 minutes/hour/day or even each time the app is opened, as it's only doing it for a single user and from a dedicated IP address.

Therefore, people who would have been forced to manually add the subscription on their device before this change and will now just used the exposed subscriptions from the NC server may have the impression that subscriptions don't get updated when they expect it.

If we were to improve how we refresh subscriptions, we might be able to not care as much about the refresh rate - IIRC we're not using a delta but completely deleting and re- importing all events. It's not going to be easy to implement but I definitely want to do that at some point.

What we could also do for subscriptions is to compare the URLs for subscription calendars and do an update on all calendars that share the same subscription URLs in a single run.

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@miaulalala miaulalala force-pushed the add-regex-to-match-Gnome-and-KDE-calendar-user-agents branch from d6f9ccf to 3e06931 Compare June 27, 2024 07:46
This was referenced Jul 30, 2024
@Altahrim Altahrim mentioned this pull request Aug 5, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 6, 2024
@skjnldsv skjnldsv merged commit 07d302d into nextcloud:master Aug 7, 2024
154 of 165 checks passed
Copy link

welcome bot commented Aug 7, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: dav feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propagate subscribed calendars to CalDAV clients
5 participants