Attachment: New patch for this issue.
3184.diff.tar.gz

Added a new patch for this issue. Removed the extra filter in my last patch. Let me know if it's good to commit.

Ignore the core/DataTable/Filter/Callback.php file in my patch. It's just noise.

Very nice!

• Rename day_of_week_n -> day_of_week
• Why the new code:

+       
+       if (is_string($columnToSumValue) && $thisColumnValue === false)
+       {
+           return $columnToSumValue;
+       }
+       

Is this code necessary?

Otherwise, looks great, please commit

Replying to matt:

Very nice!

• Rename day_of_week_n -> day_of_week
• Why the new code:

+     
+     if (is_string($columnToSumValue) && $thisColumnValue === false)
+     {
+         return $columnToSumValue;
+     }
+     

Is this code necessary?

The code is so addDataTable will work when column values are strings. For this patch, VisitsSummary.get sets bounce_count to 'X%' and not 0, so it won't add the column to an empty row w/o this code. If there's a better way to accomplish this, let me know. Otherwise I'll commit?

as discussed, it's lacking the Exception for string + string use case, otherwise looks OK!

(In [6596]) Fixes #3275, add 'Visits by Day of Week' report to VisitTime as related report for 'Visits per local time'.

Notes:

• Added ability for GenerateGraphHTML to pass data to GenerateGraphData class.
• Added ability for reports to show all x-axis ticks on bar graphs if desired.
• Modified DBStats overview report to show all x-axis ticks on bar graph.
• Fixed bug in datatable summing where string column values would not get added even if there was no corresponding value in one table.

Thanks for commit, found some issues:

