Skip to content

Fix: ContactManager search with fullmatch - #38907

Merged
JohannesGGE merged 1 commit into
masterfrom
mail/fix/8551/contact-manager-search
Jul 18, 2023
Merged

Fix: ContactManager search with fullmatch#38907
JohannesGGE merged 1 commit into
masterfrom
mail/fix/8551/contact-manager-search

Conversation

@JohannesGGE

@JohannesGGEJohannesGGE commented Jun 20, 2023

Copy link
Copy Markdown
Contributor

Summary

If I understand the sharing options in the admin settings right, the following should lead to the suggestion of a SAB user in the same group, and don't show other SAB users even if there is a full match (name, email).

image

Currently, because of the 'fullmatch' option, I pass as false in mail, the if condition is triggered and no SAB user is suggested at all. To work correctly I think there should be the check for the access to the SAB as well.

Maybe I am wrong here, correct me if that's the case :)

Checklist

@JohannesGGEJohannesGGE self-assigned this Jun 20, 2023
@JohannesGGEJohannesGGE added bug 3. to review Waiting for reviews labels Jun 20, 2023
@szaimenszaimen added this to the Nextcloud 28 milestone Jun 20, 2023

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That looks sane

Signed-off-by: Johannes Merkel <mail@johannesgge.de>
@JohannesGGE
JohannesGGEforce-pushed the mail/fix/8551/contact-manager-search branch from fffed18 to 846754eCompareJune 29, 2023 14:52

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works as expected in combination with nextcloud/mail#8551

@ChristophWurst

Copy link
Copy Markdown
Member

/backport to stable27

@ChristophWurst

Copy link
Copy Markdown
Member

/backport to stable26

@ChristophWurst

Copy link
Copy Markdown
Member

/backport to stable25


if (!$enumeration && !$fullMatch) {
// No access to system address book AND no full match allowed
continue;

@ChristophWurstChristophWurstJun 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if this PR could have any unexpected side effects. I don't think it does but let's double-check.

It would be interesting to set a breakpoint here and test if any other features that use contacts (file sharing, contacts menu) behave different with this change.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Did some testing:

  • file sharing and calender sharing work as before (seems they got the right result already from the UserPlugin)
  • contact menu is working a bit better than before (change has effect and show same group users on group:true and fullmatch:false) but \OC\Contacts\ContactsMenu\ContactsStore::getContacts is doing some filtering in the end so (fullmatch:true, group:true) doesn't show full matches.

\OC\ContactsManager::search seems to work properly :D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then let's :shipit:

@AndyXheli

Copy link
Copy Markdown
Contributor

Looks like this is also fixed #34637

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsbugfeature: carddavRelated to CardDAV internalsfeature: users and groupsintegration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@JohannesGGE@ChristophWurst@AndyXheli@miaulalala@szaimen