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

JSON Get/Post support, Test refactoring & ESLint fixes #65

Merged
merged 18 commits into from
Aug 13, 2017

Conversation

jordanwalsh23
Copy link
Contributor

This PR removes all usage of XML in the library, now only using application/json for GET/PUT/POST requests.

A number of changes were required to get this to work, including:

  • modifications to both oauth.js and application.js to remove XML serialisation/deserialisation
  • modifications to all entities to remove toXML & fromXMLObj functions
  • modifications to all entities to fix Date (these were previously String)

As part of this PR I have also refactored the tests to be in separate files, as the accountingtests.js file was almost 4k lines.

The tests are now driven by a file called testrunner.js which imports the other files as needed. Users can simply comment out the tests they don't want to be run and only the imported tests will execute.

I have updated the extend.js function to be the latest from Backbone.js.

Lastly, I've added a new parameter to the testing_config.json called selectedAppId. This is used to select an app other than the default in the dropdown when getting an OAuth token for Public/Partner applications.

The user must first browse to that screen in a proper browser, inspect the source code, and get the ID of the app they want to select and paste it into the config file.

@philals
Copy link
Contributor

philals commented Aug 2, 2017

image

@philals
Copy link
Contributor

philals commented Aug 2, 2017

Not sure why these fail now. Have not investigated.

@jordanwalsh23
Copy link
Contributor Author

jordanwalsh23 commented Aug 2, 2017 via email

@philals
Copy link
Contributor

philals commented Aug 2, 2017

So I ran master against the same Org and get 100% passes

@jordanwalsh23
Copy link
Contributor Author

jordanwalsh23 commented Aug 2, 2017 via email

@jordanwalsh23
Copy link
Contributor Author

Fixed a bunch of tests and re-tested against the Demo org in AU. I also removed a whole bunch of console.log statements from the code.

@philals would be awesome if you could pull the latest and re-test.

@philals
Copy link
Contributor

philals commented Aug 6, 2017

  1. I like the lack of console.log

I keep getting some 400s around attachments with my reset NZ Demo.

image

@jordanwalsh23
Copy link
Contributor Author

Ok, so I've tested this on demo company NZ. The issues were due to before functions not being complete prior to the tests starting.

I've updated the tests to make sure all the dependent before functions are returning a value so the tests will wait for their completion before continuing.

I've also had positive feedback from a couple of users that there aren't any backwards compatibility issues here which is great.

Back to you @philals.

@philals
Copy link
Contributor

philals commented Aug 8, 2017

All green for me locally.

@jordanwalsh23 jordanwalsh23 merged commit 075e199 into XeroAPI:master Aug 13, 2017
@jordanwalsh23 jordanwalsh23 deleted the add_json branch August 13, 2017 23:23
philals pushed a commit that referenced this pull request Sep 14, 2017
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