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

Is there any plans to add Tax API supports? #257

Closed
raecoo opened this issue Jul 22, 2015 · 20 comments
Closed

Is there any plans to add Tax API supports? #257

raecoo opened this issue Jul 22, 2015 · 20 comments

Comments

@raecoo
Copy link
Contributor

raecoo commented Jul 22, 2015

Currently, only the Tax Code query api available. Actually, we also need other Tax APIs. e.g. create and update.

@minimul
Copy link
Collaborator

minimul commented Jul 22, 2015

Everything is done on a volunteer and "as needed" (usually) basis. Since you need these abilities it would be great if you could add them back into the gem. By looking at the specs in combination with https://developer.intuit.com/docs/api/accounting it is easy to get started implementing these features.

@minimul minimul closed this as completed Jul 24, 2015
@raecoo
Copy link
Contributor Author

raecoo commented Jul 27, 2015

@minimul I tried to implement the TaxService API, but encountered the exception "System Failure Error: Cannot consume content type", Can you help to take a look?

this is my fork version https://github.com/raecoo/quickbooks-ruby
here is exception info https://gist.github.com/raecoo/b237c267127837867686
the request params and format is correct as official API docs mentioned.

thanks.

@minimul
Copy link
Collaborator

minimul commented Jul 27, 2015

Where is service/tax_service.rb?

@raecoo
Copy link
Contributor Author

raecoo commented Jul 28, 2015

@minimul Sorry, forgot committed. please pull it again.

@tchang1
Copy link

tchang1 commented Jul 29, 2015

@raecoo
Copy link
Contributor Author

raecoo commented Jul 29, 2015

@tchang1 Yes, I have already implemented the feature in my fork version, will push it later.

@minimul minimul reopened this Jul 29, 2015
@raecoo
Copy link
Contributor Author

raecoo commented Jul 29, 2015

@minimul I've pushed a beta version to my fork that implemented the TaxService API via JSON way.

@minimul
Copy link
Collaborator

minimul commented Jul 29, 2015

I know I was just looking at it. Good find and work but at the same time Intuit should be consistent and provide the same support for both data formats.

For better or worse this gem is wedded to XML requests and responses so I would like to see them flip the switch on XML for this entity.

I just put in a question https://intuitdeveloper.lc.intuit.com/questions/1195446-taxservice-is-json-only . We need to evaluate their response before we can consider a merge.

@raecoo
Copy link
Contributor Author

raecoo commented Jul 29, 2015

Yup, I agree with you about the gem is wedded to XML requests and responses. so let's waiting their feedback then consider next thing.

@minimul
Copy link
Collaborator

minimul commented Jul 31, 2015

Update: There was some more comments on this thread: https://intuitdeveloper.lc.intuit.com/questions/1195446-taxservice-is-json-only

@tchang1
Copy link

tchang1 commented Jul 31, 2015

By the way, I'm from Intuit (you may have seen me answer q's on the intuit live community) but I lurk here to see if theres any issues I can help with. The short answer is we would prefer clients to use JSON and will not be enhancing existing services for xml support.

Here's the reasoning. Supporting xml and maintaining our (legacy) xsd schema has slowed us down and created problems when we do enhancements, fixes and improvements. In general, we are encouraging JSON for development moving forward which will help us deliver higher quality and more capabilities into our API. The next major version of the QBO API will for sure be JSON only. I agree though that this Tax Service should have been built with XML support to be compatible with existing V3 clients but starting to use JSON here will help us immensely.

@ruckus
Copy link
Owner

ruckus commented Aug 1, 2015

In general, we are encouraging JSON for development moving forward which will help us deliver higher quality and more capabilities into our API. The next major version of the QBO API will for sure be JSON only

This might be the final nail in the coffin to do a top-down re-write and migrate to JSON

@tchang1
Copy link

tchang1 commented Aug 1, 2015

@ruckus I don't know if it would be needed to rewrite the current ruby library for JSON, right now, but eventually - yes I would recommend it. For the most part, existing V3 accounting endpoints/services aren't going to change. New things that would be JSON only would be for example, a set of payroll use case APIs, or eventually V4 accounting APIs - our own UI & clients would be consuming these in JSON.

@minimul
Copy link
Collaborator

minimul commented Aug 3, 2015

Let's try to work in JSON only entity support without considering a large
rewrite. Like @tchang1 said the V3 API is going to be with us a long, long
time and it almost completely supports XML. In fact, I believe in the case of
a few entities it was the opposite, devs were complaining about JSON support
not being up to par with XML.

Anyway, let's cross the v4 bridge when we come to it in which time it may be
best to just to roll a new gem (like was done in the case of v2 & v3)

Looking at @raecoo's fork I think we can at least abstract out the TaxService
service code into say a ServiceCrudJSON. Same with some of the his model code
like to_json and from_json.

In this way we can more easily add new entities or existing ones that are
JSON only.

@minimul
Copy link
Collaborator

minimul commented Aug 4, 2015

@raecoo If you could form a PR like described in my previous post that would be so appreciated. If not I plan on contributing to this solution largely based on your fork.

@minimul minimul mentioned this issue Aug 6, 2015
@raecoo
Copy link
Contributor Author

raecoo commented Aug 6, 2015

@minimul I agree with the ServiceCrudJSON solution. I've sent the PR.

@minimul
Copy link
Collaborator

minimul commented Aug 6, 2015

I am thinking more along the lines of this:

minimul@b7a5f74

I'll have more comments and code tomorrow.

@raecoo
Copy link
Contributor Author

raecoo commented Aug 7, 2015

the refactoring is awesome.

@minimul
Copy link
Collaborator

minimul commented Aug 7, 2015

I have more updates on that branch that are based off sandbox testing.
Trying to get this merged into master while I have time and it is fresh in my mind.
Probably Monday or Tuesday night, US Eastern time.
This should be a good first step as well (1.0.0 release?) to supporting either data format.

@minimul
Copy link
Collaborator

minimul commented Aug 10, 2015

Merged into master.

@minimul minimul closed this as completed Aug 10, 2015
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

No branches or pull requests

4 participants