-
Notifications
You must be signed in to change notification settings - Fork 138
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
@W-16889880 - Complete Social Login Integration: Connect Backend API to UI #2124
base: feature-passwordless-social-login
Are you sure you want to change the base?
Changes from 26 commits
eb1d1d0
5bc3223
05a867e
2285322
8e501d2
248e81c
9987857
62ce20f
2b8ff51
ae0b15b
48ad244
891f2cb
7c4217a
8388a6d
acb3f9e
166b522
9bc048e
a8cefcd
31dd1ba
c104f4a
ebf67ef
5f4a5e6
97ee0cb
76e670f
4058063
3c0317d
f7d9b34
71243c1
0e12f93
77afb51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,34 @@ export const navigateToPDPDesktop = async ({page}) => { | |
await productTile.click(); | ||
} | ||
|
||
/** | ||
* Navigates to the `Cotton Turtleneck Sweater` PDP (Product Detail Page) on Desktop | ||
* with the black variant selected. | ||
* | ||
* @param {Object} options.page - Object that represents a tab/window in the browser provided by playwright | ||
*/ | ||
export const navigateToPDPDesktopSocial = async ({page, productName, productColor, productPrice}) => { | ||
await page.goto(config.RETAIL_APP_HOME); | ||
|
||
await page.getByRole("link", { name: "Womens" }).hover(); | ||
const topsNav = await page.getByRole("link", { name: "Tops", exact: true }); | ||
await expect(topsNav).toBeVisible(); | ||
|
||
await topsNav.click(); | ||
|
||
// PLP | ||
const productTile = page.getByRole("link", { | ||
name: RegExp(productName, 'i'), | ||
}); | ||
// selecting swatch | ||
const productTileImg = productTile.locator("img"); | ||
await productTileImg.waitFor({state: 'visible'}) | ||
await expect(productTile.getByText(RegExp(`From \\${productPrice}`, 'i'))).toBeVisible(); | ||
|
||
await productTile.getByLabel(RegExp(productColor, 'i'), { exact: true }).hover(); | ||
await productTile.click(); | ||
} | ||
|
||
/** | ||
* Adds the `Cotton Turtleneck Sweater` product to the cart with the variant: | ||
* Color: Black | ||
|
@@ -254,6 +282,43 @@ export const loginShopper = async ({page, userCredentials}) => { | |
} | ||
} | ||
|
||
/** | ||
* Attempts to log in a shopper with provided user credentials. | ||
* | ||
* @param {Object} options.page - Object that represents a tab/window in the browser provided by playwright | ||
* @return {Boolean} - denotes whether or not login was successful | ||
*/ | ||
export const socialLoginShopper = async ({page}) => { | ||
try { | ||
await page.goto(config.RETAIL_APP_HOME + "/login"); | ||
|
||
await page.getByRole("button", { name: /Google/i }).click(); | ||
await expect(page.getByText(/Sign in with Google/i)).toBeVisible({ timeout: 10000 }); | ||
await page.waitForSelector('input[type="email"]'); | ||
|
||
// Fill in the email input | ||
await page.fill('input[type="email"]', '[email protected]'); | ||
await page.click('#identifierNext'); | ||
|
||
await page.waitForLoadState(); | ||
|
||
// Fill in the password input | ||
await page.fill('input[type="password"]', 'hpv_pek-JZK_xkz0wzf'); | ||
await page.click('#passwordNext'); | ||
await page.waitForLoadState(); | ||
|
||
await expect(page.getByRole("heading", { name: /Account Details/i })).toBeVisible({timeout: 20000}) | ||
await expect(page.getByText(/[email protected]/i)).toBeVisible() | ||
|
||
// Password card should be hidden for social login user | ||
await expect(page.getByRole("heading", { name: /Password/i })).toBeHidden() | ||
|
||
return true; | ||
} catch { | ||
return false; | ||
} | ||
} | ||
|
||
/** | ||
* Search for products by query string that takes you to the PLP | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ const { | |
validateWishlist, | ||
loginShopper, | ||
navigateToPDPDesktop, | ||
navigateToPDPDesktopSocial, | ||
socialLoginShopper, | ||
} = require("../../scripts/pageHelpers"); | ||
const { | ||
generateUserCredentials, | ||
|
@@ -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 cart | ||
*/ | ||
test("Registered shopper logged in through social retains persisted cart", async ({ page }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
navigateToPDPDesktopSocial({page, productName: "Floral Ruffle Top", productColor: "Cardinal Red Multi", productPrice: "£35.19"}); | ||
|
||
// Add to Cart | ||
await expect( | ||
page.getByRole("heading", { name: /Floral Ruffle Top/i }) | ||
).toBeVisible({timeout: 15000}); | ||
await page.getByRole("radio", { name: "L", exact: true }).click(); | ||
|
||
await page.locator("button[data-testid='quantity-increment']").click(); | ||
|
||
// Selected Size and Color texts are broken into multiple elements on the page. | ||
// So we need to look at the page URL to verify selected variants | ||
const updatedPageURL = await page.url(); | ||
const params = updatedPageURL.split("?")[1]; | ||
expect(params).toMatch(/size=9LG/i); | ||
expect(params).toMatch(/color=JJ9DFXX/i); | ||
await page.getByRole("button", { name: /Add to Cart/i }).click(); | ||
|
||
const addedToCartModal = page.getByText(/2 items added to cart/i); | ||
|
||
await addedToCartModal.waitFor(); | ||
|
||
await page.getByLabel("Close").click(); | ||
|
||
// Social Login | ||
const isLoggedIn = await socialLoginShopper({ | ||
page | ||
}) | ||
|
||
// Check Items in Cart | ||
await page.getByLabel(/My cart/i).click(); | ||
await page.waitForLoadState(); | ||
await expect( | ||
page.getByRole("link", { name: /Floral Ruffle Top/i }) | ||
).toBeVisible(); | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,7 @@ type AuthDataKeys = | |
| typeof DNT_COOKIE_NAME | ||
| typeof DWSID_COOKIE_NAME | ||
| 'code_verifier' | ||
| 'uido' | ||
|
||
type AuthDataMap = Record< | ||
AuthDataKeys, | ||
|
@@ -191,6 +192,10 @@ const DATA_MAP: AuthDataMap = { | |
code_verifier: { | ||
storageType: 'local', | ||
key: 'code_verifier' | ||
}, | ||
uido: { | ||
storageType: 'local', | ||
key: 'uido' | ||
} | ||
} | ||
|
||
|
@@ -574,11 +579,13 @@ class Auth { | |
responseValue, | ||
defaultValue | ||
) | ||
const {uido} = this.parseSlasJWT(res.access_token) | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are setting this value.. but where are we getting it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are getting it here |
||
} | ||
|
||
async refreshAccessToken() { | ||
|
@@ -1030,7 +1037,7 @@ class Auth { | |
* | ||
*/ | ||
async authorizeIDP(parameters: AuthorizeIDPParams) { | ||
const redirectURI = this.redirectURI | ||
const redirectURI = parameters.redirectURI || this.redirectURI | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to |
||
const usid = this.get('usid') | ||
const {url, codeVerifier} = await helpers.authorizeIDP( | ||
this.client, | ||
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Under what circumstances would the usid on parameters be undefined? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is usid on LoginIDPUserParams type optional? .. seems a little odd that it would be. |
||
const redirectURI = parameters.redirectURI || this.redirectURI | ||
|
||
const token = await helpers.loginIDPUser( | ||
|
@@ -1139,6 +1146,7 @@ class Auth { | |
// ISB format | ||
// 'uido:ecom::upn:Guest||xxxEmailxxx::uidn:FirstName LastName::gcid:xxxGuestCustomerIdxxx::rcid:xxxRegisteredCustomerIdxxx::chid:xxxSiteIdxxx', | ||
const isbParts = isb.split('::') | ||
const uido = isbParts[0].split('uido:')[1] | ||
const isGuest = isbParts[1] === 'upn:Guest' | ||
const customerId = isGuest | ||
? isbParts[3].replace('gcid:', '') | ||
|
@@ -1159,7 +1167,8 @@ class Auth { | |
dnt, | ||
loginId, | ||
isAgent, | ||
agentId | ||
agentId, | ||
uido | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ type useCustomerType = { | |
customerType: CustomerType | ||
isGuest: boolean | ||
isRegistered: boolean | ||
uido: string | null | ||
} | ||
|
||
/** | ||
|
@@ -50,10 +51,16 @@ const useCustomerType = (): useCustomerType => { | |
customerType = null | ||
} | ||
|
||
const uido: string | null = onClient | ||
? // eslint-disable-next-line react-hooks/rules-of-hooks | ||
useLocalStorage(`uido_${config.siteId}`) | ||
: auth.get('uido') | ||
|
||
Comment on lines
+54
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That
Are you going to change that? what would the return type look like? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return type is:
So I would just add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return { | ||
customerType, | ||
isGuest, | ||
isRegistered | ||
isRegistered, | ||
uido | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,8 @@ const LoginForm = ({ | |
form, | ||
isPasswordlessEnabled = false, | ||
isSocialEnabled = false, | ||
idps = [] | ||
idps = [], | ||
setLoginType | ||
}) => { | ||
return ( | ||
<Fragment> | ||
|
@@ -56,6 +57,7 @@ const LoginForm = ({ | |
handlePasswordlessLoginClick={handlePasswordlessLoginClick} | ||
isSocialEnabled={isSocialEnabled} | ||
idps={idps} | ||
setLoginType={setLoginType} | ||
/> | ||
) : ( | ||
<StandardLogin | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
export default LoginForm |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,10 @@ import PropTypes from 'prop-types' | |
import {defineMessage, useIntl} from 'react-intl' | ||
import {Button} from '@salesforce/retail-react-app/app/components/shared/ui' | ||
import logger from '@salesforce/retail-react-app/app/utils/logger-instance' | ||
import {useAuthHelper, AuthHelpers} from '@salesforce/commerce-sdk-react' | ||
import {getConfig} from '@salesforce/pwa-kit-runtime/utils/ssr-config' | ||
import {useAppOrigin} from '@salesforce/retail-react-app/app/hooks/use-app-origin' | ||
import {setSessionJSONItem, buildRedirectURI} from '@salesforce/retail-react-app/app/utils/utils' | ||
|
||
// Icons | ||
import {AppleIcon, GoogleIcon} from '@salesforce/retail-react-app/app/components/icons' | ||
|
@@ -38,6 +42,12 @@ const IDP_CONFIG = { | |
*/ | ||
const SocialLogin = ({idps}) => { | ||
const {formatMessage} = useIntl() | ||
const authorizeIDP = useAuthHelper(AuthHelpers.AuthorizeIDP) | ||
|
||
// Build redirectURI from config values | ||
const appOrigin = useAppOrigin() | ||
const redirectPath = getConfig().app.login.social?.redirectURI || '' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: It would be safer to have the optional chaining throughout this chain. E.g. |
||
const redirectURI = buildRedirectURI(appOrigin, redirectPath) | ||
|
||
const isIdpValid = (name) => { | ||
return name in IDP_CONFIG && IDP_CONFIG[name.toLowerCase()] | ||
|
@@ -64,17 +74,20 @@ const SocialLogin = ({idps}) => { | |
const config = IDP_CONFIG[name.toLowerCase()] | ||
const Icon = config?.icon | ||
const message = formatMessage(config?.message) | ||
|
||
return ( | ||
config && ( | ||
<Button | ||
onClick={() => { | ||
alert(message) | ||
onClick={async () => { | ||
// Save the path where the user logged in | ||
setSessionJSONItem('returnToPage', window.location.pathname) | ||
await authorizeIDP.mutateAsync({ | ||
hint: name, | ||
redirectURI: redirectURI | ||
}) | ||
}} | ||
borderColor="gray.500" | ||
color="blue.600" | ||
variant="outline" | ||
key={`${name}-button`} | ||
> | ||
<Icon sx={{marginRight: 2}} /> | ||
{message} | ||
|
@@ -88,7 +101,8 @@ const SocialLogin = ({idps}) => { | |
} | ||
|
||
SocialLogin.propTypes = { | ||
idps: PropTypes.arrayOf(PropTypes.string) | ||
idps: PropTypes.arrayOf(PropTypes.string), | ||
redirectURI: PropTypes.string | ||
} | ||
|
||
export default SocialLogin |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ export const ToggleCard = ({ | |
title, | ||
editing, | ||
disabled, | ||
disableEdit, | ||
onEdit, | ||
editLabel, | ||
isLoading, | ||
|
@@ -63,7 +64,7 @@ export const ToggleCard = ({ | |
> | ||
{title} | ||
</Heading> | ||
{!editing && !disabled && onEdit && ( | ||
{!editing && !disabled && onEdit && !disableEdit && ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: The new prop No action to take here, but just food for thought. |
||
<Button | ||
variant="link" | ||
size="sm" | ||
|
@@ -105,6 +106,7 @@ ToggleCard.propTypes = { | |
editing: PropTypes.bool, | ||
isLoading: PropTypes.bool, | ||
disabled: PropTypes.bool, | ||
disableEdit: PropTypes.bool, | ||
onEdit: PropTypes.func, | ||
children: PropTypes.any | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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