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.

Sign up for GitHub

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

Create an interface for output vendors #78

Merged
merged 3 commits into from
Aug 25, 2020
+52 −58
Merged

Create an interface for output vendors #78

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db329d5
Create an interface for output vendors
brafdlog Aug 22, 2020
1b9d870
Use optional chaining instead of if for calling the init function safely
brafdlog Aug 23, 2020
aab8de6
Use the active flag from config instead of an isActive function for e…
brafdlog Aug 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/originalBudgetTrackingApp/commonTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import { Transaction } from '@/originalBudgetTrackingApp/bankScraper';
import { Config } from './configManager/configManager';
import { Transaction } from './bankScraper';

export interface EnrichedTransaction extends Transaction {
accountNumber: string;
category?: string;
hash: string;
}

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

Choose a reason for hiding this comment

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

What about getting a subset of the whole Config type?

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

isActive: (config: Config) => boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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

}
50 changes: 13 additions & 37 deletions src/originalBudgetTrackingApp/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import * as bankScraper from './bankScraper';
import { ScaperScrapingResult, Transaction } from './bankScraper';
import * as ynab from './outputVendors/ynab/ynab';
import { EnrichedTransaction } from './commonTypes';
import * as googleSheets from './outputVendors/googleSheets/googleSheets';
import * as categoryCalculation from './categoryCalculationScript';
import * as configManager from './configManager/configManager';
import outputVendors from './outputVendors';
Expand Down Expand Up @@ -104,48 +103,25 @@ export function calculateTransactionHash({
}

async function createTransactionsInExternalVendors(config: Config, companyIdToTransactions: Record<string, EnrichedTransaction[]>, startDate: Date) {
await ynab.init(config);
const activeVendors: any = [];
if (config.outputVendors.ynab?.active) {
activeVendors.push({
name: 'ynab',
createTransactionFunction: ynab.createTransactions,
options: config.outputVendors.ynab.options,
});
}
if (config.outputVendors.googleSheets?.active) {
activeVendors.push({
name: 'googleSheets',
createTransactionFunction: googleSheets.createTransactionsInGoogleSheets,
options: config.outputVendors.googleSheets.options,
});
}
const executionResult = {};
const allTransactions = _.flatten(Object.values(companyIdToTransactions));

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

for (let j = 0; j < activeVendors.length; j++) {
const vendor = activeVendors[j];
const vendorConfig = config.outputVendors[vendor.name];
if (vendorConfig && vendorConfig.active) {
const vendorResult = await createTransactionsInVedor(vendor, allTransactions, startDate);
executionResult[vendor.name] = vendorResult;
for (let j = 0; j < outputVendors.length; j++) {
const outputVendor = outputVendors[j];
if (outputVendor.isActive(config)) {
if (outputVendor.init) {
await outputVendor.init(config);
}
console.log(`Start creating transactions in ${outputVendor.name}`);
const vendorResult = await outputVendor.exportTransactions(allTransactions, startDate, config);
console.log(`Finished creating transactions in ${outputVendor.name}`);
executionResult[outputVendor.name] = vendorResult;
}
}
return executionResult;
}

async function createTransactionsInVedor(vendor, transactions: EnrichedTransaction[], startDate: Date) {
console.log(`Start creating transactions in ${vendor.name}`);
const vendorResult = await vendor.createTransactionFunction(transactions, startDate, vendor.options);
if (vendorResult) {
console.log(`${vendor.name} result: `, vendorResult);
if (!Object.keys(executionResult).length) {
throw new Error('You need to set at least one output vendor to be active');
Comment on lines +119 to +120
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Achla, this is kind of what I mean.

}
console.log(`Finished creating transactions in ${vendor.name}`);
return vendorResult;
return executionResult;
}

export async function getFinancialAccountNumbers() {
Expand Down
12 changes: 10 additions & 2 deletions src/originalBudgetTrackingApp/outputVendors/googleSheets/googleSheets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
import moment from 'moment/moment';
import { EnrichedTransaction, OutputVendor } from '@/originalBudgetTrackingApp/commonTypes';
import { Config } from '../../configManager/configManager';
import * as googleSheets from './googleSheetsInternalAPI';

const GOOGLE_SHEETS_DATE_FORMAT = 'DD/MM/YYYY';

export async function createTransactionsInGoogleSheets(transactions, startDate, { spreadsheetId, sheetName, credentialsFilePath }) {
export const googleSheetsOutputVendor: OutputVendor = {
name: 'googleSheets',
exportTransactions: createTransactionsInGoogleSheets,
isActive: (config) => !!config.outputVendors.googleSheets?.active
};

export async function createTransactionsInGoogleSheets(transactions: EnrichedTransaction[], startDate: Date, config: Config) {
const { spreadsheetId, sheetName, credentialsFilePath } = config.outputVendors.googleSheets!.options;
console.log(`Got ${transactions.length} transactions to create in google sheets`);
const hashesAlreadyExistingInGoogleSheets = await googleSheets.getExistingHashes({ spreadsheetId, sheetName, credentialsFilePath });
const transactionsToCreate = transactions.filter((transaction) => !hashesAlreadyExistingInGoogleSheets.includes(transaction.hash));
Expand All @@ -22,7 +31,6 @@ export async function createTransactionsInGoogleSheets(transactions, startDate,
transaction.description,
transaction.memo,
transaction.category,
transaction.companyId,
transaction.accountNumber,
transaction.hash
]);
Expand Down
13 changes: 0 additions & 13 deletions src/originalBudgetTrackingApp/outputVendors/index.js
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions src/originalBudgetTrackingApp/outputVendors/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { ynabOutputVendor } from './ynab/ynab';
import { googleSheetsOutputVendor } from './googleSheets/googleSheets';

const outputVendors = [ynabOutputVendor, googleSheetsOutputVendor];

export default outputVendors;
9 changes: 8 additions & 1 deletion src/originalBudgetTrackingApp/outputVendors/ynab/ynab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _ from 'lodash';
import * as ynab from 'ynab';
import moment from 'moment/moment';
import { EnrichedTransaction } from '@/originalBudgetTrackingApp/commonTypes';
import { EnrichedTransaction, OutputVendor } from '@/originalBudgetTrackingApp/commonTypes';
import { getConfig, Config, YnabConfig } from '../../configManager/configManager';

const INITIAL_YNAB_ACCESS_TOKEN = 'AABB';
Expand All @@ -23,6 +23,13 @@ let ynabConfig: YnabConfig | undefined;
let ynabAPI: ynab.API | undefined;
let ynabAccountDetails: YnabAccountDetails | undefined;

export const ynabOutputVendor: OutputVendor = {
name: 'ynab',
init,
exportTransactions: createTransactions,
isActive: (config) => !!config.outputVendors.ynab?.active
};

export async function init(config?: Config) {
if (ynabConfig && ynabAPI) {
console.log('Ynab already initialized, skipping');
Expand Down