Adding a toggleable setting that adds a send confirmation modal when sending XEC or eTokens.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABCd3c77bdf01e9: [Cashtab] Send XEC/eToken confirmation modal w/ settings switch
npm start
Frontend
- navigate to settings and ensure the send confirmation switch is off by default
- switch the send confirmation toggle on, then navigate to the send page, then navigate back to settings and ensure the state persisted
- restart browser and ensure the switch selection still persisted due to local storage
- switch off the send confirmation setting
- send a normal XEC tx and ensure it is sent straight away
- send a normal eToken tx and ensure it is sent straight away
- send a multi recipient tx and ensure it is sent straight away
- send a normal XEC tx with an unencrypted message and ensure it is sent straight away
- send a normal XEC tx with an encrypted message and ensure it is sent straight away
- toggle on the send confirmation switch in settings
- send a normal XEC tx and ensure confirmation modal is displayed and the ok/cancel buttons working as expected
- send a normal eToken tx and ensure confirmation modal is displayed and the ok/cancel buttons working as expected
- send a multi recipient tx and ensure confirmation modal is displayed and the ok/cancel buttons working as expected
- send a normal XEC tx with an unencrypted message and ensure it is sent successfully after the confirmation modal
- send a normal XEC tx with an encrypted message and ensure it is sent successfully after the confirmation modal
- test with a fresh new wallet with no tx history and ensure the send confirmation toggle state persists
Web/Extension
- test browser compatibility between chrome and firefox, including validating persistence of switch state from restarting browser
- test on extension plugin, including validating switch state persistence after browser restart
Regression
- ensure that toggling of the send confirmation switch in Settings does not interfere with the local storage state of the Lock App switch. i.e. Toggle both switches to on, then turn off the confirmation modal switch and ensure the Lock App authenticator is still on
- ensure the Lock App and Send Confirmations switches are visually aligned since this is not visible on web/extension modes (authenticator will just say not supported)
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/cashtab/src/components/Send/Send.js | ||
|---|---|---|
| 142 ↗ | (On Diff #32432) | We want to preserve the current functionality where a confirmation modal is shown no matter what if txInfoFromUrl is true. Add this logic into the checkModalStatus function in Send.js Also, make the function name more descriptive as it will be different from the function in SendToken.js Something like checkForConfirmationBeforeSendXec |
| web/cashtab/src/components/Send/SendToken.js | ||
| 250 ↗ | (On Diff #32432) | make the function name more descriptive as it will be different from the function in Send.js Something like checkForConfirmationBeforeSendEtoken |
Comment Actions
- Updated checkForConfirmationBeforeSendXec in Send.js to show confirmation modal when txInfoFromUrl is true
- Updated function names across Send.js and SendToken.js to be more descriptive
Comment Actions
[The second comment below this one diagnoses the issue]
User story
- I run npm start and open the app in localstorage:3000. I have existing wallets here since I test at this URL often.
- I go to turn on the switch. The app won't turn on the switch. I click a few times. Eventually I get a react error.
- Now, if I start again, the switch works normally, test plan is good.
Same thing happens on the browser extension. There seems to be an issue here for migrating users trying to use this feature for the first time. May need to have an initial onLoad function that adds this value to settings, or some way of handling the first time the switch is flicked.
To test, can
- checkout master. add the line PORT=3001 to .env . Run npm start. Create some wallets. Send some txs.
- Now run D11071 with PORT=3001 -- is everything okay when you flick on the send confirm switch?
Comment Actions
Looking into this more, check out the loadCashtabSettings function introduced in this diff: https://reviews.bitcoinabc.org/D9829
Looks like it only adds fiat settings for wallets that never had settings before. This would work to migrate users from the case of no settings to "now we have settings." But the function will need to be modified to migrate users with a settings object that is missing some keys.
See also isValidCashtabSettings -- requires similar adjustments.
Comment Actions
- mitigated the edge case of a migrating wallet using this toggle for the first time by adjusting loadCashtabSettings and isValidCashtabSettings to check for the existance of the 'sendModal' setting even if the localSettings object exists. Tested all ok via the user story above.
- new and updated unit tests to reflect the updated validation logic in isValidCashtabSettings
| web/cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 913 ↗ | (On Diff #32568) | We want to avoid running into this issue again the next time we add a settings param. This check should be generalized. In this case, it's okay to only check for null here. That would mean that this part of the function is only checking for the case of no settings in storage. |
| 928 ↗ | (On Diff #32568) | ...and this part of the function is where we should be catching a missing param in an otherwise existing settings object. Alternatively, this function could be simplified so that isValidCashtabSettings also returns false for null. In this case though, it's okay to have the logic for "no settings in local storage" treated separately -- perhaps some future version of localforage or some future storage API we use may have a different failure mode for an object not in storage. |
| web/cashtab/src/utils/validation.js | ||
| 131 ↗ | (On Diff #32568) | This works, but we want to avoid running into this same issue every time we add a settings param. This validation function should be generalized to check against all the keys in currency.defaultSettings, rather then calling them out individually. |
Comment Actions
- Reverted the param-specific check in loadCashtabSettings and refactored isValidCashtabSettings to be a generalized validation check parsing through all setting params
- However that was still not enough to mitigate the migrating wallet toggling error, as the 'Object.keys(currentSettings).includes(key)' line in changeCashtabSettings will still fall over due to the missing key in currentSettings during migration.
- Mitigated this by adding a preceeding check to validate currentSettings via isValidCashtabSettings and if invalid, revert to defaultSettings with the requisite keys.
| web/cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 962–965 ↗ | (On Diff #32589) | Should be able to remove this if make the change below We don't want users to have their settings reset to default every time we add a new settings param. |
| 969 ↗ | (On Diff #32589) | The check on Object.keys(currentSettings).includes(key) can be removed from this block; it's enough to make sure that we are attempting to change to valid settings. |
Comment Actions
- removed Object.keys() check and tested all ok across browsers/iOS/Android and currency/modal toggle combinations
- removed settings param reset logic
- tested all ok on the previous migrating wallet scenario
| web/cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 969 ↗ | (On Diff #32611) | If we hit the case where currency.settingsValidation[key].includes(newValue) does not resolve as true, then newSettings is just an empty variable, and this will nuke the settings Add an else block to if (currency.settingsValidation[key].includes(newValue)) else {
// Set fiat price to null, which disables fiat sends throughout the app
setFiatPrice(null);
// Unlock the UI
setLoading(false);
return;
}This way, if validation does not pass, the function doesn't change anything. |