feat: [PHP] Add created/modified time search params to searchAll() - #133
Conversation
#9538 What changed: - Added fromCreatedTime, toCreatedTime, fromModifiedTime, toModifiedTime optional parameters to searchAll() and searchAllTestUsers() in src/SDK/Management/User.php, following the existing positional-arg / array_filter pattern used for the other optional search params in this file. - Added PHPDoc @PARAM entries for all 4 new params on both methods. - Updated the README "Search All Users" example to show the new trailing params. Verified: - Syntax-verified with `php -l src/SDK/Management/User.php` (no syntax errors detected), using PHP 8.5.9 installed via Homebrew for this session. Not verified: - No functional/integration testing was performed. This SDK's existing UserTest.php tests are live integration tests that skip without real DESCOPE_PROJECT_ID / DESCOPE_MANAGEMENT_KEY credentials, so the new params were not exercised against a running Descope environment.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe review strategy involves selecting contributors with broad file coverage and specific expertise. gaokevin1 offers deep overall experience, omercnet provides familiarity with the core implementation, and talberry brings focused knowledge on user management.
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Adds four optional trailing time-range params (from/toCreatedTime, from/toModifiedTime) to searchAll() and searchAllTestUsers(), plus a README example update.
Sniffed out 1 issue:
- 1 🟢 LOW: new time params skip the type normalization every sibling body field gets
Good bones — the params are added symmetrically to both methods, the README arg order matches the signature exactly, array_filter correctly drops null while keeping 0/false, and the declarations stay PHP 7.3-safe (no trailing comma). Woof!
Declared coverage: FULL — 2/2 changed files reviewed.
Co-authored-by: shuni-bot[bot] <251468265+shuni-bot[bot]@users.noreply.github.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review of the new push (42d8d7f), which applied the int-coercion fix to the time params in searchAll().
Sniffed out 1 issue:
- 1 🟢 LOW: coercion landed in
searchAll()only —searchAllTestUsers()still passes the four time params through raw
Half the bone is buried! Same one-line-per-field fix on the sibling method and this is clean. Woof!
Declared coverage: PARTIAL — 1/2 changed files reviewed.
Co-authored-by: shuni-bot[bot] <251468265+shuni-bot[bot]@users.noreply.github.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review of the new push (f02a3f0 + merge from main). The only in-scope change since last time applies the int coercion to searchAllTestUsers(), so both search methods now handle the four time params identically.
No new issues. My earlier finding is fixed, and nothing new surfaced in the delta.
Clean bill of health — good boy! Woof! 🦴
Declared coverage: PARTIAL — 1/2 changed files reviewed.
dishanthirpara-maker
changed the title
#9538 What changed: - Corrected $fromCreatedTime/$fromModifiedTime PHPDoc in both searchAll() and searchAllTestUsers(): "on or after" -> "after", to match confirmed backend behavior (SQL `>`, exclusive lower bound). - $toCreatedTime/$toModifiedTime already said "on or before" correctly (SQL `<=`, inclusive upper bound) - left unchanged. Verified: - Backend behavior confirmed directly in source across 3 repos in an earlier research pass: managementservice/internal/services/user.go (only the to* variants set Negative: true), common's search domain (negative flag selects Operator vs NegativeOperator), and userservice/internal/entities/search.go (Operator: " > ", NegativeOperator: " <= " for both createdtime and modifiedtime). - `php -l` reports no syntax errors. - Comment-only change; no functional code touched. Not verified: - No integration/functional testing against a live Descope API was performed; this is a documentation wording fix only.
…rams' into feature/9538-search-user-time-params
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review of the new push (5efc4f4). The only in-scope change is PHPDoc wording: the two from* params are now documented as exclusive ("after") while the to* params stay inclusive ("on or before") — applied symmetrically to both methods. No code changed; the int coercion is intact in both bodies.
No new issues. All earlier findings remain fixed.
Just a wording brush-up. Still a good boy! Woof! 🦴
Declared coverage: PARTIAL — 1/2 changed files reviewed.
There was a problem hiding this comment.
I saw your comments that
The new time-filter params themselves were never exercised, since the only test file that covers them (UserTest.php) skips entirely without live credentials. This is a pre-existing gap in this SDK's test coverage, not introduced by this change.
and
Recommend someone with live Descope credentials runs UserTest.php against a real environment before/at merge, to get actual functional coverage on these new params.
I think you should be able to write a test using whatever Mock mechanism is already set up for this repo. Something that asserts the new fields exist in the JSON body or similar.
Maybe also update UserTest to have a new test that calls searchAll with the right set of parameters to test this.
Also, you'll likely need to test this with a sample app (or make your own) and use credentials for your own Descope project to try it with.
#9538 Adds a mock-based test (src/tests/Management/UserSearchMockTest.php) covering searchAll() and searchAllTestUsers() with the 4 time-filter params. Uses Guzzle's MockHandler to verify the exact request body without needing live credentials. Registered in phpunit.xml so it runs in CI. Verified: composer test passes, 96 tests / 425 assertions, 0 failures.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review of the new push (2c94f1f), which adds UserSearchMockTest.php — a Guzzle-mocked regression test that injects a fake HTTP client via the SDK's documented httpClient constructor option and asserts the four time-filter params round-trip correctly into the outgoing JSON body for both searchAll() and searchAllTestUsers().
No new issues. Both prior 🟢 LOW findings (missing/inconsistent int coercion) remain resolved — verified the fix is still present in both methods. This new test closes exactly the credential-gated coverage gap flagged in the PR description; positional args line up correctly against both signatures and assertSame type/value checks are sound.
Good dog, chasing down that missing test coverage! Woof! 🦴
Declared coverage: PARTIAL — 2/4 changed files reviewed.
|
Added both a credential-free mock test (UserSearchMockTest.php, using Guzzle's MockHandler) and live-project verification against a real sandbox , confirmed the filter correctly includes/excludes a user as the time threshold crosses their creation time. See the updated Verification section above. |
|
@shuni can you do one more review before I approve this? |
|
👋 Hey @gaokevin1! Before I can help you, you'll need to sign in to Shuni. This creates your account and lets me work on your behalf. Once you sign in, I'll automatically get to work — no need to mention me again! 🐕 |
3 participants
Fixes descope/etc#9538
What
Adds time-based filtering to the PHP SDK's user search API, closing the last gap identified in #9538 for this SDK.
The user search API now includes:
fromCreatedTime/toCreatedTime— filter users by creation time range (Unix epoch milliseconds)fromModifiedTime/toModifiedTime— filter users by last-modified time range (Unix epoch milliseconds)Implementation Details
fromCreatedTime,toCreatedTime,fromModifiedTime,toModifiedTimeas new optional trailing parameters tosearchAll()andsearchAllTestUsers()insrc/SDK/Management/User.php, following the samearray_filter-based conditional body-building pattern already used for the other optional search params in this file.@paramentries describing all 4 new params on both methods.Verification
Verified that:
php -l src/SDK/Management/User.phpreports no syntax errors (checked with PHP 8.5.9, installed via Homebrew specifically for this check).UserTest.phpis not part of the configured suite (phpunit.xmlexplicitly lists 10 test files, none of which isUserTest.php):composer test/phpunit.xml): 94 tests, 415 assertions — all ran and passed, 0 failures, 0 skipped.UserTest.php(the file containing the actualsearchAll()/searchAllTestUsers()cases), run directly: 38 tests, 0 assertions, all 38 skipped — these are live-integration tests that require realDESCOPE_PROJECT_ID/DESCOPE_MANAGEMENT_KEYcredentials, which aren't available in this environment.Notes
searchAll()/searchAllTestUsers(); no other functionality inUser.phpwas touched.