Opened 3 years ago

Closed 3 years ago

Last modified 20 months ago

#2318 closed New feature (fixed)

Allow Email Reports to be sent in HTML

Reported by: JulienM Owned by: JulienM
Priority: critical Milestone: 1.4 - Piwik 1.4
Component: Core Keywords:
Cc: Sensitive: no

Description

See /trac/ticket/151#comment:11

Actions :

  • Refactor PDFRenderer as an implementation of an abstract report renderer class (Piwik_ReportRenderer)
  • Move PDFRenderer to core/ReportRenderer and rename it to Piwik_ReportRenderer_Pdf)
  • Add an HTML implementation to ReportRenderer (Piwik_ReportRenderer_Html)
  • Update PDFReport UI & database to allow sending reports in HTML

Attachments (11)

gmail-piwik-logo.jpg (130.4 KB) - added by JulienM 3 years ago.
gmail-browsers.jpg (210.2 KB) - added by JulienM 3 years ago.
msn-piwik-logo.jpg (133.1 KB) - added by JulienM 3 years ago.
msn-browsers.jpg (210.8 KB) - added by JulienM 3 years ago.
outlook2010-piwik-logo.jpg (212.1 KB) - added by JulienM 3 years ago.
outlook2010-browsers.jpg (271.7 KB) - added by JulienM 3 years ago.
thunderbird3piwik-logo.jpg (186.1 KB) - added by JulienM 3 years ago.
thunderbird3-browsers.jpg (235.8 KB) - added by JulienM 3 years ago.
yahoomail-piwik-logo.jpg (153.9 KB) - added by JulienM 3 years ago.
yahoomail-browsers.jpg (194.9 KB) - added by JulienM 3 years ago.
fixes_#2318_#2320.patch (68.6 KB) - added by JulienM 3 years ago.

Download all attachments as: .zip

Change History (53)

comment:1 follow-up: Changed 3 years ago by JulienM (JulienMoumne)

I have almost finished working on this ticket.

Images (Piwik logo, browser logos, ..) in emails have to link to a Piwik instance so they can be downloaded by email clients.

Should we add a config option with the URL of the piwik instance to use ? Or should we use

$_SERVER['SERVER_NAME']

or some sort?

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

I understand changing file names of a plugin is virtually impossible. (PDFReports -> EmailReports).

However, Is it ok to change

'name' => 'Pdf Export Plugin',

to

'name' => 'Email Report Plugin',

?

Same goes for

'PDFReports_PluginDescription' => 'Create and download your custom PDF reports, and have them emailed daily, weekly or monthly.',

'PDFReports_DefaultPDFContainingAllReports' => 'Default PDF containing all available reports.',

and many others

?

What about translations ?

comment:3 in reply to: ↑ 1 Changed 3 years ago by vipsoft (robocoder)

Replying to JulienM:

Should we add a config option with the URL of the piwik instance to use ? Or should we use

$_SERVER['SERVER_NAME']

or some sort?

See Piwik_Url.

comment:4 Changed 3 years ago by vipsoft (robocoder)

re: comment:2 it's easy to rename a plugin but a bit messy for the update script, and a PITA for translators.

That said, since pdf reports can also be downloaded, I would prefer simply "Reports".

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

Piwik_Url is what I was looking for thanks.

"Reports" is more correct indeed.

For translations, how about removing the translation for other languages so it appears as "not translated" in the translation ui ?

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

I've read the code managing database updates. I understand there is a way for plugins to define database updates the same way core does.

I have to add 'format' column to the pdf table. Should this go in core or plugin DB updates ?

comment:7 Changed 3 years ago by vipsoft (robocoder)

  • Milestone set to 1.3 - Piwik 1.3

core/Updates/

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

Excellent work mate...!

I tested report for komodo island and it worked well :)

Some feedback:

  • could logos have padding/margin right in outlook?
  • more padding in the first row
  • reuse colors / fonts / font-sizes of current Piwik UI so it looks as close as possible to current HTML
  • we could also modify PDF so they look the same as well
  • OK to modify translations for sure, but when you modify translations just change the Index of the translation, otherwise other 'outdated' translated strings will be reused. For example 'PDFReports_PluginDescription' could become 'PDFReports_PluginDescriptionReports' eg.
  • while "Reports" is maybe more correct, "Email reports" is better because it advertises the main purpose of the feature, which is to email reports. Downloading reports is rarely useful and mostly there for debugging
  • OK to put Updates in core/Updates rather than plugins/ as this is how we do it for all core plugins (when we don't it's either to show how it works, or by mistake)

