Skip to content

fix: Delete migration 115#24666

Merged
danjm merged 8 commits intodevelopfrom
delete-migration-115
May 21, 2024
Merged

fix: Delete migration 115#24666
danjm merged 8 commits intodevelopfrom
delete-migration-115

Conversation

@danjm
Copy link
Copy Markdown
Contributor

@danjm danjm commented May 21, 2024

Description

#24369 reverted the commit that introduced migration 115. Instead of deleting 115, it modified it. The problem with this is:

  • the migration now does nothing
  • the migration can't just be updated in the future, because after it runs once for a user, it will never be run again

What we need to do is delete migration 115 and then rename the subsequent migrations to one number lower (and update their contents to reflect these new migration numbers. Then, when the reverted changes are restored, the migration can be re-added as the new highest numbered migration.

This PR makes the changes needed to delete migration 115 over multiple commits, for ease of review. Reviewing this PR is really easy if you review commit by commit (but Github can't easily display the simplicity of the changes when they are all together). Below I have briefly described each commit and included a screenshot of what you will see if you click the link to look at that commit directly

e7d4cb5 Deletes the existing migration 115 files
Screenshot from 2024-05-21 05-03-03

8d78df3 Renames the existing migration 116 files to migration 115 files
Screenshot from 2024-05-21 05-03-23

3023d17 Renames the existing migration 117 files to migration 116 files
Screenshot from 2024-05-21 05-03-39

23f121b Renames the existing migration 118 files to migration 117 files
Screenshot from 2024-05-21 05-04-15

f6cbb92 Renames the existing migration 119 files to migration 118 files
Screenshot from 2024-05-21 05-04-28

f40afd7 Modifies the renamed files from the previous commits, so that any number within those files in the range 115-119 is replaced by the number one below.
Screenshot from 2024-05-21 05-05-29

088ecd5 Just deletes 1 line from app/scripts/migrations/index.js

Open in GitHub Codespaces

Manual testing steps

  1. Create a local build on the master branch, install and onboard
  2. Create a build on this branch, install
  3. In the background console, you should see logs that show migrations 115 to 118 running successfully

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@danjm danjm requested a review from a team as a code owner May 21, 2024 07:39
@github-actions
Copy link
Copy Markdown
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.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label May 21, 2024
@danjm danjm added the team-extension-platform Extension Platform team label May 21, 2024
@danjm danjm changed the title Delete migration 115 fix: Delete migration 115 May 21, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [adf5e25]
Page Load Metrics (1129 ± 570 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint581631023014
domContentLoaded8311363
load47302911291186570
domInteractive8311353
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -561 Bytes (-0.02%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@codecov
Copy link
Copy Markdown

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 92.45283% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 67.39%. Comparing base (640cad4) to head (adf5e25).
Report is 7 commits behind head on develop.

Files Patch % Lines
app/scripts/migrations/117.ts 90.91% 2 Missing ⚠️
app/scripts/migrations/118.ts 83.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24666      +/-   ##
===========================================
- Coverage    67.40%   67.39%   -0.01%     
===========================================
  Files         1289     1288       -1     
  Lines        50248    50240       -8     
  Branches     13011    13011              
===========================================
- Hits         33865    33857       -8     
  Misses       16383    16383              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danjm danjm merged commit c586d33 into develop May 21, 2024
@danjm danjm deleted the delete-migration-115 branch May 21, 2024 18:02
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2024
@gauthierpetetin gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

INVALID-PR-TEMPLATE PR's body doesn't match template release-12.0.0 Issue or pull request that will be included in release 12.0.0 team-extension-platform Extension Platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants