Add import/export functionality #295

torkelrogstad · 2020-02-11T18:34:46Z

In this PR we add import/export functionality.

Working:

Add dropdown to network (where "rename" and "delete" are) for exporting. This zips a JSON file with the current network and prompts the user for where to store the zip
Add new "import network" page where user can select a zip file, and import this

Todo:

Remove user name from stringified network in zip?
Tests
Docs
indicator while we're importing/exporting networks
Adding newly imported networks to chart and persisting this to disk
modal saying we successfully imported network

jamaljsr · 2020-02-13T18:12:39Z

@torkelrogstad I won't do a full review because you marked it as a draft, but I have skimmed through the code and ran it. Overall, it works as advertised, which is awesome. Thanks for working on this feature and let me know when you feel ready for me to do a thorough review.

torkelrogstad · 2020-02-17T15:32:06Z

This PR is now in a reviewable state, @jamaljsr. I've added some tests where I mimicked existing tests. The hairy parts of this PR (the zipping/unzipping/networks.json tweaking) is not tested, though... LMK what you think is important to test here. I'd also appreciate some pointers on how to approach that, w.r.t. best practices around tests that require real file system IO.

I also didn't tackle the first item on my checkbox from above, removing the user name of the user that exported the network. This would take some effort, and doesn't seem to be terribly important.

Here's an exported network you can pull down and import: polar-foo.zip

jamaljsr · 2020-02-17T17:51:09Z

Hey @torkelrogstad, I will give the code a thorough review by tomorrow.

Regarding testing, the simplest way to determine what tests to add is to look at the code coverage report. If you run yarn test:ci it will output a list of all of the source files along with the line numbers which are not covered by tests. After that, you can also open the /coverage/lcov-report/index.html file in a browser to see a more visual view of what code has no coverage. This tells you exactly where you should add more tests to cover all possible code paths. I do have coverage reports posting to PRs (ex: 282) but unfortunately this feature doesn't work yet with GitHub Actions on PRs from forked repos.

Using coverage as a guide also helps you organize your code better. For example, I see that you have a fair amount of logic written in the NetworkView component and button click handlers. It would be challenging to write tests for these since the testing framework has to interact with the UI. It is a better practice to keep your components as small as possible and have util functions for the heavy lifting. The only logic in these files should be to determine what to render on the screen, with little to none of the plumbing necessary to perform an action. This also allows you to reuse that functionality in other components, which keeps your codebase DRY. Since this code would be difficult to test, you would be encouraged to move it to a separate file. Then you'd have a lot more flexibility to pass in arguments to the function to induce errors and handle edge cases.

I know testing feels like a pain upfront, but I can tell you from experience that it leads to a much more developer friendly codebase in the future after years of changes.

jamaljsr · 2020-02-18T22:25:52Z

I have started reviewing this PR by trying to break it in as many ways as I can think of. Here are some issues I have found by testing this feature as a user.

The polar-foo.zip file that you posted in your comment appears to be corrupt. I receive an error when I try to import it as well as when I try to extract it in Finder.

Could not unzip /Users/jamal/Downloads/polar-foo.zip into /var/folders/zq/b69vg6b50x3gxlqfxtm6vybw0000gn/T/polar-foo: Error: invalid distance too far back
 at Zlib.zlibOnError [as onerror] (zlib.js:170)

I created a network but did not start it. I then exported it, deleted it, then tried to import it. I received this error in the console.

Uncaught (in promise) Error: ENOENT: no such file or directory, stat '/var/folders/zq/b69vg6b50x3gxlqfxtm6vybw0000gn/T/polar-import/volumes'

I created a network, started it, then added some channels and made payments. While the network is running, I exported it. I then stopped it, deleted it, and imported it back in. Polar believes that the network is still running but it cannot connect to the nodes. Obviously the docker containers are not running for a newly imported network. I then stopped the network, and started it up again. The bitcoin node came online but the LND & c-lightning nodes did not. My high-level assumption is that the node's data is now corrupt because Polar zipped up the folders while the nodes were running. They didn't gracefully shutdown properly. I suggest a simple fix of not allowing the export of running networks. Just show an error notification if the user clicks Export while the network is running.
I exported a network with a c-lightning node from a Mac and transferred the zip file to my Windows machine. I was able to import the network and start it but the c-lightning node failed to start. This is because our c-lightning docker containers are not supported on Windows yet. We should prevent importing networks with c-lightning nodes on Windows.

I also have some cosmetic suggestions to improve the UI

On the Get Started screen, I don't think it is necessary to have the Import button in the center. This screen is only shown to first-time users. I think it is highly unlikely that importing a network would be a common use case for folks just starting, so it doesn't need to be this prominent in the UI. There is also the Import link in the top nav that is easily visible for those that need it.
We should add an icon to the "Import Network" link in the top navbar. It looks a bit strange without one. I would suggest <ImportOutlined /> from antd.
On the Import screen:
- I think it would look better if the Card with the dropzone filled the full height of the screen. This should be doable with a bit of flexbox CSS.
- since the user will only ever upload one file, I don't think it's necessary to have them select a file then click on the Import button. The import process should immediately initiate after the file is dropped or selected. This would remove the need to show the Remove and Import buttons.
- there should be some sort of loader displayed while the network it being imported. It can take a few seconds and when I did it, I wasn't sure if anything was happening until the success modal popped up.

This is all the feedback I have from a user's perspective. I will go ahead and dive into the code now and provide feedback on that separately.

jamaljsr

Here's my feedback on the code for this feature. This is a really good start. I tried to be as helpful as possible in providing guidance on how the code can be improved. If there is any confusion or disagreement, please feel free to let me know.

I also saw some spelling errors in comments which I did not mention, I'll save those for a later review. If you use VS Code as your editor, I highly recommend the Code Spell Checker plugin. It's helped me a lot. Also, the TypeScript Import Sorter plugin does a really good job at keeping import statements consistent with the rules I defined in the settings.json file.

package.json

src/components/common/NavMenu.tsx

src/components/home/GetStarted.tsx

src/components/network/ImportNetwork.tsx

src/utils/files.ts

src/components/network/NetworkView.tsx

src/components/network/ImportNetwork.tsx

torkelrogstad · 2020-02-24T09:42:48Z

e2e/Import.e2e.ts

