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

Merged
merged 23 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@diosmosis
Member

diosmosis commented Aug 29, 2018

This PR is based off of #13362.

@diosmosis diosmosis added this to the 3.7.0 milestone Aug 29, 2018

@mattab mattab modified the milestones: 3.7.0, 3.6.1 Sep 1, 2018

@diosmosis diosmosis force-pushed the tracking-code-reminder branch from fe152d8 to b2b06ab Sep 7, 2018

*/
return [
'CoreAdminHome.daysToTrackedVisitsCheck' => 5,

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

Would we have this rather in config ini?

}
}
public static function isSiteEmpty($siteId)

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

Would we maybe name this hasTrackedAnyTraffic to be more clear?

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

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

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

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

This comment has been minimized.

@diosmosis

diosmosis Sep 9, 2018

Member

escaping w/ html_attr can't hurt.

@@ -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",

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

I would maybe remove the word still. Or maybe we could say something like "No traffic for %s recorded yet, get started now".

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

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

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

This comment has been minimized.

@diosmosis

diosmosis Sep 9, 2018

Member

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.

// add check for a site's tracked visits
$sites = Request::processRequest('SitesManager.getAllSites');
$daysToTrackedVisitsCheck = StaticContainer::get('CoreAdminHome.daysToTrackedVisitsCheck');

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

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

This comment has been minimized.

@tsteur

tsteur Sep 8, 2018

Member

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.

This comment has been minimized.

@diosmosis

diosmosis Sep 9, 2018

Member

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.

This comment has been minimized.

@tsteur

tsteur Sep 10, 2018

Member

I was more thinking if it was a class, we could overwrite the wrap globally for all emails. For example on Cloud.

This comment has been minimized.

@diosmosis

diosmosis Sep 10, 2018

Member

I see, overwrite just the wrapping logic, gotcha.

@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 10, 2018

Updated.

private function getDefaultSubject()
{
return Piwik::translate('CoreAdminHome_MissingTrackingCodeEmailSubject', [Site::getMainUrlFor($this->idSite)]);

This comment has been minimized.

@tsteur

tsteur Sep 17, 2018

Member

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.

}
$monthly = new Monthly();
$monthly->setTimezone('Pacific/Auckland');

This comment has been minimized.

@tsteur

tsteur Sep 17, 2018

Member

I presume timezone is set hardcoded by accident?

$createdTime = Date::factory($site['ts_created']);
$scheduledJobTime = $createdTime->addDay($daysToTrackedVisitsCheck);
if ($scheduledJobTime->isEarlier(Date::now())) {

This comment has been minimized.

@tsteur

tsteur Sep 17, 2018

Member

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

image

I might just not see it though.

This comment has been minimized.

@diosmosis

diosmosis Sep 18, 2018

Member

Will try to clarify.

diosmosis added some commits Sep 18, 2018

@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 18, 2018

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.

@tsteur

This comment has been minimized.

Member

tsteur commented Sep 18, 2018

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));
    }
@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 18, 2018

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

This comment has been minimized.

@tsteur

tsteur Sep 19, 2018

Member

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

* @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)

This comment has been minimized.

@tsteur

tsteur Sep 19, 2018

Member

Can you add the public function getCreatorLogin method here on top for consistency?

namespace Piwik\Scheduler\Schedule;
class OneTime extends Schedule

This comment has been minimized.

@tsteur

tsteur Sep 19, 2018

Member

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?

This comment has been minimized.

@diosmosis

diosmosis Sep 19, 2018

Member

Hmm, could rename it to FixedTime?

This comment has been minimized.

@diosmosis

diosmosis Sep 19, 2018

Member

Will rename it to SpecificTime, hopefully will be clearer.

@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 19, 2018

Updated.

@tsteur

This comment has been minimized.

Member

tsteur commented Sep 19, 2018

FixedTime is definitely better 👍

@diosmosis

This comment has been minimized.

Member

diosmosis commented Sep 19, 2018

@tsteur went w/ SpecificTime, since new FixedTime(Date::now()->getTimestamp()) wouldn't be fixed either. Can change it back though.

@tsteur

This comment has been minimized.

Member

tsteur commented Sep 19, 2018

@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 No traffic for 'Site Name' recorded in Matomo Analytics, get started now.

Text is
image

@mattab

This comment has been minimized.

Member

mattab commented Sep 19, 2018

Looks really good 👍

@diosmosis diosmosis force-pushed the tracking-code-reminder branch from 557cdc4 to 565b63f Sep 20, 2018

@diosmosis diosmosis force-pushed the tracking-code-reminder branch from 565b63f to 9217388 Sep 20, 2018

@diosmosis diosmosis merged commit 2d2abcc into 3.x-dev Sep 20, 2018

0 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details

@diosmosis diosmosis deleted the tracking-code-reminder branch Sep 20, 2018

InfinityVoid added a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018

Send email if no tracked data within N days. (matomo-org#13363)
* 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.

@mattab mattab added the Major label Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment