[Customer Portal] Implement User Phone Number Update Functionality - #69
Conversation
Rashmika998
added
Type/Task
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds profile-update capability: new constants and phone regex, authorization now maps JWT Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Service as Service
participant Auth as Authorization
participant SCIM as SCIM Module
participant API as SCIM API
Client->>Service: PATCH /users/me (UserUpdatePayload)
Service->>Auth: extract user from header
Auth-->>Service: UserDataPayload (includes userId)
alt phoneNumber provided
Service->>SCIM: updateUser(payload, email, uuid)
SCIM->>SCIM: determine organization from email
SCIM->>API: PATCH /organizations/[org]/users/[uuid]
alt API success
API-->>SCIM: updated User
SCIM-->>Service: User
Service->>Service: processPhoneNumber(scim:User)
Service-->>Client: 200 UpdatedUser
else API 400
API-->>SCIM: 400 error
SCIM-->>Service: error
Service-->>Client: 400 BadRequest
else API other error
API-->>SCIM: error
SCIM-->>Service: error
Service-->>Client: 500 InternalServerError
end
else timeZone provided
Service->>Service: (TODO) timezone update logic
Service-->>Client: 200 UpdatedUser
else
Service-->>Client: 400 BadRequest
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/customer-portal/backend/service.bal`:
- Line 164: The current statement `return http:BAD_REQUEST;` returns a status
constant instead of an HTTP response record; replace it with a proper response
object consistent with other error returns (e.g., return { status:
http:BAD_REQUEST, body: { error: "<brief message>" } };) so callers receive a
full http:Response-like record; update the code in the same function where
`http:BAD_REQUEST` is used and match the existing error-response shape used
elsewhere in the file.
- Around line 137-158: After a successful scim:updateUser call in the PATCH
handler (the block that checks payload.phoneNumber, calls scim:updateUser and
returns processPhoneNumber(updatedUser)), invalidate the cached "GET users/me"
entry for this user before returning; call a cache invalidation function (e.g.,
invalidateUserCache(userInfo.userId) or cache:remove(userInfo.userId)) right
after the update succeeds and before the final return, or implement such a
function if missing so subsequent GET users/me requests won't return stale data.
🧹 Nitpick comments (5)
apps/customer-portal/backend/modules/scim/types.bal (1)
60-72: Verify SCIM update payload shape for phone numbers.
UserUpdatePayloadmodelsphoneNumberwith a nestedmobilefield, while SCIM typically usesphoneNumbersarrays of{type, value}. Please confirm the SCIM operations service accepts this custom shape; otherwise map to the SCIM schema before PATCHing.♻️ If SCIM expects the standard `phoneNumbers` schema
public type UserUpdatePayload record {| # Password string password?; # Phone numbers - Phone phoneNumber?; + PhoneNumber[]? phoneNumbers?; |}; - -# Phone number. -public type Phone record {| - # Mobile number - string mobile?; -|};apps/customer-portal/backend/constants.bal (1)
19-20: Confirm the phone regex matches the intended format.
The pattern currently enforces a leading+with 10–14 digits. If full E.164 coverage is expected (up to 15 digits), consider widening the range.🔧 Possible adjustment for E.164 max length
-public const PHONE_PATTERN_STRING = "^\\+\\d{10,14}$"; +public const PHONE_PATTERN_STRING = "^\\+\\d{10,15}$";Also applies to: 27-28
apps/customer-portal/backend/types.bal (2)
106-118: Consider adding timezone validation.The
phoneNumberfield has proper E.164 format validation, which is good. However, thetimezonefield accepts any string without validation. Invalid timezone values could cause issues downstream when processing user preferences.Consider adding a constraint to validate against known timezone identifiers (e.g., IANA timezone database format like "America/New_York").
120-124: Response type may need to include timezone for consistency.The
UpdatedUsertype only containsphoneNumber, butUserUpdatePayloadalso acceptstimezone. Once the timezone update functionality is implemented (currently a TODO in service.bal), the response type should be extended to include the updated timezone as well.apps/customer-portal/backend/service.bal (1)
160-162: TODO implementation needs a return statement to avoid falling through toBAD_REQUEST.When the timezone update logic is implemented, ensure it returns an appropriate response. Currently, even after adding timezone update code here, the function would fall through to return
BAD_REQUESTon line 164 unless areturnstatement is added.Additionally, consider handling the case where both
phoneNumberandtimezoneare provided in a single request.Would you like me to help design the control flow for handling both fields together, or open an issue to track the timezone implementation?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/customer-portal/backend/service.bal`:
- Around line 170-172: The payload.timezone branch is unimplemented causing
valid requests to fall through; implement the timezone update by validating
payload.timezone (ensure non-empty and matches allowed timezone format/list),
then call the existing user update flow (e.g., invoke the repository/DAO method
such as Users.updateTimezone or the service function that updates user fields)
to persist the timezone for the target userId, handle and log DB errors, and
return the appropriate success response; if no update helper exists, add a small
repository method to update only the timezone and wire it into the handler that
checks payload.timezone so the request does not return BAD_REQUEST when timezone
is provided.
- Around line 147-168: The early return after handling payload.phoneNumber
causes payload.timezone to be ignored; modify the logic in the block that
references payload.phoneNumber and payload.timezone so both fields are processed
before returning: if both are present, either call scim:updateUser once with a
combined update object (e.g., include phoneNumber and timezone) or perform two
updates sequentially (first phone via scim:updateUser as currently done and then
timezone with scim:updateUser using userInfo.userId), propagate and handle
errors using the existing getStatusCode/getErrorMessage flow, and finally return
a combined response that includes both processPhoneNumber(updatedUser) and the
updated timezone value instead of returning immediately after the phone update.
cc303bb to
62bcc31
Compare
Rashmika998
changed the title
b053980 to
70899e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/customer-portal/backend/modules/scim/types.bal`:
- Around line 60-66: The SCIM UserUpdatePayload type contains an unused password
field; remove the password?: string; property from the public type
UserUpdatePayload in types.bal so the SCIM payload matches the actual PATCH
handling (which only uses Phone phoneNumber and timezone from the backend
payload) and update any related documentation/comments referencing
UserUpdatePayload/password to avoid stale API contract descriptions.
In `@apps/customer-portal/backend/types.bal`:
- Around line 106-126: The types UserUpdatePayload and UpdatedUser have
inconsistent field names: UserUpdatePayload declares timezone while UpdatedUser
declares timeZone; align them to a single naming convention (prefer camelCase:
timeZone) by renaming the timezone property in UserUpdatePayload to timeZone and
updating any references/validation (including the `@constraint` on phoneNumber) so
both record types use the identical field name (timeZone) and consumers map
consistently.
fff8342 to
a2aa1ae
Compare
e8c5f02
into
wso2-open-operations:customer-portal-milestone-1
github-project-automation
Bot
moved this from In Progress
to Done
in Customer Portal Development
2 participants
Description
This PR introduces support for updating user phone numbers through the SCIM module, enabling standardized and secure profile updates.
The implementation follows SCIM specifications and ensures that phone number changes are properly validated, processed, and synchronized with the identity provider.
Changes
Example Endpoint
Both fields are optional,
Related Issues
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.