@mattab opened this Issue on July 25th 2012 Member

It would be useful to visualize a "Traffic by Day Of Week" graph, that would look at the last 7 * N weeks to create a graph.

Follow up to Benaka patch in #3184

@diosmosis commented on July 25th 2012 Member

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

@diosmosis commented on August 2nd 2012 Member

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

@diosmosis commented on July 25th 2012 Member

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

@diosmosis commented on July 25th 2012 Member

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

@mattab commented on July 27th 2012 Member

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

@diosmosis commented on July 27th 2012 Member

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?

@mattab commented on July 29th 2012 Member

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

@diosmosis commented on July 30th 2012 Member

(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.
@mattab commented on July 31st 2012 Member

Thanks for commit, found some issues:

@diosmosis commented on July 31st 2012 Member

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

@diosmosis commented on July 31st 2012 Member

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.

@mattab commented on August 1st 2012 Member

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.

@diosmosis commented on August 2nd 2012 Member

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

@mattab commented on August 2nd 2012 Member

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 :)

@diosmosis commented on August 2nd 2012 Member

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.

@diosmosis commented on August 2nd 2012 Member

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.

@mattab commented on August 2nd 2012 Member

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!

@diosmosis commented on August 3rd 2012 Member

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 :)

@diosmosis commented on August 3rd 2012 Member

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

@mattab commented on August 3rd 2012 Member

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.
@julienmoumne commented on August 3rd 2012 Member

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.
@diosmosis commented on August 4th 2012 Member

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?

@mattab commented on August 4th 2012 Member

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.

@diosmosis commented on August 6th 2012 Member

(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.

@diosmosis commented on August 6th 2012 Member

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

@diosmosis commented on August 6th 2012 Member

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

@mattab commented on August 15th 2012 Member

Cool feature :)

This Issue was closed on August 15th 2012
Powered by GitHub Issue Mirror