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.

Sign up for GitHub

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

Jump to bottom

For Outlinks and Download URLs, do not exclude URL parameters and store URLs untouched #7992

Merged
merged 3 commits into from
May 25, 2015
Merged

For Outlinks and Download URLs, do not exclude URL parameters and store URLs untouched #7992

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e3f188
For Outlinks and Download URLs, do not exclude URL parameters and sto…
mattab May 25, 2015
5c73bd1
Submodule
mattab May 25, 2015
5fe115a
Add integration test for tracking Download and Outlink with fragment
mattab May 25, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions core/Tracker/Action.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,12 @@ protected function setActionUrl($url)
}
}

protected function setActionUrlWithoutExcludingParameters($url)
{
$this->rawActionUrl = PageUrl::getUrlIfLookValid($url);
$this->actionUrl = PageUrl::getUrlIfLookValid($url);
Copy link
Member

Choose a reason for hiding this comment

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

Don't have to call the same method twice here.

Also, this skips the logging in setActionUrl. Perhaps that method can be refactored to call this new method or another one.

}

abstract protected function getActionsToLookup();

protected function getUrlAndType()
Expand Down
2 changes: 1 addition & 1 deletion libs/PiwikTracker
Open in desktop
Submodule PiwikTracker updated from 16b28d to cedb28
2 changes: 1 addition & 1 deletion plugins/Actions/Actions/ActionClickUrl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class ActionClickUrl extends Action
public function __construct(Request $request)
{
parent::__construct(self::TYPE_OUTLINK, $request);
$this->setActionUrl($request->getParam('link'));
$this->setActionUrlWithoutExcludingParameters($request->getParam('link'));
}

public static function shouldHandle(Request $request)
Expand Down
2 changes: 1 addition & 1 deletion plugins/Actions/Actions/ActionDownloadUrl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class ActionDownloadUrl extends Action
public function __construct(Request $request)
{
parent::__construct(self::TYPE_DOWNLOAD, $request);
$this->setActionUrl($request->getParam('download'));
$this->setActionUrlWithoutExcludingParameters($request->getParam('download'));
}

public static function shouldHandle(Request $request)
Expand Down
7 changes: 6 additions & 1 deletion plugins/Actions/ArchivingHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,12 @@ private static function parseNameFromPageUrl($name, $type, $urlPrefix)
$urlFragment = $matches[3];

if (in_array($type, array(Action::TYPE_DOWNLOAD, Action::TYPE_OUTLINK))) {
return array(trim($urlHost), '/' . trim($urlPath));
$path = '/' . trim($urlPath);
if (!empty($urlFragment)) {
$path .= '#' . $urlFragment;
}

return array(trim($urlHost), $path);
}

$name = $urlPath;
Expand Down
6 changes: 4 additions & 2 deletions tests/PHPUnit/Fixtures/OneVisitorTwoVisits.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,17 @@ private function trackVisits()

// Click on external link after 6 minutes (3rd action)
$t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.1)->getDatetime());
self::checkResponse($t->doTrackAction('http://dev.piwik.org/svn', 'link'));

// Testing Outlink that contains a URL Fragment
self::checkResponse($t->doTrackAction('https://outlinks.org/#!outlink-with-fragment-<script>', 'link'));
Copy link
Member

Choose a reason for hiding this comment

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

In addition to modifying a fixture (and thus testing archiving as well as tracking), would it be possible to add a test in System/TrackerTest.php? This test would test just the tracker behavior. Could add it to Integration/TrackerTest.php instead, though would be better to do this after tracker encapsulation.

Copy link
Member Author

Choose a reason for hiding this comment

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

This test would test just the tracker behavior.

The test here tests for Tracker behavior + Archiver (since both were modified in this PR) and I want to test both

edit: I can add another test for tracker only

Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, purpose of my comment was just that it would be easier to debug a regression by running one test, instead of all system tests. In case it wasn't clear ;)

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 was very clear but issue was that I attempted a reply before being fully awake 👍


// Click on file download after 12 minutes (4th action)
$t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.2)->getDatetime());
self::checkResponse($t->doTrackAction('http://piwik.org/path/again/latest.zip', 'download'));

// Click on two more external links, one the same as before (5th & 6th actions)
$t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.22)->getDateTime());
self::checkResponse($t->doTrackAction('http://outlinks.org/other_outlink', 'link'));
self::checkResponse($t->doTrackAction('http://outlinks.org/other_outlink#fragment&pk_campaign=Open%20partnership', 'link'));
$t->setForceVisitDateTime(Date::factory($dateTime)->addHour(0.25)->getDateTime());
self::checkResponse($t->doTrackAction('http://dev.piwik.org/svn', 'link'));

Expand Down
5 changes: 5 additions & 0 deletions tests/PHPUnit/System/BackwardsCompatibility1XTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ public function getApiForTesting()
// the Action.getPageTitles test fails for unknown reason, so skipping it
// eg. https://travis-ci.org/piwik/piwik/jobs/24449365
'Action.getPageTitles',

// Outlinks now tracked with URL Fragment which was not the case in 1.X
'Actions.get',
'Actions.getOutlink',
'Actions.getOutlinks',
);

$apiNotToCall = array_merge($apiNotToCall, $reportsToCompareSeparately);
Expand Down
4 changes: 2 additions & 2 deletions tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.getOutlink_day.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
<label>/svn</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>2</nb_hits>
<sum_time_spent>540</sum_time_spent>
<nb_hits>1</nb_hits>
<sum_time_spent>180</sum_time_spent>
<url>http://dev.piwik.org/svn</url>
</row>
</result>
32 changes: 20 additions & 12 deletions tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.getOutlinks_day.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,42 @@
<?xml version="1.0" encoding="utf-8" ?>
<result>
<row>
<label>dev.piwik.org</label>
<nb_visits>1</nb_visits>
<label>outlinks.org</label>
<nb_visits>2</nb_visits>
<nb_hits>2</nb_hits>
<sum_time_spent>540</sum_time_spent>
<sum_time_spent>468</sum_time_spent>
<subtable>
<row>
<label>/svn</label>
<label>/#!outlink-with-fragment-&lt;script&gt;</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>2</nb_hits>
<sum_time_spent>540</sum_time_spent>
<url>http://dev.piwik.org/svn</url>
<nb_hits>1</nb_hits>
<sum_time_spent>360</sum_time_spent>
<url>https://outlinks.org/#!outlink-with-fragment-&lt;script&gt;</url>
</row>
<row>
<label>/other_outlink#fragment&amp;pk_campaign=Open partnership</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>1</nb_hits>
<sum_time_spent>108</sum_time_spent>
<url>http://outlinks.org/other_outlink#fragment&amp;pk_campaign=Open%20partnership</url>
</row>
</subtable>
</row>
<row>
<label>outlinks.org</label>
<label>dev.piwik.org</label>
<nb_visits>1</nb_visits>
<nb_hits>1</nb_hits>
<sum_time_spent>108</sum_time_spent>
<sum_time_spent>180</sum_time_spent>
<subtable>
<row>
<label>/other_outlink</label>
<label>/svn</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>1</nb_hits>
<sum_time_spent>108</sum_time_spent>
<url>http://outlinks.org/other_outlink</url>
<sum_time_spent>180</sum_time_spent>
<url>http://dev.piwik.org/svn</url>
</row>
</subtable>
</row>
Expand Down
2 changes: 1 addition & 1 deletion tests/PHPUnit/System/expected/test_OneVisitorTwoVisits__Actions.get_day.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<nb_downloads>1</nb_downloads>
<nb_uniq_downloads>1</nb_uniq_downloads>
<nb_outlinks>3</nb_outlinks>
<nb_uniq_outlinks>2</nb_uniq_outlinks>
<nb_uniq_outlinks>3</nb_uniq_outlinks>
<nb_searches>0</nb_searches>
<nb_keywords>0</nb_keywords>
<avg_time_generation>0.155</avg_time_generation>
Expand Down
Binary file modified tests/PHPUnit/System/expected/test_OneVisitorTwoVisits_csv__API.get_month.csv
View file
Open in desktop
Binary file not shown.
4 changes: 2 additions & 2 deletions ...it/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.getOutlink_day.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
<label>/svn</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>2</nb_hits>
<sum_time_spent>432</sum_time_spent>
<nb_hits>1</nb_hits>
<sum_time_spent>72</sum_time_spent>
<url>http://dev.piwik.org/svn</url>
</row>
</result>
32 changes: 20 additions & 12 deletions ...t/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.getOutlinks_day.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,42 @@
<?xml version="1.0" encoding="utf-8" ?>
<result>
<row>
<label>dev.piwik.org</label>
<nb_visits>1</nb_visits>
<label>outlinks.org</label>
<nb_visits>2</nb_visits>
<nb_hits>2</nb_hits>
<sum_time_spent>432</sum_time_spent>
<sum_time_spent>468</sum_time_spent>
<subtable>
<row>
<label>/svn</label>
<label>/#!outlink-with-fragment-&lt;script&gt;</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>2</nb_hits>
<sum_time_spent>432</sum_time_spent>
<url>http://dev.piwik.org/svn</url>
<nb_hits>1</nb_hits>
<sum_time_spent>360</sum_time_spent>
<url>https://outlinks.org/#!outlink-with-fragment-&lt;script&gt;</url>
</row>
<row>
<label>/other_outlink#fragment&amp;pk_campaign=Open partnership</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>1</nb_hits>
<sum_time_spent>108</sum_time_spent>
<url>http://outlinks.org/other_outlink#fragment&amp;pk_campaign=Open%20partnership</url>
</row>
</subtable>
</row>
<row>
<label>outlinks.org</label>
<label>dev.piwik.org</label>
<nb_visits>1</nb_visits>
<nb_hits>1</nb_hits>
<sum_time_spent>108</sum_time_spent>
<sum_time_spent>72</sum_time_spent>
<subtable>
<row>
<label>/other_outlink</label>
<label>/svn</label>
<nb_visits>1</nb_visits>
<nb_uniq_visitors>1</nb_uniq_visitors>
<nb_hits>1</nb_hits>
<sum_time_spent>108</sum_time_spent>
<url>http://outlinks.org/other_outlink</url>
<sum_time_spent>72</sum_time_spent>
<url>http://dev.piwik.org/svn</url>
</row>
</subtable>
</row>
Expand Down
2 changes: 1 addition & 1 deletion ...s/PHPUnit/System/expected/test_OneVisitorTwoVisits_withCookieSupport__Actions.get_day.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<nb_downloads>1</nb_downloads>
<nb_uniq_downloads>1</nb_uniq_downloads>
<nb_outlinks>3</nb_outlinks>
<nb_uniq_outlinks>2</nb_uniq_outlinks>
<nb_uniq_outlinks>3</nb_uniq_outlinks>
<nb_searches>1</nb_searches>
<nb_keywords>1</nb_keywords>
<avg_time_generation>0.155</avg_time_generation>
Expand Down
10 changes: 5 additions & 5 deletions ...em/expected/test_OneVisitorTwoVisits_withCookieSupport__Live.getLastVisitsDetails_day.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<type>action</type>
<url>http://example.org/store/purchase.htm</url>
<pageTitle>Checkout/Purchasing...</pageTitle>
<pageIdAction>12</pageIdAction>
<pageIdAction>13</pageIdAction>

