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): expose Nextcloud groups to Contact's system addressbook contacts #40997

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Oct 19, 2023

Closes #38432

image

And move event listeners related to SyncService to proper ones
Note: Changes to user system addressbook cards are now always done in a QueuedJob

TODO

  • Find how to handle groups being renamed or deleted
  • Decide if groups are exposed by default or not
  • Decide if the ignore list should contain admin

Checklist

@tcitworld tcitworld added 2. developing Work in progress feature: dav feature: carddav Related to CardDAV internals feature: contacts labels Oct 19, 2023
@tcitworld tcitworld added this to the Nextcloud 28 milestone Oct 19, 2023
@tcitworld tcitworld force-pushed the feat/expose-nc-groups-to-system-addressbook-contacts branch 2 times, most recently from 3ff1d95 to bd16299 Compare October 20, 2023 08:34
@tcitworld tcitworld marked this pull request as ready for review October 20, 2023 08:34
@tcitworld tcitworld added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 20, 2023
apps/dav/lib/Listener/GroupChangeListener.php Fixed Show fixed Hide fixed
}

public function handle(Event $event): void {
if (!($event instanceof UserUpdatedEvent || $event instanceof UserAddedEvent || $event instanceof UserRemovedEvent)) {

Check notice

Code scanning / Psalm

DocblockTypeContradiction Note

Docblock-defined type OCP\Group\Events\UserRemovedEvent for $event is always OCP\Group\Events\UserRemovedEvent
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.

Data exposure of groups others are members of needs to be discussed/approved first: #38432

…ntacts

And move event listeners related to SyncService to proper ones
Note: Changes to user system addressbook cards are now always done in a
QueuedJob

Closes #38432

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld tcitworld force-pushed the feat/expose-nc-groups-to-system-addressbook-contacts branch from ef01811 to 7d6fdda Compare October 27, 2023 07:23
$groupsToInclude = explode(',', $this->config->getAppValue(Application::APP_ID, 'system_addressbook_groups_to_include', ''));
$groupsToIgnore = explode(',', $this->config->getAppValue(Application::APP_ID, 'system_addressbook_groups_to_ignore', ''));
$groupNames = array_reduce($this->groupManager->getUserGroups($user), function ($groupNames, IGroup $group) use ($groupsToInclude, $groupsToIgnore) {
if (!in_array($group->getGID(), $groupsToIgnore, true) && (empty($groupsToInclude) || in_array($group->getGID(), $groupsToInclude, true))) {

Check notice

Code scanning / Psalm

DocblockTypeContradiction Note

Docblock-defined type non-empty-list for $groupsToInclude is never falsy
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@blizzz blizzz mentioned this pull request Nov 6, 2023
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.

👍 if this is an opt-in feature

Code looks clean ✨

if ($this->config->getAppValue(Application::APP_ID, 'system_addressbook_expose_groups', 'no') === 'yes') {
$groupsToInclude = explode(',', $this->config->getAppValue(Application::APP_ID, 'system_addressbook_groups_to_include', ''));
$groupsToIgnore = explode(',', $this->config->getAppValue(Application::APP_ID, 'system_addressbook_groups_to_ignore', ''));
$groupNames = array_reduce($this->groupManager->getUserGroups($user), function ($groupNames, IGroup $group) use ($groupsToInclude, $groupsToIgnore) {
Copy link
Member

Choose a reason for hiding this comment

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

I might be wrong but this could be a use case for https://www.php.net/manual/en/function.array-intersect.php

declare(strict_types=1);

/**
* @copyright 2022 Thomas Citharel <nextcloud@tcit.fr>
Copy link
Member

Choose a reason for hiding this comment

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

if you wait a little longer you can skip setting your IDE to 2023

This was referenced Nov 10, 2023
@blizzz blizzz mentioned this pull request Nov 20, 2023
5 tasks
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv
Copy link
Member

Conflicts :(

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 24, 2024
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write Nextcloud groups as CATEGORIES of system address book contacts
4 participants