Page MenuHomePhabricator

[Cashtab] Add parseInvalidSettingsForMigration function
AbandonedPublic

Authored by kieran709 on Jun 23 2022, 18:06.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Summary

Related to T2506. Function looks at the settings from local storage, determines if there is a missing paramater, then adds the default only that parameter before retesting isValidCashtabSettings.

Test Plan

Ensure that at least one setting is not default
in Ticker.js, create 'TestSetting' and set its value to be either true or false.
On save, refresh and check the application memory in devtools
Existing settings should maintain their user defined values, while the TestSetting should be set to its default value.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
parse-invalid-settings-for-migration
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19608
Build 38935: Build Diffcashtab-tests
Build 38934: arc lint + arc unit

Event Timeline

kieran709 retitled this revision from [Cashtab] Add parseInvalidSettingsForMigrationFunction to [Cashtab] Add parseInvalidSettingsForMigration function.Jun 23 2022, 18:08

Let me know if any of the inline comments are unclear. There are a number of issues to patch here.

web/cashtab/src/hooks/useWallet.js
1123 ↗(On Diff #34109)

What happens when we get to this part of the code?

This approach might happen to work, but note how this if loop does not follow the model of the rest of this ffunction. Everything else is setCashtabSettings(localSettings) followed by return currency.defaultSettings

You don't have a return call here, so this function will still get to line 1138 and overwrite all settings with the default settings.

You're also calling this async function without await (all localforage calls are async) -- though in this case, the error is that parseInvalidSettingsForMigration is using localforage...it should not be.

1137 ↗(On Diff #34109)

This is where you want to do your business. If the function gets here, it means the user has a settings object but they are not valid for some reason.

In this case, you want to parse and see why they are invalid. If they are invalid because a param is missing, then add the default of that param and return the now-modified settings object.

Then setCashtabSettings(yourModifiedLocalSettings) and return yourModifiedLocalSettings` to keep consistent with the rest of the function.

web/cashtab/src/utils/validation.js
5 ↗(On Diff #34109)

import not needed

144 ↗(On Diff #34109)

this function is only called on invalid settings. So, call the input invalidCashtabSettings

this function should not be async

146 ↗(On Diff #34109)

You don't need this line, just use currency.defaultSettings

148 ↗(On Diff #34109)

Before this, create a new settings object, migratedCashtabSettings = invalidCashtabSettings

151 ↗(On Diff #34109)

Here, you want to change migratedCashtabSettings

155 ↗(On Diff #34109)

It's not necessary for this function to touch localforage

Instead, return the now-correct migratedCashtabSettings

This revision now requires changes to proceed.Jun 23 2022, 22:57

Responding to review feedback

bytesofman added inline comments.
web/cashtab/src/hooks/useWallet.js
1137 ↗(On Diff #34201)

As mentioned in previous comments, localforage should not be in this function.

1138 ↗(On Diff #34201)

Might as well call isValidCashtabSettings(modifiedLocalSettings) just in case.

Return the default settings if invalid.

web/cashtab/src/utils/validation.js
144 ↗(On Diff #34201)

Comment no longer valid. Please update to reflect what is being done here now.

This revision now requires changes to proceed.Jun 30 2022, 21:58

Check isValidCasthabSettings(modifiedLocalSettings), updated comment in validation.js to reflect what is happening now.

bytesofman requested changes to this revision.Jul 7 2022, 21:20

Good catch that my previous comment about localforage reqs was mistaken.

  1. See inline comment
  1. Please add a comment to the loadCashtabSettings function before each return statement explaining why or why not the item is being placed in local storage.
web/cashtab/src/hooks/useWallet.js
1159 ↗(On Diff #34304)

We need an else block here to handle the case where parseInvaldSettings for whatever reason generated an invalid settings object. In this case, it can be the same as the previous block that handles an invalid settings object.

else {
// if not valid, also set cashtabSettings to default
        setCashtabSettings(currency.defaultSettings);
        return currency.defaultSettings;
}
This revision now requires changes to proceed.Jul 7 2022, 21:20

Responding to review feedback

bytesofman requested changes to this revision.Jul 8 2022, 16:36
bytesofman added inline comments.
web/cashtab/src/utils/validation.js
143 ↗(On Diff #34306)

Unit tests for this function

This revision now requires changes to proceed.Jul 8 2022, 16:36

Added unit tests to validation.test.js

bytesofman added inline comments.
web/cashtab/src/hooks/useWallet.js
1168

Replace comment

// Since this is returning default settings based on an error from reading storage, do not overwrite whatever is in storage
This revision now requires changes to proceed.Jul 11 2022, 22:45

Abandonning as arc diff attempts to commit diff to wrong revision.