Repository navigation
@W-24382352 - Send login_id + channel_id on passwordless/token and user_id + channel_id on password/action - #4075
yunakim714 wants to merge 14 commits into
Conversation
|
Git2Gus App is installed but the |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| INVALID_TOKEN_ERROR.test(errorMessage) || | ||
| MISSING_OR_MISMATCHED_IDENTIFIER_ERROR.test(String(errorMessage)) | ||
| ) { | ||
| return INVALID_TOKEN_ERROR_MESSAGE |
There was a problem hiding this comment.
Should the new MISSING_OR_MISMATCHED_IDENTIFIER_ERROR display a different error message?
jeremy-jung1
left a comment
There was a problem hiding this comment.
Thanks for picking this up — binding the passwordless token to login_id + channel_id is the right call. Inline comments have the details; summary + the bits that don't anchor to a diff line:
🔴 Blocking
login_id/channel_iddon't reach SLAS yet — see inline onauth/index.ts. Needscommerce-sdk-isomorphic@6.0.0(from commerce-sdk-isomorphic#304) plus a camelCase mapping here. The bump has to land in bothpackages/commerce-sdk-react/package.jsonandpackages/template-retail-react-app/package.json(both pin5.7.0), plus lockfiles.- 429 handling can't work and leaves the shopper with no error — see inline on
auth-utils.js.
🟠 Should fix
- Breaking change →
[Breaking]+ major version (inline on CHANGELOG) - Match real SLAS error strings (inline)
- Reset-password rate limit shown as invalid token (inline)
- Shopper email in the magic-link URL (inline on
ssr.js) - Rollout / back-compat note — these magic-link paths will send
login_id: '': links emailed before this deploys, existing generated projects (they own their copy ofssr.js), and merchants with a custom callback/email service. SLAS ignores a blanklogin_idtoday so nothing breaks yet, but it will once SLAS requires it — please call out the ordering and what merchants need to update.
🟡 Nits
- Inline: double decode, contact-info
login_idsource, registrationundefinedemail, stray blank lines in two test files. - "How to Test-Drive This PR" is still the
(step1)placeholder.
✅ Nice
- All four
LoginPasswordlessUsercall sites updated, and thelogin_idlines up with theuseridsent toauthorizePasswordless. - Dropping the stale
@ts-ignoreonresetPassworduser_idis correct; reset already sendsuser_id. - Pulling out
getResetPasswordActionErrorMessageis a nice cleanup.
…rceCommerceCloud/pwa-kit into W-24382352-send-login-channel-id
Signed-off-by: Yuna Kim <84923642+yunakim714@users.noreply.github.com>
…rceCommerceCloud/pwa-kit into W-24382352-send-login-channel-id
jeremy-jung1
left a comment
There was a problem hiding this comment.
Thanks for the updates! Most of the earlier points are addressed. The camelCase loginId/channelId mapping, the status-based 429 handling, the exact SLAS error strings, the reset-password 429 message, the authorize-time email in one-click checkout, the wire-body test, and the notify-type fix with its ssr.test.js coverage all look good. A few things remain. File-specific ones are inline.
1. CI can't run until isomorphic 6.0.0 is published, but you can reproduce it locally
Every job fails at npm ci (notarget No matching version found for commerce-sdk-isomorphic@6.0.0), so none of the tests have run in CI. You can get the same results locally without publishing anything. It takes about 10 minutes.
a) Build isomorphic at the #304 merge commit:
git clone https://github.com/SalesforceCommerceCloud/commerce-sdk-isomorphic.git
cd commerce-sdk-isomorphic
git checkout 3d2679f591bad3efd4ebf8326b855a1414464f73
yarn install --frozen-lockfile --ignore-scripts
yarn run renderTemplates
yarn build:lib
npm pack # -> commerce-sdk-isomorphic-6.0.0.tgz
--ignore-scripts only skips the prepare script, which runs the old snyk protect. These are the same steps publish.yml runs.
b) On this branch, temporarily set the isomorphic dependency in both packages/commerce-sdk-react/package.json and packages/template-retail-react-app/package.json to:
"commerce-sdk-isomorphic": "file:/absolute/path/to/commerce-sdk-isomorphic-6.0.0.tgz"
c) From the monorepo root, run npm install (not npm ci; the lockfiles don't match the tarball). Check that it worked:
node -p "require('./packages/commerce-sdk-react/node_modules/commerce-sdk-isomorphic/package.json').version" # 6.0.0
d) Run the checks:
cd packages/commerce-sdk-react && npm run lint && npm test
cd ../template-retail-react-app && npm run lint && npm test
In the retail app, run the full npm test. If you run only some test files, the coverage threshold fails even when every test passes.
e) Revert before pushing. npm install rewrites every package-lock.json in the repo:
git checkout -- '*package.json' '*package-lock.json'
Don't commit the file: path or the lockfile changes.
I ran these steps on 5e5e71d. Typecheck passes, the retail tests for the changed areas pass, and the wire-body test passes, so the 6.0.0 helper does send login_id/channel_id. Three things fail:
- One commerce-sdk-react test fails (inline).
- commerce-sdk-react lint: 3 errors in
src/auth/index.test.ts(inline). - Retail lint: 5 prettier errors in
login/index.jsx:126,auth-utils.js:32and:163, andauth-utils.test.js:124and:148. The newssr.jscondition is probably over the line length too.npm run lint:fixfixes all of them.
Please paste the passing lint and test output into the PR description so reviewers have evidence until CI can run. We'll still need one green CI run after 6.0.0 is published, because the generated-project and Windows jobs can't be reproduced this way.
2. PR description
- "How to Test-Drive This PR" still says
(step1). - Please add a short rollout note: this needs to ship before SLAS enforces
login_id/channel_idon/passwordless/token, and storefronts on older PWA Kit versions will fail passwordless login once SLAS enforces it. - The Errors bullet still says password/action rate limits map to the invalid-token UX (see the CHANGELOG comment).
| const handleOtpVerification = async (pwdlessLoginToken) => { | ||
| try { | ||
| await loginPasswordless.mutateAsync({pwdlessLoginToken}) | ||
| const loginId = form.getValues().email || initialEmail |
There was a problem hiding this comment.
Nit: one-click contact-info and registration now use the email that was sent to authorizePasswordless (lastEmailSentRef), but here and in login/index.jsx we read the current form value. If the shopper changes the email (even just its casing) between sending and verifying, SLAS returns a login_id mismatch and the shopper sees "invalid or expired code". Consider capturing the authorize-time email here too, for consistency.
There was a problem hiding this comment.
Capturing authorize-time email here for consistency, and failing fast if no email is provided
Signed-off-by: Yuna Kim <84923642+yunakim714@users.noreply.github.com>
jeremy-jung1
left a comment
There was a problem hiding this comment.
Thanks for turning this around so quickly. The last round is addressed: the test fix, the test.skip removal, lint, translations, registration error messages, authorize-time email in the modal and login page, the CHANGELOG, and the Test-Drive steps with local results all look good. Just a couple of small things left, plus an FYI.
1. FYI: I'm handling the scaffold ssr.js templates in #4092
pwa-kit-create-app has its own copies of sendMagicLinkEmail that this PR doesn't update:
assets/templates/@salesforce/retail-react-app/app/ssr.js.hbsassets/bootstrap/js/overrides/app/ssr.js.hbs
Magic links from generated and extensible projects therefore arrive without login_id. With the new early return in login/index.jsx, magic-link login in those projects would fail without showing anything.
#4092 is stacked on your branch. It:
- adds
login_idto both templates - shows the landing-page error instead of dropping it
- adds a CHANGELOG upgrade note for projects that override
ssr.js
No change needed here. I'll merge #4092 right after this one so both land in the same release.
2. PR description
- The Errors bullet under Changes still says password/action rate limits map to "the invalid-token reset-password UX". The CHANGELOG is fixed, but this bullet isn't.
- Please add a rollout note: this needs to ship before SLAS enforces
login_id/channel_idon/passwordless/token. Projects on older PWA Kit versions, or with a customssr.jsthat doesn't addlogin_id, will fail passwordless login once SLAS enforces it.
3. Nit: leftover blank lines from lint --fix
In components/_app/index.test.js:33 and pages/component-preview/index.test.js, lint --fix replaced the // eslint-disable-next-line react/prop-types comment with an empty line inside an object literal. Please delete the empty line, or revert those two files, since they aren't part of this change.
| return { | ||
| ...actual, | ||
| // eslint-disable-next-line react/prop-types | ||
|
|
There was a problem hiding this comment.
Nit: empty line left behind after lint --fix removed the eslint-disable comment. Please delete it, or revert this file (same in component-preview/index.test.js).
Description
Require
login_idandchannel_idon passwordless OTP token exchange so SLAS can bind the token to the shopper identifier and site. Forward both throughcommerce-sdk-react, pass them from all retailloginPasswordlesscall sites (including magic-link landing via login_id on the URL), and map related SLAS errors to existing UX.Types of Changes
Changes
How to Test-Drive This PR
Automated
commerce-sdk-reactandtemplate-retail-react-appunit tests against the locally builtcommerce-sdk-isomorphic-6.0.0version:template-retail-react-appcommerce-sdk-reactManual — passwordless login
Manual — one-click checkout
Manual — rate limits / errors
Manual — reset password (non-rate-limit)
Sanity
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
Localization