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: Allow shared calendars as appointment conflict calendars #48621

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented Oct 8, 2024

Resolves: nextcloud/calendar#3786
Requires: nextcloud/calendar#6411

Summary

Adjusted logic to use getCalendarsForUser to retrieve calendars, as this returns all personal and shared calendars.

Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

This looks not very efficient? Since you're always fetching all of the user's calendars even if you want to get a single one?

Maybe something like this old PR instead ? #36681

@ChristophWurst
Copy link
Member

@SebastianKrupinski what do you think about Thomas' suggestion? Is it doable?

@SebastianKrupinski
Copy link
Contributor Author

SebastianKrupinski commented Oct 16, 2024

@ChristophWurst I did have a look at the old PR. It can not be applied/used for this situation directly as the CalendarProvider uses the back end directly and not through the CalendarHome class, we could add the getSharedCalendarByUri method to the back end and then do a separate call in the CalendarProvider for any shared calendars.

As the CalendarProvider->getCalendars() takes in a array of uri's I though it would be more efficient to just make two calls to the database (all shared and all personal) and then filter out the required calendars instead of making two calls to the database for each uri (one for shared and one for personal). If the getCalendars() is called with 4 uris, one way we only make 2 database quires the other way we make 8.

What do you prefer?

@tcitworld
Copy link
Member

Ah yes, it's a different case than #36681. You're right.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

I'm not opposed this change but it has to be tested carefully.

Listing all calendars, also those which are shared by others, influences all places that use the \OCP\Calendar\IManager::getCalendarsForPrincipal public API. We can't easily build an exhaustive list of users because this API is also used by apps.

Some places that use it:

  • dav app - user status automation. Events in shared calendars will make you show as In a meeting. Intended?
  • dav app - TimezoneService. If a shared calendar with timezone information is listed before a owned calendar, it will influence the detected timezone of the user. Intended?
  • polls app

@SebastianKrupinski
Copy link
Contributor Author

I'm not opposed this change but it has to be tested carefully.

Listing all calendars, also those which are shared by others, influences all places that use the \OCP\Calendar\IManager::getCalendarsForPrincipal public API. We can't easily build an exhaustive list of users because this API is also used by apps.

Some places that use it:

* dav app - user status automation. Events in shared calendars will make you show as _In a meeting_. Intended?

* dav app - TimezoneService. If a shared calendar with timezone information is listed before a owned calendar, it will influence the detected timezone of the user. Intended?

* polls app

As per our conversation, the returns are logically the same. The previous code path would return all calendars if no uri's are provided and only the asked for calendars if uri's are provided. The only real change is where the calendars list is sourced and how its filtered.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good! Did not test

@SebastianKrupinski SebastianKrupinski force-pushed the feat/issue-3786-allow-shared-calendars branch from 4edb1a0 to 1650417 Compare October 29, 2024 01:12
@SebastianKrupinski SebastianKrupinski merged commit 9d49c75 into master Nov 4, 2024
177 checks passed
@SebastianKrupinski SebastianKrupinski deleted the feat/issue-3786-allow-shared-calendars branch November 4, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
Development

Successfully merging this pull request may close these issues.

Allow shared calendars as appointment conflict calendars
4 participants