Reports

[ratbeard]

Related to discussion in #146. Changes are:

• ReportsService.query now returns an instance of a Report model, instead of an empty intuit collection.
• Reports model renamed to Report, as its always a single item.
• Adds Report#xml method which returns the nokogiri doc, similar to how the api was used before via service.last_response_xml
• Adds Report#columns method which returns an array of column names as strings, e.g. report_with_years.columns.should == ["", "Jul 4 - Dec 31, 2013", "Jan - Dec 2014", "Jan 1 - Jun 21, 2015"]
• Adds Report#all_rows method which returns an array of all or
  in the document, e.g. things that contain . Nesting in Rows is ignored. A row is simple to work with, looking like: ["Total Expenses", BigDecimal(100), BigDecimal(80)]
• Adds Report#find_row(label) which is a simple helper to find a row by its label, e.g. report.find_row("Total Expenses")
• Add and cleanup some specs, including a new fixture data for a report with multiple columns.

~Thanks! 🙇

[ratbeard]

These weren't used and were out of date w/ all the supported reports now, so I removed them

[minimul]

Thanks. Looks good to me, although, I am not using the existing reporting implementation in production so..
@Craggar , @raksonibs any feedback?

[minimul]

Hang tight on this. This breaks the previous API so we need to evaluate.

[ratbeard]

The api has changed, but as comments in the issue thread indicate people aren't actually using the return value of service.query, they just use service.last_response_xml to get at the data. The reason people don't use the return value is the code assumes the response comes back with a root <IntuitResponse> tag, which it does not, and therefore it always parses it as an empty collection. Due to this, the Report model was never instantiated by the library either.

So basically the old way the api was used - service.last_response_xml - is still there, but now query also returns a meaningful Report model, which has a few additional helper methods for common cases of pulling out data.

No rush to merge and please confirm the above is true, I was a little confused by its behavior before and was wondering how the Report model ever worked. Looking at the git history, it looked like there was a bad fixture at one point which contained several <Report>'s wrapped in an <IntuitResponse> which would have made the code work 17ea786, but I haven't seen an actual response come back from quickbooks like this . Hmm I wonder if the old code might work if you can call reports through a bulk api call?

[minimul]

Ok, I am going to take this PR for a spin early next week.

[Craggar]

Sorry, I've been away on vacation so just saw this now - I'll take a look at some point next week, but I'd happily accept having to make a couple of changes to use this, which actually returns an object. Looks good, at a cursory glance.

[minimul]

Thanks @Craggar. I said I was going to take this for a spin but instead I am going to help out with #257

@ruckus If @cragger confirms why don't we merge and push as 0.3.0 after pushing what is in current master as 0.2.4

[ruckus]

This looks great - thank you @ratbeard

It sounds like I should merge this and release a 0.3.0 - since this introduces backwards incompatibility hence the minor version bump.

Let me know if y'all are ready. Thanks again.

[minimul]

@ruckus It may be best if you gem push the current master at 0.3.0 and then when this PR is merged go to 0.4.0. The recent JSON merge doesn't break backwards compatibility but was large enough to warrant a bump.

[minimul]

Update: Would like to get this evaluated and potentially merged soon.

[ratbeard]

Let me know if I can do anything to help.

I considered making a Row class to hold values for each row but ended up just using an array e.g: ['Savings', BigDecimal("19.99"), BigDecimal("19.99"), BigDecimal("819.99")]

[Craggar]

Hey, sorry I've not commented back for a while - I gave this a whirl, and it looks good to me. Happy to see this merged in.

[minimul]

Thanks @Craggar
We'll merge this in next week and bump the version to 0.4.0

[minimul]

Thanks @ratbeard & @Craggar – merged.