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

fix: [M3-8739] - Fix MSW 2.0 initial mock store and support ticket seeder bugs #11090

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Oct 10, 2024

Description 📝

This PR fixes an existing bug in the way the MSW2.0 was creating the initial mock store. It made itself known after #10937 was merged because that's the first time that many of us already had an existing store, and now new values were being added to it.

🐛 Bug:
When we are trying to create our initial mock store and access initialContext.supportReplies or initialContext.supportTickets, developers were encountering a console error and white screen if they already had an existing IndexedDB that did not have supportReplies or supportTickets in the mockStore. This could be worked around by clearing the Application data in the browser console to reset the IndexedDB, but it would continue to happen in the future with every new set of values we track in the store.

❓ Why:
This was happening due to the logic in createInitialMockStore. We already had old mock state (even when it is just a bunch of empty lists), so we return it as initialContext. It doesn't include supportReplies or supportTickets. When we have new values, we need the initialContext to be the emptyStore, which has the empty key-value pair for those new values.

🔧 Fix:
Adding a conditional check ensure the mockState and emptyStore keys are the same before returning that mockState. If the keys aren't the same, there's new entities to include in the mock store, so disregard the old store.

Changes 🔄

  • Update the conditional check in createInitialMockStore to return an emptyStore if there's new values to create in the store
  • This PR also fixes a seeder bug by adding a missing switch case to the removeSeeder function for consistency - it was an oversight in the original PR.

How to test 🧪

Prerequisites

(How to setup test environment)

Reproduction steps

  • Go to packages/manager/src/mocks/types.ts and make supportTickets and supportReplies optional types.
  • Go to packages/manager/src/mocks/mockState.ts and comment out supportReplies and supportTickets in the emptyStore.
  • On http://localhost:3000, open the browser dev tools > Application tab > Storage > clear site data
  • Open the Cloud Manager dev tools and toggle on the MSW
  • Observe the page crash and the console error
  • Observe that your IndexedDB is missing those supportReplies and supportTickets (browser dev tools > Application > Storage > IndexedDB > mockState

image (8)

Verification steps

(How to verify changes)

  • For the mock store bug:
    • Revert the code changes you made to reproduce the issue: git restore .
    • Refresh (🔁 ) your mockState in the browser dev console
    • Observe that the error is gone, Cloud Manager loads, and your IndexedDB now has empty lists for supportReplies and supportTickets
    • Create a mock support ticket
    • Confirm that your mock support ticket is visible in mockState
    • Turn off the MSW
    • Turn back on the MSW and confirm your previously created mock support ticket is still in mockState

Screenshot 2024-10-11 at 4 05 40 PM

  • For the seeder bug: Repeat the steps in the video below for "This Branch" and confirm that your seeded store updates to remove seeded data for support tickets if the checkbox is unchecked.
Video

Develop This Branch
Screen.Recording.2024-10-10.at.4.52.32.PM.mov
Screen.Recording.2024-10-10.at.4.54.20.PM.mov

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@mjac0bs mjac0bs added the Bug Fixes for regressions or bugs label Oct 10, 2024
@mjac0bs mjac0bs self-assigned this Oct 10, 2024
@mjac0bs mjac0bs marked this pull request as ready for review October 11, 2024 00:02
@mjac0bs mjac0bs requested a review from a team as a code owner October 11, 2024 00:02
@mjac0bs mjac0bs requested review from hana-akamai and cpathipa and removed request for a team October 11, 2024 00:02
@mjac0bs mjac0bs changed the title fix: [M3-8739] - Fix creation of MSW2.0 initial mock store fix: [M3-8739] - Fix MSW 2.0 initial mock store and support ticket seeder bugs Oct 11, 2024
Copy link

Coverage Report:
Base Coverage: 86.97%
Current Coverage: 86.97%

@mjac0bs mjac0bs marked this pull request as draft October 11, 2024 15:03

// Return the existing mockState if it includes all keys from the empty store;
// else, discard the existing mockState because we've introduced new values.
if (emptyStoreKeys.every((key) => mockStateKeys.includes(key))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix: Ensure that every empty store key is represented in mock state.

Note: I didn't compare the number of keys in the empty store and mock state, because mock state seems to always get an id key added that is not part of the empty store. (See screenshots in testing section to see it.) I am not quite sure the purpose of the id but am sure they're useful for something and maybe @abailly-akamai can clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mjac0bs it is how IndexedDB works - it's meant to behave like a DB to it has auto ID increment. We're not really using indexedDB the way it's supposed to, I just used this storage solution as a blob store

@mjac0bs mjac0bs marked this pull request as ready for review October 15, 2024 16:11
@mjac0bs mjac0bs force-pushed the M3-8739-fix-msw2-creation-of-initial-mock-store branch from 7fc4177 to 2580fdf Compare October 15, 2024 16:13
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed error is gone, CM loads, and indexedDB has initial empty list
image

Mock support ticket is visible in mockState
image

Support ticket seeded data is removed if checkbox is unchecked

Screen.Recording.2024-10-15.at.5.18.24.PM.mov

@hana-akamai hana-akamai added the Add'tl Approval Needed Waiting on another approval! label Oct 15, 2024
@cpathipa cpathipa added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Oct 17, 2024
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks @mjac0bs ! confirmed this fixes the error we were seeing

@mjac0bs mjac0bs merged commit 1d622f4 into linode:develop Oct 17, 2024
21 of 23 checks passed
Copy link

cypress bot commented Oct 17, 2024

Cloud Manager E2E    Run #6697

Run Properties:  status check passed Passed #6697  •  git commit 1d622f4c37: fix: [M3-8739] - Fix MSW 2.0 initial mock store and support ticket seeder bugs (...
Project Cloud Manager E2E
Run status status check passed Passed #6697
Run duration 45m 50s
Commit git commit 1d622f4c37: fix: [M3-8739] - Fix MSW 2.0 initial mock store and support ticket seeder bugs (...
Committer Mariah Jacobs
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Bug Fixes for regressions or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants