@pokoli opened this issue on February 4th 2013

This PR enables three new Parameters in Email Reports: - Enable Custom Email Text Activates whether custom text is Used or Not. Also enables/disables the other two parameters on the UI. - Custom Email Subject The text to be used as the subject of the Email - Custom Email Text The text to be used as the introduced text of the Email. "You can'd find the atached report text is preserverd".

For me is quiete usable now. What do you think?

@julienmoumne commented on February 4th 2013

Could you elaborate why the 'customEmailText' parameter is persisted? Couldn't you simply check for non null 'emailSubject' & 'emailContent'?

Is there no sanitizing to perform on those new parameters?

@mattab commented on February 4th 2013

I'm not sure if we want to do this... ?

if we do, at least these option should be "hidden" by default in the form, and only acessible on "click" on "advanced settings" or similar, since very few people would use that feature.

@faceleg commented on February 4th 2013

Likely that feature would be useful for those using "white labelled" installations.

@pokoli commented on February 4th 2013

Al 04/02/13 23:40, En/na Matthieu Aubry ha escrit:

I'm not sure if we want to do this... ?

if we do, at least these option should be "hidden" by default in the form, and only acessible on "click" on "advanced settings" or similar, since very few people would use that feature.

As it's implemented it's "hidden" by default, you have to check the customize email texts to enable this feature. Is this sufficient?

Could you elaborate why very few people would use this feature?

@pokoli commented on February 4th 2013

@JulienMoumne:

Could you elaborate why the 'customEmailText' parameter is persisted? Couldn't you simply check for non null '>emailSubject' & 'emailContent'?

If you persist the customEmailText you can disable the custom text and don't lose your emailSubject & emailContent texts. I don't know if that is a suficient reason to store and adition parameter to the database.

I can remove this parametre and simple check for null if you prefer.

Is there no sanitizing to perform on those new parameters?

Of course, but i don't know the best way to do it in this case, as the variable is only printed to an HTML email. Should htmlspecialchars[1] be suficient for this use case?

[1] http://www.php.net/manual/en/function.htmlspecialchars.php

@halfdan commented on February 17th 2013

Close/Reopen to trigger Travis-CI build.

@mattab commented on February 20th 2013

Thank you for proposing this, I really appreciate, but I prefer not to go ahead and add complexity in the emais at this stage. Later we will add some features in emails and we can revisit. Thanks for your understanding!

@pokoli commented on February 20th 2013

IMHO it doesn't add any complexity as you mustn't define this settings if you don't want, you can use the default settings if desired.

If you want to review it later i could understand. Could you please leave this issue open so it will be easier to find to anybody interesed?

Matthieu Aubry notifications@github.com ha escrit:

Thank you for proposing this, I really appreciate, but I prefer not to go ahead and add complexity in the emais at this stage. Later we will add some features in emails and we can revisit. Thanks for your understanding!

Reply to this email directly or view it on GitHub: https://github.com/piwik/piwik/pull/23#issuecomment-13855335

@mattab commented on February 20th 2013

I'm not sure best practise of github, but I rather leave it closed so it doesnt clutter (and take some mental effort to remember we already decided not to include it). You are very welcome to create a ticket at: dev.piwik.org with the patch there so anyone could apply it if they wish (in "Feature requests") . Thanks!

@robocoder commented on February 21st 2013

I agree with Matt. If a PR is being rejected, then the PR should be closed. Trac can be used to track the feature request.

@robocoder commented on February 21st 2013

http://dev.piwik.org/trac/ticket/3723

This issue was closed on February 20th 2013
Powered by GitHub Issue Mirror