Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console] Add UI Support for Multiple Email Addresses and Mobile Numbers per User #6583

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

PasinduYeshan
Copy link
Contributor

@PasinduYeshan PasinduYeshan commented Jul 12, 2024

Purpose

Add UI support for multiple email addresses and mobile numbers per user feature.

Key changes

When multiple email and mobile number support is enabled:

  • The Email and Mobile fields are replaced by Email Addresses and Mobile Numbers fields.
image
  • Primary Indicators: The primary email and mobile number are marked with a star icon for easy identification.

  • Verified Indicators:

    • Verified email addresses and mobile numbers display a tick icon.
    • Verification icons are shown only if email and mobile verification features are enabled.
  • Admin Actions:

    • Admin can trigger verification for email or mobile number if the relevant mobile verification or email verifications are enabled.
    • Admin can change the primary email or mobile number, if the verification on update is enabled, that email address/ or mobile number should be verified prior to make it primary.

Related Issues

Merge after

@PasinduYeshan PasinduYeshan changed the title Add UI Support for Multiple Email Addresses and Mobile Numbers per User [Console] Add UI Support for Multiple Email Addresses and Mobile Numbers per User Jul 13, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.02%. Comparing base (f045386) to head (f3976c0).
Report is 3616 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6583      +/-   ##
==========================================
+ Coverage   31.79%   32.02%   +0.22%     
==========================================
  Files          41       41              
  Lines         890      893       +3     
  Branches      204      215      +11     
==========================================
+ Hits          283      286       +3     
+ Misses        607      557      -50     
- Partials        0       50      +50     
Flag Coverage Δ
@wso2is/core 32.02% <ø> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
modules/core/src/models/profile.ts 100.00% <ø> (ø)

... and 15 files with indirect coverage changes

features/admin.users.v1/components/user-profile.scss Outdated Show resolved Hide resolved
padding: 0px;
}

.MuiAccordionSummary-root {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to override these values? typically (if Oxygen-UI components are used) adjusting these will not be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adjustment was required coz, in the Oxygen UI, the accordion details section shifts slightly to the right. While this alignment correct, in this case, the details should be displayed without the offset.

Without css:
image

With css:
image

features/admin.users.v1/components/user-profile.tsx Outdated Show resolved Hide resolved
const domainName: string[] = profileInfo?.get("userName")?.toString().split("/");
const userStoreDomain: string = (domainName.length > 1
? domainName[0]
: PRIMARY_USERSTORE)?.toUpperCase();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this expected? Is the userstore primary in both Asgardeo and IS?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not use the constants from userstoreConfig so that it will properly resolve to primary and default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended, In asgardeo users in DEFAULT user store has user store domain as prefix to username (DEFAULT/test1@abc.com), here Im checking what is the user store domain of the user, if the user name doesnt have user store domain prefixed, then it should be "PRIMARY", in both IS and Asgardeo.

if (multiValuedAttributes.includes(schemaNames[0])) {

const attributeValue: string | string[] =
userInfo[ProfileConstants.SCIM2_WSO2_CUSTOM_SCHEMA]?.[schemaNames[0]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to PRIMARY_USERSTORE const, is the ProfileConstants.SCIM2_WSO2_CUSTOM_SCHEMA expected? This also might vary within products. If it is so, get this value from the userConfig

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check in other places as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (
        schema.extended && userInfo[ProfileConstants.SCIM2_WSO2_CUSTOM_SCHEMA]
        && userInfo[ProfileConstants.SCIM2_WSO2_CUSTOM_SCHEMA][schemaNames[0]]
    ) {
        const multiValuedAttributes: string[] = [
            EMAIL_ADDRESSES_ATTRIBUTE,
            MOBILE_NUMBERS_ATTRIBUTE,
            VERIFIED_EMAIL_ADDRESSES_ATTRIBUTE,
            VERIFIED_MOBILE_NUMBERS_ATTRIBUTE
        ];

        if (multiValuedAttributes.includes(schemaNames[0])) {

            **const attributeValue: string | string[] =
                userInfo[ProfileConstants.SCIM2_WSO2_CUSTOM_SCHEMA]?.[schemaNames[0]];**

            const formattedValue: string = Array.isArray(attributeValue)
                ? attributeValue.join(",")
                : "";

            tempProfileInfo.set(schema.name, formattedValue);

            return;
        }
        tempProfileInfo.set(
            schema.name, userInfo[ProfileConstants.SCIM2_WSO2_CUSTOM_SCHEMA][schemaNames[0]]
        );

        return;
    }

Here I have added this custom logic under userInfo[ProfileConstants.SCIM2_WSO2_CUSTOM_SCHEMA] handling, part. And these new attributes are registered under wso2 custom schema in both IS and Asgardeo.


</label>
{
showVerifiedPopup(accordionLabelValue)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to use react states to dynamically display these elements? I'm only seeing JS variables used within this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These values are derived from profileInfo which is already managed by state. When verification happens, profileInfo gets updated, causing a re-render, and these values are recomputed automatically.

.changeset/cyan-rockets-switch.md Show resolved Hide resolved
* User claim update - API Keyword constants.
*/
public static readonly ENABLE_MOBILE_VERIFICATION: string = "UserClaimUpdate.MobileNumber.EnableVerification";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

features/admin.users.v1/components/user-profile.tsx Outdated Show resolved Hide resolved
modules/i18n/src/translations/en-US/portals/user.ts Outdated Show resolved Hide resolved
/**
* Excluded user stores.
*/
excludedUserStores?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excluded user stores is something that doesn't belong to profile schema interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.catch((error: AxiosError) => {
if (error?.response?.data?.detail || error?.response?.data?.description) {
dispatch(addAlert({
description: error?.response?.data?.detail || error?.response?.data?.description,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not display the error description coming from the backend, as we can't guarantee that the BE always returns a end user friendly message. Let's show the generic description defined on the frontend instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have shown the description in other places as well, I think this is useful in some cases, since console is used by Admins it's okay to show this kind of errors. For example, an LDAP-specific error message provides more insight than a generic error, guiding admins to address attribute compatibility.

https://github.com/PasinduYeshan/wso2-identity-apps/blob/22bdbde1b16ec65279643b7b03d5c7e6b5986c30/features/admin.users.v1/components/user-profile.tsx#L1189-L1194

features/admin.users.v1/components/user-profile.tsx Outdated Show resolved Hide resolved
modules/i18n/src/translations/en-US/portals/user.ts Outdated Show resolved Hide resolved
@wso2-jenkins-bot
Copy link
Contributor

🦋 Changeset detected

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants