Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

fix: Add export data button to settings page #4114

Merged
merged 7 commits into from
Nov 9, 2022
Merged

fix: Add export data button to settings page #4114

merged 7 commits into from
Nov 9, 2022

Conversation

usame-algan
Copy link
Member

@usame-algan usame-algan commented Nov 8, 2022

What it solves

Part of safe-global/safe-wallet-web#1097

How this PR fixes it

  • Adds an export button to the settings page
  • Adds a new global route /export that contains the same button
  • Downloads a .json file containing added safes and address book entries
  • The .json file is versioned with version 1.0 and contains a data key with the localStorage data

How to test it

  1. Open the Safe app
  2. Navigate to Settings
  3. Observe a section with a Download button
  4. Click the button
  5. Observe a .json file being downloaded that contains the localStorage

Screenshots

Screenshot 2022-11-08 at 12 51 54

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

<Block className={classes.formContainer}>
<Heading tag="h2">Export your data</Heading>
<Paragraph>
Download your local storage data to keep your added safes and address book entries for later reimport.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Download your local storage data to keep your added safes and address book entries for later reimport.
Download your local storage data with your added safes and address book. You can import it on app.safe.global.

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Can't we just do a simple JSON.stringify(localStorage)?

@coveralls
Copy link

coveralls commented Nov 8, 2022

Pull Request Test Coverage Report for Build 3429390754

  • 1 of 15 (6.67%) changed or added relevant lines in 4 files are covered.
  • 8 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 41.869%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/export/Export.tsx 0 1 0.0%
src/routes/index.tsx 0 1 0.0%
src/routes/safe/components/Settings/DataExport/index.tsx 0 12 0.0%
Files with Coverage Reduction New Missed Lines %
src/components/PsaBanner/index.tsx 8 0%
Totals Coverage Status
Change from base Build 3408444922: 0.03%
Covered Lines: 4778
Relevant Lines: 10421

💛 - Coveralls

src/routes/safe/components/Settings/DataExport/index.tsx Outdated Show resolved Hide resolved
src/routes/safe/components/Settings/DataExport/index.tsx Outdated Show resolved Hide resolved
<Block className={classes.formContainer}>
<Heading tag="h2">Export your data</Heading>
<Paragraph>
Download your local storage data to keep your added safes and address book entries for later reimport.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Download your local storage data to keep your added safes and address book entries for later reimport.
Download your local data to keep your added Safes and address book entries for later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with Ivans suggestion as it also includes a reference to the new domain.

Copy link
Member

Choose a reason for hiding this comment

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

I would still remove "storage" to be less technical and capitalise "safe", e.g. "Download your local data with your added Safes and address book. You can import it on..."

@usame-algan
Copy link
Member Author

usame-algan commented Nov 8, 2022

Can't we just do a simple JSON.stringify(localStorage)?

I think it depends on where we want to parse the data. In the future we want to support export and import in web-core so we would have to parse the old format there as well. If we parse it here into the web-core format the implementation there could be more simple. Wdyt? Lets export everything so we don't have to release a new version if we decide to extend it. We also already have the parsing functions in web-core.

@iamacook
Copy link
Member

iamacook commented Nov 8, 2022

Can't we just do a simple JSON.stringify(localStorage)?

Although this is a simple approach, it will export a lot of unnecessary data and we'd have to parse it in web-core.

Lets export everything so we don't have to release a new version if we decide to extend it. We also already have the parsing functions in web-core.

The added Safes are stored differently in the localStorage in safe-react so we can't use the migration logic we have in place as well.

@katspaugh
Copy link
Member

katspaugh commented Nov 8, 2022

I think the users should be able to just take all their data out.

The added Safes are stored differently in the localStorage in safe-react so we can't use the migration logic we have in place as well.

Not sure what you mean. As Usame said, we do the same kind of migration via an iframe.

@iamacook
Copy link
Member

iamacook commented Nov 8, 2022

Not sure what you mean. As Usame said, we do the same kind of migration via an iframe.

This can be disregarded. The Redux store uses Immutable.js for the added Safes but it looks like there is separate initiation logic outside of our "standard" persistence implementation for added Safes (loadSafesFromStorage).

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

👍

@katspaugh
Copy link
Member

Something I realized while giving adhoc support on exporting: it’s not easy to link to a safe-specific route.
I suggest we additionally create an /export route for this feature. Similar to /welcome. The local data isn’t safe-specific.

@iamacook
Copy link
Member

iamacook commented Nov 9, 2022

Something I realized while giving adhoc support on exporting: it’s not easy to link to a safe-specific route. I suggest we additionally create an /export route for this feature. Similar to /welcome. The local data isn’t safe-specific.

An /import route could also be beneficial on web-core as it imports the entire app data.

@usame-algan usame-algan merged commit 1927709 into dev Nov 9, 2022
@usame-algan usame-algan deleted the export-data branch November 9, 2022 15:31
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants