Implement queryTaxPayer & registerCreditEntry function#85
Implement queryTaxPayer & registerCreditEntry function#85passatgt wants to merge 9 commits intoewngs:masterfrom
Conversation
ert78gb
left a comment
There was a problem hiding this comment.
thank you for the PR. I have some modification request please review them.
I would like to ask you next time please open individual pull request for every feature or bug fix.
lib/CreditEntry.js
Outdated
| @@ -0,0 +1,45 @@ | |||
| import assert from 'assert' | |||
| import merge from 'merge' | |||
There was a problem hiding this comment.
please delete the unused import
lib/CreditEntry.js
Outdated
| import {wrapWithElement} from './XMLUtils.js' | ||
| import {PaymentMethod, PaymentMethods} from './Constants.js' | ||
|
|
||
| const defaultOptions = { |
There was a problem hiding this comment.
Not needed the defaultOptions just overcomplicate the code. You could simply set the default values in the line 11 and 12
lib/Client.js
Outdated
| } | ||
|
|
||
| async queryTaxPayer (taxPayerId) { | ||
| assert(typeof taxPayerId === 'number' && /^[0-9]{8}$/.test(taxPayerId.toString()), 'taxPayerId must be an 8-digit number'); |
There was a problem hiding this comment.
You expect the taxPayerId should be a number but after you convert it to string to validate with regexp.
taxPayerId is string in the xsd documentation. In the response we also return as string so I think better not expect a number type.
We have 2 options expect a string type or don't expect any type because the regexp do string coercion.
I would like if we expect string type. Don't forget to update the readme.md too
| return data | ||
| } | ||
|
|
||
| async queryTaxPayer (taxPayerId) { |
There was a problem hiding this comment.
Please add a jsdoc especially of the input and returns type.
|
|
||
| const response = await this._sendRequest('action-szamla_agent_taxpayer', xml) | ||
| const parsedBody = await parseString(response.data) | ||
| const cleanParsedBody = removeNamespaces(parsedBody); |
There was a problem hiding this comment.
I think the removeNamespaces utility function is not necessary it is just an overhead. We could read the properties of the object with namespace.
There was a problem hiding this comment.
what if the namespace changes? happened before... :)
There was a problem hiding this comment.
I have never used the Nav API. I found the https://github.com/nav-gov-hu/Online-Invoice, looks like it is up to date.
Based in it the address DetailedAddressType in the base namespace.
All of the xml example response contain descriptive namespace prefixes. I don't know why the szamlazz.hu returns with nsX. Anyway the response contains the namespace definition xmlns:nsX="http://schemas.nav.gov.hu/OSA/3.0/base". Based on that we could properly find the proper namespace prefix of the schema.
If you think it is overcomplicated and Nav never will use the same element name from more namespace we could strip the prefixes but in this case please use the built in feature of the xml parser lib.
Just for a note. I called 4 times the queryTaxPayer method and once I got NAV API is not available response. Looks like it not HA.
| */ | ||
| export function createCreditEntry(CreditEntry) { | ||
| return new CreditEntry({ | ||
| paymentMethod: PaymentMethod.BankTransfer, |
There was a problem hiding this comment.
PaymentMethod does not have the BankTransfer property. I am fixing other invalid setup in the file after the CR
|
|
||
| return wrapWithElement('tetel', [ | ||
| [ 'megnevezes', this._options.label ], | ||
| [ 'azonosito', this._options.id ], |
There was a problem hiding this comment.
please extend the readme.md with this optional property
| '</xmlszamlakifiz>' | ||
|
|
||
| const httpResponse = await this._sendRequest('action-szamla_agent_kifiz', xml) | ||
| const data = { |
There was a problem hiding this comment.
please implement error branch too
| }; | ||
| } | ||
|
|
||
| async registerCreditEntry (options, creditEntries) { |
There was a problem hiding this comment.
To follow the lib pattern please create a new complex type that contains the options and creditEntries.
Move the assert from this method to the new type.
There was a problem hiding this comment.
Are you sure about this one? I don't see this being used by other already implemented functions in the Client class.
There was a problem hiding this comment.
All of the public interfaces of the Client accept only 1 input argument. The options and creditEntries can be easily merge into 1 object like
const creditEntry = new CreditEntry({
invoiceId: 'something',
taxNumber: '',
additive: true,
entries: [
new CreditItem({
date: new Date(),
paymentMethod: PaymentMethods.BankTransfer,
amount: 0
})
]
})There was a problem hiding this comment.
BTW just now recognised in the szamlazz.hu documentation it allows maximum 5 kifizetes type
| beforeEach(() => { | ||
| nock('https://www.szamlazz.hu') | ||
| .post('/szamla/') | ||
| .replyWithFile(200, RESPONSE_FILE_PATHS.SUCCESS_WITHOUT_PDF, { |
There was a problem hiding this comment.
the success response of the request is xmlagentresponse=DONE text. If I read well the documentation.
| @@ -0,0 +1,27 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
please keep the namespace and other meta data elements like header, funcCode, software, etc
| 'beallitasok', [ | ||
| ...this._getAuthFields(), | ||
| ['szamlaszam', options.invoiceId], | ||
| ['adoszam', options.taxNumber || ''], |
There was a problem hiding this comment.
it is an optional filed. please don't set a default value . We save some bandwidth with it
This pull request introduces the queryTaxPayer function, which queries taxpayer details from the Számlázz.hu API(which mirrors the NAV API). The implementation ensures proper XML handling, response parsing. The response contains the validity of the tax payer id and if its valid, it will return the address and name in the following format:
Also adds the registerCreditEntry function, which you can use to mark an invoice as paid by registering a credit entry.