+  await t.expect(getPageUrl()).match(/network_import/);
+});
+
+test('when the user aborts the file dialog, nothing should happen', async t => {


I'm having issues with this test. I can't get Testcafe to trigger the file dialog... Any idea what I'm doing wrong?

I did a bit of playing around with this and I think I see the problem. The antd Dragger component uses the native HTML5 input[type=file] element to handle the file selection. This means that the electron dialog.showOpenDialog function is not being used, so the setElectronDialogHandler handler will never be called in the test. It looks like Testcafe has a different setFilesToUpload API to trigger file selection for inputs. You can give that a try. Or if you want to take a different approach, you could create an upload component that uses dialog.showOpenDialog instead of the antd Dragger with an input[type=file] element. I do not have a suggestion on which one is better/easier than the other.

src/store/models/network.spec.ts

torkelrogstad · 2020-02-24T09:50:23Z

src/utils/zip.ts

+
+    // finished
+    archive.on('finish', () => resolve());
+    archive.on('error', err => {


I'm having some issues with testing the error and warning branches here. How would I solve this with a mock?

This was a bit tricky because the archiver package exposes its main function as the default export. After a little tinkering, I've gotten a couple tests passing which simulate the error and warning events.

The jest.mock('archiver, ...);` code should be placed in a global mock file because it should be mocked in all tests. I just put it all in one file for easier pasting.

I mocked fs in these tests because your code used that module. I suggest switching to fs-extra which already has a global mock in __mocks__. It would just need to be updated with any missing functions declared there, like createWriteStream.

import fs from 'fs'; import * as archiver from 'archiver'; import { PassThrough } from 'stream'; import { zip } from './zip'; jest.mock('fs'); jest.mock('archiver', () => { // TODO: this code should live in __mocks__/archiver.js // eslint-disable-next-line @typescript-eslint/no-var-requires const { PassThrough } = require('stream'); let mockStream: any; // return a fake stream when "archiver()" is called in the app const ctor = function() { mockStream = new PassThrough(); mockStream.file = jest.fn(); mockStream.directory = jest.fn(); mockStream.append = jest.fn(); mockStream.finalize = jest.fn(); return mockStream; }; // attach a func to emit events on the stream from the tests ctor.mockEmit = (event: string, data: any) => mockStream.emit(event, data); return ctor; }); const fsMock = fs as jest.Mocked<typeof fs>; const archiverMock = archiver as jest.Mocked<any>; it('should fail if there is an archiver error', async () => { fsMock.createWriteStream.mockReturnValueOnce(new PassThrough() as any); const promise = zip({ destination: 'path-to-zip', objects: [], paths: [], }); // emit an error after a small delay const mockError = new Error('test-error'); setTimeout(() => { archiverMock.mockEmit('error', mockError); }, 100); await expect(promise).rejects.toEqual(mockError); }); it('should fail if there is an archiver warning', async () => { fsMock.createWriteStream.mockReturnValueOnce(new PassThrough() as any); const promise = zip({ destination: 'path-to-zip', objects: [], paths: [], }); // emit an error after a small delay const mockError = new Error('test-warning'); setTimeout(() => { archiverMock.mockEmit('warning', mockError); }, 100); await expect(promise).rejects.toEqual(mockError); });

torkelrogstad · 2020-02-24T09:52:10Z

src/components/network/ImportNetwork.tsx

+const ImportNetwork: React.FC<RouteComponentProps> = () => {
+  const { navigateTo, notify } = useStoreActions(s => s.app);
+  const { importNetwork } = useStoreActions(s => s.network);
+  const { l } = usePrefixedTranslation('cmps.network.ImportNetwork');
+  const [importing, setImporting] = useState(false);
+
+  const doImportNetwork = (file: RcFile) => {
+    setImporting(true);
+
+    // we kick off the import promise, but don't wait for it
+    importNetwork(file.path)
+      .then(network => {
+        notify({ message: l('importSuccess', { name: network.name }) });
+        navigateTo(HOME);
+      })
+      .catch(error => {
+        notify({ message: l('importError', { file: file.name }), error });
+      })
+      .then(() => {
+        setImporting(false);
+      });
+
+    // return false to prevent the Upload.Dragger from sending the file somewhere
+    return false;
+  };


This function is not tested in unit tests. One solution here could be to create a hidden element on the page that has a onClick set to this, and then assert on that in tests. Any thoughts on that?

Maybe you could trigger the change event on the file input in order to execute this code. I'm not sure if that will work, but it's worth a try.

jamaljsr

This review took a lot longer than I expected because I was playing around with the tests. I've left a bunch of feedback on how this can be further improved. Please let me know if you have any questions of need assistance with anything.

jamaljsr · 2020-02-24T19:04:20Z

e2e/Import.e2e.ts

+  await t.expect(getPageUrl()).match(/network_import/);
+});
+
+test('when the user aborts the file dialog, nothing should happen', async t => {


I did a bit of playing around with this and I think I see the problem. The antd Dragger component uses the native HTML5 input[type=file] element to handle the file selection. This means that the electron dialog.showOpenDialog function is not being used, so the setElectronDialogHandler handler will never be called in the test. It looks like Testcafe has a different setFilesToUpload API to trigger file selection for inputs. You can give that a try. Or if you want to take a different approach, you could create an upload component that uses dialog.showOpenDialog instead of the antd Dragger with an input[type=file] element. I do not have a suggestion on which one is better/easier than the other.

src/store/models/network.spec.ts

jamaljsr · 2020-02-24T22:55:49Z

src/__mocks__/electron.js

 module.exports = {
  remote: {
    app: {
      getPath: p => `ELECTRON_PATH[${p}]`,
      getLocale: () => 'en-US',
    },
+    dialog: {
+      showSaveDialog: async () => ({


This mock would not be needed if you do not use electron.remote.dialog.showSaveDialog() in the app.

jamaljsr · 2020-02-25T02:22:57Z

src/components/network/ImportNetwork.tsx

+const ImportNetwork: React.FC<RouteComponentProps> = () => {
+  const { navigateTo, notify } = useStoreActions(s => s.app);
+  const { importNetwork } = useStoreActions(s => s.network);
+  const { l } = usePrefixedTranslation('cmps.network.ImportNetwork');
+  const [importing, setImporting] = useState(false);
+
+  const doImportNetwork = (file: RcFile) => {
+    setImporting(true);
+
+    // we kick off the import promise, but don't wait for it
+    importNetwork(file.path)
+      .then(network => {
+        notify({ message: l('importSuccess', { name: network.name }) });
+        navigateTo(HOME);
+      })
+      .catch(error => {
+        notify({ message: l('importError', { file: file.name }), error });
+      })
+      .then(() => {
+        setImporting(false);
+      });
+
+    // return false to prevent the Upload.Dragger from sending the file somewhere
+    return false;
+  };


Maybe you could trigger the change event on the file input in order to execute this code. I'm not sure if that will work, but it's worth a try.

src/components/network/ImportNetwork.tsx

src/utils/network.ts

src/utils/zip.ts

jamaljsr · 2020-02-25T06:12:12Z

src/utils/zip.ts

+
+    // finished
+    archive.on('finish', () => resolve());
+    archive.on('error', err => {


This was a bit tricky because the archiver package exposes its main function as the default export. After a little tinkering, I've gotten a couple tests passing which simulate the error and warning events.

The jest.mock('archiver, ...);` code should be placed in a global mock file because it should be mocked in all tests. I just put it all in one file for easier pasting.

I mocked fs in these tests because your code used that module. I suggest switching to fs-extra which already has a global mock in __mocks__. It would just need to be updated with any missing functions declared there, like createWriteStream.

import fs from 'fs'; import * as archiver from 'archiver'; import { PassThrough } from 'stream'; import { zip } from './zip'; jest.mock('fs'); jest.mock('archiver', () => { // TODO: this code should live in __mocks__/archiver.js // eslint-disable-next-line @typescript-eslint/no-var-requires const { PassThrough } = require('stream'); let mockStream: any; // return a fake stream when "archiver()" is called in the app const ctor = function() { mockStream = new PassThrough(); mockStream.file = jest.fn(); mockStream.directory = jest.fn(); mockStream.append = jest.fn(); mockStream.finalize = jest.fn(); return mockStream; }; // attach a func to emit events on the stream from the tests ctor.mockEmit = (event: string, data: any) => mockStream.emit(event, data); return ctor; }); const fsMock = fs as jest.Mocked<typeof fs>; const archiverMock = archiver as jest.Mocked<any>; it('should fail if there is an archiver error', async () => { fsMock.createWriteStream.mockReturnValueOnce(new PassThrough() as any); const promise = zip({ destination: 'path-to-zip', objects: [], paths: [], }); // emit an error after a small delay const mockError = new Error('test-error'); setTimeout(() => { archiverMock.mockEmit('error', mockError); }, 100); await expect(promise).rejects.toEqual(mockError); }); it('should fail if there is an archiver warning', async () => { fsMock.createWriteStream.mockReturnValueOnce(new PassThrough() as any); const promise = zip({ destination: 'path-to-zip', objects: [], paths: [], }); // emit an error after a small delay const mockError = new Error('test-warning'); setTimeout(() => { archiverMock.mockEmit('warning', mockError); }, 100); await expect(promise).rejects.toEqual(mockError); });

tsconfig.json

.eslintignore

jamaljsr · 2020-03-01T03:10:56Z

Hey @torkelrogstad are you still working on this PR? I want to package up a new release this coming week and would love to include this feature. If you don't have time to finish this up now, I am happy to work on it myself. I just don't want to take over if you plan to continue with it yourself.

torkelrogstad · 2020-03-03T13:25:31Z

@jamaljsr Sorry for being a bit off the grid for the last days. Hoping to finish it up today, working on addressing your latest round of review now.

jamaljsr · 2020-03-03T15:40:51Z

Ok great. Thanks for letting me know.

torkelrogstad · 2020-03-03T16:13:37Z

I pushed what I had time to do today. I've addressed some of your comments, but did not get around to all of them. If you want to get this feature wrapped up so you can do a release, feel free to push changes here.

jamaljsr · 2020-03-03T16:24:54Z

Ok, this looks pretty close to being ready. I'll finish it up in a day or two. Thanks for contributing what you have so far.

codecov-io · 2020-03-07T04:32:48Z

Codecov Report

Merging #295 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #295    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files         109    111     +2     
  Lines        2804   2954   +150     
  Branches      509    535    +26     
======================================
+ Hits         2804   2954   +150

Impacted Files	Coverage Δ
src/components/routing/Routes.tsx	`100% <ø> (ø)`	⬆️
src/components/network/NetworkActions.tsx	`100% <ø> (ø)`	⬆️
src/components/home/GetStarted.tsx	`100% <ø> (ø)`	⬆️
src/utils/zip.ts	`100% <100%> (ø)`
src/store/models/app.ts	`100% <100%> (ø)`	⬆️
src/components/home/NetworkCard.tsx	`100% <100%> (ø)`	⬆️
src/components/network/ImportNetwork.tsx	`100% <100%> (ø)`
src/utils/network.ts	`100% <100%> (ø)`	⬆️
src/components/common/NavMenu.tsx	`100% <100%> (ø)`	⬆️
src/components/network/NetworkView.tsx	`100% <100%> (ø)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75acb8f...1a6c872. Read the comment docs.

Add functionality to export and import networks and data from the application. On export we zip the docker-compose file, the data directories for the nodes and JSON representations of the chart and network definitions. This zip file can later be imported, where we do the reverse operation and copy the docker-compose file and the data directories, and insert the network and chart definitions into the networks.json file. fix 203

feat: add import/export network functionality Modularize import/export network and zip/unzip functionality. We also add some tests.

feat: add import/export network functionality Add thunk and (attempt at) test for importing network.

feat: add import/export network functionality

jamaljsr · 2020-03-10T09:13:10Z

@torkelrogstad I finished up this PR and will merge it. Thank so much for your work on this feature. 👍

torkelrogstad force-pushed the 2020-02-11-import-export branch from 0522785 to 1661f44 Compare February 17, 2020 15:28

torkelrogstad marked this pull request as ready for review February 17, 2020 15:28

torkelrogstad requested a review from jamaljsr February 17, 2020 15:28

torkelrogstad changed the title ~~Add import/export functionaliity~~ Add import/export functionality Feb 17, 2020

jamaljsr requested changes Feb 19, 2020

View reviewed changes

torkelrogstad force-pushed the 2020-02-11-import-export branch 2 times, most recently from 15b16dc to f1f8e38 Compare February 24, 2020 09:42

torkelrogstad commented Feb 24, 2020

View reviewed changes

torkelrogstad force-pushed the 2020-02-11-import-export branch 2 times, most recently from e24da98 to 5c481ae Compare February 24, 2020 09:45

torkelrogstad commented Feb 24, 2020

View reviewed changes

src/store/models/network.spec.ts Outdated Show resolved Hide resolved

torkelrogstad commented Feb 24, 2020

View reviewed changes

torkelrogstad force-pushed the 2020-02-11-import-export branch from 5c481ae to bbf2735 Compare February 24, 2020 11:56

jamaljsr requested changes Feb 25, 2020

View reviewed changes

jamaljsr force-pushed the master branch from c910874 to 8423ce7 Compare February 26, 2020 03:56

jamaljsr linked an issue Feb 27, 2020 that may be closed by this pull request

Feature request: Allow export and import of networks #203

Closed

jamaljsr force-pushed the master branch from d530ec1 to 2a5411d Compare February 29, 2020 07:31

torkelrogstad force-pushed the 2020-02-11-import-export branch 2 times, most recently from 35ba38c to bb7628d Compare March 3, 2020 16:13

jamaljsr self-requested a review March 7, 2020 03:28

jamaljsr force-pushed the 2020-02-11-import-export branch 2 times, most recently from c8de332 to e49ac58 Compare March 7, 2020 04:25

jamaljsr removed their request for review March 7, 2020 22:55

jamaljsr force-pushed the master branch from 9fd3ef4 to 4e81530 Compare March 9, 2020 20:53

jamaljsr force-pushed the 2020-02-11-import-export branch 2 times, most recently from 36b966b to d47760a Compare March 10, 2020 07:10

torkelrogstad and others added 10 commits March 10, 2020 03:52

Refactor and modularize export/import code

25588b8

feat: add import/export network functionality Modularize import/export network and zip/unzip functionality. We also add some tests.

Add thunk for importing network

a318b05

feat: add import/export network functionality Add thunk and (attempt at) test for importing network.

Add more tests for import network functionality

fbfbc31

feat: add import/export network functionality

Continue addressing code review and feedback

4baf7fe

test(export): fix export unit tests

4c9b106

chore(deps): fix broken yarn.lock file

52cec6e

feat(export): refactor and reduce import/export code

6650bab

test(import): update unit tests for the ImportNetwork component

f28da2f

test(export): add and update unit tests for import & export

1a6c872

jamaljsr force-pushed the 2020-02-11-import-export branch from d47760a to 1a6c872 Compare March 10, 2020 08:06

jamaljsr merged commit 15cd5fc into jamaljsr:master Mar 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add import/export functionality #295

Add import/export functionality #295

torkelrogstad commented Feb 11, 2020 •

edited

Loading

jamaljsr commented Feb 13, 2020

torkelrogstad commented Feb 17, 2020

jamaljsr commented Feb 17, 2020

jamaljsr commented Feb 18, 2020 •

edited

Loading

jamaljsr left a comment

torkelrogstad Feb 24, 2020

jamaljsr Feb 24, 2020 •

edited

Loading

torkelrogstad Feb 24, 2020

jamaljsr Feb 25, 2020 •

edited

Loading

torkelrogstad Feb 24, 2020

jamaljsr Feb 25, 2020

jamaljsr left a comment

jamaljsr Feb 24, 2020 •

edited

Loading

jamaljsr Feb 24, 2020

jamaljsr Feb 25, 2020

jamaljsr Feb 25, 2020 •

edited

Loading

jamaljsr commented Mar 1, 2020

torkelrogstad commented Mar 3, 2020

jamaljsr commented Mar 3, 2020

torkelrogstad commented Mar 3, 2020

jamaljsr commented Mar 3, 2020

codecov-io commented Mar 7, 2020 •

edited

Loading

jamaljsr commented Mar 10, 2020

Add import/export functionality #295

Add import/export functionality #295

Conversation

torkelrogstad commented Feb 11, 2020 • edited Loading

jamaljsr commented Feb 13, 2020

torkelrogstad commented Feb 17, 2020

jamaljsr commented Feb 17, 2020

jamaljsr commented Feb 18, 2020 • edited Loading

jamaljsr left a comment

Choose a reason for hiding this comment

torkelrogstad Feb 24, 2020

Choose a reason for hiding this comment

jamaljsr Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

torkelrogstad Feb 24, 2020

Choose a reason for hiding this comment

jamaljsr Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

torkelrogstad Feb 24, 2020

Choose a reason for hiding this comment

jamaljsr Feb 25, 2020

Choose a reason for hiding this comment

jamaljsr left a comment

Choose a reason for hiding this comment

jamaljsr Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

jamaljsr Feb 24, 2020

Choose a reason for hiding this comment

jamaljsr Feb 25, 2020

Choose a reason for hiding this comment

jamaljsr Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

jamaljsr commented Mar 1, 2020

torkelrogstad commented Mar 3, 2020

jamaljsr commented Mar 3, 2020

torkelrogstad commented Mar 3, 2020

jamaljsr commented Mar 3, 2020

codecov-io commented Mar 7, 2020 • edited Loading

Codecov Report

jamaljsr commented Mar 10, 2020

torkelrogstad commented Feb 11, 2020 •

edited

Loading

jamaljsr commented Feb 18, 2020 •

edited

Loading

jamaljsr Feb 24, 2020 •

edited

Loading

jamaljsr Feb 25, 2020 •

edited

Loading

jamaljsr Feb 24, 2020 •

edited

Loading

jamaljsr Feb 25, 2020 •

edited

Loading

codecov-io commented Mar 7, 2020 •

edited

Loading