@W-16889880 - Complete Social Login Integration: Connect Backend API to UI #2124

yunakim714 · 2024-11-11T16:38:34Z

Description

Wire up Social Login API calls to the newly implemented UI.
Login should work in 3 locations:

Login modal
Login page
Checkout page

We have configured Google and Apple as OOTB IDPs.

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

Add commerce-sdk-react API calls to the newly implemented UI
Add redirectURI as parameter, as this will be different from the default callback path that has already been set in app/components/_app-config here
If there is an error with the login API call, there will be an error on the redirect page as so:

How to Test-Drive This PR

Navigate to https://wasatch-mrt-social-login-poc.mrt-storefront-staging.com/ where social and passwordless login have been enabled
Open the login modal
Verify that you can log in with both Google and Apple
- Verify that you can log in from all 3 locations: login page (/login), login modal, and checkout page (/checkout)
- If using the login modal or checkout page, verify that you are redirected back to the page where you initiated login

To run the e2e test:

Point the test to the deployed environment by setting an env var like so: export RETAIL_APP_HOME=https://wasatch-mrt-social-login-poc.mrt-storefront-staging.com
Run npx playwright test --ui from the monorepo root
Run the test: Registered shopper logged in through social retains persisted cart
Verify that the user is logged in and sees their user details in the secure account page as well as the items they added to cart as a guest
⚠️I have added the NEW Google PWA Kit account credentials into Password Manager in the Imported Mobify - Product folder !!⚠️

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

…al-login

…al-login Signed-off-by: Yuna Kim <[email protected]>

yunakim714 · 2024-11-14T17:36:07Z

packages/commerce-sdk-react/src/auth/index.ts

