Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP]add export Invoices and Contacts to CSV #143

Closed
wants to merge 13 commits into from

Conversation

AlphaStyle
Copy link
Contributor

I need tips on how to improve. Related to Issue 96.

I put the ipc listeners in select-export-directory.js. I should probably make a seperate file or rename the select-export-directory.js file.

Import does not work as of right now.
I made the export / import buttons at settings in general tab without styles.

This PR is not ready for merge!

@hql287
Copy link
Owner

hql287 commented Jan 12, 2018

@AlphaStyle Great to see you finally opened the PR 👍

I think it's better if you use another file to handle import and export explicitly. Then you can use any exported helpers from pouchDB to interact with the data.

@AlphaStyle
Copy link
Contributor Author

So app/helpers/export.js and app/helpers/import.js and then perhaps main/select-export-path.js and main/select-import-path.js?

Then two helper functions in pouchDB.js, for example dumpDB and loadDB or something?

Did I understand correctly ?

@hql287
Copy link
Owner

hql287 commented Jan 13, 2018

If import and export aren't long I would combine them into one file, maybe app/helpers/import-export.js.

Also, select-path can (should) be abstracted away as a single function to use in both import and export situations because the only thing that it needs to return is a string represents the path.

You may even try to refactor this already exist helper ./main/select-export-directory.js

@hql287 hql287 changed the title add export Invoices and Contacts to CSV [WIP]add export Invoices and Contacts to CSV Jan 15, 2018
@AlphaStyle
Copy link
Contributor Author

@hql287 Are the refactoring changes more what you had in mind? Just want some confirmation I am on the right track before I continue 😛

@hql287 hql287 closed this Jan 19, 2018
@hql287 hql287 reopened this Jan 19, 2018
@@ -79,6 +89,67 @@ runMigration(
}
);

// Get PouchDB Invoices (Used for Exporting)
const pouchDBInvoices = () => {
Copy link
Owner

@hql287 hql287 Jan 20, 2018

Choose a reason for hiding this comment

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

Why don't we use getAllDocs instead? 🧐

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like you need this to return different kind of doc than getAllDocs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once again I didn't do enough testing 😞 I tried to use it in the beginning and saw the end result being different, so I just assumed it would be wrong to use. But now that I can compare getAllDocs('invoices') and pouchDBInvoices I see the only difference is that I have to wrap getAllDocs with an array and that it is missing the _revisions object wich is not important.

I will use getAllDocs instead 🙂 And remove the extra pouchDBInvoices and pouchDBContacts helpers which will reduce dependencies!

Thank you!

};

// Get PouchDB Contacts (Used for Exporting)
const pouchDBContacts = () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above, why don't we use getAllDocs instead?

@hql287
Copy link
Owner

hql287 commented Jan 20, 2018

Is this working for you? I have this error on my machine

screen shot 2018-01-20 at 18 15 09

@AlphaStyle
Copy link
Contributor Author

Did you install the new dependencies yarn install ? That error occures when you are not able to dump the pouchDB, which needs the pouchdb-replication-stream decepndency.

But this will no longer be an issue because I will use getAllDocs instead which removes this dependency.

@AlphaStyle
Copy link
Contributor Author

@hql287 Could you check whether the new changes work for you?

You might notice the column names are a little funny, such as docs[].rows[].something, they were meant to make it possible to reverse the CSV into correct JSON with both Objects and Arrays. The [] in column name represent an Array and . (dott) represent an Object.

This is where the Import issues occur, because it doesnt parse name[].value as an Array with some value in it, it becomes an Object.

I will try and use CSV Parser and CSV Stringify instead of csvjson and see if I can get both Export and Import to work.

@hql287
Copy link
Owner

hql287 commented Jan 20, 2018

Did you install the new dependencies yarn install?

Yes, I did.

Could you check whether the new changes work for you?

Checking it now!

@hql287
Copy link
Owner

hql287 commented Jan 20, 2018

Alright, it's working now, good progress 👍

There're still a couple of issues need to be fixed though:

  • Non-readable column name. Users don't care if Manta needs it, they need to be able to read it.
  • Invoices & contacts data should be exported to 2 different files.
  • We should come up with a naming convention for backups. For example:
    manta_invoices_backup_${timestamp}.csv and manta_contacts_backup_${timestamp}.csv
  • UI of Export/Import is not matched with the current design.

Btw, here are some thoughts on the user flow, I think we can handle this in 2 ways (assuming we'll name the files).

Option 1:

  • The user clicks on the Export button
  • The user chooses a directory for export.
  • Manta automatically exports 2 files to the selected folder.

Done. This will take at least 2 clicks every time.

Option 2:

  • Make Backup Directory an option in settings, similar to PDF Export Directory.
  • The user only needs to do this once.
  • The user clicks on the Export button.
  • Manta automatically exports 2 files to the selected folder.
  • Send a system notification with click to revealthe backup.

I think options 2 is better:

  • Better UX (requires only 1 click to make a backup).
  • The export feature can be reused in other places, such as automatic-backup if we want to do it later as the selecting directory is now handled separately. This is separation of concern in action.

Anyway, I hope I don't sound too demanding 😉

@AlphaStyle
Copy link
Contributor Author

Anyway, I hope I don't sound too demanding 😉

@hql287 Not at all, this is exactly the type of guidance and challenges I need. You are teaching me the workflow of a project, how to communicate and share thought and how to make the app user friendly.

Invoices & contacts data should be exported to 2 different files.

Exporting 2 separate files will force the user to select 2 files for import, right?

CSV to JSON

Originally I wanted to use the node-csv libraries, but I dont fully understand them 😞 By the time I learn them and modify them to produce the wished result, I could within this time write my own little script that does exactly this and it will give me full flexibility to tweak everything. So it is exactly what I will try to do 🙂 This will reduce dependencies and hopfully this will fix Non-readable column name as well.

Options

I, myself prefer Option 1 because it give the user full control where to save and what to save it as. The Electron saveDialog does provide the option for defaultPath which we could save after first export?

But I agree Option 2 will play along much better with automatically backup. If we place Export and Import buttons next to the input field you choose export path the user will sort of get full control as I explained in Option 1.

@hql287
Copy link
Owner

hql287 commented Jan 23, 2018

@hql287 Not at all, this is exactly the type of guidance and challenges I need. You are teaching me the workflow of a project, how to communicate and share thought and how to make the app user friendly.

Aww, this is very sweet. Thanks! :)

Invoices & contacts data should be exported to 2 different files.

No, once you know where to export the data, just go ahead and export the 2 files

// Get the data
const invoicesData = ...
cont contactsData = ...

//Export the data as files
export (invoicesData, exportDir);
export (contactsData, exportDir);

@hql287
Copy link
Owner

hql287 commented Jan 23, 2018

I, myself prefer Option 1 because it give the user full control where to save and what to save it as.

True, but do they really need to though? I think the content of the file is much more important than its name and the users would only care about these 3 things:

  • How easy is it to export the data? By using option 2, we can make this 1-click action, can't get easier than that.
  • Where are the files? We provide a notification that user can click-to-reveal. This is also 1-click action.
  • The content of the files. (This still needs more work, though)

Also, I think you and I as developers both know the burden of naming things well. So by removing this cognitive load, I think our users would have a much better experience while using Manta 😉

@hql287
Copy link
Owner

hql287 commented Jan 25, 2018

Any update on this @AlphaStyle?

@AlphaStyle
Copy link
Contributor Author

@hql287 Sorry I haven't had much chance to work on this lately 😞

JSON to CSV

The JSON to CSV conversion script was harder than I anticipated 😛 You could test my latest push to see whether the column naming is alright or not?

Importing

Invoices & contacts data should be exported to 2 different files.

Exporting 2 separate files will force the user to select 2 files for import, right?

No, once you know where to export the data, just go ahead and export the 2 files

I was referring to Importing 😛 For importing the user would have to select one import for Contacts and an other one for Invoices, right?

Lets go for Option 2

I have some additional thoughts about option 2. What if we have one export directory in general for all uses? So the user would select one path where to save/export everything and then depending on what we save/export we just put it in it's appropriate sub directory such as exportdir/PDF, exportdir/backup, etc?

UI Styling

I'm going to need your help for the styling 😜

@hql287 hql287 mentioned this pull request Mar 2, 2018
5 tasks
@hql287
Copy link
Owner

hql287 commented Mar 7, 2018

Closed due to inactivity!

@hql287 hql287 closed this Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants