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

Feature: Implement Reports API #146

Closed
cdmwebs opened this issue Jul 31, 2014 · 25 comments
Closed

Feature: Implement Reports API #146

cdmwebs opened this issue Jul 31, 2014 · 25 comments

Comments

@cdmwebs
Copy link

cdmwebs commented Jul 31, 2014

It looks like QBO recently exposed a reports API. I'd like to take a stab at implementing some of the more important reports (Balance Sheet, P&L).

Any tips as to approach? I was planning to create a Report base class and inherit for the individual reports, but I'm not sure how much is shared.

@ruckus
Copy link
Owner

ruckus commented Aug 1, 2014

Awesome! Yeah if you wanted to take a stab it would be much appreciated.

So I just had a quick peek at the API and it appears to only return JSON,
even if you specify Content-Type: application/xml

An example output of the AgedReceivables report is here:

https://gist.github.com/ruckus/747c702d7ab7f4243c69

Rather than attempt to analyze / parse the JSON (in quickbooks-ruby) I'm
thinking just to return the raw JSON to the caller and let them handle it
how they want. What do you think?

So basically your report classes should just handle accepting query
parameters which are specific to each Report and ensuring they fit
acceptable values (for the report), firing off the request and returning
the raw JSON back to the user. Maybe even not bother with parsing the JSON
per se and returning the raw string? That would at least get us around from
having to add a new JSON parser dependent gem to the gemspec.

What do you think?

Thanks again!

On Thu, Jul 31, 2014 at 2:52 PM, Chris Moore [email protected]
wrote:

It looks like QBO recently exposed a reports API
https://developer.intuit.com/docs/0025_quickbooksapi/0050_data_services/reports.
I'd like to take a stab at implementing some of the more important reports
(Balance Sheet, P&L).

Any tips as to approach? I was planning to create a Report base class and
inherit for the individual reports, but I'm not sure how much is shared.


Reply to this email directly or view it on GitHub
#146.

@johnroa
Copy link

johnroa commented Aug 7, 2014

+1!

@cdmwebs
Copy link
Author

cdmwebs commented Aug 11, 2014

@ruckus makes sense. JSON is part of stdlib now, so would it be okay to use the builtin parser?

@ruckus
Copy link
Owner

ruckus commented Aug 11, 2014

Oh nice, I didn't know that JSON was included in std lib. So yeah, if you want to parse it really quick and return the parsed representation than go for it!

Thanks again.

On Aug 11, 2014, at 2:44 PM, Chris Moore [email protected] wrote:

@ruckus makes sense. JSON is part of stdlib now, so would it be okay to use the builtin parser?


Reply to this email directly or view it on GitHub.

@cdmwebs
Copy link
Author

cdmwebs commented Aug 11, 2014

@ruckus cool. I'm planning to pair on it this week.

@mguterl
Copy link

mguterl commented Aug 14, 2014

@ruckus - I want to check some of my assumptions about the project:

  1. Services are the objects that are responsible for interacting with the QBO API endpoints.
  2. Models are the objects responsible for mapping from Ruby to QBO API XML. Models are passed to the corresponding service in order to take action (create, update, destroy). Models are also returned from the service when querying.
  3. BaseService is dependent upon XML and a URL structure with a query, query parameter.

Given this information, it seems like there should be a new layer, similar in functionality to BaseService for interacting with the Reports API.

Is my understanding correct?

@ruckus
Copy link
Owner

ruckus commented Aug 14, 2014

Yes, looks like you have it understood correctly.

In the case of Reports I'm not sure how beneficial it would be to model out
the whole response class (e.g. convert from JSON to Ruby objects), since
the output can vary so much (well it appears it can). Thus my idea to just
take the response JSON and shove it back to the user to deal with. Its not
like you will be converting from JSON to Ruby and then want to manipulate
the Ruby object to send back somewhere. Reports are read-only, obviously.

Thus, if it were up to me I'd just build a BaseReportsService class (or
something similar) and then a series of Models which are really the
requests, e.g. APAgingDetailRequest - which supports all of the inputs and
formats stuff accordingly, sends it off to the service class, gets the JSON
and then maybe stuff the JSON back into the model and return the whole
model (which at that point contains both the request and the response).
This just occurred to me, returning the request ruby object along with the
JSON payload as one, because it might make sense for the callers to inspect
the response in the context of the request.

I hope this helps!

On Thu, Aug 14, 2014 at 4:58 AM, Michael Guterl [email protected]
wrote:

@ruckus https://github.com/ruckus - I want to check some of my
assumptions about the project:

  1. Services are the objects that are responsible for interacting with
    the QBO API endpoints.
  2. Models are the objects responsible for mapping from Ruby to QBO API
    XML. Models are passed to the corresponding service in order to take action
    (create, update, destroy). Models are also returned from the service when
    querying.
  3. BaseService is dependent upon XML and a URL structure with a query,
    query parameter.

Given this information, it seems like there should be a new layer, similar
in functionality to BaseService for interacting with the Reports API.

Is my understanding correct?


Reply to this email directly or view it on GitHub
#146 (comment)
.

@GildedHonour
Copy link

One thing which isn't related to this issue. I'd like to inspect what is sent and received by this gem at as deep level as possible. This isn't that useful because it shows quite the high level information:

Quickbooks.logger = Rails.logger
Quickbooks.log = true
# Pretty-printing logged xml is true by default
Quickbooks.log_xml_pretty_print = false

Is there any way to make it show the information at a deeper level?

P.S. I'm aware about wireshark, though, but since I'm using WiFi with a password, wireshark shows the encrypted data, not plain .

@ruckus
Copy link
Owner

ruckus commented Aug 19, 2014

What do you mean by "show information at a deeper level?"

Do you mean more of the HTTP information?

During development I find it extremely useful to use an HTTP proxy, which you are with wireshark. And yes, the proxy app does need to support SSL decryption.

I think adding a ton more debugging to quickbooks-ruby is outside the scope and I recommend to use an HTTP proxy.

I use Charles Proxy: http://www.charlesproxy.com/

and I have heard good things about mitmproxy: http://mitmproxy.org/ which also supports SSL.

On Aug 19, 2014, at 3:44 AM, Alex Maslakov [email protected] wrote:

One thing which isn't related to this issue. I'd like to inspect what is sent and received by this gem at as deep level as possible. This isn't that useful because it shows quite the high level information:

Quickbooks.logger = Rails.logger
Quickbooks.log = true

Pretty-printing logged xml is true by default

Quickbooks.log_xml_pretty_print = false
Is there any way to make it show the information at a deeper level?

P.S. I'm aware about wireshark, though, but since I'm using WiFi with a password, wireshark shows the encrypted data, not plain .


Reply to this email directly or view it on GitHub.

@GildedHonour
Copy link

Thanks but I can't get along with mitmproxy. What is need it just to be able to see what information quickbooks-ruby sends and receives exactly (what headers look like, body, etc...) to be able to do the same thing in Python. Quickbooks.logger doesn't show that much.

@ruckus
Copy link
Owner

ruckus commented Aug 19, 2014

Well there is a free trial of Charles Proxy.... If you're unsure how to point Quickbooks to a Proxy app running on your machine its just this config:

$qb = OAuth::Consumer.new($consumer_key, $consumer_secret, {
    :site                 => "https://oauth.intuit.com",
    :request_token_path   => "/oauth/v1/get_request_token",
    :authorize_path       => "/oauth/v1/get_access_token",
    :access_token_path    => "/oauth/v1/get_access_token",
    :proxy => "http://127.0.0.1:8888"
})

Notice the :proxy attribute...

However, if Quickbooks.log is enabled than you should see most of the HTTP I/O, enabled in the code at:

https://github.com/ruckus/quickbooks-ruby/blob/master/lib/quickbooks/service/base_service.rb#L204

When I run it with:

Quickbooks.log = true
Quickbooks.logger = Logger.new($stdout)
# ... call a service ...

I get output like:

I, [2014-08-19T10:54:21.379817 #50699]  INFO -- : ------ QUICKBOOKS-RUBY REQUEST ------
I, [2014-08-19T10:54:21.379872 #50699]  INFO -- : METHOD = get
I, [2014-08-19T10:54:21.379897 #50699]  INFO -- : RESOURCE = https://quickbooks.api.intuit.com/v3/company/1002341430/query?query=SELECT+*+FROM+Invoice+STARTPOSITION+1+MAXRESULTS+20
I, [2014-08-19T10:54:21.379919 #50699]  INFO -- : REQUEST BODY:
I, [2014-08-19T10:54:21.379942 #50699]  INFO -- : {}
I, [2014-08-19T10:54:21.379971 #50699]  INFO -- : REQUEST HEADERS = {"Content-Type"=>"application/xml", "Accept"=>"application/xml", "Accept-Encoding"=>"gzip, deflate"}
I, [2014-08-19T10:54:23.798912 #50699]  INFO -- : ------ QUICKBOOKS-RUBY RESPONSE ------
I, [2014-08-19T10:54:23.798995 #50699]  INFO -- : RESPONSE CODE = 200
I, [2014-08-19T10:54:23.799027 #50699]  INFO -- : RESPONSE BODY:
I, [2014-08-19T10:54:23.809336 #50699]  INFO -- : <?xml version="1.0" encoding="UTF-8" standalone="yes"?>

If the above logging is not working then feel free to submit a Pull Request with more debugging enabled.

@GildedHonour
Copy link

As for QuickBooks.log, I see this:

[2014-08-19T23:44:03.611420 #53679]  INFO -- : REQUEST BODY:
I, [2014-08-19T23:44:03.611458 #53679]  INFO -- : {:file_content_0=>#<UploadIO:0x0000010277f6e8 @content_type="image/jpeg", @original_filename="aaa.bbb", @local_path="/Users/alex/Documents/aaa.bbb", @io=#<File:/Users/alex/Documents/aaa.bbb>, @opts={}>}

That's not helpful, I can't translate this in Python because what is shown is an abstraction at a high level, I need to know what the body looks like exactly in lower level. The same for the headers, but I've already found out what they should look like (I hope).

@GildedHonour
Copy link

Charles Proxy - quite useful, however.

@ruckus
Copy link
Owner

ruckus commented Aug 19, 2014

That's not helpful, I can't translate this in Python because what is shown is an abstraction at a hight level, I need to know what the body looks like exactly.

Feel free to fork the repo and add some deeper debugging code.

On Aug 19, 2014, at 11:18 AM, Alex Maslakov [email protected] wrote:

As for QuickBooks.log, I see this:

[2014-08-19T23:44:03.611420 #53679] INFO -- : REQUEST BODY:
I, [2014-08-19T23:44:03.611458 #53679] INFO -- : {:file_content_0=>#<UploadIO:0x0000010277f6e8 @content_type="image/jpeg", @original_filename="a.c", @local_path="/Users/alex/Documents/aaa.bbb", @io=#File:/Users/alex/Documents/aaa.bbb, @opts={}>}

That's not helpful, I can't translate this in Python because what is shown is an abstraction at a hight level, I need to know what the body looks like exactly. The same for the headers, but I've already found out what they should look like (I hope).


Reply to this email directly or view it on GitHub.

@johnroa
Copy link

johnroa commented Sep 4, 2014

Hi guys any update on incorporating the Reports API?

@XanderGlassman
Copy link

+1, a reports API feature would be great!

@johnroa
Copy link

johnroa commented Oct 30, 2014

Anything here guys?

@cdmwebs
Copy link
Author

cdmwebs commented Oct 30, 2014

@johnroa not yet, but it's still on my list! If someone gets to it before me, that would be swell.

@rdeshpande
Copy link
Contributor

+1!

@minimul
Copy link
Collaborator

minimul commented Dec 18, 2014

FYI: There is has been a WIP PR put in over here #204

@ratbeard
Copy link
Contributor

I am wondering if anyone has gotten the code in #204 that was merged in to work. I always get an empty Collection response when I try to use the ReportService.

Looking at the code, ReportService tries to parse the response xml as an collection, which expects the xml response root to be an [1]. As I query the API, and look at the examples inside the spec/fixtures directory, I'm seeing the root node is [2]. Therefore this if branch [3] never gets run, and an empty Collection is always returned. The tests pass because they only checking the fixture xml contains a certain node, not that the a proper Report model is returned [4].

Maybe Intuits api changed, or as mentioned in the discussion in #204 maybe people are doing something like:

client.query('BalanceSheet')
xml = Nokogiri.parse(client.last_reponse_xml)
...

If thats the only way to get access to the underlying report data, then I don't understand the purpose of Models::Report or code like

addition = xml.xpath(path_to_nodes)[0].xpath("//xmlns:Currency").children.to_s if "#{model::XML_NODE}" == "Reports"
entry.currency = addition if "#{model::XML_NODE}" == "Reports"
collection.body = response.body if "#{model::XML_NODE}" == "Reports"
as they are never run.

I plan to work on this so at the minimum client.query returns a Report object with a body attribute containing the xml, and just wanted to check in to make sure I'm not missing something; perhaps a way to call Intuits report api and get back a collection of reports? Otherwise I'll just rip out some of the code that got added and won't worry too much about breaking the api as it currently does not work.

Thanks!
🐀

[1]

parse_collection(response, model)

[2] https://github.com/ruckus/quickbooks-ruby/blob/e3b2cb50ff4605e625c0dffd54b752a753bfa23c/spec/fixtures/balancesheet.xml
[3]
query_response = xml.xpath("//xmlns:IntuitResponse/xmlns:QueryResponse")[0]
if query_response

[4]
balance_sheet_xml.xpath('//xmlns:ReportName').children.to_s == 'BalanceSheet'

@Craggar
Copy link
Contributor

Craggar commented Jul 17, 2015

I use the report service - the xml returns is pretty messy, and not easy to break down into smaller elements (for example it contains 'Rows' objects which contain one or more 'Row', and those can in turn contain one or more 'Rows' or 'Row'). It's not pleasant to work with, but that's exactly what I'm doing: using Nokogiri to break it down, manually parsing the entire response, to extract the data I need.

I only need a couple of headline figures from a couple of very specific reports, though, so haven't revisited it to look at ways to make the response more of a flexible 'entity'.

I have a function (where 'category' is actually just the string "reports") that is something like this:

def get_report(category, options={})
  service = Quickbooks::Service::Reports.new
  service.company_id = @token_attributes[:realm_id]
  service.access_token = oauth_access_token
  reportString = options[:report_string]     #report_string is the name of the report
  options.delete(:report_string)             #the options contain additonal options like start_date, end_date, columns, etc
  result = service.query(reportString, {}, options)
  resultXML = service.last_response_xml
end

I then have a controller that parses that XML. For example:

doc = Nokogiri::XML(report.to_s)
header = doc.at_css "Header"
if header
  headerObject = {}
  headerObject[:time] = getXMLText(header, "Time")
  headerObject[:name] = getXMLText(header, "ReportName")
  #etc

With the nesting of row(s) within row(s) it gets pretty messy and recursive after that...

So, yes, it does work, but it could definitely benefit from some attention and improvement, in my opinion.

@minimul
Copy link
Collaborator

minimul commented Jul 17, 2015

Thanks @Craggar . Let us know @ratbeard if that solves it for you.

With respect to improvements:
Personally, I don't think there is a great solution or abstraction that can
be done here. You basically need to roll up you sleeves with Nokogiri or Oga
and "data mine" and yeah, the XML returned is total spaghetti.
Nokogiri and Oga have advanced CSS3 selector support so it is somewhat easy to
get the data you want e.g.

Nokogiri::XML(report.to_s).css('Rows > Header + Rows > Row').each ...
Nokogiri::XML(report.to_s).at('Rows > Header + Rows > Row:last-child').content

One improvement could be in spec/lib/quickbooks/service/reports_spec.rb is to have advanced
XPath and CSS parsing examples, which could also make its way into the readme.

@ratbeard
Copy link
Contributor

Thanks for posting that @Craggar. I wrote a simple find_row() function yesterday as my use case is similar to yours, I just need to pick out a few 'Total' rows and will ignore the rest, and I don't care how deeply the Rows are nested. I was thinking this morning I actually want a flattened_rows function that returns a list of all rows without the nesting; then I can loop through each row and grab the data I care about and log out ones I don't. Just because I'm not sure what all the possible rows customers could have at this point and I want a little bit of debugging help at first.

@minimul I agree that the way nested Rows are gross and providing better API's on top of that might not be worth the effort. What I was getting at above though is that if the only way to get access to the data is through last_response_xml then the following code is dead and should be removed and/or improved:

  • ReportService tries to parse the response as a collection, which assumes the response has a root element of <IntuitResponse>. The actual response has a root of <Report>, so query always returns an empty intuit collection.
  • Given that, the if query_response check is always nil and so the following code that was added never can be reached:
    addition = xml.xpath(path_to_nodes)[0].xpath("//xmlns:Currency").children.to_s if "#{model::XML_NODE}" == "Reports"
    entry.currency = addition if "#{model::XML_NODE}" == "Reports"
    collection.body = response.body if "#{model::XML_NODE}" == "Reports"
  • Also given that, there is no way for Models::Report to ever be instantiated.
  • Models::Report doesn't make sense how it uses ROXML to declare xml_accessors, but is setting the attributes manually in the code I just linked. It could instead just declare them as attr_accessors.

So given all that I think my improvements I'm suggesting would be:

  • Update ReportService to not try and parse the result as a collection. Instead it should return an instance of ReportModel. Update the specs to test that a meaningful object is returned, not that the fixture xml contains a certain node.
  • Update ReportModel to not try and use ROXML as it is too awkward to try and map the data out due to the recursive nature of Rows. It can just expose a property like body at a minimum as it currently does.
  • Additionally, it may be nice to add some helper methods on to ReportModel like find_row or flattened_rows. It could do the xpath'ing search for ColData's and mapping of sibling elements for you, so the api could be something like:
report.find_row('Total Cost of Goods Sold')
=> ['Total Cost of Goods Sold', 2000, 2500]

That last improvement can lead to a lot of bikeshedding and what not, so any ideas on an api would be very welcome, but regardless of it we should still definitely fix the other issues.

@ruckus
Copy link
Owner

ruckus commented Jul 17, 2015

Hi @ratbeard - good questions you raise. I havent used any of the Reporting stuff myself, so I havent noticed this before. You're right in that the BaseService#parse_collection has logic in there to handle report model rather than just kick it out raw.

One approach to instructing query to not try and parse the XML is to maybe include a :raw => true option to query(), like:

body = client.query('BalanceSheet', :raw => true)
xml = Nokogiri.parse(body)

This would then allow us to rip out all of the the lines you highlighted:

addition = xml.xpath(path_to_nodes)[0].xpath("//xmlns:Currency").children.to_s if "#{model::XML_NODE}" == "Reports"
entry.currency = addition if "#{model::XML_NODE}" == "Reports"
collection.body = response.body if "#{model::XML_NODE}" == "Reports"

Anyways, it looks like this area could use some cleaning up and if you want to tackle that it would be much appreciated. Thank you!

@ratbeard ratbeard mentioned this issue Jul 27, 2015
@ruckus ruckus closed this as completed Aug 30, 2016
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

10 participants