Skip to content
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
Merged

Conversation

diosmosis
Copy link
Member

This PR is based off of #13362.

@diosmosis diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Aug 29, 2018
@diosmosis diosmosis added this to the 3.7.0 milestone Aug 29, 2018
@diosmosis diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Aug 30, 2018
@mattab mattab modified the milestones: 3.7.0, 3.6.1 Sep 1, 2018
*/

return [
'CoreAdminHome.daysToTrackedVisitsCheck' => 5,
Copy link
Member

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)
Copy link
Member

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?

Copy link
Member

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">
Copy link
Member

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

Copy link
Member Author

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.

@@ -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",
Copy link
Member

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

"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.",
Copy link
Member

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

Copy link
Member Author

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.

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

$daysToTrackedVisitsCheck = StaticContainer::get('CoreAdminHome.daysToTrackedVisitsCheck');
Copy link
Member

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)
{
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@diosmosis
Copy link
Member Author

Updated.


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

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.

"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.",
Copy link
Member

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.

}

$monthly = new Monthly();
$monthly->setTimezone('Pacific/Auckland');
Copy link
Member

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?

$createdTime = Date::factory($site['ts_created']);

$scheduledJobTime = $createdTime->addDay($daysToTrackedVisitsCheck);
if ($scheduledJobTime->isEarlier(Date::now())) {
Copy link
Member

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

image

I might just not see it though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try to clarify.

@diosmosis
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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();
Copy link
Member

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)
Copy link
Member

@tsteur tsteur Sep 19, 2018

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?


namespace Piwik\Scheduler\Schedule;

class OneTime extends Schedule
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member Author

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.

@diosmosis
Copy link
Member Author

Updated.

@tsteur
Copy link
Member

tsteur commented Sep 19, 2018

FixedTime is definitely better 👍

@diosmosis
Copy link
Member Author

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

@tsteur
Copy link
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
Copy link
Member

mattab commented Sep 19, 2018

Looks really good 👍

@diosmosis diosmosis merged commit 2d2abcc into 3.x-dev Sep 20, 2018
@diosmosis diosmosis deleted the tracking-code-reminder branch September 20, 2018 22:53
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* 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 Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants