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.

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

Fix xml not formatted correctly #2

Merged
merged 2 commits into from
Jul 28, 2016

Conversation

JackIsBack
Copy link

Fix freshbook webservice response "Your XML is not formatted correctly" that happened when the posted XML contains characters that are more than 1 byte size encoded in UTF-8, such as french accents for example éàù, or others characters like € (3 bytes) and way more others.

The problem comes from the 'Content-Length': string.length in index.js, this only worked if the xml contains 1 byte characters encoded. If not, the Content-Length sent is too short and the xml is cutted before the end.

I fixed this using Buffer.byteLength: Returns the actual byte length of a string.
(https://nodejs.org/api/buffer.html#buffer_class_method_buffer_bytelength_string_encoding)

Fix freshbook webservice response "Your XML is not formatted correctly" that happened when the posted XML contains characters that are more than 1 byte size encoded in UTF-8, such as french accents for example éàù, or others characters like € (3 bytes) and way more others.
The problem comes from the 'Content-Length': string.length in index.js, this only worked if the xml contains 1 byte characters encoded. If not, the Content-Length sent is too short and the xml is cutted before the end.

I fixed this using Buffer.byteLength: Returns the actual byte length of a string.
https://nodejs.org/api/buffer.html#buffer_class_method_buffer_bytelength_string_encoding
@fiznool
Copy link

fiznool commented Jul 28, 2016

Thanks for this!

Could you please add a spec to cover off this case?

I'd suggest adding a new update spec in one of the spec files, e.g. Invoice.js, and assert that sending one of these characters returns an OK response from freshbooks.

@JackIsBack
Copy link
Author

I added a new update spec in Invoice.js, i hope it's ok for you.
The Travis build didn't passed, but the error seems not linked to my update:
Error: CANNOT UPDATE CATEGORY: Invalid value for field 'name'. Unique value required, value provided was already used.

@fiznool fiznool merged commit 232490f into flowxo:master Jul 28, 2016
@fiznool
Copy link

fiznool commented Jul 28, 2016

Thanks, you are right this other spec is failing in some other way, I'll get that sorted too.

Thanks again for your contribution, I'll try to get a new release out soon!

@fiznool
Copy link

fiznool commented Jul 28, 2016

@JackIsBack qq, would you consider this a breaking change, would you anticipate anybody using the current version to experience errors with this update? I'm trying to decide whether this warrants a major or patch release.

@JackIsBack
Copy link
Author

I had a lot Invoices that wasn't created because of this. My clients are from all around the world, so a lot of them had some specific characters in their name or addresses, so from my point of view it's kinda major. But maybe i can test it a bit (i put my fork in production this afternoon), and then i will tell you if problems still occurs.

@fiznool
Copy link

fiznool commented Jul 28, 2016 via email

@JackIsBack
Copy link
Author

Sure no problem, i'll let you know around next Wednesday.

@JackIsBack
Copy link
Author

Hey, just for infos, i had no problem yet with my fix about that Content-Length problem. Since last friday 7 invoices were created with success (all full of accents and special chars).
So on my side, everything works fine for now.

@fiznool
Copy link

fiznool commented Aug 3, 2016

Great news. I published v2.0.0 which includes your PR.

Thanks again for your help!

@JackIsBack
Copy link
Author

Ok great ! No problem, and thank you for your work on this lib too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants