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

[MOB-19513] Migration logic #968

Merged
merged 6 commits into from
Oct 2, 2023

Conversation

cdhoffmann
Copy link
Contributor

The migration logic for migrating data from UserDefaults to new datastore

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #968 (135f157) into migrateOffUserDefaults (fef2c3a) will increase coverage by 0.09%.
Report is 15 commits behind head on migrateOffUserDefaults.
The diff coverage is 98.31%.

@@                    Coverage Diff                     @@
##           migrateOffUserDefaults     #968      +/-   ##
==========================================================
+ Coverage                   89.99%   90.08%   +0.09%     
==========================================================
  Files                         131      133       +2     
  Lines                        8221     8279      +58     
==========================================================
+ Hits                         7398     7458      +60     
+ Misses                        823      821       -2     

}

private func needToMigrate() -> Bool {
let installDateKey = keyWithPrefix(datastoreName: constants.Lifecycle.DATASTORE_NAME, key: constants.Lifecycle.DataStoreKeys.INSTALL_DATE.rawValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not do the migration based on this key as it will be present only for customers using Lifecycle extension.
I can't think of a key to use as a basis for the migration as even our config keys won't be available when using bundled configuration. Can you run the migration logic with out the needToMigrate check as it should become a no-op after the first run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the pattern we use for the other migrators. Do you think we should add a key to our DataStore post migration that we can check later to avoid running through the loops on every launch?

Copy link
Contributor

@pfransenadb pfransenadb left a comment

Choose a reason for hiding this comment

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

Minor nit/changes requested around logging in migrator.


private func getAndDelete(key: String) -> Any? {
guard let value = defaults.object(forKey: key) else {
Log.debug(label: LOG_TAG, "Failed to get value to migrate from UserDefaults")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer to use language other than "failed" here -- as we fully expect a bunch of the keys to not exist (Depending on implementation). Can we change to a verbose log and change verbiage to "No value for (key) found."

Copy link
Contributor

@pfransenadb pfransenadb 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 👍

enum UserDefaultMigratorConstants {

static let MIGRATION_STORE_NAME = "com.adobe.migration"
static let MIGRATION_COMPLETE = "migrationComplete"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also change the value to also track the type of migration that was done? migration.userdefaults.complete

@cdhoffmann cdhoffmann merged commit b431e5a into adobe:migrateOffUserDefaults Oct 2, 2023
18 checks passed
@cdhoffmann cdhoffmann deleted the migration branch October 2, 2023 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants