Easy-digital-downloads: Combine earnings reports to use a single query.

[pippinsplugins]

Currently the earnings / sales graphs perform 2 queries (for each point on the graph):

• a query to get the earnings for that point
• a query to get the sales for that point

When a graph has a lot of points on it, this can result in a LOT of queries being performed.

Since the EDD_Stats class lets us query based on data ranges, we should be able to perform just two queries for a graph but querying the data and then setting it up into an array of timestamp => value pairs, which is exactly what the EDD_Graph class expects as its data input anyhow.

[pippinsplugins]

How's this coming, @justinsainton?

[JustinSainton]

Just about there. Had to put it on the backburner while I put out a few client fires, but should be able to wrap it up by end of week.

[pippinsplugins]

Great, looking forward to it!

[pippinsplugins]

@JustinSainton Do you have a fork where your progress on this is? I'd be happy to take over on it.

[evertiro]

@JustinSainton ping?

[cklosowski]

I'm punting this to 2.5. That's going to be a reports focused release, and this one might be in vein if we decide to refactor that page at all.

[cklosowski]

Moving to the EDD_Payment_Stats class will cause us to refactor how we did #3833. No big deal, just a reminder to notate the changes necessary.

[cklosowski]

@sunnyratilal take a look here, has your work on #5118 made this a moot point?

[sunnyratilal]

Sales have been combined into a single query - we still need to combine earnings into a single query.

[sunnyratilal]

Update on this issue: implementation is very nearly complete.

To Do:

• [x] Add indexing for the returned earnings
• [x] Move all the function calls to use the get_earnings_by_range() function
• [ ] Implement tax inclusion/exclusion

The graphs will need extensive testing to ensure the correct data is being plotted.

[sunnyratilal]

The immediate performance and efficiency benefits we've gained from this is fantastic! Before moving to a single query, we had 321 queries running on the reports page. Now we have 51. 😄

[sunnyratilal]

PR opened at #5316 and ready for some testing.

[pippinsplugins]

When viewing This Year, I see a bunch of notices and warnings. @sunnyratilal check on your end. If you don't see them, I can give the exact warnings / notices. There are a dozen or more.

[sunnyratilal]

@pippinsplugins Fixed in 5aa326f3d5e8cc0c5e51aebb708d578e91e0878f

[pippinsplugins]

A couple of date ranges still show notices / warnings:

• Last week
• This week

(edited comment after realizing I forgot to pull down)

[sunnyratilal]

Hmm not seeing that - all the aforementioned graphs are displaying correctly for me. 🤔

Are the seeing the errors/incorrectly graphs even after the transients have been cleared? All of those are cached graphs that's why.

[pippinsplugins]

@sunnyratilal just tried again. Same errors.

[cklosowski]

@sunnyratilal I'm not seeing errors, but I am seeing incorrect data. I'm seeing 0 for the sales counts. The eranings look right. I've updated the branch to have the new EDD CLI command to create random payments in the last 30 days.:

wp edd payments create --number=100 --date=random

That helps build a lot more data.

[sunnyratilal]

@pippinsplugins Could you post the errors you're receiving here please?

[sunnyratilal]

@cklosowski I'm not seeing any incorrect data. I'm getting all the data as it is supposed to be? Perhaps clear transients?

[cklosowski]

My issues have gone away, guessing it was transients related. I had cleared them, I thought, but all good now. Let's see what @pippinsplugins errors are and we can then look into merging.

[sunnyratilal]

@pippinsplugins Are you still receiving errors?

[sunnyratilal]

We cannot progress with #5164 until this is merged.

[pippinsplugins]

Will test in the morning again.

[pippinsplugins]

@sunnyratilal Looking better!

Custom date ranges (at least those that cross multiple years) do not work. The stats are wrong and the graph doesn't show the full date range.

[sunnyratilal]

@pippinsplugins Can't replicate that.

@SDavisMedia @cklosowski could you test please?

[SDavisMedia]

I can't replicate any of the errors. So I'm all good there as well.

For the data, I thoroughly checked 200+ payments from 2016 to 2017. All sales and earnings were correct for me, even filtered with a custom date range that spanned multiple years. I added and deleted records to check changes to the data. Couldn't break it.

Damn that's fast. :+1:

[pippinsplugins]

I flushed all of my transients to see if that was the cause. Didn't make any difference.

Will try on another install soon.

[cklosowski]

@sunnyratilal the predefined ranges seem to be right. i'm having a problem with the custom dates as well though. For example, this is this month:

And this is a custom from Jan 1 - Jan 31 of 2017:

Notice the dates are getting cut off on the 16th. This was my last date with a sale this month...so it's not letting us get reports for dates past the last date with sales, and that have not happened yet.

[SDavisMedia]

I just replicated that ^^^ ... with future dates in general, the graph shows no data (0 is still data).

Filtering by custom dates in February 2017, on master, you can still see the flatlined Earnings and Sales run across the bottom. On issue/1980, the graph is completely blank.

[sunnyratilal]

@SDavisMedia @cklosowski Could you pull and retry please?

[cklosowski]

@sunnyratilal @SDavisMedia @pippinsplugins Custom date ranges seem to be working for me now.

[SDavisMedia]

Looking good here! :+1:

[pippinsplugins]

Nice work here!

[sunnyratilal]

Merged into release/2.7!

[pippinsplugins]

The new get_earnings_by_range() and get_sales_by_range() methods need to include a filter that allow additional post statuses to be included in the query.

This is important to ensure that plugins like Recurring Payments can inject edd_subscription into the list. Without it, the renewal payments in recurring do not get included in the range.

Right now the status is hard coded like this:

AND (posts.post_status = 'publish' OR posts.post_status = 'revoked')

That could be changed to AND posts.post_status IN( 'publish', 'revoked' ); with 'publish', 'revoked' created by exploding an array.

[sunnyratilal]

@pippinsplugins PR opened at #5441

[pippinsplugins]

@sunnyratilal @cklosowski The new filter name is good but I think it should be extended to the get_sales() and get_earnings methods. We should have a single filter that applies to all four.

[sunnyratilal]

@pippinsplugins get_sales() and get_earnings() has its own filter edd_count_payments_where because both of those functions call edd_count_payments().

[pippinsplugins]

@cklosowski opinions?

[pippinsplugins]

Works great. Closing.