Create an interface for output vendors #78

brafdlog · 2020-08-22T13:14:48Z

This is a basic implementation of the outputVendor interface that stays very high level, basically one main function of exportTransactions.

A few things I am considering and would like to get your input:

Some of the output vendor flow is common - filtering duplicate transactions, a function that formats the transaction to match what the output vendor expects. So, initially I thought we would have one common flow of exporting transactions to vendors and the output vendors would implement functions for each part of this process.
Implementing the output vendors as classes, may be cleaner.
Should the output vendor expose something for the ui of the config to work with? When we discussed this last we said that while we have only a few output vendors, we could have a ui component for managing the config of each vendor. If that is the case, is there a need to expose something related to the config from the output vendor?
Would be nice to have a unified type for the response of creating transactions for all the output vendors. Something that could be used for reporting the results. If you have any input there, would love to hear it.

baruchiro · 2020-08-23T17:18:11Z

Output vendors would implement functions for each part of this process: This is kind of good implementation, but what value do we get from this? I think we need to think about that after the merge, if any.
Implementing the output vendors as classes: yeah, especially I sew a call to ynab.init. we can save a state in the class. Usually, I don't like to use classes, but if we need to break the logic to multiple methods... it will be cleaner.
Should the output vendor expose something for the ui of the config to work with? as we discussed, I also tried to work on this interface, and I found that we need to expose "list of names and requirements" just for easily bind the name of the output vendor, for example. It is like the SCRAPERS object in israeli-bank-scrapers.
But I'm not sure if your current code is supporting this requirement or not, so I think you can keep the code, and during the work, we (@Arielgordon123 and I) will see exactly if we need something more.
a unified type for the response of creating transactions for all the output vendors: I don't familiar with the current implementation of the output vendors. I guess we can start with enum for "Pass" and "Fail", and expand as needed.

baruchiro · 2020-08-23T17:22:23Z

src/originalBudgetTrackingApp/commonTypes.ts

+  name: string;
+  init?: (config: Config) => Promise<void>;
+  exportTransactions: (transactionsToCreate: EnrichedTransaction[], startDate: Date, config: Config) => Promise<any>;
+  isActive: (config: Config) => boolean;


This is strange. I also wondered about that, and I realized that maybe we don't need to use isActive at all.
Think about that, we have a method that takes config and does scraping (and we have completely different module to save the config), so if you don't want to export to somewhere, you just need to send the config without it.

Now I'm thinking again. Maybe in the theory, we don't need that, but in real life, we will prefer to set enabled: false instead of removing it from config and saving it somewhere, just to test something.

So it should be a field in config, not method in the outputVendor.

You mean the export transactions would check the relevant flag in the config and skip if it is not enabled instead of having the check if active or not happen before calling exportTransactions?

Thats fine, the only drawback is that every output vendor needs to remember to check this flag

Another option is to have the config's of the output vendors extend an interface that will have the active boolean field, then the calling code can get for each output vendor it's config and check the active boolean.

Pushed the solution that takes it from the config but still in a generic way

src/originalBudgetTrackingApp/index.ts

baruchiro · 2020-08-23T17:27:18Z

src/originalBudgetTrackingApp/commonTypes.ts

