[Customer Portal][BE] Update users/me endpoint to fetch the last password update time - #375
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded SCIM schema attribute handling and propagated lastPasswordUpdateTime from SCIM into the user model and /users/me response via a new utility and type updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant SCIM_Client
participant TypesModule
Client->>Service: GET /users/me
Service->>SCIM_Client: searchUsers(attributes: phoneNumbers, username, urn:scim:wso2:schema)
SCIM_Client-->>Service: userResults (includes schema scope)
Service->>SCIM_Client: (or directly) scim.processLastPasswordUpdateTime(user)
SCIM_Client-->>Service: lastPasswordUpdateTime?
Service->>TypesModule: build types:User (phoneNumber, lastPasswordUpdateTime)
Service-->>Client: 200 OK with types:User
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/customer-portal/backend/service.bal (1)
138-160:⚠️ Potential issue | 🟠 MajorAvoid caching partial
/users/medata when SCIM lookup fails.If SCIM lookup fails or returns empty,
phoneNumberandlastPasswordUpdateTimestay unset, but Line 163 still caches that partial object for up to the cache TTL. A transient SCIM outage can therefore hide recovered values for too long.🛠️ Proposed fix
string? phoneNumber = (); string? lastPasswordUpdateTime = (); + boolean shouldCacheUser = true; scim:User[]|error userResults = scim:searchUsers(userInfo.email); if userResults is error { // Log the error and return nil log:printError("Error retrieving user phone number from scim service", userResults); + shouldCacheUser = false; } else { if userResults.length() == 0 { log:printError(string `No user found while searching phone number for user: ${userInfo.userId}`); + shouldCacheUser = false; } else { phoneNumber = scim:processPhoneNumber(userResults[0]); lastPasswordUpdateTime = scim:processLastPasswordUpdateTime(userResults[0]); } } @@ - error? cacheError = userCache.put(cacheKey, user); - if cacheError is error { - log:printWarn("Error writing user information to cache", cacheError); + if shouldCacheUser { + error? cacheError = userCache.put(cacheKey, user); + if cacheError is error { + log:printWarn("Error writing user information to cache", cacheError); + } } return user;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/backend/service.bal` around lines 138 - 160, The SCIM lookup can fail leaving phoneNumber and lastPasswordUpdateTime unset but the code still builds and caches the types:User user; change the flow so that scim:searchUsers + scim:processPhoneNumber + scim:processLastPasswordUpdateTime must succeed (userResults not error and userResults.length() > 0) before constructing/caching the types:User user from userDetails; if the SCIM call errors or returns empty, avoid constructing or persisting the partial user (either return/skip caching early or populate explicit nulls and refresh the cache immediately), so move the user construction inside the successful branch or add an explicit guard around caching to prevent storing incomplete /users/me data.
🧹 Nitpick comments (1)
apps/customer-portal/backend/service.bal (1)
142-146: Update SCIM error logs to reflect both fields now being fetched.Current log messages mention only phone-number lookup, which is now incomplete context during incidents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/customer-portal/backend/service.bal` around lines 142 - 146, The error/log lines that currently call log:printError with messages mentioning only "phone number" should be updated to reflect that the SCIM lookup now fetches both fields; locate the log:printError calls around userResults and userInfo.userId (in the block handling no results and error cases) and change the messages to say something like "phone number and [other SCIM field]" or "phone number and additional SCIM field(s)" so the context is accurate during incidents; ensure both the error branch (log:printError("Error retrieving ...", userResults)) and the no-results branch (log:printError("No user found while searching ...")) are updated to reference both fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/customer-portal/backend/modules/scim/utils.bal`:
- Line 42: Fix the docstring typo in the SCIM user processing comment: change
"lst" to "last" in the comment that currently reads "Process SCIM user to
extract the lst password update time." so it reads "Process SCIM user to extract
the last password update time." This update should be made near the SCIM user
password extraction helper in utils.bal to improve generated docs/readability.
---
Outside diff comments:
In `@apps/customer-portal/backend/service.bal`:
- Around line 138-160: The SCIM lookup can fail leaving phoneNumber and
lastPasswordUpdateTime unset but the code still builds and caches the types:User
user; change the flow so that scim:searchUsers + scim:processPhoneNumber +
scim:processLastPasswordUpdateTime must succeed (userResults not error and
userResults.length() > 0) before constructing/caching the types:User user from
userDetails; if the SCIM call errors or returns empty, avoid constructing or
persisting the partial user (either return/skip caching early or populate
explicit nulls and refresh the cache immediately), so move the user construction
inside the successful branch or add an explicit guard around caching to prevent
storing incomplete /users/me data.
---
Nitpick comments:
In `@apps/customer-portal/backend/service.bal`:
- Around line 142-146: The error/log lines that currently call log:printError
with messages mentioning only "phone number" should be updated to reflect that
the SCIM lookup now fetches both fields; locate the log:printError calls around
userResults and userInfo.userId (in the block handling no results and error
cases) and change the messages to say something like "phone number and [other
SCIM field]" or "phone number and additional SCIM field(s)" so the context is
accurate during incidents; ensure both the error branch (log:printError("Error
retrieving ...", userResults)) and the no-results branch (log:printError("No
user found while searching ...")) are updated to reference both fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b43ad2e-8ca4-4261-b29c-dc62c470357a
📒 Files selected for processing (6)
apps/customer-portal/backend/modules/scim/constants.balapps/customer-portal/backend/modules/scim/scim.balapps/customer-portal/backend/modules/scim/types.balapps/customer-portal/backend/modules/scim/utils.balapps/customer-portal/backend/modules/types/types.balapps/customer-portal/backend/service.bal
Description
This PR updates the
users/meendpoint to return the user's last password update time.Changes
users/meresponse model to include the last password update timestampReason
The current
users/meresponse does not expose when the user last updated their password.Including this field helps:
Testing
users/meresponse includes the last password update timeImpact
Summary by CodeRabbit