I originally posted this issue here ...
http://www.ubercart.org/issue/4178/rc2_custom_sales_report_broken_date_r...
but have since figured out that the bug is related to the order status that are being passed on the query string. If you choose the "custom sales summary" the stock report works fine. If you try to customize the date range and the statues returned, the results get funny. The bug is related to the piece of code that sets the status for the query string ...
function uc_reports_sales_custom_form_submit($form_id, $form_values) {
...
foreach ($form_values['status'] as $order_status => $title) {
$status .= $order_status .', ';
}
...
}This for loop adds a comma and a space to each status of in the query string . The request gets forwarded using drupal_goto() and the report is run. But, the report does matching on the query string for statuses. The problem is that the last status, in most default installs, the completed status, has a comma appended if you check the query string. This causes the query to fail.
To see what I mean, run this report and then remove the trailing "%2C" from the status query string element. Then you'll see the "completed" statuses showup. I say completed because the default weight of completed is "20" which makes it last in the list. You'll see different behaviour if your status weight order doesn't ending with "completed."
To fix the bug, I'd suggest splitting the statuses on the comma and not appending the space in the first place. At line 563, you'll see where the problem comes in after the above issue ...
$order_statuses = (!$args['status']) ? _uc_reports_order_statuses() : "('". implode("', '", $args['status']) ."')";So, it's trying to implode on comma space -- but the last value doesn't have a comma space, just comma. So, the trailing comma is retained and causes the bug.
I hope this helps, definitely a bug.
implode() doesn't split
implode() doesn't split strings, it combines array elements into one string and glues them together with "', '". That's getting put into an SQL query, so spaces between the commas don't matter.
No, the real problem is in the first bit of code you gave, which adds the ', ' to the end of the $status variable. The following line takes off one character, when it should take two. This came about through a naive search and replace to make the coder.module happy with the source.
Thanks for finding it.
Re: implode() doesn't split
So... it was making the URL right but not parsing it. Fixed that. Also, I'm rewriting that bit of code (and it will be hard to restrict myself to just that part) to get rid of the use of GET variables.
Re: implode() doesn't split
Sorry, I wrote it up late -- implode and explode get more blurry as the evening rolls on! Thanks for fixing it.
Re: Re: implode() doesn't split
I've committed for testing some updates to the reports module. Specifically, please review the three different sales reports for accuracy.
There is a known bug that I've wasted too much time trying to solve with custom sales reports. For some reason, with monthly reports it's not calculating December right, and it always screws up for February.
I think we may need to rework the way reports are generated in the future, but this just may have to be a bug (although it's not necessarily a bug... it may be working exactly as PHP intends it to work) for the 1.0 version.
Please reply in this thread.
Re: Custom Sales Report Bug - Status matching with extra space.
Hey Ryan, thanks. I'll check it out. Have you considered the timezone offset could be causing you headaches? There's the server time, mysql, php, drupal and ubercart (should just inherit Drupal). I've been wondering about windowing and leap year implications for different offsets. Also, what happens when the server and mysql time don't match Drupal time? I know this is a bit vague, I just haven't had time to fully explore it. Thanks for your hard work.
Re: Re: Custom Sales Report Bug - Status matching with extra spa
Yeah, I haven't decided to what extent the timezone offset is giving me grief. We're calculating the timestamps in GMT using gmmktime(), and then we're adding the Drupal timezone offset to that. I believe that part is working properly, but there could always be a misapplication of it in there somewhere.