@@ -987,7 +987,7 @@ class Auth {
     *
     */
    async authorizeIDP(parameters: AuthorizeIDPParams) {
-        const redirectURI = this.redirectURI
+        const redirectURI = parameters.redirectURI || this.redirectURI


Similar to loginIDPUser, if a user passes in a redirectURI, we want to prioritize using this value

yunakim714 · 2024-11-18T16:32:52Z

packages/commerce-sdk-react/src/auth/index.ts

@@ -1013,7 +1013,7 @@ class Auth {
    async loginIDPUser(parameters: LoginIDPUserParams) {
        const codeVerifier = this.get('code_verifier')
        const code = parameters.code
-        const usid = parameters.usid
+        const usid = this.get('usid')


We need to use the guest usid in order to run mergeBasket

Can you tell me more about this change here? We are now ignoring a parameter that is being passed in? Why aren't we doing something like the fallback where const usid = parameters.usid || this.get('usid') like you did for the redirect uri?

I changed this to use this.get('usid')because merge basket will only work if the /authorize and /token calls receive the same guest usid. The Auth class stores the same usid until the user is logged in so this would work.

However, I can still also do const usid = parameters.usid || this.get('usid') - the /authorize response body will return the same usid that is passed in the request params, and will only generate a new usid if there isn't one provided.

yunakim714 · 2024-11-18T16:33:38Z

packages/template-retail-react-app/app/hooks/use-auth-modal.test.js

@@ -122,6 +124,23 @@ test('Renders login modal by default', async () => {
 })

 test('Renders check email modal on email mode', async () => {
+    // Store the original useForm function


Mocking the form's success state in order to render the Check Your Email modal

yunakim714 · 2024-11-18T16:34:21Z

packages/template-retail-react-app/app/pages/social-login-redirect/index.jsx

+            mergeBasket.mutate({
+                headers: {
+                    // This is not required since the request has no body
+                    // but CommerceAPI throws a '419 - Unsupported Media Type' error if this header is removed.
+                    'Content-Type': 'application/json'
+                },
+                parameters: {
+                    createDestinationBasket: true
+                }
+            })


We merge the user's basket if the user is logged in successfully through social login!

yunakim714 · 2024-11-18T16:38:24Z

packages/template-retail-react-app/config/default.js

@@ -22,7 +22,8 @@ module.exports = {
            },
            social: {
                enabled: false,
-                idps: ['google', 'apple']
+                idps: ['google', 'apple'],
+                redirectURI: '/social-callback'


I have added the redirectURI here so that users can configure the redirectURI they want to use specifically for social login. This may differ from the redirectURI they have configured in AppConfig here

bredmond-sf · 2024-11-18T18:39:42Z

e2e/tests/desktop/registered-shopper.spec.js

@@ -165,3 +167,44 @@ test("Registered shopper can add item to wishlist", async ({ page }) => {
  // wishlist
  await validateWishlist({page})
 });
+
+/**
+ * Test that social login persists a user's shopping car


[NIT] shopping cart :)

yunakim714 · 2024-11-18T22:02:12Z

packages/template-retail-react-app/app/components/social-login/index.jsx

+                                        // Save the path where the user logged in
+                                        setSessionJSONItem('returnToPage', window.location.pathname)
+                                        // Set local storage item to indicate this is a external login
+                                        window.localStorage.setItem('isSocialProfile', true)


Hey @bendvc - Wanted to get your input on this. Nate, Pratik, and I have been discussing ways to resolve the "Social Login first" issue, where profiles that have been created through Social Login are unable to edit any profile credentials, because well, there aren't any.

For a short-term solution, I was thinking we could use local storage to denote that the current user is a social login user, and if so, disable edit in the Account page. I think in future work we're hoping the getCustomer endpoint will somehow be able to indicate whether the user profile was created through an external IDP or ECOM.

Here's how the account page would look like for social login users:

That is an interesting problem. If there is absolutely no way to tell via the JWT, I think this should be an acceptable solution. To make things a little more robust I might seek to use something like loginSource = credentials|idp instead of isSocialProfile because you can imagine a non-social first profile that just happened to log in via IDP doesn't mean that it's a social profile.

I guess the next question is someone that has a "non-social first" account, but logs in via IDP.. can they change there password? If they can we there isn't really a way to tell if we should be limiting them on changing their password, and it would be more like an artificial limitation that we impose and they have to log in with their password in order to make those types of changes?

This might deserve a short convo

bendvc · 2024-11-19T16:45:18Z

packages/template-retail-react-app/app/components/toggle-card/index.jsx

@@ -63,7 +64,7 @@ export const ToggleCard = ({
                        >
                            {title}
                        </Heading>
-                        {!editing && !disabled && onEdit && (
+                        {!editing && !disabled && onEdit && !disableEdit && (


NIT: The new prop disableEdit seems to have the same outcome as not passing in a onEdit value. This isn't a big deal, but typically you might have a different visual treatment if you were supplying 2 params like this. E.g. you have an onEdit callback, but it's disabled via disableEdit so it shows as disabled. But here we are not showing it, same as no passing in onEdit.

No action to take here, but just food for thought.

bendvc · 2024-11-19T16:53:30Z

packages/template-retail-react-app/app/pages/account/profile.jsx

@@ -343,7 +349,7 @@ const PasswordCard = () => {
    )
 }

-const AccountDetail = () => {
+const AccountDetail = ({isSocialProfile = false}) => {


As I get further down in this PR I see more and more prop drilling of this isSocialProfile. It's not a horrible pattern but no used as much these days.

Did you ever thing about having this information made available via a provider/hook combo? Something that comes to mind is to have it implemented in the commerce-sdk-react so you can reuse some code there.

@kevinxh would be a good person to ask about whether or not thats a good idea to implement in that lib.

I think ideally the commerce-sdk-react library should handle the state isSocialProfile, it would be easy if that information is embedded in the jwt token, if not, we should seek to manually store the state in the Auth class as a class property.

something like

// commerce-sdk-react auth.js class Auth { isSocialLogin = false loginSocial = () => { // ...do the login this.isSocialLogin = true // potentially also store the IDP information like google, facebook, etc } } // Then we can provide a utility hook // commerce-sdk-react useSocialLogin() export const useSocialLogin = () => { const auth = useAuth() return auth.isSocialLogin }

bendvc · 2024-11-19T16:57:52Z

packages/template-retail-react-app/app/pages/social-login-redirect/index.jsx

+        if (searchParams.code && searchParams.usid) {
+            loginIDPUser.mutateAsync({
+                code: searchParams.code,
+                redirectURI: redirectURI
+            })
+        }


Think about inverting this condition so that your code is a little flatter, like:

if (!searchParams.code || !searchParams.usid) { return } loginIDPUser.mutateAsync({ code: searchParams.code, redirectURI: redirectURI })

Also.. what happens if that condition doesn't result in a login? Or if someone goes to this page without the params?

@bendvc I added error handling in case this condition doesn't result in a login.

If someone goes to this page without the params they will not be redirected to the secure account page as this login API will not be called.

bendvc · 2024-11-19T16:59:21Z

packages/template-retail-react-app/app/pages/social-login-redirect/index.jsx

    useEffect(() => {
-        if (isRegistered) {
-            navigate('/account')
+        if (customer?.isRegistered) {


You can think about inverting this condition again. As a rule of thumb if all the code is wrapped in a condition, then it's a good candidate to invert, but if you have an equally size "else" then maybe not.

bendvc · 2024-11-19T17:00:00Z

packages/template-retail-react-app/app/pages/social-login-redirect/index.jsx

+            if (locatedFrom) {
+                navigate(locatedFrom)
+            } else {
+                navigate('/account')
+            }


Good call on checking for this value before navigating.

bendvc · 2024-11-19T17:05:43Z

packages/template-retail-react-app/app/pages/social-login-redirect/index.jsx

+                headers: {
+                    // This is not required since the request has no body
+                    // but CommerceAPI throws a '419 - Unsupported Media Type' error if this header is removed.
+                    'Content-Type': 'application/json'
+                },


I noticed that this is a copy of out other basket merge. I'm wondering if the commerce-sdk-react should be adding this for us. Kevin might be a good person to ask about that. Looking at the api docs the body is alway json, so I don't know why we would have to specify that all the time.

yunakim714 · 2024-11-20T17:11:23Z

packages/commerce-sdk-react/src/hooks/useCustomerType.ts

+    const uido: string | null = onClient
+        ? // eslint-disable-next-line react-hooks/rules-of-hooks
+          useLocalStorage(`uido_${config.siteId}`)
+        : auth.get('uido')
+


@kevinxh @bendvc The isb claim in the SLAS JWT access token tells us whether the user is an ecom user or not. Would it be okay to return this value as part of the existing useCustomerType hook ?

That useCustomerType hook has this return type:

export type CustomerType = null | 'guest' | 'registered'

Are you going to change that? what would the return type look like?

The return type is:

type useCustomerType = { customerType: CustomerType isGuest: boolean isRegistered: boolean uido: string | null }

So I would just add uido !

My gut is telling me that this right. First, the name of this type if probably not the best.. but imagine the type is called "CustomerType" ... you can see that it doesn't make sense to have anything but "type" in it. Is seems like we are sneaking uido into this hook. Maybe we can talk about it a little more.

yunakim714 · 2024-11-22T19:11:58Z

packages/template-retail-react-app/app/routes.jsx

@@ -89,7 +90,7 @@ export const routes = [
        exact: true
    },
    {
-        path: '/social-callback',
+        path: config.login.social?.redirectURI || '/social-callback',


If the user sets the redirectURI to be a different relative path in their config file, we want this reflected here in routes.jsx. The default will be /social-callback

yunakim714 · 2024-11-22T22:10:18Z

packages/template-retail-react-app/app/pages/account/profile.test.js

+    })
+})
+
+test('Non ECOM user cannot see the password card', async () => {


Adding test to verify that non ECOM users cannot edit nor see the Password card

…al-login

bendvc · 2024-11-27T17:48:58Z

e2e/scripts/pageHelpers.js

+        await page.waitForLoadState();
+
+        // Fill in the password input
+        await page.fill('input[type="password"]', 'hpv_pek-JZK_xkz0wzf');


I wonder if this is going to get caught by security? @shethj do we have an precedence for this, are we using secrets and environment variables for anything that we can copy the implementation?

@yunakim714 can you reach out to jainam for this.. I think he's knowledgable in the e2e landscape.

Yeah we set all secrets in Github Actions settings and they can be set as env vars at runtime.
Values can be read from process.env in the tests.
Refer this code where we read the storefront domain from env var:

pwa-kit/e2e/config.js

Line 10 in 57c24e1

process.env.RETAIL_APP_HOME ||

bendvc · 2024-11-27T17:51:39Z

e2e/tests/desktop/registered-shopper.spec.js

+/**
+ * Test that social login persists a user's shopping cart
+ */
+test("Registered shopper logged in through social retains persisted cart", async ({ page }) => {


Will the basket persist for this user over time. For example will the e2e run one day to the next and accumulate items in that cart until it potentially errors out?

We could possibly remove the item from the cart after checking for its existence to be safe.

bendvc · 2024-11-27T17:54:39Z

packages/commerce-sdk-react/src/auth/index.ts

@@ -1056,7 +1063,7 @@ class Auth {
    async loginIDPUser(parameters: LoginIDPUserParams) {
        const codeVerifier = this.get('code_verifier')
        const code = parameters.code
-        const usid = parameters.usid
+        const usid = parameters.usid || this.get('usid')


Under what circumstances would the usid on parameters be undefined?

Is usid on LoginIDPUserParams type optional? .. seems a little odd that it would be.

bendvc · 2024-11-27T17:57:55Z

packages/commerce-sdk-react/src/auth/index.ts

        const expiresDate = this.convertSecondsToDate(refreshTokenTTLValue)
        this.set('refresh_token_expires_in', refreshTokenTTLValue.toString())
        this.set(refreshTokenKey, res.refresh_token, {
            expires: expiresDate
        })
+        this.set('uido', uido)


We are setting this value.. but where are we getting it?

We are getting it here const {uido} = this.parseSlasJWT(res.access_token) in line 582

bendvc · 2024-11-27T18:06:05Z

packages/template-retail-react-app/app/components/social-login/index.jsx

+
+    // Build redirectURI from config values
+    const appOrigin = useAppOrigin()
+    const redirectPath = getConfig().app.login.social?.redirectURI || ''


NIT: It would be safer to have the optional chaining throughout this chain. E.g. getConfig()?.app?.login?.social?.redirect

bendvc · 2024-11-27T18:10:46Z

packages/template-retail-react-app/app/components/login/index.jsx

@@ -94,7 +96,8 @@ LoginForm.propTypes = {
    form: PropTypes.object,
    isPasswordlessEnabled: PropTypes.bool,
    isSocialEnabled: PropTypes.bool,
-    idps: PropTypes.arrayOf(PropTypes.string)
+    idps: PropTypes.arrayOf(PropTypes.string),
+    setLoginType: PropTypes.func


Lets talk about this setLoginType.. it's being prop drilled pretty deep and I kinda gave up after a couple files.. maybe there is a better solution here.

bendvc · 2024-11-27T18:14:35Z

packages/template-retail-react-app/app/pages/account/profile.jsx

@@ -60,7 +62,7 @@ const Skeleton = forwardRef(({children, height, width, ...rest}, ref) => {

 Skeleton.displayName = 'Skeleton'

-const ProfileCard = () => {
+const ProfileCard = ({isEcomAccount = false}) => {


There might be other use cases for wanting to disable the password reset. Might be wise to make this a little more generic by making this literal.. like "canResetPassword" or "allowPasswordChange". This way the component will be more flexible.

…ceCloud/pwa-kit into W-16889880-social-login

yunakim714 and others added 7 commits November 11, 2024 11:36

wire up slas api calls

eb1d1d0

Merge branch 'feature-passwordless-social-login' into W-16889880-soci…

5bc3223

…al-login

Merge branch 'feature-passwordless-social-login' into W-16889880-soci…

05a867e

…al-login Signed-off-by: Yuna Kim <[email protected]>

lint

2285322

add redirect uri as param

8e501d2

lint

248e81c

use session storage to navigate back to checkout page

9987857

yunakim714 marked this pull request as ready for review November 13, 2024 20:44

yunakim714 requested a review from a team as a code owner November 13, 2024 20:44

yunakim714 added 2 commits November 14, 2024 10:35

cleanup

62ce20f

upadte tests

2b8ff51

yunakim714 requested review from bredmond-sf, hajinsuha1 and jeremy-jung1 November 14, 2024 17:34

yunakim714 added the skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated label Nov 14, 2024

yunakim714 commented Nov 14, 2024

View reviewed changes

yunakim714 added 6 commits November 14, 2024 12:46

implement util function to build redirecturi

ae0b15b

add tests for utils function

48ad244

add initial outline of social login e2e tests

891f2cb

complete e2e test

7c4217a

mergebasket after social login

8388a6d

run merge basket if customer is registered

acb3f9e

yunakim714 requested a review from bendvc November 18, 2024 16:32

yunakim714 commented Nov 18, 2024

View reviewed changes

cleanup

166b522

yunakim714 commented Nov 18, 2024

View reviewed changes

bredmond-sf approved these changes Nov 18, 2024

View reviewed changes

use local storage to indicate user is a social login user

9bc048e

yunakim714 commented Nov 18, 2024

View reviewed changes

check that password card is hidden in e2e test

a8cefcd

bendvc reviewed Nov 19, 2024

View reviewed changes

yunakim714 added 2 commits November 20, 2024 12:06

add uido to auth data keys and customer type

31dd1ba

cleanup

c104f4a

yunakim714 commented Nov 20, 2024

View reviewed changes

yunakim714 added 2 commits November 21, 2024 11:30

add error handling and invert conditions

ebf67ef

make social redirect url path dynamic

5f4a5e6

yunakim714 commented Nov 22, 2024

View reviewed changes

yunakim714 added 3 commits November 22, 2024 14:12

cleanup

97ee0cb

slas and ecom are same user types

76e670f

add profile page tests

4058063

yunakim714 commented Nov 22, 2024

View reviewed changes

Merge branch 'feature-passwordless-social-login' into W-16889880-soci…

3c0317d

…al-login

bendvc reviewed Nov 27, 2024

View reviewed changes

yunakim714 added 4 commits November 27, 2024 13:53

change prop name to be more descriptive

f7d9b34

Merge branch 'W-16889880-social-login' of github.com:SalesforceCommer…

71243c1

…ceCloud/pwa-kit into W-16889880-social-login

set e2e user credentials as env vars

0e12f93

fix env var name

77afb51

@W-16889880 - Complete Social Login Integration: Connect Backend API to UI #2124

Are you sure you want to change the base?

@W-16889880 - Complete Social Login Integration: Connect Backend API to UI #2124

Conversation

yunakim714 commented Nov 11, 2024 • edited Loading

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunakim714 commented Nov 11, 2024 •

edited

Loading