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

PWA-2571: Regression fix disabled TTL support in BrowserPersistence #3729

Merged
merged 5 commits into from Mar 23, 2022

Conversation

michaelyu0123
Copy link
Contributor

  • Reverted ttl support for getItem

Description

Problem
A user auth bug was fixed in v11.0.0 regression, but it appears to have removed support for an important feature for TTL support of entries. While this may have solved the bug, this utility is public API, and anyone else who was using it expecting items to expire now have large stale caches for their shoppers.

Introduced by: https://github.com/magento/pwa-studio/pull/3306/files#diff-44b7b17ad1392c88b1e3411c80f7378782e6a568d280b68a48349061293b604cL51-L54

Acceptance Criteria
TTL is restored as supported in BrowserPersistence
No regressions related to the user auth bug this intended to fix

Related Issue

Closes #PWA-2571

Acceptance

Verification Stakeholders

Specification

Verification Steps

Test Execution
Verify #3306. still works
Verify TTL support works as expected. Browser stale cache should expire.
Verify above 2 scenarios in all supported browsers and devices.
Verify apollo does not store any sensitive data after user session logout
Verify above scenario in both DEfault. and French store
Cypress Test coverage -
Changes should be covered in unit test level. No cypress unless unit test does not support this kind of scenario to test.

Test scenario(s) for direct fix/feature

Test scenario(s) for any existing impacted features/areas

Test scenario(s) for any Magento Backend Supported Configurations

Is Browser/Device testing needed?

Any ad-hoc/edge case scenarios that need to be considered?

Screenshots / Screen Captures (if appropriate)

Breaking Changes (if any)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@michaelyu0123 michaelyu0123 added the version: Patch This changeset includes backwards compatible bug fixes. label Mar 4, 2022
@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Mar 4, 2022
@michaelyu0123
Copy link
Contributor Author

run all tests

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 4, 2022

Successfully started codebuild job for the following test suites:

scaffold-pwa
cypress
lighthouse-desktop
lighthouse-mobile
pr-test

@pwa-studio-bot
Copy link
Collaborator

pwa-studio-bot commented Mar 4, 2022

Fails
🚫

node failed.

🚫 Lighthouse assertions failed 😔. All assertions must pass before this PR can be merged
https://pr-3729.pwa-venia.com/

Cumulative Layout Shift (maxNumericValue)

  • Expected: 0.1
  • Actual: 0.21942616102430557
  • Result: Failed
  • Docs link: https://web.dev/cls/
https://pr-3729.pwa-venia.com/valeria-two-layer-tank.html

First Contentful Paint (maxNumericValue)

  • Expected: 3100
  • Actual: 3139.169
  • Result: Failed
  • Docs link: https://web.dev/first-contentful-paint/

Log

ERROR ON TASK: lighthouseTests


Error:  Danger had errors running. See message(s) above for more details.
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 46146c9

@pwa-studio-bot
Copy link
Collaborator

pwa-studio-bot commented Mar 4, 2022

Messages
📖

Associated JIRA tickets: PWA-2571.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next pr-test build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 46146c9

@michaelyu0123
Copy link
Contributor Author

run all tests

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 4, 2022

Successfully started codebuild job for the following test suites:

lighthouse-desktop
lighthouse-mobile
pr-test
scaffold-pwa
cypress

@michaelyu0123
Copy link
Contributor Author

run scaffold-pwa

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 4, 2022

Successfully started codebuild job for scaffold-pwa

@michaelyu0123
Copy link
Contributor Author

run cypress

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 4, 2022

Successfully started codebuild job for cypress

anthoula
anthoula previously approved these changes Mar 9, 2022
@tjwiebell
Copy link
Contributor

run cypress

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 10, 2022

Successfully started codebuild job for cypress

@tjwiebell
Copy link
Contributor

run lighthouse-desktop

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 10, 2022

Successfully started codebuild job for lighthouse-desktop

@tjwiebell
Copy link
Contributor

❓ Verify #3306. still works
✅ Verify TTL support works as expected. Browser stale cache should expire.
✅ Verify above 2 scenarios in all supported browsers and devices.
❌ Verify apollo does not store any sensitive data after user session logout
✅ Verify above scenario in both DEfault. and French store

@michaelyu0123 Found one scenario where sensitive data is kept around, so some of this original bug may have resurfaced. Simulated a shopper returning to the site with an expired auth token in local storage (edit the ttl), customer data still there. Problem does not exist when letting the timer remove the token, so race condition likely.

Screen Shot 2022-03-10 at 1 29 21 PM

@michaelyu0123
Copy link
Contributor Author

@tjwiebell yes you are correct, it is mainly due to the reducer fetching the initial state call which also attempts to get the signin_token which removed the signin_token before user components useEffect has a chance to fetch it which in turn caused it to not run a async logout event that flushes customer information. I have modified the initial state fetching to just call the get item on signin_token with none ttl checking method to let the user component get a chance to handle it properly. Perhaps need a separate workflow for the login ttl in the future since it has to be handled differently.

- Updated user reducers to not use the ttl checking getItem to let user component validate.
@michaelyu0123
Copy link
Contributor Author

run all tests

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 10, 2022

Successfully started codebuild job for the following test suites:

scaffold-pwa
cypress
lighthouse-desktop
lighthouse-mobile
pr-test

@michaelyu0123
Copy link
Contributor Author

run lighthouse-mobile

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 11, 2022

Successfully started codebuild job for lighthouse-mobile

@m2-community-project m2-community-project bot moved this from Reviewer Approved to Review in Progress in Pull Request Progress Mar 18, 2022
@tjwiebell
Copy link
Contributor

✅ QA Passed

Ready to merge after review of 0c7fd5c.

@m2-community-project m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Mar 23, 2022
@tjwiebell
Copy link
Contributor

run lighthouse-mobile

@pwa-test-bot
Copy link

pwa-test-bot bot commented Mar 23, 2022

Successfully started codebuild job for lighthouse-mobile

@magento magento deleted a comment from pwa-test-bot bot Mar 23, 2022
@magento magento deleted a comment from pwa-test-bot bot Mar 23, 2022
@tjwiebell
Copy link
Contributor

lighthouse-mobile scores are within range of develop.pwa-venia.com, @dpatil-magento will merge.

@dpatil-magento dpatil-magento merged commit 5aae89a into develop Mar 23, 2022
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Mar 23, 2022
@dpatil-magento dpatil-magento deleted the PWA-2571 branch March 23, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine Progress: done version: Patch This changeset includes backwards compatible bug fixes.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants