Conversation
hamza221
requested review from
ChristophWurst,
GretaD and
kesselb
as code owners
There was a problem hiding this comment.
Pull request overview
Adds/updates unit tests around contacts integration behavior (notably after ForceSAB removal), covering both service-level contact lookup and controller endpoints including autocomplete caching.
Changes:
- Added new unit tests for
ContactsIntegration::getContactsWithNamecovering normal results, filtering, and edge cases. - Introduced a new
ContactIntegrationControllerTestcoveringmatch,addMail,newContact, andautoComplete(cache hit/miss/invalid JSON).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| tests/Unit/Service/ContactsIntegrationTest.php | Adds coverage for getContactsWithName mapping/filtering and empty/missing-field scenarios |
| tests/Unit/Controller/ContactIntegrationControllerTest.php | New controller unit tests validating JSON responses and caching behavior for autocomplete |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function testGetContactsWithName(): void { | ||
| $name = 'John'; | ||
| $searchResult = [ | ||
| [ | ||
| 'UID' => 'jd', | ||
| 'FN' => 'John Doe', | ||
| 'EMAIL' => 'john@doe.com', | ||
| ], | ||
| [ | ||
| 'UID' => 'js', | ||
| 'FN' => 'John Smith', | ||
| 'EMAIL' => ['john@smith.com', 'jsmith@example.com'], | ||
| ], | ||
| ]; | ||
|
|
||
| $this->config->expects($this->once()) | ||
| ->method('getAppValue') | ||
| ->with('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') | ||
| ->willReturn('yes'); | ||
| $this->contactsManager->expects($this->once()) | ||
| ->method('search') | ||
| ->with($name, ['FN'], [ | ||
| 'strict_search' => false, | ||
| 'limit' => 20, | ||
| ]) | ||
| ->willReturn($searchResult); |
There was a problem hiding this comment.
The mock setup for getAppValue and contactsManager->search(...) is repeated across multiple new tests (same options array, same default value parameter). Consider extracting a small helper (e.g., expectShareDialogEnumeration(string $value) and expectContactsSearch(string $term, array $result)) or using a data provider to reduce duplication and make future changes (like changing search options/limit) less error-prone.
|
|
||
| $actual = $this->contactsIntegration->getContactsWithName($name); | ||
|
|
||
| $this->assertEquals($expected, $actual); |
There was a problem hiding this comment.
These assertions compare structured arrays that include null and nested arrays. Using assertSame here (and in the other newly added tests) would make the assertions stricter and better aligned with declare(strict_types=1), catching unintended type coercions (e.g., '0' vs 0).
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
hamza221
force-pushed
the
chore/test/contact-integration
branch
from
70a89e6 to
c5259b4
Compare
#10856
ForceSABwas removedneeds #12460