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
Send email if no tracked data within N days. #13363
Conversation
fe152d8
to
b2b06ab
Compare
*/ | ||
|
||
return [ | ||
'CoreAdminHome.daysToTrackedVisitsCheck' => 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we have this rather in config ini?
} | ||
} | ||
|
||
public static function isSiteEmpty($siteId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we maybe name this hasTrackedAnyTraffic
to be more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if it could be even moved out of the plugin class since it is more like a model but not too important as it was here before already anyway.
<table style="width:100%; background-color: {{ themeStyles.colorHeaderBackground }}; color: {{ themeStyles.colorHeaderText }}; padding:10px 0; margin: 0 0 25px 0; height:64px;"> | ||
<tr> | ||
<td> | ||
<a style="{{ fontStyle }}; font-size:16px;padding:0 15px;color: {{ themeStyles.colorHeaderText }};height: 22px;display: inline-block;vertical-align: inherit;" rel="noreferrer noopener" target="_blank" href="{{ piwikUrl }}" style="lineheight:17px"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to escape those attributes in a special way? like fontStyle
etc? I presume it is fine since it is straight from the theme... so maybe even need to use raw
filter? not sure...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
escaping w/ html_attr can't hurt.
plugins/CoreAdminHome/lang/en.json
Outdated
@@ -104,6 +104,10 @@ | |||
"YouMayOptOutBis": "To make that choice, please click below to receive an opt-out cookie.", | |||
"OptingYouOut": "Opting you out, please wait...", | |||
"ProtocolNotDetectedCorrectly": "You are currently viewing Matomo over a secure SSL connection (using https), but Matomo could only detect a non secure connection on the server. ", | |||
"ProtocolNotDetectedCorrectlySolution": "To make sure Matomo securely requests and serves your content over HTTPS, you may edit your %1$s file and either configure your proxy settings, or you may add the line %2$s below the %3$s section. %4$sLearn more%5$s" | |||
"ProtocolNotDetectedCorrectlySolution": "To make sure Matomo securely requests and serves your content over HTTPS, you may edit your %1$s file and either configure your proxy settings, or you may add the line %2$s below the %3$s section. %4$sLearn more%5$s", | |||
"MissingTrackingCodeEmailSubject": "%s is still missing the Matomo tracking code", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe remove the word still
. Or maybe we could say something like "No traffic for %s recorded yet, get started now".
plugins/CoreAdminHome/lang/en.json
Outdated
"ProtocolNotDetectedCorrectlySolution": "To make sure Matomo securely requests and serves your content over HTTPS, you may edit your %1$s file and either configure your proxy settings, or you may add the line %2$s below the %3$s section. %4$sLearn more%5$s" | ||
"ProtocolNotDetectedCorrectlySolution": "To make sure Matomo securely requests and serves your content over HTTPS, you may edit your %1$s file and either configure your proxy settings, or you may add the line %2$s below the %3$s section. %4$sLearn more%5$s", | ||
"MissingTrackingCodeEmailSubject": "%s is still missing the Matomo tracking code", | ||
"JsTrackingCodeMissingEmail1": "Just a quick note - we checked and your website, %s, does not seem to have the Matomo tracking code installed.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could have it a bit more neutral as it might be a mobile app or so? It might not even be the tracking code but a mobile sdk etc. Maybe we could make it dependent on the type? Or maybe we can make the message more positive. Instead of saying "you haven't done this or that" could say "To start tracking and getting insights install the tracking code now."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's really an issue for other types of sites. There's no easy way (AFAIK) to start tracking in a mobile app (I mean it will always take real developer time, they can't just copy paste some code), so focusing on websites should be ok. But I will tweak the messages to take other types of measurables into account.
plugins/CoreAdminHome/Tasks.php
Outdated
// add check for a site's tracked visits | ||
$sites = Request::processRequest('SitesManager.getAllSites'); | ||
|
||
$daysToTrackedVisitsCheck = StaticContainer::get('CoreAdminHome.daysToTrackedVisitsCheck'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder can we set this setting to 0
or -1
to disable this feature?
@@ -52,6 +53,23 @@ public function setDefaultFromPiwik() | |||
$this->setFrom($fromEmailAddress, $fromEmailName); | |||
} | |||
|
|||
public function setWrappedHtmlBody(View $body) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering... should this be maybe in a new class which can be injected etc? Then we could also replace it through DI etc... I'd say the class does otherwise maybe too many things like formatting plus setting mail etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here so derived classes can use it.
The idea in this PR is that future emails will create their own class that derived from Mail, then in event handlers plugins can detect exactly which email is about to be sent. The derived class would call this method to set the HTML body. So the methods in this class should done one of two things: configure the email to be sent, or send the email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was more thinking if it was a class, we could overwrite the wrap globally for all emails. For example on Cloud.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, overwrite just the wrapping logic, gotcha.
Updated. |
|
||
private function getDefaultSubject() | ||
{ | ||
return Piwik::translate('CoreAdminHome_MissingTrackingCodeEmailSubject', [Site::getMainUrlFor($this->idSite)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the name instead of the URL? AFAIK a URL is optional (eg not needed for mobile apps) and also reads better maybe. I was wondering if we can incorporate "analytics" or "Matomo" somehow into the subject but I can see it is tricky.
plugins/CoreAdminHome/lang/en.json
Outdated
"ProtocolNotDetectedCorrectlySolution": "To make sure Matomo securely requests and serves your content over HTTPS, you may edit your %1$s file and either configure your proxy settings, or you may add the line %2$s below the %3$s section. %4$sLearn more%5$s" | ||
"ProtocolNotDetectedCorrectlySolution": "To make sure Matomo securely requests and serves your content over HTTPS, you may edit your %1$s file and either configure your proxy settings, or you may add the line %2$s below the %3$s section. %4$sLearn more%5$s", | ||
"MissingTrackingCodeEmailSubject": "No traffic for %s recorded, get started now", | ||
"JsTrackingCodeMissingEmail1": "Just a quick note - we checked and your Matomo doesn't seem to have any recorded traffic for %s.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tested the feature and I think it may be good to start with a sentence like
A few days ago you created the site "$sitename" in your Matomo Analytics.
Just as a reminder for them to understand what they did and why they receive that mail.
plugins/CoreAdminHome/Tasks.php
Outdated
} | ||
|
||
$monthly = new Monthly(); | ||
$monthly->setTimezone('Pacific/Auckland'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume timezone is set hardcoded by accident?
plugins/CoreAdminHome/Tasks.php
Outdated
$createdTime = Date::factory($site['ts_created']); | ||
|
||
$scheduledJobTime = $createdTime->addDay($daysToTrackedVisitsCheck); | ||
if ($scheduledJobTime->isEarlier(Date::now())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been looking at this code for quite a long time and trying to understand it... I know I asked you before and you have tests but are you sure this works? I set the time to email to 1 day in config, had a created site from Sep 14th
, today is Sep17th
and it wouldn't email me. Only after I changed to isLater
it sent me that email notification. Not sure I'm seeing it correct... and I still don't get the monthly below even after your explanation how it will be sent only once :)
I might just not see it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try to clarify.
Updated, but it's still a bit funny looking. I think the scheduler is missing an ability to schedule one off tasks in the future. Will look into adding this capability. |
Yeah It's still a bit funny looking and not quite clear how often it'll be executed and how it all works etc. Maybe we just add a simple flag once the check was performed which should make it much easier to read maybe? Like $x = new MeasurableSettingsTable($idSite, 'CoreAdminHome');
$settings = $x->load();
if (empty($settings['trackingcodemissingcheck'])) {
// perform logic
$x->save(array('trackingcodemissingcheck' => 1));
} |
Updated, also noticed that Timetable::removeInactiveTasks() didn't save the timetable so inactive tasks would just stay in the timetable forever. Fixed that. |
@@ -53,6 +53,7 @@ public function removeInactiveTasks($activeTasks) | |||
unset($this->timetable[$taskName]); | |||
} | |||
} | |||
$this->save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was at first wondering if it's maybe so TasksTimetable still shows them but doesn't look like it / wouldn't make any sense. Good find :)
core/Site.php
Outdated
* @param int $idsite The site ID. | ||
* @return string|null If null, the site was created before the creation user was tracked. | ||
*/ | ||
public static function getCreatorLoginFor($idsite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the public function getCreatorLogin
method here on top for consistency?
core/Scheduler/Schedule/OneTime.php
Outdated
|
||
namespace Piwik\Scheduler\Schedule; | ||
|
||
class OneTime extends Schedule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if it is maybe a bit misleading? As a developer might expect this to be executed only once, but when using something like $schedule = new OneTime(Date::now()->getTimestamp());
it would be executed more than once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, could rename it to FixedTime
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will rename it to SpecificTime
, hopefully will be clearer.
Updated. |
FixedTime is definitely better 👍 |
@tsteur went w/ |
@diosmosis tested it and works 👍 made also a small performance tweak to it and put the name of the site in the title in quotes. LGTM. Maybe @mattab wants to review the text... subject was eg |
Looks really good 👍 |
557cdc4
to
565b63f
Compare
565b63f
to
9217388
Compare
* Remember user who created a site. * Send email if no tracked data within N days. * Add test and get to pass. * Fixes after manual tests of emails * Bump version & change column name to creator_login. * Email tweaks. * Rename Site::getCreationUserFor * Modify Site:: access methiod name * Applying PR feedback. * Move email HTML content generation logic to separate class in DI. * tweak translations * Apply PR review feedback. * Couple more tweaks. * Make tracking code check a one time task + and save timetable when removing inactive tasks. * Update save call. * Apply more PR feedback. * small performance tweak and put the site name in quotes * Fixing tests. * Update expected file.
This PR is based off of #13362.