From 3b020078117c5e87fc4c9325fed34e33858c8017 Mon Sep 17 00:00:00 2001 From: Thomas Steur Date: Wed, 9 Dec 2015 01:06:05 +0000 Subject: [PATCH] fix creating new visits when no referrer keyword is set --- core/Tracker/Visitor.php | 6 +- plugins/Referrers/Columns/Base.php | 97 ++++++++++------- plugins/Referrers/Columns/Campaign.php | 2 +- plugins/Referrers/Columns/Keyword.php | 2 +- plugins/Referrers/Columns/ReferrerName.php | 2 +- plugins/Referrers/Columns/ReferrerType.php | 2 +- plugins/Referrers/Columns/ReferrerUrl.php | 2 +- plugins/Referrers/Columns/Website.php | 2 +- .../Columns/ReferrerKeywordTest.php | 100 ++++++++++++++++++ .../Integration/Columns/ReferrerNameTest.php | 2 +- plugins/Referrers/tests/System/ApiTest.php | 53 ++++++++++ 11 files changed, 222 insertions(+), 48 deletions(-) create mode 100644 plugins/Referrers/tests/Integration/Columns/ReferrerKeywordTest.php diff --git a/core/Tracker/Visitor.php b/core/Tracker/Visitor.php index 2619c98987d..40e027b26fe 100644 --- a/core/Tracker/Visitor.php +++ b/core/Tracker/Visitor.php @@ -9,7 +9,6 @@ namespace Piwik\Tracker; use Piwik\Config; -use Piwik\Plugin\Dimension\VisitDimension; use Piwik\Tracker; use Piwik\Tracker\Visit\VisitProperties; @@ -49,6 +48,11 @@ public function isVisitorKnown() return $this->visitorKnown === true; } + public function isNewVisit() + { + return !$this->isVisitorKnown(); + } + private function setIsVisitorKnown($isVisitorKnown) { return $this->visitorKnown = $isVisitorKnown; diff --git a/plugins/Referrers/Columns/Base.php b/plugins/Referrers/Columns/Base.php index a4da79d3899..655ea20449f 100644 --- a/plugins/Referrers/Columns/Base.php +++ b/plugins/Referrers/Columns/Base.php @@ -34,12 +34,12 @@ abstract class Base extends VisitDimension protected $currentUrlParse; protected $idsite; + private static $cachedReferrerSearchEngine = array(); + // Used to prefix when a adsense referrer is detected const LABEL_PREFIX_ADWORDS_KEYWORD = '(adwords) '; const LABEL_ADWORDS_NAME = 'AdWords'; - private static $cachedReferrer = array(); - /** * Returns an array containing the following information: * - referer_type @@ -69,14 +69,8 @@ abstract class Base extends VisitDimension * @param int $idSite * @return array */ - protected function getReferrerInformation($referrerUrl, $currentUrl, $idSite, Request $request) + protected function getReferrerInformation($referrerUrl, $currentUrl, $idSite, Request $request, Visitor $visitor) { - $cacheKey = $referrerUrl . $currentUrl . $idSite; - - if (isset(self::$cachedReferrer[$cacheKey])) { - return self::$cachedReferrer[$cacheKey]; - } - $this->idsite = $idSite; // default values for the referer_* fields @@ -101,7 +95,7 @@ protected function getReferrerInformation($referrerUrl, $currentUrl, $idSite, Re $this->referrerHost = $this->referrerUrlParse['host']; } - $referrerDetected = $this->detectReferrerCampaign($request); + $referrerDetected = $this->detectReferrerCampaign($request, $visitor); if (!$referrerDetected) { if ($this->detectReferrerDirectEntry() @@ -131,17 +125,15 @@ protected function getReferrerInformation($referrerUrl, $currentUrl, $idSite, Re 'referer_url' => $this->referrerUrl, ); - self::$cachedReferrer[$cacheKey] = $referrerInformation; - return $referrerInformation; } - protected function getReferrerInformationFromRequest(Request $request) + protected function getReferrerInformationFromRequest(Request $request, Visitor $visitor) { $referrerUrl = $request->getParam('urlref'); $currentUrl = $request->getParam('url'); - return $this->getReferrerInformation($referrerUrl, $currentUrl, $request->getIdSite(), $request); + return $this->getReferrerInformation($referrerUrl, $currentUrl, $request->getIdSite(), $request, $visitor); } /** @@ -150,28 +142,36 @@ protected function getReferrerInformationFromRequest(Request $request) */ protected function detectReferrerSearchEngine() { - $searchEngineInformation = SearchEngineDetection::getInstance()->extractInformationFromUrl($this->referrerUrl); - - /** - * Triggered when detecting the search engine of a referrer URL. - * - * Plugins can use this event to provide custom search engine detection - * logic. - * - * @param array &$searchEngineInformation An array with the following information: - * - * - **name**: The search engine name. - * - **keywords**: The search keywords used. - * - * This parameter is initialized to the results - * of Piwik's default search engine detection - * logic. - * @param string referrerUrl The referrer URL from the tracking request. - */ - Piwik::postEvent('Tracker.detectReferrerSearchEngine', array(&$searchEngineInformation, $this->referrerUrl)); + if (isset(self::$cachedReferrerSearchEngine[$this->referrerUrl])) { + $searchEngineInformation = self::$cachedReferrerSearchEngine[$this->referrerUrl]; + } else { + $searchEngineInformation = SearchEngineDetection::getInstance()->extractInformationFromUrl($this->referrerUrl); + + /** + * Triggered when detecting the search engine of a referrer URL. + * + * Plugins can use this event to provide custom search engine detection + * logic. + * + * @param array &$searchEngineInformation An array with the following information: + * + * - **name**: The search engine name. + * - **keywords**: The search keywords used. + * + * This parameter is initialized to the results + * of Piwik's default search engine detection + * logic. + * @param string referrerUrl The referrer URL from the tracking request. + */ + Piwik::postEvent('Tracker.detectReferrerSearchEngine', array(&$searchEngineInformation, $this->referrerUrl)); + + self::$cachedReferrerSearchEngine[$this->referrerUrl] = $searchEngineInformation; + } + if ($searchEngineInformation === false) { return false; } + $this->typeReferrerAnalyzed = Common::REFERRER_TYPE_SEARCH_ENGINE; $this->nameReferrerAnalyzed = $searchEngineInformation['name']; $this->keywordReferrerAnalyzed = $searchEngineInformation['keywords']; @@ -354,7 +354,7 @@ protected function getParameterValueFromReferrerUrl($adsenseReferrerParameter) /** * @return bool */ - protected function detectReferrerCampaign(Request $request) + protected function detectReferrerCampaign(Request $request, Visitor $visitor) { $isCampaign = $this->detectReferrerCampaignFromTrackerParams($request); if (!$isCampaign) { @@ -363,12 +363,30 @@ protected function detectReferrerCampaign(Request $request) $this->detectCampaignKeywordFromReferrerUrl(); - if ($this->typeReferrerAnalyzed != Common::REFERRER_TYPE_CAMPAIGN) { - return false; - } + $isCurrentVisitACampaignWithSameName = $visitor->getVisitorColumn('referer_name') == $this->nameReferrerAnalyzed; + $isCurrentVisitACampaignWithSameName = $isCurrentVisitACampaignWithSameName && $visitor->getVisitorColumn('referer_type') == Common::REFERRER_TYPE_CAMPAIGN; + // if we detected a campaign but there is still no keyword set, we set the keyword to the Referrer host if (empty($this->keywordReferrerAnalyzed)) { - $this->keywordReferrerAnalyzed = $this->referrerHost; + if ($isCurrentVisitACampaignWithSameName) { + $this->keywordReferrerAnalyzed = $visitor->getVisitorColumn('referer_keyword'); + // it is an existing visit and no referrer keyword was used initially (or a different host), + // we do not use the default referrer host in this case as it would create a new visit. It would create + // a new visit because initially the referrer keyword was not set (or from a different host) and now + // we would set it suddenly. The changed keyword would be recognized as a campaign change and a new + // visit would be forced. Why would it suddenly set a keyword but not do it initially? + // This happens when on the first visit when the URL was opened directly (no referrer or different host) + // and then the user navigates to another page where the referrer host becomes the own host + // (referrer = own website) see https://github.com/piwik/piwik/issues/9299 + } else { + $this->keywordReferrerAnalyzed = $this->referrerHost; + } + } + + if ($this->typeReferrerAnalyzed != Common::REFERRER_TYPE_CAMPAIGN) { + $this->keywordReferrerAnalyzed = null; + $this->nameReferrerAnalyzed = null; + return false; } $this->keywordReferrerAnalyzed = Common::mb_strtolower($this->keywordReferrerAnalyzed); @@ -384,7 +402,6 @@ protected function detectReferrerCampaign(Request $request) */ public function getValueForRecordGoal(Request $request, Visitor $visitor) { - $referrerTimestamp = $request->getParam('_refts'); $referrerUrl = $request->getParam('_ref'); $referrerCampaignName = $this->getReferrerCampaignQueryParam($request, '_rcn'); $referrerCampaignKeyword = $this->getReferrerCampaignQueryParam($request, '_rck'); @@ -422,7 +439,7 @@ public function getValueForRecordGoal(Request $request, Visitor $visitor) elseif (!empty($referrerUrl)) { $idSite = $request->getIdSite(); - $referrer = $this->getReferrerInformation($referrerUrl, $currentUrl = '', $idSite, $request); + $referrer = $this->getReferrerInformation($referrerUrl, $currentUrl = '', $idSite, $request, $visitor); // if the parsed referrer is interesting enough, ie. website or search engine if (in_array($referrer['referer_type'], array(Common::REFERRER_TYPE_SEARCH_ENGINE, Common::REFERRER_TYPE_WEBSITE))) { diff --git a/plugins/Referrers/Columns/Campaign.php b/plugins/Referrers/Columns/Campaign.php index c29b6223368..be42acb34b6 100644 --- a/plugins/Referrers/Columns/Campaign.php +++ b/plugins/Referrers/Columns/Campaign.php @@ -50,7 +50,7 @@ public function shouldForceNewVisit(Request $request, Visitor $visitor, Action $ return false; } - $information = $this->getReferrerInformationFromRequest($request); + $information = $this->getReferrerInformationFromRequest($request, $visitor); if ($information['referer_type'] == Common::REFERRER_TYPE_CAMPAIGN && $this->isReferrerInformationNew($visitor, $information) diff --git a/plugins/Referrers/Columns/Keyword.php b/plugins/Referrers/Columns/Keyword.php index b3bfc374b1c..7c927d64924 100644 --- a/plugins/Referrers/Columns/Keyword.php +++ b/plugins/Referrers/Columns/Keyword.php @@ -42,7 +42,7 @@ public function getName() */ public function onNewVisit(Request $request, Visitor $visitor, $action) { - $information = $this->getReferrerInformationFromRequest($request); + $information = $this->getReferrerInformationFromRequest($request, $visitor); if (!empty($information['referer_keyword'])) { return Common::mb_substr($information['referer_keyword'], 0, 255); diff --git a/plugins/Referrers/Columns/ReferrerName.php b/plugins/Referrers/Columns/ReferrerName.php index ded0a48f9e8..ef21e7a277e 100644 --- a/plugins/Referrers/Columns/ReferrerName.php +++ b/plugins/Referrers/Columns/ReferrerName.php @@ -36,7 +36,7 @@ protected function configureSegments() */ public function onNewVisit(Request $request, Visitor $visitor, $action) { - $information = $this->getReferrerInformationFromRequest($request); + $information = $this->getReferrerInformationFromRequest($request, $visitor); if (!empty($information['referer_name'])) { return Common::mb_substr($information['referer_name'], 0, 70); diff --git a/plugins/Referrers/Columns/ReferrerType.php b/plugins/Referrers/Columns/ReferrerType.php index 5303a1c360f..ef3a66325c6 100644 --- a/plugins/Referrers/Columns/ReferrerType.php +++ b/plugins/Referrers/Columns/ReferrerType.php @@ -42,7 +42,7 @@ public function getName() */ public function onNewVisit(Request $request, Visitor $visitor, $action) { - $information = $this->getReferrerInformationFromRequest($request); + $information = $this->getReferrerInformationFromRequest($request, $visitor); return $information['referer_type']; } diff --git a/plugins/Referrers/Columns/ReferrerUrl.php b/plugins/Referrers/Columns/ReferrerUrl.php index 21c5cd5bdcc..ab523740a75 100644 --- a/plugins/Referrers/Columns/ReferrerUrl.php +++ b/plugins/Referrers/Columns/ReferrerUrl.php @@ -35,7 +35,7 @@ protected function configureSegments() */ public function onNewVisit(Request $request, Visitor $visitor, $action) { - $information = $this->getReferrerInformationFromRequest($request); + $information = $this->getReferrerInformationFromRequest($request, $visitor); return $information['referer_url']; } diff --git a/plugins/Referrers/Columns/Website.php b/plugins/Referrers/Columns/Website.php index 42d05068d60..33484b6de15 100644 --- a/plugins/Referrers/Columns/Website.php +++ b/plugins/Referrers/Columns/Website.php @@ -41,7 +41,7 @@ public function shouldForceNewVisit(Request $request, Visitor $visitor, Action $ return false; } - $information = $this->getReferrerInformationFromRequest($request); + $information = $this->getReferrerInformationFromRequest($request, $visitor); if ($information['referer_type'] == Common::REFERRER_TYPE_WEBSITE && $this->isReferrerInformationNew($visitor, $information) diff --git a/plugins/Referrers/tests/Integration/Columns/ReferrerKeywordTest.php b/plugins/Referrers/tests/Integration/Columns/ReferrerKeywordTest.php new file mode 100644 index 00000000000..f5ff10e11c4 --- /dev/null +++ b/plugins/Referrers/tests/Integration/Columns/ReferrerKeywordTest.php @@ -0,0 +1,100 @@ +keyword = new Keyword(); + } + + /** + * @dataProvider getReferrerUrls + */ + public function test_onNewVisit_shouldDetectCorrectReferrerType($expectedType, $idSite, $url, $referrerUrl) + { + $request = $this->getRequest(array('idsite' => $idSite, 'url' => $url, 'urlref' => $referrerUrl)); + $type = $this->keyword->onNewVisit($request, $this->getNewVisitor(), $action = null); + + $this->assertSame($expectedType, $type); + } + + public function getReferrerUrls() + { + $url = 'http://piwik.org/foo/bar'; + $noReferrer = ''; + $directReferrer = 'http://piwik.org'; + $externalReferrer = 'http://example.org'; + + $noReferrerKeyword = null; + $emptyReferrerKeyword = ''; + + return array( + // website referrer types usually do not have a keyword + array($noReferrerKeyword, $this->idSite1, $url, $externalReferrer), + // direct entries do usually not have a referrer keyowrd + array($noReferrerKeyword, $this->idSite1, $url, $directReferrer), + + // it is a campaign but there is no referrer url and no keyword set specifically, we cannot detect a keyword + // it does not return null as it is converted to strlower(null) + array($emptyReferrerKeyword, $this->idSite1, $url . '?pk_campaign=test', $noReferrer), + + // campaigns, coming from same domain should have a keyword + array('piwik.org', $this->idSite1, $url . '?pk_campaign=test', $directReferrer), + // campaigns, coming from different domain should have a keyword + array('example.org', $this->idSite2, $url . '?pk_campaign=test', $externalReferrer), + // campaign keyword is specifically set + array('campaignkey1', $this->idSite2, $url . '?pk_campaign=test&pk_keyword=campaignkey1', $externalReferrer), + array('campaignkey2', $this->idSite2, $url . '?pk_campaign=test&utm_term=campaignkey2', $externalReferrer), + + // search engine should have keyword the search term + array('piwik', $this->idSite2, $url, 'http://google.com/search?q=piwik'), + ); + } + + private function getRequest($params) + { + return new Request($params); + } + + private function getNewVisitor() + { + return new Visitor(new VisitProperties()); + } + +} diff --git a/plugins/Referrers/tests/Integration/Columns/ReferrerNameTest.php b/plugins/Referrers/tests/Integration/Columns/ReferrerNameTest.php index db34a25c46c..6c0bcfb1e0e 100644 --- a/plugins/Referrers/tests/Integration/Columns/ReferrerNameTest.php +++ b/plugins/Referrers/tests/Integration/Columns/ReferrerNameTest.php @@ -74,7 +74,7 @@ public function getReferrerUrls() $url = 'http://piwik.org/foo/bar'; $referrer = 'http://piwik.org'; - $directEntryReferrerName = ''; + $directEntryReferrerName = null; return array( // domain matches but path does not match for idsite1 diff --git a/plugins/Referrers/tests/System/ApiTest.php b/plugins/Referrers/tests/System/ApiTest.php index 28aede37239..fb91e57ca26 100644 --- a/plugins/Referrers/tests/System/ApiTest.php +++ b/plugins/Referrers/tests/System/ApiTest.php @@ -8,7 +8,10 @@ namespace Piwik\Plugins\Referrers\tests\System; +use Piwik\API\Request; +use Piwik\DataTable; use Piwik\Tests\Fixtures\TwoSitesManyVisitsOverSeveralDaysWithSearchEngineReferrers; +use Piwik\Tests\Framework\Fixture; use Piwik\Tests\Framework\TestCase\SystemTestCase; /** @@ -55,6 +58,56 @@ public function getApiForTesting() return $apiToTest; } + public function test_forceNewVisit_shouldNotForceANewVisitWhenNoKeywordIsSetAndNoReferrerWasSetInitially() + { + $dateTime = '2015-01-02'; + $idSite = self::$fixture->idSite; + + $t = Fixture::getTracker($idSite, $dateTime . ' 00:01:02', $defaultInit = true); + // track a campaign that was opened directly (no referrer) + $t->setUrlReferrer(''); + $t->setUrl('http://piwik.net/?pk_campaign=adwbuccc'); + $t->doTrackPageView('My Title'); + + // navigate to next page on same page + $t->setUrlReferrer('http://piwik.net/?pk_campaign=adwbuccc'); + $t->setCustomTrackingParameter('_rcn', 'adwbuccc'); // this parameter would be set by piwik.js from cookie / attributionInfo + $t->setCustomTrackingParameter('_rck', ''); // no keyword was used in previous tracking request + $t->setUrl('http://piwik.net/page1'); + $t->doTrackPageView('Page 1'); + + /** @var DataTable $visits */ + $visits = Request::processRequest('VisitsSummary.get', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime)); + + $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits')); + $this->assertEquals(2, $visits->getFirstRow()->getColumn('nb_actions')); + } + + public function test_forceNewVisit_shouldNotForceANewVisitWhenNoKeywordIsSetAndReferrerHostChanges() + { + $dateTime = '2015-01-03'; + $idSite = self::$fixture->idSite; + + $t = Fixture::getTracker($idSite, $dateTime . ' 00:01:02', $defaultInit = true); + // track a campaign that was opened directly (no referrer) + $t->setUrlReferrer('http://www.google.com'); + $t->setUrl('http://piwik.net/?pk_campaign=adwbuccc'); + $t->doTrackPageView('My Title'); + + // navigate to next page on same page + $t->setUrlReferrer('http://piwik.net/?pk_campaign=adwbuccc'); + $t->setCustomTrackingParameter('_rcn', 'adwbuccc'); // this parameter would be set by piwik.js from cookie / attributionInfo + $t->setCustomTrackingParameter('_rck', ''); // no keyword was used in previous tracking request + $t->setUrl('http://piwik.net/page1'); + $t->doTrackPageView('Page 1'); + + /** @var DataTable $visits */ + $visits = Request::processRequest('VisitsSummary.get', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime)); + + $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits')); + $this->assertEquals(2, $visits->getFirstRow()->getColumn('nb_actions')); + } + public static function getOutputPrefix() { return '';