-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
feat: Allow shared calendars as appointment conflict calendars #48621
Conversation
2ab7d1e
to
4edb1a0
Compare
There was a problem hiding this 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
@SebastianKrupinski what do you think about Thomas' suggestion? Is it doable? |
@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? |
Ah yes, it's a different case than #36681. You're right. |
There was a problem hiding this 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
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. |
There was a problem hiding this 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
Signed-off-by: SebastianKrupinski <[email protected]>
4edb1a0
to
1650417
Compare
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.