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(search): allow contacts person search #41457

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

hamza221
Copy link
Contributor

@hamza221 hamza221 commented Nov 14, 2023

Summary

TODO

  • ...

Checklist

@hamza221 hamza221 added the 3. to review Waiting for reviews label Nov 14, 2023
@hamza221 hamza221 self-assigned this Nov 14, 2023
@hamza221 hamza221 force-pushed the feat/contacts/advanced-search branch 2 times, most recently from 81de941 to 0f373c7 Compare November 14, 2023 13:14
@ChristophWurst
Copy link
Member

ChristophWurst commented Nov 14, 2023

Likely unrelated to your changes but I ran into OCA\DAV\Search\ContactsSearchProvider::getDavUrlForContact(): Argument #1 ($principalUri) must be of type string, null given. This happens when $contactRow is a system contact. $addressBooksById does not contain the SAB for me.

There must be a discrepancy between \OCA\DAV\CardDAV\CardDavBackend::getAddressBooksForUser and \OCA\DAV\CardDAV\CardDavBackend::searchPrincipalUri. The former doesn't return the SAB, the latter searches in it.

@ChristophWurst
Copy link
Member

@hamza221 how do I best test this? still the curl curl -u admin:admin -k -H "accept: application/json" "https://localhost/ocs/v2.php/search/providers/contacts/search?term=AAAA&person=nc-user_admin"?

@hamza221
Copy link
Contributor Author

@hamza221 how do I best test this? still the curl curl -u admin:admin -k -H "accept: application/json" "https://localhost/ocs/v2.php/search/providers/contacts/search?term=AAAA&person=nc-user_admin"?

yes that's how I tested

@ChristophWurst
Copy link
Member

@hamza221 set an avatar for your test user then you will run into #41457 (comment) too 🙊

@ChristophWurst
Copy link
Member

The db query is wrong. There is a big OR at the top level that circumvents the scoping to address books

SELECT
  DISTINCT `cp`.`cardid`
FROM
  `*PREFIX*cards_properties` `cp`
WHERE
  (
    (
      (`cp`.`addressbookid` = :dcValue1)
      OR (`cp`.`addressbookid` = :dcValue2)
      OR (`cp`.`addressbookid` = :dcValue3)
      OR (`cp`.`addressbookid` = :dcValue4)
      OR (`cp`.`addressbookid` = :dcValue5)
    )
    AND (`cp`.`name` IN (:dcValue6))
    AND (
      `cp`.`value` COLLATE utf8mb4_general_ci LIKE :dcValue7
    )
  )
  OR (
    `cp`.`value` COLLATE utf8mb4_general_ci LIKE :dcValue8
  )
LIMIT
  5

@@ -1182,6 +1183,13 @@ private function searchByAddressBookIds(array $addressBookIds,
$query2->setFirstResult($options['offset']);
}

if(isset($options['person'])){
if('' !== $pattern){
$query2->orWhere($query2->expr()->ilike('cp.value', $query2->createNamedParameter('%' . $this->db->escapeLikeParameter($options['person']) . '%')));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$query2->orWhere($query2->expr()->ilike('cp.value', $query2->createNamedParameter('%' . $this->db->escapeLikeParameter($options['person']) . '%')));
$query2->andWhere($query2->expr()->ilike('cp.value', $query2->createNamedParameter('%' . $this->db->escapeLikeParameter($options['person']) . '%')));

to fix #41457 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

With this change the feature works as expected for me. If there is a regular contact with the same name as the filtered user, I will see the contact. If there is no match, I see nothing 👍

@ChristophWurst ChristophWurst mentioned this pull request Nov 14, 2023
@ChristophWurst ChristophWurst force-pushed the feat/contacts/advanced-search branch from bf9afc2 to 19c21f2 Compare November 14, 2023 17:28
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.

Works 🚀

Signed-off-by: hamza221 <hamzamahjoubi221@gmail.com>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 14, 2023
@jospoortvliet
Copy link
Member

whooot somebody is going to hit that merge button!!!!

/me holds breath

@ChristophWurst
Copy link
Member

Some actions fail. But it's not the job/checks that fail but the runner.

@blizzz blizzz disabled auto-merge November 14, 2023 19:46
@blizzz blizzz merged commit 138ce53 into master Nov 14, 2023
46 of 50 checks passed
@blizzz blizzz deleted the feat/contacts/advanced-search branch November 14, 2023 19:47
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: contacts feature: search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🔎 Allow contact and manager search in advanced search
6 participants