Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#2706 closed New feature (fixed)

Display Graphs in scheduled Email reports (PDF / HTML)

Reported by: matt Owned by: JulienM
Priority: major Milestone: 1.7 Piwik 1.7
Component: Core Keywords:
Cc: Sensitive: no

Description

Now that ImageGraph plugin is included in core (see #2670), we could add graphs in the Email reports.

Proposal graphs feature in Email reports:

  • By default, we could display evolution graphs for all Simple metrics report ie. all the "Goals", "Visits summary", where the "Evolution" graph actually adds analytics value. Explanation:
    • a bar graph showing "Top browsers" or "Top keywords" is a duplicate of the dataset presented in the table: it does not add anything useful except a new visualization.
    • however a line graph showing "Ecommerce conversions" over the last 30 days (or months), or a line graph showing "Visits" for the last 30 weeks is very interesting and useful to have by default in all PDF. Because the report only shows by default the "Current month" visits/ecommerce orders...
  • Also, some users like looking at graphs. So, for them, we could add an option in the report User interface: "Also include a Graph next to each dataset. Note: by default, some reports display an historical line graph (Goals, Ecommerce, Visits), but when you enable this feature all the other reports will display a vertical bar graph."

Ideally the graph and data set would both fit on 1 page but I'm not sure how we can make it look nice.

Thoughts?

Attachments (3)

2706.patch (34.5 KB) - added by JulienM 2 years ago.
Graphs and Tables.pdf (672.4 KB) - added by JulienM 2 years ago.
Graphs Only.pdf (563.7 KB) - added by JulienM 2 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 3 years ago by matt (mattab)

(In [5296]) Fixes #2670

  • Metadata output now contains "imageGraphUrl"
  • the function used by piwik mobile and others, API.getReportMetadata now accepts period and date parameters
    • for "Evolution graphs" (goals, visitssummary, etc.) we must "plot" multiple days previous to the currently selected date. The metadata APi automatically does the $date parameter rewrite, when period != range, and date is not already a range. Therefore the imageGraphUrl can be used as is.
  • New page available at: index.php?module=ImageGraph&action=index&idSite=1&period=month&date=2011-04-17
    • This page will call metadata APi then display the graph for all existing reports. This is a useful debug/test page to view all graphs as they will be displayed in Piwik Mobile (and PDF reports later refs #2706

Refs #1721

  • Showing less vertical bars in graph by default
  • Displaying empty graph when there is no data

comment:2 Changed 3 years ago by JulienM (JulienMoumne)

  • Owner set to JulienM

comment:3 Changed 3 years ago by JulienM (JulienMoumne)

  • Status changed from new to assigned

comment:4 Changed 3 years ago by JulienM (JulienMoumne)

Concerning

add an option in the report User interface: "Also include a Graph next to each dataset. Note: by default, some reports display an historical line graph (Goals, Ecommerce, Visits), but when you enable this feature all the other reports will display a vertical bar graph."

and

Ideally the graph and data set would both fit on 1 page but I'm not sure how we can make it look nice.


For non-historical reports, could we simply only display graphs instead of tables ?

Option text would be :

For non-historical reports, display a graph instead of a table

In that case I understand we need to come up with better wording.

We could even go further and keep an option to insert both tables and graphs.

It all boils down to users' needs.

comment:5 Changed 3 years ago by matt (mattab)

Interesting idea. In this case I think we could offer these options:

  • For non historical reports:
    • Display Reports tables only (default)
    • Display Graphs only
    • Display Graphs and tables

I agree the wording could be better but I can't think of a better one now?!

comment:6 Changed 3 years ago by JulienM (JulienMoumne)

Does it impair user understandability to only allow those settings for a specific subset of reports (ie. non-historical reports) ?

comment:7 Changed 2 years ago by matt (mattab)

I think it's OK to "force" always display evolution graph above "VisitsSummary" "Goals summary", etc. since they are very useful (and I really see it as a missing feature that they are currently not displayed). Does it sound good?

comment:8 follow-up: Changed 2 years ago by JulienM (JulienMoumne)

Concerning the three options

* For non historical reports:
    o Display Reports tables only (default)
    o Display Graphs only
    o Display Graphs and tables

1) What is the default ? (For new reports and reports created before this evolution)

2) Shall I add a new column in the _pdf table ?

Wording

How about replacing "non-historical reports" by "aggregate reports" or "break down reports" or something similar ?

comment:9 in reply to: ↑ 8 Changed 2 years ago by matt (mattab)

1) What is the default ? (For new reports and reports created before this evolution)

Default is to NOT show graphs for non historical.

But, previous reports and newly created reports will ALWAYS show the graphs for VisitsSummary, Goals, etc.

2) Shall I add a new column in the _pdf table ?

Sounds good

Wording

How about replacing "non-historical reports" by "aggregate reports" or "break down reports" or something similar ?

Aggregated reports sounds good :)

comment:10 Changed 2 years ago by JulienM (JulienMoumne)

Shall we add/edit a column in the report list to display which setting is currently applied to a report ? Shall we use icons ? text ?

In the edit form, should we use a drop down list with the 3 options ?

comment:11 Changed 2 years ago by matt (mattab)

I think it's ok NOT to display it in the list. Mostly, people will leave the default which is only display graphs for historical report. It's OK to just display the option when editing/creating a report (less noise!)

comment:12 Changed 2 years ago by JulienM (JulienMoumne)

Concerning HTML reports, at first I was thinking that we could simply include an IMG markup with a SRC attribute linking to the Piwik instance (same mechanism for Piwik and country logo).

However, when anonymous read access is not activated, it means the token auth is sent in the email content.

Is this ok?

comment:13 Changed 2 years ago by matt (mattab)

Good point. Sending token_auth in clear text is not a good thing to do (no existing Piwik core mechanism leak the token_auth). I think it's safer if the IMG is fetched during report generation and then included in the HTML/PDF directly.

comment:14 Changed 2 years ago by JulienM (JulienMoumne)

For PDF reports, no problem.

For HTML reports, we're in a pickle.

As I understand, there are two ways to include the binary content of images in an e-mail. One uses the src="data:BASE64_ENCODED_IMAGE" and one uses e-mail attachment. According to http://www.campaignmonitor.com/blog/post/1761/embedding-images-in-email/ and http://www.campaignmonitor.com/blog/post/1759/embedding-images-revisited/ it seems those methods are not universally supported by mail clients.

comment:15 Changed 2 years ago by matt (mattab)

Good point. We can do the "Image as attachment" solution which works in Desktop clients, better than nothing. Otherwise, for mail clients it will not work for HTML report. This is an acceptable limtation. Better than disclosing the token_auth for sure!!

An alternative solution would involve only enabling graphs to be rendered for a given token, added in the email for this report only, and The Report plugin would act as a proxy to the ImageGraph API and give the proper token_auth if the second token was provided correctly... But too complicated for the gain here I think?

Since it works as attacment in desktop clients + in PDF reports, it sounds good...

comment:16 Changed 2 years ago by JulienM (JulienMoumne)

Images as attachment using a content id appear correctly and in-place on gmail and yahoo.

Changed 2 years ago by JulienM (JulienMoumne)

comment:17 Changed 2 years ago by JulienM (JulienMoumne)

Patch submitted for review. @Review are questions left to reviewers.

  • HTML Reports:
  • when downloaded, raw image data is included using
src="data:image/png;base64,DATA_STRING"
  • when sent by mail, images are sent as attachments using cid (tested with thunderbird, gmail & yahoo)
src="cid:{$reportId}"
  • PDF Reports:
  • Display 3 graphs per page when 'Aggregate Report Format' = 'Display Graphs Only'.
  • We need to come-up with a better page-fitting algorithm for 'Display Tables Only' to avoid as much blank space as possible


  • Metadata API:
  • 'Goals_Ecommerce' category appears first when calling getReportMetadata. 'sort' has been updated to restore 'VisitsSummary' first
  • ImageGraph Plugin
  • additional security
  • new outputType to retrieve the generated image as a PHP return variable
  • filename fixes
  • DB Update:
  • 1.7.php contains the update code for 'piwik_pdf'

Can't upload my 'Tables Only' PDF : Submission rejected as potential spam (Content contained these blacklisted patterns: 'rx')
Nor the HTML report : Submission rejected as potential spam (Content contained these blacklisted patterns: 'DJ', 'rx')

comment:18 Changed 2 years ago by matt (mattab)

  • Milestone changed from 1.7 Piwik 1.7 to 1.6.x Piwik 1.6.x
  • Priority changed from normal to major

julien, excellent patch!! Quite neat changes done :)

  • Have you had chance to check Outlook? I know it sucks, but so many people still use it.

Should we add an "alt" attribute ?

Yes, empty probably

@Review 'isAggregateReport' report should ideally be in the metadata ? or somewhere in core ?

Actually I propose to remove the helper function, and simply have a local variable every time it's used: isAggregateReport = !empty($reportdimension?);
There is no need to add overhead in the metadata in this case since it is simply a boolean check.

@Review : Is this the right place ?

Idem, simply remove function

For the spam, you can upload a zip with all files, then it doesn't check for spam ;) tricky but this trac is a beast to modify and we run a 4 yo update haha...

Looks good otherwise!

comment:19 Changed 2 years ago by JulienM (JulienMoumne)

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [5415]) fixes #2706

  • refs #2318, #71 : Graphs now supported
  • refs #2670, #898 : Restoring VisitsSummary report metadata before eCommerce
  • refs #1721 : Additional security, filename fixes and new internal outputType

comment:20 Changed 2 years ago by JulienM (JulienMoumne)

Commit r5415 includes all remarks from comment:18

Tested on outlook 2010

Includes a better page-fitting algorithm for table only reports (mentioned in comment:17)

comment:21 Changed 2 years ago by matt (mattab)

  • Resolution fixed deleted
  • Status changed from closed to reopened

Great patch and code :)!

Feedback

  • PDFReports test is failing
         [exec] <span class="fail">Exception</span>: /home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/tests/PDFReports.test.php -&gt; Test_Piwik_PDFReports -&gt; test_addReport_getReports -&gt; <strong>Unexpected exception of type [Exception] with message [General_ExceptionInvalidAggregateReportsFormat] in [/home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/API.php line 666]</strong><br />
         [exec] <span class="fail">Exception</span>: /home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/tests/PDFReports.test.php -&gt; Test_Piwik_PDFReports -&gt; test_getReports_invalidPermission -&gt; <strong>Unexpected exception of type [Exception] with message [General_ExceptionInvalidAggregateReportsFormat] in [/home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/API.php line 666]</strong><br />
         [exec] <span class="fail">Exception</span>: /home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/tests/PDFReports.test.php -&gt; Test_Piwik_PDFReports -&gt; test_updateReport -&gt; <strong>Unexpected exception of type [Exception] with message [General_ExceptionInvalidAggregateReportsFormat] in [/home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/API.php line 666]</strong><br />
         [exec] <span class="fail">Exception</span>: /home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/tests/PDFReports.test.php -&gt; Test_Piwik_PDFReports -&gt; test_deleteReport -&gt; <strong>Unexpected exception of type [Exception] with message [General_ExceptionInvalidAggregateReportsFormat] in [/home/www/data/root/jenkins.private/jobs/Piwik/workspace/build/plugins/PDFReports/API.php line 666]</strong><br />
    
    
  • Not directly related but would be nice to refactor all strings in the lang file that look like:
     	'General_ExceptionMethodNotFound' => 'The method \'%s\' does not exist or is not available in the module \'%s\'.',
     	'General_ExceptionInvalidRendererFormat' => 'Renderer format \'%s\' not valid. Try any of the following instead: %s.',
     	'General_ExceptionInvalidReportRendererFormat' => 'Report format \'%s\' not valid. Try any of the following instead: %s.',
    +	'General_ExceptionInvalidAggregateReportsFormat' => 'Aggregate reports format \'%s\' not valid. Try any of the following instead: %s.',
     	'General_ExceptionInvalidPeriod' => 'The period \'%s\' is not supported. Try any of the following instead: %s',
    


comment:22 Changed 2 years ago by matt (mattab)

It would be nice to fix the Graphs legending because most graphs are currently useless because X axis does not show labels.... see #2704

I love the new fitting algorithm and 3 graphs in the same page, looks very nice.

comment:23 Changed 2 years ago by matt (mattab)

(In [5417]) Refs #2706

  • Moved to version 1.7-b1 and rename so that update is picked up
  • Simplify UI and text to clarify the feature (hopefully ;)
  • Fix 1 integration test (1 other unit test is failing)

comment:24 Changed 2 years ago by JulienM (JulienMoumne)

(In [5423]) refs #2706 - fixing unit tests

comment:25 Changed 2 years ago by JulienM (JulienMoumne)

(In [5424]) refs #2706 removing magic numbers

comment:26 Changed 2 years ago by JulienM (JulienMoumne)

(In [5436]) refs #2706, #1454 API category reports excluded from PDFReports Plugin forms

comment:27 follow-up: Changed 2 years ago by JulienM (JulienMoumne)

(In [5437]) refs #2706, r5436 - API category reports also need to be excluded in the test report ($idReport == 0)

comment:28 in reply to: ↑ 27 Changed 2 years ago by notevil

Hello Everyone I am new in piwik code I have a bug in the implementation of patch graphs Support Email reports in 2706 and gives me error as: Fatal error: Call to undefined method Piwik_Common: json_encode () in ... \ plugins \ PDFReports \ Controller.php on line 47

comment:29 Changed 2 years ago by matt (mattab)

(In [5544]) Refs #2706

  • Scheduled reports should generate when GD is disabled (but without graph)
  • refactored gd check

comment:30 Changed 2 years ago by JulienM (JulienMoumne)

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [5582]) * fixes #2706, #2828, #2704, refs #1721, #2637, #2711, #2318, #71 : horizontal static graph implemented

  • fixes #2788, refs #2670, #1721, #2637, #2711 : default graph type logic moved to ImageGraph API, improved date/period logic, new parameter graphs_default_period_to_plot_when_period_range
  • fixes #2704, #2804, refs #1721 : pChart updated to 2.1.3, pChart code removed from Piwik code, OOP refactoring, support for unifont.ttf if present in ImageGraph/fonts, testAllSizes now uses report metadata ImageGraphUrl
  • refs #71 : space between report title and report table reduced to avoid page overflow
  • refs #2829 : TODO display percentages
  • r5544, r5547, r5549 merged

comment:31 Changed 2 years ago by matt (mattab)

  • Summary changed from Support graphs in Email reports to Display Graphs in scheduled Email reports (PDF / HTML)
Note: See TracTickets for help on using tickets.