-
Notifications
You must be signed in to change notification settings - Fork 6
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
add prices and other improvements #11
base: master
Are you sure you want to change the base?
Conversation
@@ -9,7 +9,8 @@ | |||
], | |||
"scripts": { | |||
"build": "tsc", | |||
"test": "./node_modules/.bin/nyc ./node_modules/.bin/mocha -r ts-node/register test/*.ts" | |||
"auth": "./node_modules/.bin/ts-node ./test/http-server-oauth-authorize.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get OAuth2 token: npm run auth
. As far as I understand, now fortnox only allows OAuth2 for "new" apps
@@ -9,7 +9,8 @@ | |||
], | |||
"scripts": { | |||
"build": "tsc", | |||
"test": "./node_modules/.bin/nyc ./node_modules/.bin/mocha -r ts-node/register test/*.ts" | |||
"auth": "./node_modules/.bin/ts-node ./test/http-server-oauth-authorize.ts", | |||
"test": "./node_modules/.bin/nyc ./node_modules/.bin/mocha -r ts-node/register test/*.spec.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes .ts
=> .spec.ts
(now test folder contains also non-test helper files)
@@ -8,12 +8,10 @@ type ArticleResult = { | |||
|
|||
export class Articles { | |||
private dispatch: Dispatch; | |||
private util: Util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made Util use static methods (they are private anyway and not used outside). There seem to be no reason for extra allocations and repeating code.
@@ -22,10 +20,7 @@ export class Articles { | |||
} | |||
|
|||
async getAll(filter?: string) { | |||
let path = this.path; | |||
if (filter) | |||
path += '?filter=' + filter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unified all "filter" parameters (remove repeated code)
} | ||
|
||
throw await Util.makeErrorFromResponse(response) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"exponential" exception handling for "too many requests".
} | ||
|
||
async post(path: string, body: any) { | ||
const response = await fetch(`${this.host}${path}`, { method: 'POST', headers: this.defaults.headers, body: JSON.stringify(body, null, 4) }); | ||
if (response.status === 201) | ||
return await response.json() as object; | ||
throw new Error(response.statusText); | ||
throw await Util.makeErrorFromResponse(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use FORTNOX error, do not ignore it - it is really useful to figure out what went wrong
constructor(config: { host: string, clientSecret: string, accessToken: string }){ | ||
constructor(config: { | ||
host: string, | ||
bearerToken?: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add bearer token support for OAuth2
this.articles = new Articles(dispatch); | ||
this.customers = new Customers(dispatch); | ||
this.invoices = new Invoices(dispatch); | ||
this.supplierInvoices = new SupplierInvoices(dispatch); | ||
this.suppliers = new Suppliers(dispatch); | ||
this.prices = new Prices(dispatch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add prices support
async getAll(filter: string) { | ||
const result = await this.util.getAllPages(this.path + '?filter=' + filter, 'Invoices', this.dispatch) as InvoiceResult[]; | ||
async getAll(filter?: string) { | ||
const result = await Util.getAllPages(this.path, 'Invoices', this.dispatch, filter) as FNInvoice[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix a bug - the results is array of FNInvoice[] actually, not of InvoiceResult[].
Price: FNPrice | ||
} | ||
|
||
export class Prices { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prices/price lists support
const result = await this.dispatch.get(this.path) as SupplierResult; | ||
return supplierNumber ? result.Supplier : result.Suppliers; | ||
async get(supplierNumber: string) { | ||
const result = await this.dispatch.get(`${this.path}/${supplierNumber}`) as SupplierResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix a bug - parameter not used (should be part of the query path)
@@ -1,21 +1,47 @@ | |||
import { Dispatch } from "./dispatch"; | |||
|
|||
export class Util { | |||
async getAllPages(path: string, arrayName: string, dispatch: Dispatch) { | |||
static async getAllPages(path: string, arrayName: string, dispatch: Dispatch, filter?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unify filter/paging. Before it had an issue that filter + paging did not work together.
|
||
// get fortnox error if exist | ||
if (response?.headers?.get('content-type')?.startsWith('application/json')) { | ||
const fnError = await response.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fortnox error is packed like { ErrorInformation: { message: "bla-bla-bla", code: 100500 } }
import { assert } from 'chai'; | ||
import { Util } from '../src/utils'; | ||
|
||
const makeFakeFortnoxResponse = (message: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests for error handling
@@ -88,6 +94,48 @@ describe('fortnox', () => { | |||
const response = await fn.articles.get(newArticleNumber); | |||
assert.equal(response.Description, anArticle.Description); | |||
}), | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests for prices
Is this being considered for inclusion? |
Thank you for the project, it was really useful for me to work with the Fortnox from node.
Some improvements from my side, pls your feedback:
getAllPages
of results (it was trying to append two parameters with "?" now it should append them properly (unified all methods to go through getAllPages to fix that)