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
Merged

Create an interface for output vendors #78

Show file tree
Hide file tree
Changes from all commits
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
14 changes: 13 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,19 @@
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 enum OutputVendorName {
YNAB = 'ynab',
GOOGLE_SHEETS = 'googleSheets'
}

export interface OutputVendor {
name: OutputVendorName;
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.

}
8 changes: 5 additions & 3 deletions src/originalBudgetTrackingApp/configManager/configManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,19 @@ export interface Config {
};
}

export interface GoogleSheetsConfig {
export interface OutputVendorConfig {
active: boolean;
}

export interface GoogleSheetsConfig extends OutputVendorConfig {
options: {
credentialsFilePath: string;
sheetName: string;
spreadsheetId: string;
}
}

export interface YnabConfig {
active: boolean;
export interface YnabConfig extends OutputVendorConfig {
options: {
accessToken: string;
accountNumbersToYnabAccountIds: { [key: string]: string };
Expand Down
48 changes: 11 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,23 @@ 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 (config.outputVendors[outputVendor.name]?.active) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what I meant

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
11 changes: 9 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,17 @@
import moment from 'moment/moment';
import { EnrichedTransaction, OutputVendor, OutputVendorName } 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: OutputVendorName.GOOGLE_SHEETS,
exportTransactions: createTransactionsInGoogleSheets,
};

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 +30,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;
10 changes: 8 additions & 2 deletions src/originalBudgetTrackingApp/outputVendors/ynab/ynab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import _ from 'lodash';
import * as ynab from 'ynab';
import moment from 'moment/moment';
import { EnrichedTransaction } from '@/originalBudgetTrackingApp/commonTypes';
import { getConfig, Config, YnabConfig } from '../../configManager/configManager';
import { EnrichedTransaction, OutputVendor, OutputVendorName } from '@/originalBudgetTrackingApp/commonTypes';
import { Config, getConfig, YnabConfig } from '../../configManager/configManager';

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

export const ynabOutputVendor: OutputVendor = {
name: OutputVendorName.YNAB,
init,
exportTransactions: createTransactions
};

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