feat: [Ruby] Add created/modified time search params to search_all_users() - #227
feat: [Ruby] Add created/modified time search params to search_all_users()#227dishanthirpara-maker wants to merge 6 commits into
Conversation
#9538 What changed: - Added from_created_time, to_created_time, from_modified_time, to_modified_time optional keyword params to search_all_users() and search_all_test_users() in lib/descope/api/v1/management/user.rb, following the existing `body[:key] = value unless value.nil?` conditional-body pattern used for the other optional params in these methods. - Added YARD @PARAM docs for all 4 new params on both methods. - Added new RSpec test cases ("is expected to include time-range filters when provided") to both the .search_users and .search_all_test_users contexts in spec/lib.descope/api/v1/management/user_spec.rb, mocking `post` and asserting the exact request body, matching the existing test style. - Updated the README "Search all users" example comment to mention the new time-filter params. Verified: - Syntax-verified with `ruby -c`. The system Ruby here is 2.6.10, but this codebase requires Ruby 3.3+ and uses 3.1+ shorthand hash syntax (`key:,`) throughout the file, which 2.6 can't parse. To get a real signal, both changed files were copied to scratch, had only that pre-existing shorthand syntax mechanically rewritten to explicit `key: key` form, and re-checked: both reported "Syntax OK". No changes were made to the actual repo files during this process. Not verified: - The new RSpec tests were not actually run (no Ruby 3.3+ runtime available in this session), so they are unexecuted since being written. No integration/functional testing against a live API was performed either.
#9538 What changed: - Fixed the "is expected to include time-range filters when provided" test under .search_users in spec/lib.descope/api/v1/management/user_spec.rb: the expected request body was missing text: nil. search_all_users() always includes a text: key in its base body hash (even when the text: param is nil), unlike some other optional fields that are conditionally added — the test fixture didn't account for that. Verified: - Installed Ruby 4.0.6 via Homebrew (system Ruby was 2.6.10, too old for this repo's Ruby 3.3+ requirement) and ran `bundle exec rspec spec/lib.descope/api/v1/management/user_spec.rb` for real. This is what caught the bug: before this, the spec file had only been syntax-checked (`ruby -c`), never executed. - Full suite now passes: 46 examples, 0 failures, including both new time-range filter tests (.search_users and .search_all_test_users). Not verified: - No integration/functional testing against a live Descope API was performed; this is unit-level coverage with a mocked `post` call only.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe review assignment balances coverage across implementation, testing, and documentation, with a focus on those most familiar with core user management features.
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 epoch-ms time-range filters to search_all_users() / search_all_test_users(), following the existing conditional-body pattern, plus unit tests and a README comment.
Sniffed out 1 issue:
- 1 🟡 MEDIUM: new fields go to the
/v1search path while an unused/v2constant exists — worth confirming the endpoint accepts them
Good bones otherwise — I traced both request bodies key-by-key and the two new spec expectations match exactly what the methods build (including the always-present text: nil). Woof!
Declared coverage: FULL — 3/3 changed files reviewed.
dishanthirpara-maker
changed the title
#9538 What changed: - Corrected from_created_time/from_modified_time YARD docs in both search_all_users() and search_all_test_users(): "on or after" -> "after", to match confirmed backend behavior (SQL `>`, exclusive lower bound). - to_created_time/to_modified_time 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). - 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 after sync. The only change since my last pass is YARD wording in user.rb — from_created_time / from_modified_time now read "after this time" instead of "on or after", while the to_* params stay "on or before". No code, test, or README changes.
No new issues. Nothing to fetch here — docs-only delta, and the request-body logic I verified previously is untouched.
One carry-over from my earlier review (no thread to reply to, so noting it here): search_all_users still posts to /v1/mgmt/user/search while the unused USER_SEARCH_PATH = "/v2/mgmt/user/search" sits in common.rb:41. Worth one live call to confirm v1 honors the new time fields rather than dropping them. Good bones! Woof!
Declared coverage: PARTIAL — 1/3 changed files reviewed.
joshwertheim
left a comment
joshwertheim
left a comment
There was a problem hiding this comment.
Everything else looks pretty good but I'd definitely recommend trying it out if you haven't first, and then you can say you verified it 👍
|
Tested this against a live project using the sample app in this repo, just pointed it at my local branch temporarily. Confirmed it correctly picks up recent users and correctly excludes everyone when I set the bound way in the future. Reverted the sample app changes after, so none of that's part of this PR. |
3 participants
Fixes descope/etc#9538
What
Adds time-based filtering to the Ruby SDK's user search API. This closes the last gap identified in #9538 for this SDK.
The user search API now supports:
from_created_time/to_created_time: filter users by when they were created (Unix epoch milliseconds)from_modified_time/to_modified_time: filter users by when they were last modified (Unix epoch milliseconds)Note on how the bounds work:
from_created_timeandfrom_modified_timeare exclusive (strictly after the given time), whileto_created_timeandto_modified_timeare inclusive (on or before the given time). This matches how the backend actually compares these values.Implementation Details
from_created_time,to_created_time,from_modified_time, andto_modified_timeas new optional keyword arguments tosearch_all_users()andsearch_all_test_users()inlib/descope/api/v1/management/user.rb, following the same pattern already used for the other optional params in these methods.@paramdocs for all 4 new params on both methods.from_created_timeandfrom_modified_timefrom "on or after" to "after", after tracing the actual backend comparison logic in managementservice.spec/lib.descope/api/v1/management/user_spec.rb.Verification
examples/ruby/management/user_app.rb), temporarily pointed at this local branch. Confirmed thatfrom_created_timecorrectly includes users created within the window, and correctly excludes everyone when the bound is set to an impossible future date. The temporary changes were reverted afterward and aren't part of this PR.Notes
search_all_users()andsearch_all_test_users(). No other functionality inuser.rbwas touched.