I also vote for including in 1.4 as 1.3 is now final and will be released anytime.

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

  • Milestone changed from 1.3 - Piwik 1.3 to 1.4 - Piwik 1.4
  • Summary changed from Allow Email Reports to be sent in HTML to Allow Email Reports to be sent in HTML & ODT

Now the ground work is done it's very easy to add new formats. I'm going to implement Piwik_ReportRenderer_Odt (using http://www.odtphp.com/) if that's ok.

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

  • logo padding ok for all clients
  • TH padding ok for all clients
  • new scheme applied to html and pdf as well
  • added core/Updates/1.4-rc1.php

comment:11 Changed 3 years ago by vipsoft (robocoder)

It looks like the ODTPHP is license incompatible with the GPLv3. Also, development on the library seems to have tapered off -- what version of the Open Document Format does it support?

In the meantime, feel free to add support for odt, but check for the presence of the library in the event we can't include the library in the distribution.

comment:13 Changed 3 years ago by vipsoft (robocoder)

phpdocwriter is LGPL (ok), but also hasn't been updated in a while.

If odtphp is a better library, you could try asking the devs to update the license (eg from "GPLv2" to "GPLv2 or above").

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

phpdocwriter generates .sxw files http://phpdocwriter.sourceforge.net/phpdocwriter/examples/ex6.php so I'm going to send an email to the developers of ODTPHP

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

On this page http://www.odtphp.com/index.php?i=credits they link to http://www.gnu.org/copyleft/gpl.html.

In http://odtphp.svn.sourceforge.net/viewvc/odtphp/trunk/library/odf.php?revision=63&view=markup they also link to http://www.gnu.org/copyleft/gpl.html.

http://www.gnu.org/copyleft/gpl.html seems to be v3.

Do they only need to update the text in http://www.odtphp.com/index.php?i=credits from

OdtPHP is released under the terms of the GNU Public License. To get more informations about the GPL license, click here. 

to

OdtPHP is released under the terms of the GNU Public License. To get more informations about the GPLv3 license, click here. 

?

comment:16 Changed 3 years ago by vipsoft (robocoder)

It must have been the sleep deprivation -- I swear I saw "v2" somewhere. Thanks for double checking. Go head.

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

I'm running into a bit of trouble with that library.

To zip/unzip odt data they use the PclZip library.

With the latest packaged version (1.0.1) it is not possible to override the temp directory used by PclZip.

Looking at the latest revision http://odtphp.svn.sourceforge.net/viewvc/odtphp?view=revision&revision=51 second point:

- Option to choose tmp directory by appplication works correctly now

shows they have fixed the issue.

However, it has not been packaged yet.

Should I update the file myself or drop the idea of having odt export for lack of quality libraries?

comment:18 Changed 3 years ago by vipsoft (robocoder)

grab the latest from their svn

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

I should have investigated before. This library doesn't allow table generation with a variable number of columns.

They suggest to use http://pear.php.net/package/OpenDocument/

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

No table support in this library.

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

As per discussion I think safer not to implement ODT at this point.

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

  • Summary changed from Allow Email Reports to be sent in HTML & ODT to Allow Email Reports to be sent in HTML

Updates:

  • generate report as inlined content (outputType=0) is dropped. This feature is not that useful, not future proof (can any format be inlined in the browser?) and necessitate a Null Renderer so the ResponseBuilder doesn't append the success message at the end of the content.
  • toc is kept for HTML but removed for PDF (bookmarks are good enough)

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

Here is the proposed patch

  • @Review markers are remaining questions
  • includes r4415, r4505, r4506 (PDFRenderer & API).
  • styles are inlined because gmail doesn't support other means
  • fixes #2320

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

Feedback

  • @Review, should this be abstracted in a Piwik_EmailReport_x ?

looks good as is

  • rename _epilogue to _footer for consistency?
  • function Piwik_Smarty() should be function construct()
  • great work on the Piwik_ReportRenderer refactoring!
  • the comment " * To render a report as a pdf file, simply do: [...]" is not useful, because users will have to use the API to get any report, not use the low level classes. Only core will use these, and the code is clean & self documented as it is
  • I propose to change $outputFilename = PIWIK_INCLUDE_PATH . '/tmp/' . $filename; to use /tmp/assets instead. there have been permission problems with deleting files in tmp/ which I think will work better in tmp/assets since this directory is created by the webserver user, therefore there will be better umask there (or am I wrong...?)
  • @Review : Code inspired from TCPDF Plugin

Here I wouldn't put all the caching headers etc. just keep it simple (by design, none of the API is cached at present). There is currently one piece of code that allows download of a file, it is in Csv.php > renderHeader() which is a lot simpler than TCPDF code (and similar header to Csv.php is probably enough for serving HTML content)

  • don't forget to change to // $smarty->assign("currentHost", "http://".Piwik_Url::getCurrentHost()); ;)
  • are both _epilogue.tpl and _header.tpl or could it just be _header.tpl?
  • when I generate an HTML report, only the Logo/intro and TOC are displayed, no reports are displayed. But maybe this is because the patch didn't apply properly or something..?
  • in the UI, when report is set to HTML, can you display a HTML logo, for example this one looks nice.
  • also would it be possible to capitalize HTML and PDF in the UI
  • and show the icons in the SELECT box as well as before the Download button?

After these modifications, OK to commit. Very good code, great work Julien!! :)

