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

Merge with the israeli-bank-scrapers-desktop repo #51

Closed
6 of 14 tasks
brafdlog opened this issue Jun 5, 2020 · 15 comments · Fixed by #50
Closed
6 of 14 tasks

Merge with the israeli-bank-scrapers-desktop repo #51

brafdlog opened this issue Jun 5, 2020 · 15 comments · Fixed by #50
Assignees

Comments

@brafdlog
Copy link
Owner

brafdlog commented Jun 5, 2020

Project for issues we want to handle pre merge: https://github.com/brafdlog/budget-tracking/projects/4

Related issues:

Post Merge Actions:

  • Decide how to release (by Tag or by pushing to master)
  • Copy relevant issues from israeli-bank-scrapers-desktop.
  • New repo/app name?
  • Check if there unused packages
  • Add MacOS to tests
  • Create Splash screen (until the renderer process loaded)
  • Set show browser in configManager? Define it for all importers?
  • Add ability to disable importer and exporter (and keep them configured)
@baruchiro
Copy link
Collaborator

Note that the folder nuxt contains the Github Pages, do you want to keep it?

I think the regular users need a beautiful webpage in Hebrew, with a big download button and help content.

But if you want to rethink that, maybe to use another technology...

@baruchiro
Copy link
Collaborator

About the file structure, I already started to reorg the project in baruchiro/israeli-bank-scrapers-desktop#247, but it is not important, since I think the most of my code will move to BudgetTrackingApp, or just gone, and the rest of the code is the components folder.

@brafdlog
Copy link
Owner Author

brafdlog commented Jun 5, 2020

Regarding the githup pages, you mean you wanted to use something else instead?

I am fine with removing it.

@brafdlog
Copy link
Owner Author

brafdlog commented Jun 5, 2020

About the file structure, I already started to reorg the project in baruchiro/israeli-bank-scrapers-desktop#247, but it is not important, since I think the most of my code will move to BudgetTrackingApp, or just gone, and the rest of the code is the components folder.

Ok, I updated the todo to be to remove the code that is no longer relevant when the ui starts using the budget tracking code.

@baruchiro
Copy link
Collaborator

Hi, I updated the mindMap. We need to define the interface of the output vendors.
Each output vendor has a configuration that saved by configManager, but it also has actions- the output action, of course, but also we need a declaration of which field it has to show them in the input UI.

@baruchiro
Copy link
Collaborator

About the Vue vuex-persist, I choose this way to save the state from the Vuex state manager to configManager, each time the state changes.

I think using the configManager itself in any place in the app is not a good way because in this way the config is not reactive.

The limitation of the vuex-persist way is that it act as a "side effect". I mean, if there is a problem when saving the state with configManager, it will affect the flow. In this case, you can use the app and you don't know that your changes are not saving in the file system.

@brafdlog
Copy link
Owner Author

I think that is ok as long as if there is an error in persisting the config we show the error. This is basically optimistic UI which is quite a common pattern - the ui works as if the backend update succeeded even before it knows if it did, and if there was an error it shows it when it happens.

@brafdlog
Copy link
Owner Author

If in case of an error we refetch the data from the configManager (and show an error) we will only have a short time of inconsistent state

@baruchiro
Copy link
Collaborator

I added Show error when vuex-persist failed, and re-fetch the config task to this issue, because as I said, saving the config by vuex-persist is not in the main flow- it working in the background, so we need to register a callback or something.

(I wrote it under TODOs, but you can drag it to the other sections, it's very cool 🎉)

@baruchiro
Copy link
Collaborator

@brafdlog I created a new branch called cleanIndex, but it turns to be a very large change, so I leaving it, but I'm sharing with you what I learned.

I want to remove all the code from the root index.ts, to get a clear list about what the backend exporting. I have three (or more) points:

  1. Move all the code from index.ts to separate files in a logical order. The export functionality should be in its file, the same with the import functionality, and the method that takes from import to export in another file.
    In this way, we can validate (I hope there is lint for that) the Vue access only to the @/originalBudgetTrackingApp, and the backend itself access only to the flow module, and this module uses its "internal" methods to import and export.
    With this structure you can understand half of the code only by its module.
  2. Inject sub-config to internal functions. The function should get only the relevant config, for example, the export function should get only the outputVendors part from the Config.
  3. Separate types from the code. I have no much knowledge about best practices with TypeScript types declaration, but I think if we have a module with a lot of types declarations and more than small function, we should declare the types in a separated file.

I'm sure you share my thoughts, but I just wanted to document what I did, because it came out too big so I decided to stop for now.

@brafdlog
Copy link
Owner Author

I general I agree. For these types of things I suggest opening and issue and linking it to here so it will be easier to follow.

Next week I will have more time and one of the things I plan is to organize the open issues in a clearer way so we have a clearer view about where things stand and what are the open issues.

@baruchiro
Copy link
Collaborator

My suggestion is to create a new "board" for "pre-merge" issues.

@brafdlog
Copy link
Owner Author

brafdlog commented Sep 7, 2020

@baruchiro I opened a project for the pre merge issues: https://github.com/brafdlog/budget-tracking/projects/4 and removed them from the description here.

Regarding the Todo's in the code, I suggest you open tickets for the ones you think we should handle and remove the ones we don't. We can sync on this if needed.

@brafdlog
Copy link
Owner Author

brafdlog commented Sep 7, 2020

@brafdlog I created a new branch called cleanIndex, but it turns to be a very large change, so I leaving it, but I'm sharing with you what I learned.

I want to remove all the code from the root index.ts, to get a clear list about what the backend exporting. I have three (or more) points:

  1. Move all the code from index.ts to separate files in a logical order. The export functionality should be in its file, the same with the import functionality, and the method that takes from import to export in another file.
    In this way, we can validate (I hope there is lint for that) the Vue access only to the @/originalBudgetTrackingApp, and the backend itself access only to the flow module, and this module uses its "internal" methods to import and export.
    With this structure you can understand half of the code only by its module.
  2. Inject sub-config to internal functions. The function should get only the relevant config, for example, the export function should get only the outputVendors part from the Config.
  3. Separate types from the code. I have no much knowledge about best practices with TypeScript types declaration, but I think if we have a module with a lot of types declarations and more than small function, we should declare the types in a separated file.

I'm sure you share my thoughts, but I just wanted to document what I did, because it came out too big so I decided to stop for now.

Opened issues for 1 and 2:
#89
#90

Regarding 3 its a matter of taste, we can discuss.

@brafdlog brafdlog pinned this issue Sep 15, 2020
@brafdlog
Copy link
Owner Author

@baruchiro

Make the node code expose an api as defined here

The current library code already exposes the config manager and the scrapeAndUpdateOutputVendors so I think we are good on that.

Update the electron UI flow to work with two main flows - configure, run scraping as defined here

We did implement those main flows except for the configuration of the category logic for which we have an issue - #31

So I am marking those two as done

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 a pull request may close this issue.

3 participants
@brafdlog @baruchiro and others