Move single visit re-attribution logic to VisitorGeolocator from attribute command and add tests to VisitorGeolocatorTest.

4 files changed, 296 insertions, 67 deletions

diff --git a/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php b/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php
index fa703e85bc..585409f011 100644
--- a/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php
+++ b/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php
@@ -29,17 +29,6 @@ class AttributeHistoricalDataWithLocations extends ConsoleCommand
     const SEGMENT_LIMIT_OPTION_DEFAULT = 1000;
 
     /**
-     * @var string[]
-     */
-    private static $logVisitFieldsToUpdate = array(
-        'location_country'   => LocationProvider::COUNTRY_CODE_KEY,
-        'location_region'    => LocationProvider::REGION_CODE_KEY,
-        'location_city'      => LocationProvider::CITY_NAME_KEY,
-        'location_latitude'  => LocationProvider::LATITUDE_KEY,
-        'location_longitude' => LocationProvider::LONGITUDE_KEY
-    );
-
-    /**
      * @var RawLogDao
      */
     protected $dao;
@@ -124,7 +113,7 @@ class AttributeHistoricalDataWithLocations extends ConsoleCommand
 
     protected function processSpecifiedLogsInChunks(OutputInterface $output, $from, $to, $segmentLimit)
     {
-        $visitFieldsToSelect = array_merge(array('idvisit', 'location_ip'), array_keys(self::$logVisitFieldsToUpdate));
+        $visitFieldsToSelect = array_merge(array('idvisit', 'location_ip'), array_keys(VisitorGeolocator::$logVisitFieldsToUpdate));
 
         $lastId = 0;
         do {
@@ -144,20 +133,15 @@ class AttributeHistoricalDataWithLocations extends ConsoleCommand
                 continue;
             }
 
-            $location = $this->getVisitLocation($row);
-            $valuesToUpdate = $this->getValuesToUpdate($row, $location);
+            $updatedValues = $this->visitorGeolocator->attributeExistingVisit($row);
 
             $this->onVisitProcessed($output);
 
             $idVisit = $row['idvisit'];
-
-            if (empty($valuesToUpdate)) {
+            if (empty($updatedValues)) {
                 $this->writeIfVerbose($output, 'Nothing to update for idvisit = ' . $idVisit . '. Existing location info is same as geolocated.');
             } else {
                 $this->writeIfVerbose($output, 'Updating visit with idvisit = ' . $idVisit . '.');
-
-                $this->dao->updateVisits($valuesToUpdate, $idVisit);
-                $this->dao->updateConversions($valuesToUpdate, $idVisit);
             }
         }
     }
@@ -183,37 +167,6 @@ class AttributeHistoricalDataWithLocations extends ConsoleCommand
     }
 
     /**
-     * Returns location log values that are different than the values currently in a log row.
-     *
-     * @param array $row The visit row.
-     * @param array $location The location information.
-     * @return array The location properties to update.
-     */
-    protected function getValuesToUpdate(array $row, $location)
-    {
-        $valuesToUpdate = array();
-        foreach (self::$logVisitFieldsToUpdate as $column => $locationKey) {
-            if (empty($location[$locationKey])) {
-                continue;
-            }
-
-            $locationPropertyValue = $location[$locationKey];
-            $existingPropertyValue = $row[$column];
-
-            if ($locationKey === LocationProvider::COUNTRY_CODE_KEY) {
-                if (strtolower($locationPropertyValue) != strtolower($existingPropertyValue)) {
-                    $valuesToUpdate[$column] = strtolower($locationPropertyValue);
-                }
-            } else {
-                if ($locationPropertyValue != $existingPropertyValue) {
-                    $valuesToUpdate[$column] = $locationPropertyValue;
-                }
-            }
-        }
-        return $valuesToUpdate;
-    }
-
-    /**
      * Print information about progress.
      * @param OutputInterface $output
      */
@@ -255,13 +208,6 @@ class AttributeHistoricalDataWithLocations extends ConsoleCommand
         return true;
     }
 
-    private function getVisitLocation($row)
-    {
-        $ip = IPUtils::binaryToStringIP($row['location_ip']);
-        $location = $this->visitorGeolocator->getLocation(array('ip' => $ip));
-        return $location;
-    }
-
     private function getDateRangeToAttribute(InputInterface $input)
     {
         $dateRangeString = $input->getArgument(self::DATES_RANGE_ARGUMENT);
diff --git a/plugins/UserCountry/VisitorGeolocator.php b/plugins/UserCountry/VisitorGeolocator.php
index 0a6ef43098..78446408c2 100644
--- a/plugins/UserCountry/VisitorGeolocator.php
+++ b/plugins/UserCountry/VisitorGeolocator.php
@@ -11,6 +11,8 @@ namespace Piwik\Plugins\UserCountry;
 use Piwik\Cache\Cache;
 use Piwik\Cache\Transient;
 use Piwik\Common;
+use Piwik\DataAccess\RawLogDao;
+use Piwik\Network\IPUtils;
 use Piwik\Plugins\UserCountry\LocationProvider\DefaultProvider;
 use Piwik\Tracker\Visit;
 
@@ -35,6 +37,17 @@ require_once PIWIK_INCLUDE_PATH . "/plugins/UserCountry/LocationProvider.php";
 class VisitorGeolocator
 {
     /**
+     * @var string[]
+     */
+    public static $logVisitFieldsToUpdate = array(
+        'location_country'   => LocationProvider::COUNTRY_CODE_KEY,
+        'location_region'    => LocationProvider::REGION_CODE_KEY,
+        'location_city'      => LocationProvider::CITY_NAME_KEY,
+        'location_latitude'  => LocationProvider::LATITUDE_KEY,
+        'location_longitude' => LocationProvider::LONGITUDE_KEY
+    );
+
+    /**
      * @var Cache
      */
     protected static $defaultLocationCache = null;
@@ -55,11 +68,12 @@ class VisitorGeolocator
     private $locationCache;
 
     /**
-     * @param LocationProvider $provider
-     * @param LocationProvider $backupProvider
-     * @param Cache $locationCache
+     * @var RawLogDao
      */
-    public function __construct(LocationProvider $provider = null, LocationProvider $backupProvider = null, Cache $locationCache = null)
+    protected $dao;
+
+    public function __construct(LocationProvider $provider = null, LocationProvider $backupProvider = null, Cache $locationCache = null,
+                                RawLogDao $dao = null)
     {
         if ($provider === null) {
             // note: Common::getCurrentLocationProviderId() uses the tracker cache, which is why it's used here instead
@@ -76,6 +90,7 @@ class VisitorGeolocator
 
         $this->backupProvider = $backupProvider ?: $this->getDefaultProvider();
         $this->locationCache = $locationCache ?: self::getDefaultLocationCache();
+        $this->dao = $dao ?: new RawLogDao();
     }
 
     public function getLocation($userInfo, $useClassCache = true)
@@ -131,6 +146,58 @@ class VisitorGeolocator
     }
 
     /**
+     * Geolcates an existing visit and then updates it if it's current attributes are different than
+     * what was geolocated. Also updates all conversions of a visit.
+     *
+     * **This method should NOT be used from within the tracker.**
+     */
+    public function attributeExistingVisit($visit, $useClassCache = true)
+    {
+        $ip = IPUtils::binaryToStringIP($visit['location_ip']);
+        $location = $this->getLocation(array('ip' => $ip), $useClassCache);
+
+        $valuesToUpdate = $this->getVisitFieldsToUpdate($visit, $location);
+
+        if (!empty($valuesToUpdate)) {
+            $idVisit = $visit['idvisit'];
+
+            $this->dao->updateVisits($valuesToUpdate, $idVisit);
+            $this->dao->updateConversions($valuesToUpdate, $idVisit);
+        }
+
+        return $valuesToUpdate;
+    }
+
+    /**
+     * Returns location log values that are different than the values currently in a log row.
+     *
+     * @param array $row The visit row.
+     * @param array $location The location information.
+     * @return array The location properties to update.
+     */
+    private function getVisitFieldsToUpdate(array $row, $location)
+    {
+        if (isset($location[LocationProvider::COUNTRY_CODE_KEY])) {
+            $location[LocationProvider::COUNTRY_CODE_KEY] = strtolower($location[LocationProvider::COUNTRY_CODE_KEY]);
+        }
+
+        $valuesToUpdate = array();
+        foreach (self::$logVisitFieldsToUpdate as $column => $locationKey) {
+            if (empty($location[$locationKey])) {
+                continue;
+            }
+
+            $locationPropertyValue = $location[$locationKey];
+            $existingPropertyValue = $row[$column];
+
+            if ($locationPropertyValue != $existingPropertyValue) {
+                $valuesToUpdate[$column] = $locationPropertyValue;
+            }
+        }
+        return $valuesToUpdate;
+    }
+
+    /**
      * @return LocationProvider
      */
     public function getProvider()
diff --git a/plugins/UserCountry/tests/Unit/UserCountryTest.php b/plugins/UserCountry/tests/Unit/UserCountryTest.php
index b4ada3e4d1..9b62518e3e 100644
--- a/plugins/UserCountry/tests/Unit/UserCountryTest.php
+++ b/plugins/UserCountry/tests/Unit/UserCountryTest.php
@@ -19,7 +19,7 @@ use Exception;
 require_once PIWIK_INCLUDE_PATH . '/plugins/UserCountry/UserCountry.php';
 require_once PIWIK_INCLUDE_PATH . '/plugins/UserCountry/functions.php';
 
-class UserCountryTest extends \PHPUnit_Framework_Testcase
+class UserCountryTest extends \PHPUnit_Framework_TestCase
 {
     /**
      * @group Plugins
diff --git a/plugins/UserCountry/tests/Unit/VisitorGeolocatorTest.php b/plugins/UserCountry/tests/Unit/VisitorGeolocatorTest.php
index a28a48bb14..d783924845 100644
--- a/plugins/UserCountry/tests/Unit/VisitorGeolocatorTest.php
+++ b/plugins/UserCountry/tests/Unit/VisitorGeolocatorTest.php
@@ -9,9 +9,12 @@
 namespace Piwik\Plugins\UserCountry\tests\Unit;
 
 use PHPUnit_Framework_MockObject_MockObject;
-use PHPUnit_Framework_TestCase;
+use Piwik\Common;
+use Piwik\Db;
+use Piwik\Network\IPUtils;
 use Piwik\Plugins\UserCountry\VisitorGeolocator;
 use Piwik\Plugins\UserCountry\LocationProvider;
+use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
 use Piwik\Tracker\Cache;
 use Piwik\Tracker\Visit;
 use Piwik\Tests\Framework\Mock\LocationProvider as MockLocationProvider;
@@ -21,8 +24,10 @@ require_once PIWIK_INCLUDE_PATH . '/tests/PHPUnit/Framework/Mock/LocationProvide
 /**
  * @group UserCountry
  */
-class VisitorGeolocatorTest extends PHPUnit_Framework_TestCase
+class VisitorGeolocatorTest extends IntegrationTestCase // TODO: move
 {
+    const TEST_IP = '1.2.3.4';
+
     public function test_getLocation_shouldReturnLocationForProvider_IfLocationIsSetForCurrentProvider()
     {
         $location = array(
@@ -43,9 +48,6 @@ class VisitorGeolocatorTest extends PHPUnit_Framework_TestCase
         );
     }
 
-    /**
-     *
-     */
     public function test_getLocation_shouldReturnLocationForProvider_IfLocationCountryCodeIsNotSetShouldSetAsxx()
     {
         $location = array(
@@ -125,6 +127,104 @@ class VisitorGeolocatorTest extends PHPUnit_Framework_TestCase
         $this->assertEquals(MockLocationProvider::ID, $geolocator->getProvider()->getId());
     }
 
+    public function getDataForAttributeExistingVisitTests()
+    {
+        $basicTestLocation = array(
+            LocationProvider::COUNTRY_CODE_KEY => 'US',
+            LocationProvider::REGION_CODE_KEY => 'rg',
+            LocationProvider::CITY_NAME_KEY => 'the city',
+            LocationProvider::LATITUDE_KEY => '29.959698',
+            LocationProvider::LONGITUDE_KEY => '-90.064880'
+        );
+
+        $basicExpectedVisitProperties = array(
+            'location_country' => 'us',
+            'location_region' => 'rg',
+            'location_city' => 'the city',
+            'location_latitude' => '29.959698',
+            'location_longitude' => '-90.064880'
+        );
+
+        return array(
+            array( // test normal re-attribution
+                $basicTestLocation,
+
+                $basicExpectedVisitProperties
+            ),
+
+            array( // test w/ garbage in location provider result
+                array(
+                    LocationProvider::COUNTRY_CODE_KEY => 'US',
+                    'garbage' => 'field',
+                    LocationProvider::REGION_CODE_KEY => 'rg',
+                    LocationProvider::CITY_NAME_KEY => 'the city',
+                    LocationProvider::LATITUDE_KEY => '29.959698',
+                    LocationProvider::LONGITUDE_KEY => '-90.064880',
+                    'another' => 'garbage field'
+                ),
+
+                array(
+                    'location_country' => 'us',
+                    'location_region' => 'rg',
+                    'location_city' => 'the city',
+                    'location_latitude' => '29.959698',
+                    'location_longitude' => '-90.064880'
+                )
+            ),
+
+            array( // test when visit has some correct properties already
+                $basicTestLocation,
+
+                $basicExpectedVisitProperties,
+
+                array(
+                    'location_country' => 'US',
+                    'location_region' => 'rg',
+                    'location_city' => 'the city'
+                ),
+
+                array(
+                    'location_country' => 'us',
+                    'location_latitude' => '29.959698',
+                    'location_longitude' => '-90.064880'
+                )
+            ),
+
+            array( // test when visit has all correct properties already
+                $basicTestLocation,
+
+                $basicExpectedVisitProperties,
+
+                $basicExpectedVisitProperties,
+
+                array()
+            )
+        );
+    }
+
+    /**
+     * @dataProvider getDataForAttributeExistingVisitTests
+     */
+    public function test_attributeExistingVisit_CorrectlySetsLocationProperties_AndReturnsCorrectResult(
+        $mockLocation, $expectedVisitProperties, $visitProperties = array(), $expectedUpdateValues = null)
+    {
+        $mockLocationProvider = $this->getProviderMockThatGeolocates($mockLocation);
+
+        $visit = $this->insertVisit($visitProperties);
+        $this->insertTwoConversions($visit);
+
+        $geolocator = new VisitorGeolocator($mockLocationProvider);
+        $valuesUpdated = $geolocator->attributeExistingVisit($visit, $useCache = false);
+
+        $this->assertEquals($expectedVisitProperties, $this->getVisit($visit['idvisit']));
+
+        $expectedUpdateValues = $expectedUpdateValues === null ? $expectedVisitProperties : $expectedUpdateValues;
+        $this->assertEquals($expectedUpdateValues, $valuesUpdated);
+
+        $conversions = $this->getConversions($visit);
+        $this->assertEquals(array($expectedVisitProperties, $expectedVisitProperties), $conversions);
+    }
+
     /**
      * @return PHPUnit_Framework_MockObject_MockObject|LocationProvider
      */
@@ -135,4 +235,120 @@ class VisitorGeolocatorTest extends PHPUnit_Framework_TestCase
             ->disableOriginalConstructor()
             ->getMock();
     }
+
+    private function getProviderMockThatGeolocates($locationResult)
+    {
+        $mock = $this->getProviderMock();
+        $mock->expects($this->once())->method('getLocation')->will($this->returnCallback(function ($info) use ($locationResult) {
+            if ($info['ip'] == VisitorGeolocatorTest::TEST_IP) {
+                return $locationResult;
+            } else {
+                return null;
+            }
+        }));
+        return $mock;
+    }
+
+    private function insertVisit($visit = array())
+    {
+        $defaultProperties = array(
+            'idsite' => 1,
+            'idvisitor' => hex2bin('ea95f303f2165aa0'),
+            'visit_last_action_time' => '2012-01-01 00:00:00',
+            'config_id' => hex2bin('ea95f303f2165aa0'),
+            'location_ip' => IPUtils::stringToBinaryIP(self::TEST_IP),
+            'visitor_localtime' => '2012-01-01 00:00:00',
+            'location_country' => 'xx',
+            'config_os' => 'xxx',
+            'visit_total_events' => 0,
+            'visitor_days_since_last' => 0,
+            'config_quicktime' => 0,
+            'config_pdf' => 0,
+            'config_realplayer' => 0,
+            'config_silverlight' => 0,
+            'config_windowsmedia' => 0,
+            'config_java' => 0,
+            'config_gears' => 0,
+            'config_resolution' => 0,
+            'config_resolution' => '',
+            'config_cookie' => 0,
+            'config_director' => 0,
+            'config_flash' => 0,
+            'config_browser_version' => '',
+            'visitor_count_visits' => 1,
+            'visitor_returning' => 0,
+            'visit_total_time' => 123,
+            'visit_entry_idaction_name' => 0,
+            'visit_entry_idaction_url' => 0,
+            'visitor_days_since_order' => 0,
+            'visitor_days_since_first' => 0,
+            'visit_first_action_time' => 0,
+            'visit_goal_buyer' => 0,
+            'visit_goal_converted' => 0,
+            'visit_exit_idaction_name' => 0,
+            'referer_url' => '',
+            'location_browser_lang' => 'xx',
+            'config_browser_engine' => '',
+            'config_browser_name' => '',
+            'referer_type' => 0,
+            'referer_name' => '',
+            'visit_total_actions' => 0,
+            'visit_total_searches' => 0
+        );
+
+        $visit = array_merge($defaultProperties, $visit);
+
+        $this->insertInto('log_visit', $visit);
+
+        $idVisit = Db::fetchOne("SELECT LAST_INSERT_ID()");
+        return $this->getVisit($idVisit, $allColumns = true);
+    }
+
+    private function getVisit($idVisit, $allColumns = false)
+    {
+        $columns = $allColumns ? "*" : "location_country, location_region, location_city, location_latitude, location_longitude";
+        return Db::fetchRow("SELECT $columns FROM " . Common::prefixTable('log_visit') . " WHERE idvisit = ?", array($idVisit));
+    }
+
+    private function insertTwoConversions($visit)
+    {
+        $conversionProperties = array(
+            'idvisit' => $visit['idvisit'],
+            'idsite' => $visit['idsite'],
+            'idvisitor' => $visit['idvisitor'],
+            'server_time' => '2012-01-01 00:00:00',
+            'idgoal' => 1,
+            'buster' => 1,
+            'url' => '',
+            'location_longitude' => $visit['location_longitude'],
+            'location_latitude' => $visit['location_latitude'],
+            'location_region' => $visit['location_region'],
+            'location_country' => $visit['location_country'],
+            'location_city' => $visit['location_city'],
+            'visitor_count_visits' => $visit['visitor_count_visits'],
+            'visitor_returning' => $visit['visitor_returning'],
+            'visitor_days_since_order' => 0,
+            'visitor_days_since_first' => 0
+        );
+
+        $this->insertInto('log_conversion', $conversionProperties);
+
+        $conversionProperties['buster'] = 2;
+        $this->insertInto('log_conversion', $conversionProperties);
+    }
+
+    private function insertInto($table, $row)
+    {
+        $columns = implode(', ', array_keys($row));
+        $columnsPlaceholders = Common::getSqlStringFieldsArray($row);
+        $values = array_values($row);
+
+        Db::query("INSERT INTO " . Common::prefixTable($table) . " ($columns) VALUES ($columnsPlaceholders)", $values);
+    }
+
+    private function getConversions($visit)
+    {
+        return Db::fetchAll("SELECT location_country, location_region, location_city, location_latitude, location_longitude
+                               FROM " . Common::prefixTable('log_conversion') . " WHERE idvisit = ?", array($visit['idvisit']));
+    }
 }
\ No newline at end of file