• when selecting Month as period, I get the exception "Trying to add two strings values in DataTable_Row::sumRowArray."
  • when hovering on the bar graph, there is this JS error many times:```
    TypeError: this.params.axes.xaxis.labels is undefined
    http://localhost/trunk/plugins/CoreHome/templatesjqplot.js?cb=bde08bd5662a74cd78d3842713135da8
    Line 455 var label = this.params.axes.xaxis.labels[i];

(In [6618]) Refs #3275, fix bar graph regression & made exception message in sumRowArray more descriptive.

Replying to matt:

Thanks for commit, found some issues:

• when selecting Month as period, I get the exception "Trying to add two strings values in DataTable_Row::sumRowArray."

I can't reproduce this issue... Do you know what report is causing it? Maybe it's due to archiving some report? Though the tests should fail in that case.

On the demo you can reproduce the issue: Trying to add two strings values in DataTable_Row::sumRowArray: '58%' + '57%'

I guess it happens if at least 2 days with data on the same week day in the selected period.
For example at this url

Also I noticed another bug: ordering of week days works well by default, but when selecting another metric in the metric picker, the days are not ordered anymore, appearing in random order.

Attachment: Patch to fix bug & show goals info.
3275.diff.tar.gz

I put up a patch that will fix the bug and show goals related info. What do you think of it?

Nice try, it looks like it's working but I think this code is too complex, and would require more refactoring for example to re-use the existing Goal summing code in: enrichConversionsByLabelArray()

However I don't think we want to do this as it's probably too complicated.

Can you please just commit the change to make it work for Months and leave out the Goal icon and metrics? we can revisit later but I don't think it matters too much, and the added code complexity is not worth the benefits here :)

Replying to matt:

Nice try, it looks like it's working but I think this code is too complex, and would require more refactoring for example to re-use the existing Goal summing code in: enrichConversionsByLabelArray()

However I don't think we want to do this as it's probably too complicated.

Can you please just commit the change to make it work for Months and leave out the Goal icon and metrics? we can revisit later but I don't think it matters too much, and the added code complexity is not worth the benefits here :)

The Months issue is a problem w/ the approach I took. Using VisitsSummary.get & GroupBy means sumRow will be called w/ many string values (which are percents). The only way to get it to work (that I can think of) is to get the metrics by hand. I don't do any summing though... I just move the individual Goal_X_metric metrics into 'goals' => array(X => array(metric => value, ...)) structures.

Replying to matt:

Nice try, it looks like it's working but I think this code is too complex, and would require more refactoring for example to re-use the existing Goal summing code in: enrichConversionsByLabelArray()

However I don't think we want to do this as it's probably too complicated.

Can you please just commit the change to make it work for Months and leave out the Goal icon and metrics? we can revisit later but I don't think it matters too much, and the added code complexity is not worth the benefits here :)

To clarify, are you ok w/ the changes to AddColumnsProcessedMetrics.php and the use of getDataTableNumeric? Do you mind if I keep the changes to AddColumnsProcessedMetricsGoal.php (even though w/o the goal related code, it won't be used)? If so, I'll commit those changes.

Actually that function in the filter is already available in: Piwik_ArchiveProcessing::getCoreMetrics()

Don't leave the changes in AddColumnsProcessedMetricsGoal since unused code shouldn't be there.

I thought some more and the correct way to do this, would be to request the datatable while disabling generic/queued filters. Then you get the raw datatable. Then we would use the core archiving function such as: updateInterestStats() and updateGoalStats() which contains the logic to sum the rows, then we would apply the filters

But that will be for later since it's non trivial!

Replying to matt:

Actually that function in the filter is already available in: Piwik_ArchiveProcessing::getCoreMetrics()

Don't leave the changes in AddColumnsProcessedMetricsGoal since unused code shouldn't be there.

Ok, will commit soon.

I thought some more and the correct way to do this, would be to request the datatable while disabling generic/queued filters. Then you get the raw datatable. Then we would use the core archiving function such as: updateInterestStats() and updateGoalStats() which contains the logic to sum the rows, then we would apply the filters

But that will be for later since it's non trivial!

I don't think this will work. VisitsSummary.get uses ->filter not ->queueFilter. It could be changed, but I don't think there's a way to disable filters queued on the DataTable, only on the ViewDataTable. Or maybe I'm missing something :)

(In [6658]) Fixes #3275, fix issue w/ VisitTime.getByDayOfWeek report where sumRow complained of strings being added.

Reopening, as I have further suggesions to make this new feature more visible and useful!

• Widget: add a widget in the same category as Server time for this new report.
• Metadata: the report should appear in the metadata when period!=day so that it's available to Piwik Mobile and PDF/HTML reports. Let's check that it works fine in html/pdf reports. It should not generate for daily reports even if it was somehow selected in the UI. In fact the report could maybe be hidden from the UI when period=day.
• When period=day, the widget would load and use period=month (ie.4 weeks in the month) to build the report.
  • However I think it's better not to display the link below the Time of visits graphs for period=day as it currently is. Only the widget should work to make sure it works in all cases since users wouldn't know this particular use case. The message in footer make clear the dates used.

Replying to matt:

• Metadata: the report should appear in the metadata when period!=day so that it's available to Piwik Mobile and PDF/HTML reports. Let's check that it works fine in html/pdf reports. It should not generate for daily reports even if it was somehow selected in the UI. In fact the report could maybe be hidden from the UI when period=day.

I do not think this report should be hidden when period=day :

• a report can be configured to be sent daily but the user can download it with a different period, e.g: month, by using the calendar. In that case, the user would never be able to preview this new report
• when editing a report, switching from daily to weekly/monthly would display back this new report. If the user does not realize this, he might not select it. Should it actually be automatically ticked in that case?
• handling this case either requires specific code or substantial refactoring if you want it to be generic (ie. having period dependent reports). In either cases, I believe the code complexity is way too high for the added value of hiding/showing this report. When period=day, the report could simply show as usual "No data for this report". If it annoys the user, he could simply disable it.

Regarding what should be shown when period=day:

Right now, the API will return the report for period=day as it would for any other period. The result will show a bunch of visits for that one day, and 0 visits for the rest. This information is useless, but I don't think it would be a problem to show it
in the widget & report. What do you guys think?

it's OK to show the widget with period=day and have it show only 1 column of data (for selected day of week), users should understand that it will work when larger date range is selected.

(In [6679]) Refs #3275 & refs #3192, added report metadata for visits by day of week report and added said report as widget. Also moved code that removed filter_limit/offset params for graphs so related reports wouldn't be affected by #3192 bug.

(In [6680]) Refs #3275, fix build

(In [6681]) Refs #3275, merge two translations.

Cool feature :)