Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display Graphs in scheduled Email reports (PDF / HTML) #2706

Closed
mattab opened this issue Oct 14, 2011 · 31 comments
Closed

Display Graphs in scheduled Email reports (PDF / HTML) #2706

mattab opened this issue Oct 14, 2011 · 31 comments
Assignees
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Oct 14, 2011

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?

@mattab
Copy link
Member Author

mattab commented Oct 14, 2011

(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

Refs #1721

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

@julienmoumne
Copy link
Member

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.

@mattab
Copy link
Member Author

mattab commented Oct 20, 2011

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?!

@julienmoumne
Copy link
Member

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

@mattab
Copy link
Member Author

mattab commented Oct 24, 2011

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?

@julienmoumne
Copy link
Member

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 ?

@mattab
Copy link
Member Author

mattab commented Oct 25, 2011

  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.

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

@julienmoumne
Copy link
Member

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 ?

@mattab
Copy link
Member Author

mattab commented Oct 28, 2011

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

@julienmoumne
Copy link
Member

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?

@mattab
Copy link
Member Author

mattab commented Oct 30, 2011

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.

@julienmoumne
Copy link
Member

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.

@mattab
Copy link
Member Author

mattab commented Oct 31, 2011

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

@julienmoumne
Copy link
Member

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

@julienmoumne
Copy link
Member

Attachment:
2706.patch

@julienmoumne
Copy link
Member

Attachment:
Graphs and Tables.pdf

@julienmoumne
Copy link
Member

Attachment:
Graphs Only.pdf

@julienmoumne
Copy link
Member

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

@mattab
Copy link
Member Author

mattab commented Nov 3, 2011

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($report['dimension']);
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!

@julienmoumne
Copy link
Member

(In [5415]) fixes #2706

@julienmoumne
Copy link
Member

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)

@mattab
Copy link
Member Author

mattab commented Nov 9, 2011

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',

@mattab
Copy link
Member Author

mattab commented Nov 9, 2011

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.

@mattab
Copy link
Member Author

mattab commented Nov 9, 2011

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

@julienmoumne
Copy link
Member

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

@julienmoumne
Copy link
Member

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

@julienmoumne
Copy link
Member

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

@julienmoumne
Copy link
Member

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

@anonymous-matomo-user
Copy link

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

@mattab
Copy link
Member Author

mattab commented Dec 9, 2011

(In [5544]) Refs #2706

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

@julienmoumne
Copy link
Member

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

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

3 participants