docs: [Node] Document created/modified time search params on search() - #786
docs: [Node] Document created/modified time search params on search()#786dishanthirpara-maker wants to merge 4 commits into
Conversation
#9538 What changed: - Added a JSDoc block above the search() function in lib/management/user.ts documenting fromCreatedTime, toCreatedTime, fromModifiedTime, and toModifiedTime, matching the JSDoc style already used on the deprecated searchAll() function above it. - Updated the README "Search all users" example to show fromCreatedTime/toModifiedTime usage. - No functional/type code was changed: these 4 fields already existed on the SearchRequest type (with only inline `//` comments) and were already wired through to the request body via the `...searchReq` spread in search() — they just lacked a proper JSDoc block on the function itself. - Per explicit instruction, the deprecated searchAll() function (which does not support these params) was left untouched. Verified: - Manual diff review only: the change is a JSDoc comment block plus a README edit, added directly above an existing, unmodified arrow function — no syntax risk to the surrounding code. Not verified: - No TypeScript compile/typecheck was run (`tsc --noEmit` or similar) because node_modules were not installed in this session and doing so was out of scope for this check. No integration/functional testing against a live API was performed.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe review assignment considers breadth of file coverage and expertise, selecting contributors who have worked on both the core logic and documentation to ensure high-quality review.
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
Docs-only PR: adds a JSDoc block for user.search() covering the four time-range filters and updates the README example. No functional code touched — the fields were already on SearchRequest and already forwarded via the ...searchReq spread, and user.test.ts:1300-1320 already asserts they reach the request body.
Sniffed out 1 issue:
- 1 🟡 MEDIUM: new docs assert inclusive time bounds, contradicting the existing field comments in the same file
Good bones otherwise — JSDoc placement and style match the neighbouring searchAll() block. Woof!
Declared coverage: FULL — 2/2 changed files reviewed.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-sniffed after the sync: the only new commit is the main merge (removeRecoveryCodes + its README section came in from upstream, not from this PR). The PR's own diff is byte-identical to the last review — 14 added lines of JSDoc and README docs, no logic touched.
Sniffed out 1 issue (carried over, still unaddressed):
- 1 🟡 MEDIUM: docs assert inclusive time bounds, contradicting the
SearchRequestfield comments in the same file
Same bone, still buried. Woof!
Declared coverage: FULL — 2/2 changed files reviewed.
| * @param searchReq.fromCreatedTime only include users created on or after this time (epoch in milliseconds) | ||
| * @param searchReq.toCreatedTime only include users created on or before this time (epoch in milliseconds) | ||
| * @param searchReq.fromModifiedTime only include users modified on or after this time (epoch in milliseconds) | ||
| * @param searchReq.toModifiedTime only include users modified on or before this time (epoch in milliseconds) |
There was a problem hiding this comment.
🟡 MEDIUM — These docs claim inclusive bounds ("on or after"/"on or before"), but the field comments at lib/management/user.ts:60-63 say "after"/"before" (exclusive). The same inclusive wording is repeated in README.md:921-922.
Since documenting these params is this PR's whole purpose, the boundary semantics need to be right: anyone chunking a time window (feeding the previous page's max created time into the next fromCreatedTime) will double-count or skip users at the boundary depending on which is true.
Fix: confirm the server-side comparison, then use one consistent phrasing across the JSDoc, the type comments, and the README.
There was a problem hiding this comment.
✅ Fixed in ebc4cc4. All three locations now agree on the same asymmetric semantics — from* exclusive ("after"), to* inclusive ("on or before"):
- JSDoc
lib/management/user.ts:681-684 SearchRequestfield commentslib/management/user.ts:60-63- README example
README.md:921-922
Resolving.
dishanthirpara-maker
changed the title
#9538
What changed:
- Corrected the inclusive/exclusive boundary wording on all 4
time-filter params (fromCreatedTime, toCreatedTime, fromModifiedTime,
toModifiedTime) in lib/management/user.ts and README.md, based on the
actual backend comparison operators (traced through
managementservice -> common -> userservice):
- fromCreatedTime / fromModifiedTime: backend uses SQL `>` (exclusive)
-> wording is "after", not "on or after"
- toCreatedTime / toModifiedTime: backend uses SQL `<=` (inclusive)
-> wording is "on or before", not "before"
- Fixed the JSDoc @PARAM lines on search() (added earlier in this PR)
for fromCreatedTime/fromModifiedTime: "on or after" -> "after".
- Fixed the pre-existing SearchRequest type inline comments for
toCreatedTime/toModifiedTime, which predate this PR and were already
wrong before it: "before" -> "on or before".
- Fixed the README example comment for fromCreatedTime to match.
Verified:
- Backend behavior confirmed directly in source across 3 repos, not
assumed: managementservice/internal/services/user.go (builds the
SearchField, only Negative is set on the to* variants), common's
search domain (negative flag selects Operator vs NegativeOperator),
and userservice/internal/entities/search.go (the actual operator
strings: Operator: " > ", NegativeOperator: " <= " for both
createdtime and modifiedtime).
Not verified:
- No integration/functional testing against a live Descope API was
performed; this is a documentation/comment wording fix only, no
functional code was touched.
…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
Docs-only PR: JSDoc for the four time filters on user.search(), plus a README example. ebc4cc4 fixed my earlier finding — JSDoc, the SearchRequest field comments, and the README now all agree on from* exclusive / to* inclusive. No logic touched, wire format unchanged.
Sniffed out 0 issues — nothing survived verification.
Note: I could not mark the earlier thread resolved (the resolve call returned permission denied), so it stays open on GitHub despite being fixed in code. Clean bill of health — good bones! Woof!
⚠️ Review follow-up issue: Request failed due to following response errors:
- Resource not accessible by integration Unresolved prior review threads: PRRT_kwDOHT4Hw86YT86U.
Declared coverage: FULL — 2/2 changed files reviewed.
1 participant
Fixes descope/etc#9538
What
Documents the time-based user search filters on the Node SDK's
search()function, closing the last gap identified in #9538 for this SDK — the fields already existed and worked, they just weren't documented with proper JSDoc.The
search()function's documentation now includes:fromCreatedTime/toCreatedTime— filter users by creation time range (epoch milliseconds)fromModifiedTime/toModifiedTime— filter users by last-modified time range (epoch milliseconds)Implementation Details
search()function inlib/management/user.tsdocumenting all 4 time-filter fields, matching the JSDoc style already used on the deprecatedsearchAll()function above it.fromCreatedTime/toModifiedTimeusage.SearchRequesttype (with only inline//comments) and were already wired through to the request body via the...searchReqspread insearch().searchAll()function (which does not support these params) was intentionally left untouched.Verification
Verified that:
npm install(1008 packages) and ran a real TypeScript typecheck:npx tsc --noEmit -p tsconfig.json→ "No errors found." This confirms the new JSDoc block and its@param searchReq.xxxtags don't break compilation and are syntactically valid against the project's actualtsconfig.json.Not verified:
Notes
searchAll()function.