TODO post commit

  • add integration tests to test HTML output
    • when 1 report: no TOC
    • with all reports that are metadata compatible

(I can do it when this is committed)

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

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

(In [4512]) fixes #2318 #2320

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

(In [4517]) refs #2318 Prevent "Notice undefined index" for existing (PDF) reports

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

(In [4518]) refs #2318 xss protection + working if piwik installed in sub dir (eg. in http://localhost/piwik)

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

(In [4519]) Adding link on logo. refs #2318
I think this feature is done, kuddos Julien!
I will just try and add integratio tests on the html output

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening, to not forget: HTML wont work correctly when sent from cron, since the hostname can't be read most of the time. We need to record the piwik hostname+path in a flag in the DB, that would be used in place in $smarty->assign("currentPath", Piwik_Url::getCurrentUrlWithoutFileName() );

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

  • Priority changed from normal to critical

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

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

(In [4552]) Fixes #2318, #2325 - URL will be checked for in each piwik request to UI and API (not tracker), and will be set if not already set.
then, we use this URL for the HTML email images & link to the piwik reports

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

(In [4568]) Fixing Notice:</strong> <i>Undefined variable: reportFont</i> in <b>/home/www/dev5.piwik.org/core/ReportRenderer/Pdf.php</b> on line <b>95</b> Refs #2318

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

Regression in PDF: in Greek for example, letters now appear as ???? after 1.4-rc2 upgrade (reproducible on localhost). Before, it used to display OK. Maybe a refactoring miss out?

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

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

(In [4577]) fixes #2318 refs #2320 : revert font to the one used before trying to make style uniform

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

(In [4619]) refs #2318 fixing font-size incorrectly interpreted in gmail

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

(In [4659]) refs #2318 font & size enhancements

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

(In [5313]) refs #2318 fix for r4518, xss protection broke URLs in HTML Reports, applying same protection as in datatable_cell.tpl

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

(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:40 Changed 2 years ago by JulienM (JulienMoumne)

(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:41 Changed 20 months ago by JulienM (JulienMoumne)

(In [6849]) refs #3323 #3088 #2708 #71 #2318

  • generate and compare HTML, PDF & SMS reports in Test_Piwik_Integration_EcommerceOrderWithItems & Test_Piwik_Integration_TwoVisitors_TwoWebsites_DifferentDays
  • report content as return value of PDFReports->generateReport() with new output type OUTPUT_RETURN

comment:42 Changed 20 months ago by JulienM (JulienMoumne)

(In [6938])

refs #3227

  • include back <ImageGraphUrl>

refs #2318

  • reset smarty cycle between each report
Note: See TracTickets for help on using tickets.