Backup Optic Tokens to a file to allow importing from another device #590

D-Pagey · 2022-07-08T09:16:10Z

Closes #537

Outstanding Design Questions:

if the user has secrets and imports, should that overwrite existing secrets? Merge secrets? (the current behaviour will first clean the existing secrets and then will import the backup)
if the user has secrets and imports, how do we inform them about the answer to 1.? (with the current behaviour an alert is shown when trying to import notifying the user that importing a backup will reset the existing tokens)

PR update 2022-07-12

Settings screen layout updated
Added toast messages for import and export operations
Added alert box when importing tokens: 'This will permanently remove the existing tokens, replacing them with the ones you are importing. Are you sure you want to continue?'
Added reset method in secretsManager
The export feature is now using two methods, one for Android (StorageAccessFramework) and one for iOS (expo-sharing)
Backup file name will contain the timestamp in the name, so that multiple files are created without any issues
If there are no secrets and you try to export, a toast will appear: 'There are no secrets to export'

Added working import export feature for both Android and iOS devices

Updated tests snapshots because of the UI changes in the SettingsScreen and added the reset function in the TokenScreen test

Added missing reset function in HomeScreen test

github-actions · 2022-07-12T08:36:37Z

This pull request was automatically deployed using Expo GitHub Actions!

Project: @nearform/optic-expo
Channel: pr-590

simoneb · 2022-07-12T08:54:11Z

@davideroffo on the outstanding design question. If there are secrets when trying to import, we should warn the user that they will be wiped away and replaced. An alert would do.

davideroffo · 2022-07-12T08:55:34Z

@simoneb I already added that alert, you can see it described in one of the bullet points in the PR description:

Added alert box when importing tokens: 'This will permanently remove the existing tokens, replacing them with the ones you are importing. Are you sure you want to continue?'

simoneb · 2022-07-12T08:58:47Z

@simoneb I already added that alert, you can see it described in one of the bullet points in the PR description:

Added alert box when importing tokens: 'This will permanently remove the existing tokens, replacing them with the ones you are importing. Are you sure you want to continue?'

Then we're good 🆗

simoneb

Overall LGTM, please see inline comments

src/screens/SettingsScreen.tsx

Changes due to the PR comments

Added .txt file extension to optic-backup

simoneb · 2022-07-12T11:56:54Z

src/lib/settings.ts

+    const parsedSecrets = JSON.parse(result) as Secret[]
+    return parsedSecrets
+  } catch (err) {
+    console.log(err)


we're not handling the error. if there is an error, the user won't know

simoneb · 2022-07-12T11:57:25Z

src/screens/SettingsScreen.tsx

+    if (Platform.OS === 'android') {
+      androidExport(fileName, fileContent)
+    } else {
+      iosExport(fileName, fileContent)
+    }


why don't we move this logic inside the export utils?

simoneb · 2022-07-12T11:58:20Z

src/screens/SettingsScreen.tsx

+    try {
+      const documentMeta = await DocumentPicker.getDocumentAsync()
+
+      if (documentMeta.type === 'success') {


why don't we invert this logic so we reduce the nesting and early return in case un error/cancel?

simoneb · 2022-07-12T11:59:12Z

src/screens/SettingsScreen.tsx

+    }
+  }
+
+  const showImportConfirmAlert = (onConfirm: () => void) => {


this function doesn't need to live inside the component body

moved in the settings.ts file

simoneb · 2022-07-12T11:59:47Z

src/screens/SettingsScreen.tsx

+
+        await reset()
+        for (const secret of parsedSecrets) {
+          await add(secret)


isn't there a quicker way to bulk insert secrets?

I've added a new addAll() method in the Secrets Context for adding all the secrets in one shot

Improving the pull request addressing some of the PR comments

Removed commented code and unuseful check on the document type different from success. Document type can be only success or cancel.

simoneb · 2022-07-12T12:54:52Z

src/screens/SettingsScreen.tsx

+      await reset()
+      await addAll(parsedSecrets)


do we need to call these two functions? isn't addAll actually replacing the secrets? in which case, maybe a better name is needed?

I removed reset() and I renamed addAll() to replace()

simoneb · 2022-07-12T12:56:12Z

src/screens/SettingsScreen.tsx

+  const { secrets, reset, addAll } = useSecrets()
+
+  const handleExport = async () => {
+    if (secrets.length === 0) {


let's disable the export button if there are no secrets to export

simoneb · 2022-07-12T12:57:25Z

src/lib/settings.ts

+  if (Platform.OS === 'android') {
+    androidExport(fileName, fileContent)
+  } else {
+    iosExport(fileName, fileContent)
+  }


these functions return promises, we should return the resulting promise back to the caller

simoneb · 2022-07-12T13:00:19Z

src/lib/settings.ts

+
+const mimeType = 'application/octet-stream'
+
+const androidExport = async (fileName, fileContent) => {


this function and the one for ios have duplicated error handling. plus, this is somewhat inconsistent with the way we handle errors in the import function. we should identify a single error handling approach and apply it throughout. I would leave the error handling (and showing error messages to the user) to the caller code, meaning the React component.

if you look at the import function, you will also realize there is redundancy because error handling is done both in the function itself AND in the component which calls the function.

I do realize that there are some edge cases (things which are simpler to handle within the function and show a toast when they happen). But apart from those edge cases, let's make sure that exceptions are raised back to the caller and handle them only there.

I simplified it a bit by throwing errors with custom messages in the functions. In this way, we are showing the toasts just in the component as you suggested.
I did the same with the edge case in the import function (getSecretsFromFile())
Let me know if now looks good.

Fixed PR comments: error handling only in the components, removed reset and addAll used replace instead, minor other changes

simoneb

Please make sure the deploy preview is tested thoroughly before merging. Test especially:

both ios and android
import and export work fine

D-Pagey self-assigned this Jul 8, 2022

D-Pagey force-pushed the feature/backup-import-tokens branch from dbced36 to 1fe478f Compare July 8, 2022 15:49

feat(backup): handle exporting/importing secrets from Settings

26349af

D-Pagey force-pushed the feature/backup-import-tokens branch from 1fe478f to 26349af Compare July 8, 2022 15:51

D-Pagey and others added 5 commits July 8, 2022 17:42

feat(import): rehydrate secrets on import

063485b

add working import export feature

05b6a19

Added working import export feature for both Android and iOS devices

add check on secrets length

1152a42

update tests

649bae6

Updated tests snapshots because of the UI changes in the SettingsScreen and added the reset function in the TokenScreen test

update tests - add missing reset function

95324c8

Added missing reset function in HomeScreen test

davideroffo self-assigned this Jul 12, 2022

davideroffo marked this pull request as ready for review July 12, 2022 08:37

davideroffo requested a review from simoneb July 12, 2022 08:37

D-Pagey removed their assignment Jul 12, 2022

davideroffo mentioned this pull request Jul 12, 2022

Encrypt exported tokens file #591

Closed

3 tasks

simoneb reviewed Jul 12, 2022

View reviewed changes

src/screens/SettingsScreen.tsx Outdated Show resolved Hide resolved

src/screens/SettingsScreen.tsx Outdated Show resolved Hide resolved

src/screens/SettingsScreen.tsx Outdated Show resolved Hide resolved

src/screens/SettingsScreen.tsx Outdated Show resolved Hide resolved

pr comments changes

ccc2713

Changes due to the PR comments

davideroffo requested a review from simoneb July 12, 2022 10:53

add .txt file extension

aa5c97d

Added .txt file extension to optic-backup

simoneb requested changes Jul 12, 2022

View reviewed changes

davideroffo added 2 commits July 12, 2022 14:25

fix PR comments

e667fbd

Improving the pull request addressing some of the PR comments

remove commented code and unuseful check

fca4e48

Removed commented code and unuseful check on the document type different from success. Document type can be only success or cancel.

davideroffo requested a review from simoneb July 12, 2022 12:50

simoneb requested changes Jul 12, 2022

View reviewed changes

fix PR comments

350bbef

Fixed PR comments: error handling only in the components, removed reset and addAll used replace instead, minor other changes

davideroffo requested a review from simoneb July 12, 2022 13:41

rename settings.ts to importExport.ts

51e2561

rename settings.ts to importExport.ts

3540fb0

simoneb approved these changes Jul 12, 2022

View reviewed changes

simoneb merged commit b6d8054 into master Jul 13, 2022

simoneb deleted the feature/backup-import-tokens branch July 13, 2022 16:42

lucascontreiro mentioned this pull request Jul 14, 2022

Unable to switch accounts after first login #596

Closed

This was referenced Sep 20, 2022

[OPTIC-RELEASE-AUTOMATION] release/v1.0.5 #714

Merged

[OPTIC-RELEASE-AUTOMATION] release/v1.0.6 #721

Closed

[OPTIC-RELEASE-AUTOMATION] release/v1.0.6 #723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backup Optic Tokens to a file to allow importing from another device #590

Backup Optic Tokens to a file to allow importing from another device #590

D-Pagey commented Jul 8, 2022 •

edited by davideroffo

Loading

github-actions bot commented Jul 12, 2022

simoneb commented Jul 12, 2022

davideroffo commented Jul 12, 2022 •

edited

Loading

simoneb commented Jul 12, 2022

simoneb left a comment

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb Jul 12, 2022

davideroffo Jul 12, 2022

simoneb left a comment


		const mimeType = 'application/octet-stream'

		const androidExport = async (fileName, fileContent) => {

Backup Optic Tokens to a file to allow importing from another device #590

Backup Optic Tokens to a file to allow importing from another device #590

Conversation

D-Pagey commented Jul 8, 2022 • edited by davideroffo Loading

Outstanding Design Questions:

PR update 2022-07-12

github-actions bot commented Jul 12, 2022

simoneb commented Jul 12, 2022

davideroffo commented Jul 12, 2022 • edited Loading

simoneb commented Jul 12, 2022

simoneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simoneb left a comment

Choose a reason for hiding this comment

D-Pagey commented Jul 8, 2022 •

edited by davideroffo

Loading

davideroffo commented Jul 12, 2022 •

edited

Loading