+export interface OutputVendor {
+  name: string;
+  init?: (config: Config) => Promise<void>;
+  exportTransactions: (transactionsToCreate: EnrichedTransaction[], startDate: Date, config: Config) => Promise<any>;


What about getting a subset of the whole Config type?

Thats a good question. I considered making the OutputVendor generic and get the type of its own config. The problem there is that the calling code needs to know which config to fetch. This could be solved by assuming that the name of the output vendor will be the name of the output vendor config. To make that work I think we will need to make the output vendor name and enum instead of string. I will take a look

It works, but I will need to ts ignore the code that loops over the output vendors and calls export transactions because there the ts compiler is not able to know that we are passing the correct config.

Perhaps if output vendor is a class and would have a base class that has a getVendorConfig function that will return the config for that vendor (generically typed).

Will push a solution with ts ignore and when I do the class implementation will see if the solution with the base class works.

It's all good, but I'm afraid that I'm over-thinking. I think we can merge it, and I'm sure we will touch this code soon, so maybe the next time we will touch here we will find another solution.

In my head I always thinking about the israeli-bank-scrapers implementation- they have an object with the names and the fields, this is like your enum and config, and they have a factory to create the instance of outputVendor (in our case).
But I'm sure after we continue to work, if we did something wrong, we will find and fix it.

baruchiro · 2020-08-23T17:31:45Z

src/originalBudgetTrackingApp/index.ts

+  if (!Object.keys(executionResult).length) {
+    throw new Error('You need to set at least one output vendor to be active');


I didn't understand this Error, and now more.
You already did the (none) output, so what is the error?
You just need to warn, with console.warn for example.

I know, there is no meaning of scraping without exporting, because we don't save any state, but still, something feels wired to me. Maybe because I'm running the process without output, for test the scraping, and I know there are no outputs, so why it's error?

This error exists because when setting up the project you could easily forget to set any output vendor as active and then it would look as if all went well but actually nothing happened.

This type of check should be an indication in the ui instead of an error thrown. I am all for removing it, once we add an indication in the ui thats makes it clear that you have no active exporters defined.

Achla, this is kind of what I mean.

brafdlog · 2020-08-23T20:50:31Z

Agree.
Yes, will make them classes.
Lets discuss this on the phone next time we talk
Currently the response is just logged. The most basic is success/failure but I think it should be an object, not an enum, to allow extending it easily in the future without breaking the code that uses it.

Output vendors would implement functions for each part of this process: This is kind of good implementation, but what value do we get from this? I think we need to think about that after the merge, if any.

Implementing the output vendors as classes: yeah, especially I sew a call to ynab.init. we can save a state in the class. Usually, I don't like to use classes, but if we need to break the logic to multiple methods... it will be cleaner.

Should the output vendor expose something for the ui of the config to work with? as we discussed, I also tried to work on this interface, and I found that we need to expose "list of names and requirements" just for easily bind the name of the output vendor, for example. It is like the SCRAPERS object in israeli-bank-scrapers.
But I'm not sure if your current code is supporting this requirement or not, so I think you can keep the code, and during the work, we (@Arielgordon123 and I) will see exactly if we need something more.

a unified type for the response of creating transactions for all the output vendors: I don't familiar with the current implementation of the output vendors. I guess we can start with enum for "Pass" and "Fail", and expand as needed.

…very output vendor

baruchiro · 2020-08-24T04:30:34Z

src/originalBudgetTrackingApp/index.ts

@@ -108,7 +108,7 @@ async function createTransactionsInExternalVendors(config: Config, companyIdToTr

  for (let j = 0; j < outputVendors.length; j++) {
    const outputVendor = outputVendors[j];
-    if (outputVendor.isActive(config)) {
+    if (config.outputVendors[outputVendor.name]?.active) {


That's what I meant

baruchiro · 2020-08-24T04:41:35Z

Currently the response is just logged. The most basic is success/failure but I think it should be an object, not an enum, to allow extending it easily in the future without breaking the code that uses it.

So just create an object with enum, exportedTransactionsCount, and message (for error), and we will extend it if we want.

I'm approving this PR. If you want to merge without finish these comments, just remember to add them to #51 or something.

Create an interface for output vendors

db329d5

brafdlog requested a review from baruchiro August 22, 2020 13:14

brafdlog mentioned this pull request Aug 22, 2020

Add Json Exporter #77

Merged

baruchiro reviewed Aug 23, 2020

View reviewed changes

src/originalBudgetTrackingApp/index.ts Outdated Show resolved Hide resolved

baruchiro reviewed Aug 23, 2020

View reviewed changes

Use optional chaining instead of if for calling the init function safely

1b9d870

Use the active flag from config instead of an isActive function for e…

aab8de6

…very output vendor

baruchiro reviewed Aug 24, 2020

View reviewed changes

baruchiro approved these changes Aug 24, 2020

View reviewed changes

brafdlog merged commit fd79195 into unifyRepos Aug 25, 2020

brafdlog deleted the outputVendorsInterface branch August 25, 2020 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an interface for output vendors #78

Create an interface for output vendors #78

brafdlog commented Aug 22, 2020

baruchiro commented Aug 23, 2020 •

edited

Loading

baruchiro Aug 23, 2020

baruchiro Aug 23, 2020

brafdlog Aug 23, 2020

brafdlog Aug 23, 2020

brafdlog Aug 23, 2020

baruchiro Aug 23, 2020

brafdlog Aug 23, 2020

brafdlog Aug 23, 2020

baruchiro Aug 24, 2020

baruchiro Aug 23, 2020

brafdlog Aug 23, 2020

baruchiro Aug 24, 2020

brafdlog commented Aug 23, 2020 •

edited

Loading

baruchiro Aug 24, 2020

baruchiro commented Aug 24, 2020

		if (!Object.keys(executionResult).length) {
		throw new Error('You need to set at least one output vendor to be active');

Create an interface for output vendors #78

Create an interface for output vendors #78

Conversation

brafdlog commented Aug 22, 2020

baruchiro commented Aug 23, 2020 • edited Loading

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

brafdlog commented Aug 23, 2020 • edited Loading

Choose a reason for hiding this comment

baruchiro commented Aug 24, 2020

baruchiro commented Aug 23, 2020 •

edited

Loading

brafdlog commented Aug 23, 2020 •

edited

Loading