Comments

googletorp’s picture

StatusFileSize
new25.06 KB

The sales overview showed what could be the root cause, 2 x november month, when doing a report with a monthly granularity.

cvangysel’s picture

Status: Active » Postponed (maintainer needs more info)

Right, that's weird behaviour and once it's fixed in the table view the chart should automatically follow.

Could you maybe find out how to reproduce the error? December seems to be completely skipped, so for some reason it thinks the data retrieved from the database in December is actually November.

Which time zone is configured for the Drupal installation?

torgospizza’s picture

Title: The sales overview is of by a month » The sales overview is off by a month
Status: Postponed (maintainer needs more info) » Active
StatusFileSize
new289.2 KB

We're getting this same issue, on a new install of Drupal Commerce and Commerce Reports. In our case there are two Novembers and nothing showing up for March, except for the first / earliest instance in which February is nullified. We're set to the timezone Los Angeles (-0700). I'm attempting to dig into this now.

pjcdawkins’s picture

StatusFileSize
new19.01 KB

We also have freaky months.

pjcdawkins’s picture

Version: 7.x-3.0-alpha4 » 7.x-3.0-beta1
pjcdawkins’s picture

Sorry for multiple comments. It looks like the Views query does this:

GROUP BY DATE_FORMAT((DATE_ADD('19700101', INTERVAL commerce_order.created SECOND) + INTERVAL 3600 SECOND), '%Y-%m'), field_data_commerce_order_total_commerce_order_entity_type
ORDER BY commerce_order_created_granularity DESC

This is in BST timezone (which I assume is the reason for the 3600 seconds). So that looks right.

I expect the problem lies somewhere in commerce_reports_handler_field_date::post_execute(), which I don't understand at all yet.

cvangysel’s picture

I promise I'll fix this if someone can give me steps (or settings) to reproduce the problem.

torgospizza’s picture

For me, there were no steps, except that I've been migrating old orders, products, and order line items (products) from Ubercart to Commerce. All of our data is the same, and as you can see in our earlier screenshot, all of our data is intact.

When I visit the Sales Reports page, if I'm in a "several years, broken down monthly" report, that is where the duplicate and empty months are shown. So in the screenshot I posted in #3, there is an empty March, an empty February. But if I were to run a report for that month, broken down by days or weeks, all of the product reports are there, and I'm given (presumably) correct totals for each day of those "empty" months. I haven't quite figured out where the duplicates come from, though.

Aside from the migrated data, this was a new Commerce install that I created a couple months back, and have kept up with updates as they are made available. I could theoretically wipe the database and start with the latest code, but I'm not convinced that will make a difference.

So with regard to "steps to reproduce" for us, there are none: simply run reports for a year more, broken down by month, and the errors are created. In fact even in your earlier screenshots for the module, you can see that your month of March had no sales! (http://drupal.org/files/project-images/dashboard_highcharts_0.png and http://drupal.org/files/project-images/dashboard_0.png)

Perhaps the issue is more closely related to Views and how it aggregates some of this data? I haven't had a lot of time to dig back into it myself (I'm now dealing with issues involving Commerce File) but am happy to try things if it means getting this very important functionality fixed :)

cvangysel’s picture

I'm actually guessing there's something going wrong with
DATE_FORMAT((DATE_ADD('19700101', INTERVAL commerce_order.created SECOND) + INTERVAL 3600 SECOND), '%Y-%m')
and it probably is related to timezone issues.

Could someone that's having the problem run the following query on their MySQL installation:
SELECT @@global.time_zone, @@session.time_zone;
and post the results back? Please also include the time zone configured in your PHP installation.

What also might come in handy is the raw results of the query used by Views, combined with the content generated by Views.

torgospizza’s picture

I ran the query you asked for and received "SYSTEM SYSTEM".

In our server's phpinfo() "Default Timezone" is set to America/Los_Angeles, however under "directives" there is no value set for date.timezone.

Also in the Date settings for Drupal. America/Los_Angeles is configured as the default timezone as well.

torgospizza’s picture

StatusFileSize
new42.63 KB

And here is, according to Views, the query that is run for the Yearly reports. In the Preview it results in a chart, but I can see that the chart also has the same month issues - duplicate Novembers, and nothing for March.

SELECT MIN(commerce_order.created) AS commerce_order_created_granularity, COUNT(commerce_order.order_id) AS order_id, MIN(commerce_order.order_id) AS order_id_1, 'commerce_order' AS field_data_commerce_order_total_commerce_order_entity_type, SUM(field_data_commerce_order_total.commerce_order_total_amount) AS field_data_commerce_order_total_commerce_order_total_amount, SUM(field_data_commerce_order_total.commerce_order_total_currency_code) AS field_data_commerce_order_total_commerce_order_total_currenc, AVG(field_data_commerce_order_total.commerce_order_total_amount) AS field_data_commerce_order_total_commerce_order_total_amount_1, AVG(field_data_commerce_order_total.commerce_order_total_currency_code) AS field_data_commerce_order_total_commerce_order_total_currenc_1, 'commerce_reports_sales:chart_year' AS view_name
FROM 
{commerce_order} commerce_order
LEFT JOIN {field_data_commerce_order_total} field_data_commerce_order_total ON commerce_order.order_id = field_data_commerce_order_total.entity_id AND (field_data_commerce_order_total.entity_type = 'commerce_order' AND field_data_commerce_order_total.deleted = '0')
WHERE (( (created BETWEEN 1317517874 AND 1351839600) )AND(( (commerce_order.status IN  ('pending', 'processing', 'completed')) )))
GROUP BY DATE_FORMAT((DATE_ADD('19700101', INTERVAL commerce_order.created SECOND) + INTERVAL -25200 SECOND), '%Y-%m'), field_data_commerce_order_total_commerce_order_entity_type, view_name
ORDER BY commerce_order_created_granularity ASC

See screengrab of the resulting chart, attached.

torgospizza’s picture

StatusFileSize
new6.51 KB

Running the above query through the mysql command line appears to return correct results. I've pasted it here as a text file since the columns are extremely wide.

cvangysel’s picture

Thanks! I identified the problem and I'm thinking about a fix.

I'll write it somewhere this week. I need to think about it a bit because I realise there are also implications on correctness of the solution.

torgospizza’s picture

Excellent! Let me know if there's anything else I can provide, or if you'd like to discuss your proposed solution. Happy to help.

pjcdawkins’s picture

Seconding everything torgosPizza has said; I seem to have very similar set-up, just a different timezone and different source data.

cvangysel’s picture

Status: Active » Needs review

I think I fixed this in the HEAD of 7.x-3.x, can someone verify this?

pjcdawkins’s picture

Just tested commit f94522b - it doesn't appear to have made a difference. Will try HEAD later.

pjcdawkins’s picture

Just to update: no it doesn't seem to make any difference at all.

By the way, for the overview graphs, would it be possible to utilise something like the Annotated Timeline (http://code.google.com/apis/ajax/playground/?type=visualization#annotate...)? Then the graph would take care of the date calculations. But I realise the SQL queries themselves need to be fixed to show the tabular reports.

torgospizza’s picture

Status: Needs review » Needs work

If it's still broken, I'll set this to needs work. I can't test yet but will try to soon.

pjcdawkins’s picture

Marked #1849208: Zeros appear in the weekly sales report as a duplicate of this issue.

Potentially relevant comment in that thread, by aidanlis:
http://drupal.org/node/1849208#comment-6772600

I see the bug is in the commerce_reports_handler_field_date:post_execute method. Disabling this method and the zeroes disappear. Nice handler btw ;)

cvangysel’s picture

The reason for commerce_reports_handler_field_date:post_execute is that when you query the database you only get results for the months data is available for. commerce_reports_handler_field_date:post_execute tries to fill the data that isn't available.

sher1’s picture

We having this same no sales for August 2012 but we also have 2 rows for July with different sales amounts. I have upgraded to beta2 and the speed increase is very noticeable and very appreciated but the problem is still there. When I run the views query against mysql directly (removing curly braces of course) it seems to work the way it would be expected to.

sher1’s picture

