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
Comments
(In [5296]) Fixes #2670
Refs #1721
|
Concerning
For non-historical reports, could we simply only display graphs instead of tables ? Option text would be :
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. |
Interesting idea. In this case I think we could offer these options:
I agree the wording could be better but I can't think of a better one now?! |
Does it impair user understandability to only allow those settings for a specific subset of reports (ie. non-historical reports) ? |
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? |
Concerning the three options
Wording How about replacing "non-historical reports" by "aggregate reports" or "break down reports" or something similar ? |
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.
Sounds good
Aggregated reports sounds good :) |
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 ? |
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!) |
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? |
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. |
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. |
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... |
Images as attachment using a content id appear correctly and in-place on gmail and yahoo. |
Attachment: |
Attachment: |
Attachment: |
Patch submitted for review. @review are questions left to reviewers.
''Can't upload my 'Tables Only' PDF : Submission rejected as potential spam (Content contained these blacklisted patterns: 'rx') |
julien, excellent patch!! Quite neat changes done :)
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! |
(In [5415]) fixes #2706
|
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) |
Great patch and code :)! Feedback
|
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. |
(In [5417]) Refs #2706
|
(In [5423]) refs #2706 - fixing unit tests |
(In [5424]) refs #2706 removing magic numbers |
(In [5437]) refs #2706, r5436 - API category reports also need to be excluded in the test report ($idReport == 0) |
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 |
(In [5544]) Refs #2706
|
(In [5582]) * fixes #2706, #2828, #2704, refs #1721, #2637, #2711, #2318, #71 : horizontal static graph implemented
|
Now that ImageGraph plugin is included in core (see #2670), we could add graphs in the Email reports.
Proposal graphs feature in Email reports:
Explanation:
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?
The text was updated successfully, but these errors were encountered: