Improve handling of missing primary email and mobile number#10086
Improve handling of missing primary email and mobile number#10086sadilchamishka wants to merge 3 commits intowso2:masterfrom
Conversation
…ary email or mobile number is not included in the multi valued list
📝 WalkthroughWalkthroughPatch-level changes: added a Changeset for user packages. Updated user-profile initialization to include primary email/mobile in multi-valued fields. Added Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Pull request overview
This PR improves initialization of the user profile form’s multi-valued email address and mobile number fields so that the primary email/mobile is always present in the corresponding list (when the multiple email/mobile feature is enabled), avoiding duplicate entries.
Changes:
- Prepend the primary email to
emailAddresseswhen missing (and multiple emails/mobiles is enabled). - Prepend the primary mobile number to
mobileNumberswhen missing (and multiple emails/mobiles is enabled). - Add a changeset documenting the patch-level improvement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
features/admin.users.v1/components/user-profile/user-profile-form.tsx |
Ensures primary email/mobile are included in their multi-valued lists when missing, without duplicating existing entries. |
.changeset/shaggy-wombats-whisper.md |
Records the patch change for @wso2is/admin.users.v1 and @wso2is/console. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isMultipleEmailAndMobileNumberEnabled && primaryEmail && !emailAddresses.includes(primaryEmail)) { | ||
| ( | ||
| initialValues[systemSchema] as Record<string, unknown> | ||
| )[emailAddressesField] = [ primaryEmail, ...emailAddresses ]; | ||
| } |
There was a problem hiding this comment.
This initial-values logic for email/mobile lists looks very similar to the shared implementation in @wso2is/common.users.v1/utils/profile-utils (prepareInitialValues/getFlattenedInitialValues). Since this file now has slightly different behavior (inserting the primary value even when the list is non-empty), consider centralizing the behavior in the shared utility (or aligning both) to avoid future drift and inconsistent initialization across screens that rely on the common helper.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10086 +/- ##
=======================================
Coverage 56.05% 56.05%
=======================================
Files 42 42
Lines 1024 1024
Branches 254 246 -8
=======================================
Hits 574 574
Misses 416 416
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx (1)
63-66:⚠️ Potential issue | 🔴 CriticalBreaking change:
triggerSubmitis now async but interface and callers don't await it.The
triggerSubmitfunction is async (line 103), but:
- The interface at line 64 declares
triggerSubmit: () => voidinstead of() => Promise<void>- All callers across both files invoke
triggerSubmit()without awaiting:
approval-workflow-create-page.tsx: lines 372–373, 428–429, 481–482, 534–535approval-workflow-edit.tsx: lines 316, 730, 735, 737, 739This causes race conditions where the name uniqueness check (performed by the async call) may complete after subsequent operations have started. The edit page explicitly relies on synchronous ordering with the comment "Order matters: notification must update ref before config submit reads it" (line 726).
Additionally:
- Lines 90–91 use
anytype (violates explicit type annotations rule)- Line 107 uses inline type definition instead of a named interface
Update the interface to
triggerSubmit: () => Promise<void>and await all callers, or refactor to perform the name uniqueness check on field blur/change rather than on submit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx` around lines 63 - 66, Update the GeneralApprovalWorkflowDetailsFormRef so triggerSubmit is declared as triggerSubmit: () => Promise<void>, then update every caller to await formRef.current.triggerSubmit() (ensuring the create/edit pages that rely on ordering wait for the async name-uniqueness check to complete); alternatively, move the name-uniqueness check off submit into onBlur/onChange to avoid awaiting at call sites. Also replace the use of any with an explicit type and extract the inline anonymous type used near the triggerSubmit implementation into a named interface so all types are explicit and consistent.
🧹 Nitpick comments (3)
features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx (3)
136-138: Consider logging the error for debugging purposes.While silently failing to not block submission is reasonable, logging the error would help with debugging in production without affecting UX.
Suggested enhancement
} catch (_error: unknown) { - // Silently fail — do not block submission on a lookup error. + // Silently fail — do not block submission on a lookup error. + console.warn("Workflow name uniqueness check failed:", _error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx` around lines 136 - 138, The catch block that currently swallows errors in general-approval-workflow-details-form.tsx (catch (_error: unknown) { /* Silently fail */ }) should log the caught error for observability without changing UX; replace the silent catch with a logging call (e.g., catch (error) { console.error('Lookup failed during submission in GeneralApprovalWorkflowDetailsForm:', error); } or use the project's logger if one exists) so the error is recorded but submission still proceeds.
90-91: Avoidanytype—use proper FinalForm types.The coding guidelines state to never use
any. TheformApiRefshould useFormApifromfinal-form, andcurrentValuesRefshould use the form values interface.Suggested fix
+import { FormApi } from "final-form"; + - const currentValuesRef: any = useRef(null); - const formApiRef: MutableRefObject<any> = useRef<any>(null); + const currentValuesRef: MutableRefObject<GeneralDetailsFormValuesInterface | null> = useRef<GeneralDetailsFormValuesInterface | null>(null); + const formApiRef: MutableRefObject<FormApi<GeneralDetailsFormValuesInterface> | null> = useRef<FormApi<GeneralDetailsFormValuesInterface> | null>(null);As per coding guidelines: "Never use
any; use proper types orunknownwith type guards."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx` around lines 90 - 91, The refs are typed as any; replace them with proper Final Form types: import FormApi from 'final-form' and change formApiRef to MutableRefObject<FormApi<ApprovalWorkflowFormValues> | null> (or the actual form values interface name used in this component), and set currentValuesRef to useRef<ApprovalWorkflowFormValues | null>(null) (or unknown with guarded access if you prefer); update any usages to respect the concrete types and avoid `any`.
111-119: UseWorkflowListResponseInterfaceinstead of inline type.The response is typed inline as
{ workflows?: { id: string; name: string }[] }, butWorkflowListResponseInterfaceis already imported (via the API function's return type). Using the proper interface ensures type consistency.Suggested fix
- const response: { workflows?: { id: string; name: string }[] } = + const response: WorkflowListResponseInterface = await getApprovalWorkflows(currentName);And add the import if not already available:
import { WorkflowListResponseInterface } from "../../models/approval-workflows";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx` around lines 111 - 119, Replace the inline response type with the shared interface: change the type of the variable response (from the getApprovalWorkflows call) to WorkflowListResponseInterface and ensure WorkflowListResponseInterface is imported (e.g., from "../../models/approval-workflows"); keep the rest of the logic using response?.workflows as-is so type-checking uses the canonical interface throughout (refer to the response variable and getApprovalWorkflows call to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx`:
- Around line 63-66: Update the GeneralApprovalWorkflowDetailsFormRef so
triggerSubmit is declared as triggerSubmit: () => Promise<void>, then update
every caller to await formRef.current.triggerSubmit() (ensuring the create/edit
pages that rely on ordering wait for the async name-uniqueness check to
complete); alternatively, move the name-uniqueness check off submit into
onBlur/onChange to avoid awaiting at call sites. Also replace the use of any
with an explicit type and extract the inline anonymous type used near the
triggerSubmit implementation into a named interface so all types are explicit
and consistent.
---
Nitpick comments:
In
`@features/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx`:
- Around line 136-138: The catch block that currently swallows errors in
general-approval-workflow-details-form.tsx (catch (_error: unknown) { /*
Silently fail */ }) should log the caught error for observability without
changing UX; replace the silent catch with a logging call (e.g., catch (error) {
console.error('Lookup failed during submission in
GeneralApprovalWorkflowDetailsForm:', error); } or use the project's logger if
one exists) so the error is recorded but submission still proceeds.
- Around line 90-91: The refs are typed as any; replace them with proper Final
Form types: import FormApi from 'final-form' and change formApiRef to
MutableRefObject<FormApi<ApprovalWorkflowFormValues> | null> (or the actual form
values interface name used in this component), and set currentValuesRef to
useRef<ApprovalWorkflowFormValues | null>(null) (or unknown with guarded access
if you prefer); update any usages to respect the concrete types and avoid `any`.
- Around line 111-119: Replace the inline response type with the shared
interface: change the type of the variable response (from the
getApprovalWorkflows call) to WorkflowListResponseInterface and ensure
WorkflowListResponseInterface is imported (e.g., from
"../../models/approval-workflows"); keep the rest of the logic using
response?.workflows as-is so type-checking uses the canonical interface
throughout (refer to the response variable and getApprovalWorkflows call to
locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a8a87c86-e4c1-491b-9d54-bdad7f4b8766
📒 Files selected for processing (2)
features/admin.approval-workflows.v1/api/approval-workflow.tsfeatures/admin.approval-workflows.v1/components/create/general-approval-workflow-details-form.tsx
Purpose
This pull request improves how multiple email addresses and mobile numbers are displayed and initialized in the user profile form, especially when the "multiple email and mobile number" feature is enabled. The logic now ensures that the primary email and mobile number are included in their respective lists if they're not already present, providing a more accurate and user-friendly experience.
Enhancements to user profile form initialization:
user-profile-form.tsx).user-profile-form.tsx).Documentation and changelog:
Related Issues