fix(mgmt): add failoncontrol support for update email and phone Wait, l - #360
Conversation
The backend silently merges and deletes the conflicting user when failOnConflict is omitted, matching the option already exposed by the Node, Python, and Go SDKs. New overloads preserve backward compatibility for existing 3-arg callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersReview assignment strategy: Selected 4 reviewers with comprehensive coverage of all changed files and strong domain expertise. slavikm (contributor 0) is the primary reviewer with 12 commits across all three files, making them the most qualified to ensure consistency with existing patterns. Bars92 (contributor 1) provides secondary validation across all files with 3 commits. dorsha (contributor 2) covers interface and implementation expertise. talberry (contributor 4) ensures test coverage quality and implementation consistency. This combination balances depth of expertise with breadth of file coverage.
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. |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds failOnConflict overloads for updateEmail/updatePhone in UserService/UserServiceImpl, delegating the existing 3-arg methods to the new 4-arg ones (matches the generateEmbeddedLink overload convention already in the file).
No issues found — the delegation preserves backward compatibility, addIfNotNull is used consistently with sibling optional fields, and the new tests correctly capture and assert the request map. Good bones! 🐾
Declared coverage: FULL — 3/3 changed files reviewed.
Fixes descope/etc#17716
View Shuni run
Fixed. Summary:
failOnConflictoverloads toUserService.updateEmail/updatePhone(interface) and their implementations inUserServiceImpl.java, following the existing overload-delegation convention already used in this file (e.g.generateEmbeddedLink). The 3-arg methods delegate to the new 4-arg ones passingnull, so existing callers are unaffected; when explicitly set,failOnConflictis added to the REST request body viaaddIfNotNull.testUpdateEmailWithFailOnConflictForSuccess,testUpdatePhoneWithFailOnConflictForSuccess) that capture the outgoing request map and assertfailOnConflictis sent correctly.mvn checkstyle:check,mvn test(allUserServiceImplTestunit tests pass; the 10 pre-existingtestFunctional*failures are unrelated — they require liveDESCOPE_PROJECT_ID/DESCOPE_MANAGEMENT_KEYcredentials), andmvn package.24590fd.Created by Shuni 🐕