<pageId>9</pageId>
<generationTime>0.13s</generationTime>
Expand Down Expand Up @@ -147,7 +147,7 @@
</row>
<row>
<type>outlink</type>
<url>http://dev.piwik.org/svn</url>
<url>https://outlinks.org/#!outlink-with-fragment-&lt;script&gt;</url>
<pageTitle />
<pageIdAction>5</pageIdAction>

Expand All @@ -171,7 +171,7 @@
</row>
<row>
<type>outlink</type>
<url>http://outlinks.org/other_outlink</url>
<url>http://outlinks.org/other_outlink#fragment&amp;pk_campaign=Open%20partnership</url>
<pageTitle />
<pageIdAction>7</pageIdAction>

Expand All @@ -185,7 +185,7 @@
<type>outlink</type>
<url>http://dev.piwik.org/svn</url>
<pageTitle />
<pageIdAction>5</pageIdAction>
<pageIdAction>8</pageIdAction>

<pageId>6</pageId>
<timeSpent>72</timeSpent>
Expand Down Expand Up @@ -221,7 +221,7 @@
<type>action</type>
<url>http://example.org/index.htm</url>
<pageTitle>Looking at homepage after site search...</pageTitle>
<pageIdAction>10</pageIdAction>
<pageIdAction>11</pageIdAction>

<pageId>8</pageId>
<generationTime>0.02s</generationTime>
Expand Down