Check that. The report shows
December 2012 15 $101.00 $6.73
November 2012 281 $2,227.75 $7.93
October 2012 100 $555.75 $5.56
September 2012 127 $616.50 $4.85
August 2012 0
July 2012 200 $1,265.50 $6.33
July 2012 571 $1,015.00 $1.78
June 2012 96 $595.50 $6.20

While this query, taken from views
SELECT date_format(from_unixtime(MIN(commerce_order.created)),'%M %Y') AS order_date,
MIN(commerce_order.created) AS commerce_order_created_granularity,
COUNT(commerce_order.order_id) AS order_id,
MIN(commerce_order.order_id) AS order_id_1,
'commerce_order' AS field_data_commerce_order_total_commerce_order_entity_type,
SUM(field_data_commerce_order_total.commerce_order_total_amount) AS field_data_commerce_order_total_commerce_order_total_amount,
SUM(field_data_commerce_order_total.commerce_order_total_currency_code) AS field_data_commerce_order_total_commerce_order_total_currenc,
AVG(field_data_commerce_order_total.commerce_order_total_amount) AS field_data_commerce_order_total_commerce_order_total_amount_1,
AVG(field_data_commerce_order_total.commerce_order_total_currency_code) AS field_data_commerce_order_total_commerce_order_total_currenc_1
FROM
commerce_order
LEFT JOIN field_data_commerce_order_total ON commerce_order.order_id = field_data_commerce_order_total.entity_id AND (field_data_commerce_order_total.entity_type = 'commerce_order' AND field_data_commerce_order_total.deleted = '0')
WHERE (( (commerce_order.created BETWEEN 1322697600 AND 1357084800) )AND(( (commerce_order.status IN ('pending', 'processing', 'completed')) )))
GROUP BY DATE_FORMAT((DATE_ADD('19700101', INTERVAL commerce_order.created SECOND) + INTERVAL -25200 SECOND), '%Y-%m'), field_data_commerce_order_total_commerce_order_entity_type
ORDER BY commerce_order_created_granularity DESC
shows
December 2012 1354401743 15 2684 commerce_order 10100 0 673.3333 0
November 2012 1351796754 281 2169 commerce_order 222775 0 792.7936 0
October 2012 1349106501 100 1971 commerce_order 55575 0 555.7500 0
September 2012 1346517426 127 1714 commerce_order 61650 0 485.4331 0
August 2012 1343806642 200 1292 commerce_order 126550 0 632.7500 0
July 2012 1341154149 571 239 commerce_order 101500 0 177.7583 0
June 2012 1338579980 96 21 commerce_order 59550 0 620.3125 0

The sales totals don't match at all. Is this just some views hocus pocus? ;)

torgospizza’s picture

@sher1: See the comment above, in #20,

I see the bug is in the commerce_reports_handler_field_date:post_execute method. Disabling this method and the zeroes disappear. Nice handler btw ;)

To test this theory out, you'd have to comment out or remove that method from the module file. At the moment, this seems to indicate that the Views "post_execute" method in the date handler, which sounds like it is triggered after the Views query is executed, appears to be affecting the reports in this way.

sher1’s picture

@torgosPizza, that did it. Problem went away completely. I even read comment #20 above and it just didn't click. Thanks for pointing it out.

pjcdawkins’s picture

Please note comment #21: to display reasonable graphs, dates do have to be interpolated. The code in the commerce_reports_handler_field_date::post_execute() method is necessary. Bypassing the method is only a weak workaround. We've identified that there's probably a bug in that code. Further comments here should be about fixing that bug.

pjcdawkins’s picture

[edit: sorry, double post]

cvangysel’s picture

Status: Needs work » Needs review

I rewrote the post_execute interpolation bit using an easier to understand (but less performant) version.

The decrease in performance will not be noticeable in most cases, so I hope this fixes this problem. If you want to test the updated code, checkout the Git repository or install the development release of the module.

aidanlis’s picture

Not working here ... on both the stable and the dev I'm seeing weirdness. Whole months without sales and "today/yesterday" which don't match any of the days.

The SQL query outputs the correct data, so it's definitely that post processing in the views handler which is making weirdness.

aidanlis’s picture

If you have a high volume store and don't need interpolation (because you make a sale at least every day), this fixes everything:

diff --git a/commerce_reports b/commerce_reports
index 286d3e7..674ad7a 100644
--- a/includes/views/commerce_reports_handler_field_date.inc
+++ b/includes/views/commerce_reports_handler_field_date.inc
@@ -246,6 +246,7 @@ class commerce_reports_handler_field_date extends views_handler_field {
    * Interpolates the missing dates linearly.
    */
   function post_execute(&$values) {
+    return;
     $retrieved = array();
 
     foreach ($values as $record) {
torgospizza’s picture

Which is pretty much the same thing as commenting the entire function out :)

A hack like this is probably not an ideal fix for the module at large.

aidanlis’s picture

Right, I didn't have time to look at the interpolation logic ... but given most eCommerce sites sell at least one item per day, simply disabling the function will solve most peoples problems.

torgospizza’s picture

You may have missed the comment in #26 about focusing on fixing the actual bugs contained in the post_execute() method. Let's try to take care of the root cause rather than finding new ways to bypass it! (I'll take a look at it myself next chance I get..)

aidanlis’s picture

Ah, I did indeed miss that comment. What about letting the SQL do the work and doing a LEFT JOIN on a generated date table? That way we bypass the timezone problems which seem to be popping up too.

This would let us mark up holidays, weekends, sales, etc too.

pjcdawkins’s picture

Status: Needs review » Needs work
strings6’s picture

First, a note of thank you to the author for this module. Much appreciated!

I'm having the same issue with 7.x-3.0-beta2. I'm getting 0's for March and January. Curious-- what's the status on fixing this?

strings6’s picture

Hello,

Can anyone tell me if the 7.x-3.x-dev version fixes this issue, and if so, when it is slotted to be pushed to the recommended release version?

Thanks.

torgospizza’s picture

Status: Needs work » Reviewed & tested by the community

This actually appears to be fixed in the latest dev. I won't mark as fixed yet unless someone can confirm (and I can defer to the maintainers). But as of now it looks great - no repeated months and nothing with an erroneous 0! Great work all.

torgospizza’s picture

Status: Reviewed & tested by the community » Active

On second look it's not quite correct. If I select, say, April 2010 for my Start Date, the report itself actually starts at February 2010. So setting this back to active.

strings6’s picture

cvangysel - any near-future plans to work on this? A lot of fiscal years end on June 30, so clients will likely be wanting to use Commerce Reports to look at yearly numbers. No pressure :-)

aidanlis’s picture

Timezones are a pain in the arse ... if someone wants to rewrite it in the database layer using a GROUP BY %Y-%m we can remove all of the interpolation code.

torgospizza’s picture

I just wanted to confirm that it's definitely a timezone issue. It seems that the issue is just because of, with a difference of a couple hours, a report for June 2012 through June 2013 will take, for example, "June 1 at 7am" as all time intervals (using the MIN interpolation code) but instead of May at 7am adds and converts the first month's day as June 1st again but at midnight. This means the report goes ends up having a duplicate June 2012.

Oddly enough, setting my site's default time zone to UTC seems to be a good workaround for now.

marcus178’s picture

I just tried the dev version and it's not working right on that either, for me March is always empty. I can also confirm changing to the UTC timezone clears the problem but this is not an ideal resolution for myself

michfuer’s picture

I was running into the same types of problems when setting 'Monthly' granularity on the filter. It appears the _normalizeMonthly() function doesn't always properly return the first day of the month for a given timestamp, so in post_execute() when the $values are first normalized you can end up getting duplicates. Applying the attached patch to the 7.x-3.0-beta2 fixes the problem for me.

michfuer’s picture

I ended up running into a couple of more issues. The first was that _normalizeWeekly() was causing duplicates as well when running the reports against that granularity. I updated it to return 12am on Monday of the week corresponding to the input date. The other problem is that the $end_date in query() is calculated with hardcoded intervals (the seconds in a month, a week, etc.). This causes inconsistencies in the results because each month isn't the same interval. I added a conversion function that takes the normalized end date, and returns the appropriate query end date for a given granularity input. See the attached patch.

michfuer’s picture

This patch is the same as #45 but adds the year to the weekly and daily end dates in the conversion function. This should add a bit more robustness to the calculations.

michfuer’s picture

This adds a Daily normalization function. This is important for eliminating duplicates in post_execute() when granularity is Daily.

michfuer’s picture

So I was trying to keep all the patches independent of each other by diff'ing them against the 7.x-3.0-beta2 version. The problem is that there are enough changes in the field date handler file, that trying to apply each patch, one after the other, can cause some of the subsequent patches to fail. So this latest patch is basically a bundle of the previous ones along with the removal of pretty much all uses of the hardcoded 'interval' in the date handler.

risse’s picture

Status: Active » Reviewed & tested by the community

Checked it in version 7.x-3.0-beta2, seems to work just fine. Big thanks for this patch, I have waited for it for a while!

torgospizza’s picture

Awesome work, @michfuer! Thanks for taking the initiative.

Will the patch apply to the latest dev? I know there were some changes in 3.x-dev in an attempt to fix the sales reporting issue; would it be possible to get a reroll against the current dev version? (It appears that this is general procedure when writing patches for Drupal code, as per https://drupal.org/node/707484.)

marcus178’s picture

I've applied the last patch to beta2 but not seeing any improvement, it's still reporting zero sales for March.

pjcdawkins’s picture

Version: 7.x-3.0-beta1 » 7.x-3.x-dev
Status: Reviewed & tested by the community » Needs work

Unfortunately I had the same experience as @marcus178. Many thanks for the work though. I'm sure it fixes other bugs and may help to fix this one. But the bug isn't fixed.

Can future patches be made against -dev? I think that's important: there may be new work in -dev that needs to be rethought anyway.

michfuer’s picture

Status: Needs work » Needs review
StatusFileSize
new3.84 KB

@torgosPizza, @pjcdawkins: Yeah, I should've patched against dev, but when I first started working on it I noticed that dev had different issues of it's own (namely the Date field won't sort in the Sales report), so I took the path of least resistance since beta2 seemed to be working better.

Here's the patch against dev. The biggest difference in the dev version is that post_execute() was simplified quite a bit. The Date sort still doesn't work, but that's a problem for another issue (and day!) :)

strings6’s picture

@michfuer and @everyone else who worked on this:
You guys rock! The new dev version is working for me, looking great.

Does anyone find any reason not to push the dev version to a stable release? It is 10x's better than the beta-2 version in my opinion because the sales reports works correctly, but I'm not aware of what other issues could be wrong with the dev version. Anyone have any objections to that motion?

dpearcefl’s picture

Any other comments on the patch in #53?

strings6’s picture

Hi all,

Now I'm a bit curious, because I made the assumption that the Sept 30 update to the dev branch included this new patch from michfuer, but when we checked, it had not been included.

I reported previously that the problem appeared to be fixed in dev, so now I'm not sure why it appears to be working just fine if the patch wasn't really added. Did something else in dev fix (or perhaps mostly fix) the problem?

Our ultimate goal here is to get all the patches applied to dev that are needed, and then once everything seems to be gelling, finally update the stable release that hasn't been updated for quite some time.

Any input on that?

aidanlis’s picture

@strings6 I think the bug is mostly timezone issues so I wouldn't be surprised if it looked fixed but wasn't.

mglaman’s picture

I have had the same issue and frankly it seems like more work to fix this than to migrate Commerce Reports to utilize Views Date Format SQL module.

It took 20 minutes to switch over the Sales report and provides better date grouping in my opinion and leaves less arbitrary code to be maintained within the module.

I plan on submitting the patch in next few days, was wondering if others had feedback.

dpearcefl’s picture

Are we waiting on a patch for this issue? Just trying to understand why it's marked "Needs review".

mglaman’s picture

Please review the 7.x-4.x release which should resolve date ranges, and note module changes as stated in #2129735: Commerce Reports 4.x Review

mglaman’s picture

Issue summary: View changes
Status: Needs review » Closed (fixed)

Closing out, since granularity filter is not relevant to 4.x

  • Commit 1eac191 on 7.x-3.x, 7.x-4.x, 2023381-commerce-coupon by cvangysel:
    Issue #1781572: sales data views handler date interpolation code wasn't...