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

@W-17039090@ : [Server Affinity] - Attach dwsid to SCAPI request headers #2090

Merged
merged 10 commits into from
Oct 22, 2024

Conversation

shethj
Copy link
Collaborator

@shethj shethj commented Oct 21, 2024

Description

For a Phased Launch storefront, SCAPI requires each request to contain ECOM dwsid in sfdc_dwsid header with each request.
To allow PWA Kit to read dwsid from cookies we require the cookie to be set with httpOnly=false.
Plugin SLAS manually sets response cookies and has been setting dwsid with httpOnly=false
We can leverage that cookie to be used in PWA Kit to send with each SCAPI request.

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

  • Added sfdc_dwsid header to all requests made in commerce-sdk-react
  • Remove ocapi proxy from the retail app and generator templates

How to Test-Drive This PR

  • Logon to prd2.phased-launch-testing.com
  • Clear Site Data using DevTools
  • Load Home Page
  • Open Network Tab and confirm /sessions requests are not made
  • Open Cookies and confirm dwsid is not being set
  • Add item to cart and navigate to cart page
  • Open cookies and verify dwsid is now set
  • Navigate back to PWA Kit page
  • Inspect any SCAPI request and confirm sfdc_dwsid exists in request headers

Local test run results:
Screenshot 2024-10-22 at 10 00 21 AM

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...

Localization

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

@shethj shethj requested a review from a team as a code owner October 21, 2024 15:13
@shethj shethj changed the title Attach dwsid to SCAPI request headers Server Affinity - Attach dwsid to SCAPI request headers Oct 21, 2024
@shethj shethj changed the title Server Affinity - Attach dwsid to SCAPI request headers [Server Affinity] - Attach dwsid to SCAPI request headers Oct 21, 2024
@@ -57,7 +57,6 @@ const AppConfig = ({children, locals = {}}) => {
redirectURI={`${appOrigin}/callback`}
proxy={`${appOrigin}${commerceApiConfig.proxyPath}`}
headers={headers}
OCAPISessionsURL={`${appOrigin}${proxyBasePath}/ocapi/s/${locals.site?.id}/dw/shop/v22_8/sessions`}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's good that we're removing the OCAPI sessions URL from the generated apps since we've removed it from template-retail-react-app already.

@vcua-mobify
Copy link
Contributor

Should we modify the config files in template-retail-react-app and the generator to remove the OCAPI proxy?
I think our test mocks set up an OCAPI proxy mock that we'll need to clean up too.

@unandyala
Copy link
Collaborator

Should we modify the config files in template-retail-react-app and the generator to remove the OCAPI proxy? I think our test mocks set up an OCAPI proxy mock that we'll need to clean up too.

Is this the only url/api that uses ocapi?

unandyala
unandyala previously approved these changes Oct 21, 2024
Copy link
Collaborator

@unandyala unandyala left a comment

Choose a reason for hiding this comment

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

LGTM

@shethj
Copy link
Collaborator Author

shethj commented Oct 22, 2024

Should we modify the config files in template-retail-react-app and the generator to remove the OCAPI proxy? I think our test mocks set up an OCAPI proxy mock that we'll need to clean up too.

Is this the only url/api that uses ocapi?

Yes. It was the only OCAPI call we were making.

@shethj
Copy link
Collaborator Author

shethj commented Oct 22, 2024

@vcua-mobify @unandyala Great callouts about OCAPI proxy. I tried removing the proxy and ran tests on the deployed store. Looks good to me.

@shethj shethj changed the title [Server Affinity] - Attach dwsid to SCAPI request headers @W-17039090@ : [Server Affinity] - Attach dwsid to SCAPI request headers Oct 22, 2024
@vcua-mobify
Copy link
Contributor

One more spot we might want to clean up: https://github.com/SalesforceCommerceCloud/pwa-kit/blob/feature/server-affinity-cleanup/packages/pwa-kit-react-sdk/setup-jest.js#L35

@vcua-mobify
Copy link
Contributor

This might be a separate clean up ticket but the generator prompts is for an instanceUrl that, according to a comment in the generator, is only used to set up the OCAPI proxy: https://github.com/SalesforceCommerceCloud/pwa-kit/blob/feature/server-affinity-cleanup/packages/pwa-kit-create-app/scripts/create-mobify-app.js#L801

@shethj
Copy link
Collaborator Author

shethj commented Oct 22, 2024

This might be a separate clean up ticket but the generator prompts is for an instanceUrl that, according to a comment in the generator, is only used to set up the OCAPI proxy: https://github.com/SalesforceCommerceCloud/pwa-kit/blob/feature/server-affinity-cleanup/packages/pwa-kit-create-app/scripts/create-mobify-app.js#L801

Yeah. I'm leaving all generator changes out of this PR. I'll have a follow up ticket for that since it involves testing and changes to tests as well. It'll blow up the scope of this one.

Copy link
Contributor

@vcua-mobify vcua-mobify left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes! +1

@shethj shethj merged commit ca0bc0b into develop Oct 22, 2024
29 checks passed
@bfeister bfeister deleted the feature/server-affinity-cleanup branch October 22, 2024 17:41
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.

3 participants