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

chore: Migrate default preference to mmkv #10373

Merged
merged 12 commits into from
Jul 29, 2024

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Jul 22, 2024

Description

  • Migration for migrate default preference data to mmkv
  • Refactor of the default preferences usage across the app
Before (Default Preferences)
{"@MetaMask:MetaMetricsId": "558de5e5-d382-4a9e-9f24-cce477116414", "@MetaMask:analyticsDataRecorded": "true", "@MetaMask:metricsOptIn": "agreed", "@MetaMask:onboardingWizard": "explored"}
After  (MMKV)
 MetaMetricsId 558de5e5-d382-4a9e-9f24-cce477116414
 analyticsDataRecorded true
 metricsOptIn agreed
 onboardingWizard explored

Related issues

Fixes:

Manual testing steps

  1. install the app
  2. login
  3. close onboarding flow
  4. update the app with the code on this PR
  5. the onboarding flow shouldn't appear

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@tommasini tommasini changed the title chore: migration to migrate default preference to mmkv chore: Migrate default preference to mmkv Jul 23, 2024
@tommasini tommasini added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 23, 2024
@tommasini tommasini marked this pull request as ready for review July 23, 2024 18:19
@tommasini tommasini requested review from a team as code owners July 23, 2024 18:19
Copy link
Contributor

github-actions bot commented Jul 23, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 66269bd
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e90e6322-f87d-4e4c-8c66-a46072ec438b

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 23, 2024
Copy link
Contributor

github-actions bot commented Jul 23, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 5d51964
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/03f78720-3805-49d5-849c-b6f8af50379c

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@MarioAslau MarioAslau self-requested a review July 23, 2024 22:19
@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 23, 2024
andreahaku
andreahaku previously approved these changes Jul 25, 2024
Copy link
Contributor

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

Nice job on this, a lot of places to change indeed.
I commented some issue on the migration unit test as some parts are not testing anything to me (I can be wrong though) and suggested idea to make your migration coverage stronger.

Also I wonder if you could make something to deprecate the DefaultPreferences until we get rid of the dependency. Otherwise we get the risk that it's used again somewhere.
Thanks!

app/store/migrations/050.test.ts Outdated Show resolved Hide resolved
app/store/migrations/050.ts Outdated Show resolved Hide resolved
app/store/migrations/050.test.ts Show resolved Hide resolved
app/store/migrations/index.test.ts Show resolved Hide resolved
Copy link

Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Great work mate!

@tommasini tommasini added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 26, 2024
Copy link
Contributor

github-actions bot commented Jul 26, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 0d58b7b
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/41f14fc9-7307-4d36-9556-086b23193b6f

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@tommasini tommasini merged commit ee371b2 into main Jul 29, 2024
38 checks passed
@tommasini tommasini deleted the chore/migrate-default-preference-to-mmkv branch July 29, 2024 10:39
@github-actions github-actions bot locked and limited conversation to collaborators Jul 29, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 29, 2024
@metamaskbot metamaskbot added the release-7.29.0 Issue or pull request that will be included in release 7.29.0 label Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.29.0 Issue or pull request that will be included in release 7.29.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants