Geolocating existing log entries attribution task #7381

diosmosis · 2015-03-07T10:09:33Z

Updated version of this pull request: #6793

@piwik/core-team Ready for review.

@mattab Not sure what milestone to put this in?

…to geo-attribution-task Conflicts: misc/others/geoipUpdateRows.php plugins/UserCountry/Columns/Base.php

…thods. Remove PDO cursor and use segments query instead.

…to geo-attribution-task Conflicts: misc/others/geoipUpdateRows.php plugins/UserCountry/Columns/Base.php

Conflicts: misc/others/geoipUpdateRows.php

… documented as service).

…nFetcherTest).

…location created by fixture in DB, so command will re-attribute, and test API output of UserCountry reports to ensure accuracy. Made the test a System test. Also includes fix for IntegrationTestCase base type.

mnapoli · 2015-03-08T20:40:36Z

core/DataAccess/RawLogUpdater.php

+                array($idVisit)
+            )
+        );
+    }


A minor detail but why not turn that method into an update one:

protected function update($table, array $values, $idVisit) { return ... }

(which I think eventually insert/update/select methods should exist in the DBAL, like Doctrine DBAL does, but that's another topic :p)

I could do this, this was left over from the old pull request; maybe I should review it again...

mnapoli · 2015-03-08T20:46:15Z

misc/others/geoipUpdateRows.php was removed: are users using it and will they be confused?

mattab · 2015-03-08T20:53:33Z

misc/others/geoipUpdateRows.php was removed: are users using it and will they be confused?

+1 maybe we can simply output a text like the script was moved - please use this new command instead "./console ....

Also we'll need to update the FAQ https://piwik.org/faq/how-to/faq_167/

tsteur · 2015-03-08T21:09:16Z

core/DataAccess/RawLogUpdater.php

+use Piwik\Common;
+use Piwik\Db;
+
+class RawLogUpdater


Wondering why you split RawLogFetcher and RawLogUpdater into two Classes and not into one? It looks like they belong together unless there is a good reason for it.

That was marcin's decision, not sure what the reason behind it was. We could merge them together. Would you have an issue w/ the name RawLogDao or RawLogDataDao or RawLog(Data)Model?

I don't have an issue with names as long as they don't contain Data or Info :) Maybe the first or last one?

Going the DAO way would be great (following standard names and patterns)

…cator instead of locationFetcher.

…o not use correct location provider).

diosmosis · 2015-03-11T13:07:03Z

If build passes, should be ready for another review and/or merge.

tsteur · 2015-03-12T20:11:34Z

plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php

+    }
+
+    protected function processSpecifiedLogsInChunks(OutputInterface $output, $from, $to, $segmentLimit)
+    {


Maybe you can extract this into a class? Also maybe related methods like reattributeVisitLogs? The name could be Something...Processor? Most likely the related methods that are used there like reattributeVisitLogs need to go into another class where they are public and testable. I do understand it is not as easy as you also want to output things in the middle but I'm sure there are solutions for this and if it is just an event.

…ailure due to QueuedTracking test that sets tracker database connection and starts transaction. Results in one LocalTracker call fail to add a visit in fixture for AttributeHistoricalDataWithLocationsTest, causing test to fail. Fixed by resetting static tracker DB connection in Fixture.php.

…ibute command and add tests to VisitorGeolocatorTest.

…tion test now. Also remove unused use statement in AttributeHistoricalDataWithLocations.php.

diosmosis · 2015-03-16T03:02:20Z

@mattab Feel free to take another look or merge.

mattab · 2015-03-16T05:29:47Z

@diosmosis the build is failing with some rounding errors on MYSQLI: https://travis-ci.org/piwik/piwik/jobs/54514265

…olocator when determining visit fields to update.

diosmosis · 2015-03-16T20:32:31Z

@mattab Mysqli issues fixed.

EDIT: UI test failure is random fail.

mattab · 2015-03-17T04:02:15Z

please confirm when you have tested it on a real database to check that visits are geo located correctly.

Once confirmed, 👍 for merging this!

…made aware of configuration issues), move all verbose logging to VisitorGeolocator, change some arguments to options, add --force option to force even if location provider is not working.

Adding command to attribute or re-attribute existing visits w/ geolocated location info. Includes small refactor to location dimensions and new UserCountry service, VisitorGeolocator. Also includes new DAO class in Piwik core.

… a description to the command.

czolnowski and others added 22 commits October 17, 2014 16:39

User country attribution command.

3e38ba2

Remove getLocationDetail from LocationFetcher class.

873a610

User country attribution command.

9d93d4d

Remove getLocationDetail from LocationFetcher class.

c3d0616

Merge branch 'geo-attribution-task' of github.com:czolnowski/piwik in…

2dcad7c

…to geo-attribution-task Conflicts: misc/others/geoipUpdateRows.php plugins/UserCountry/Columns/Base.php

Split user country attribution command execute method into smaller me…

2e3c4f7

…thods. Remove PDO cursor and use segments query instead.

Remove PDO from doc comment.

5efbe89

Remove repository and move classes with DAO to DataAccess namespace.

baba9de

Add few comments.

f2de22c

Remove LocationFetcherProvider and change tests.

d8a246c

User country attribution command.

8b913e0

Remove getLocationDetail from LocationFetcher class.

8de6742

Split user country attribution command execute method into smaller me…

0e4980e

…thods. Remove PDO cursor and use segments query instead.

Remove PDO from doc comment.

edeb29d

Remove repository and move classes with DAO to DataAccess namespace.

0727b42

Add few comments.

803006a

Remove LocationFetcherProvider and change tests.

e854335

Merge branch 'geo-attribution-task' of github.com:czolnowski/piwik in…

dd88afa

…to geo-attribution-task Conflicts: misc/others/geoipUpdateRows.php plugins/UserCountry/Columns/Base.php

Merge branch 'master' into geo-attribution-task

e2cfb61

Conflicts: misc/others/geoipUpdateRows.php

Refactoring tests & LocationFetcher (renamed to VisitorGeolocator and…

59008a5

… documented as service).

More refactoring and fixing VisitorGeolocatorTest (used to be Locatio…

b2150f3

…nFetcherTest).

Improve AttributeHistoricalDataWithLocationsTest, remove existing geo…

d77bfb0

…location created by fixture in DB, so command will re-attribute, and test API output of UserCountry reports to ensure accuracy. Made the test a System test. Also includes fix for IntegrationTestCase base type.

diosmosis added the Needs Review PRs that need a code review label Mar 7, 2015

mnapoli reviewed Mar 8, 2015
View reviewed changes

tsteur reviewed Mar 8, 2015
View reviewed changes

mattab mentioned this pull request Mar 8, 2015

Geo attribution task #6793

Closed

diosmosis added 3 commits March 11, 2015 04:57

Name variables that store VisitorGeolocator instances to visitorGeolo…

1caf60c

…cator instead of locationFetcher.

Merge branch 'master' into geo-attribution-task

446554e

Fix build failure (due to dimension cache causing VisitorGeolocator t…

d2a2d2b

…o not use correct location provider).

Merge branch 'master' into geo-attribution-task

665e84b

tsteur reviewed Mar 12, 2015
View reviewed changes

diosmosis added 6 commits March 12, 2015 15:08

Merge branch 'master' into geo-attribution-task

2c325c6

Move single visit re-attribution logic to VisitorGeolocator from attr…

ff897db

…ibute command and add tests to VisitorGeolocatorTest.

Move VisitorGeolocatorTest to Integration/ dir since it is an integra…

abd429d

…tion test now. Also remove unused use statement in AttributeHistoricalDataWithLocations.php.

Fix broken file mv.

ab6c00d

Fix new integration test.

46fdac3

diosmosis added 2 commits March 16, 2015 10:38

Try to fix rounding errors for new test in MYSQLI.

85077a6

Correct floating point comparison for latitude/longitude in VisitorGe…

e43871c

…olocator when determining visit fields to update.

mattab assigned diosmosis Mar 17, 2015

diosmosis added 2 commits March 17, 2015 09:48

Merge branch 'master' into geo-attribution-task

ec3bc91

Adding diagnostics for location provider selection (so users will be …

82cf982

…made aware of configuration issues), move all verbose logging to VisitorGeolocator, change some arguments to options, add --force option to force even if location provider is not working.

diosmosis merged commit 00c3940 into master Mar 17, 2015

diosmosis deleted the geo-attribution-task branch March 17, 2015 19:54

diosmosis removed the Needs Review PRs that need a code review label Mar 17, 2015

mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Mar 22, 2015

diosmosis pushed a commit that referenced this pull request Mar 22, 2015

Refs #7381, add usercountry:attribute command to CHANGELOG.md and add…

88da133

… a description to the command.

mattab added the c: i18n For issues around internationalisation and localisation. label Oct 13, 2015

mattab added the answered For when a question was asked and we referred to forum or answered it. label Oct 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geolocating existing log entries attribution task #7381

Geolocating existing log entries attribution task #7381

diosmosis commented Mar 7, 2015

mnapoli Mar 8, 2015

mnapoli Mar 8, 2015

diosmosis Mar 9, 2015

mnapoli commented Mar 8, 2015

mattab commented Mar 8, 2015

tsteur Mar 8, 2015

diosmosis Mar 9, 2015

tsteur Mar 9, 2015

mnapoli Mar 9, 2015

diosmosis commented Mar 11, 2015

tsteur Mar 12, 2015

diosmosis commented Mar 16, 2015

mattab commented Mar 16, 2015

diosmosis commented Mar 16, 2015

mattab commented Mar 17, 2015

Geolocating existing log entries attribution task #7381

Geolocating existing log entries attribution task #7381

Conversation

diosmosis commented Mar 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnapoli commented Mar 8, 2015

mattab commented Mar 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented Mar 11, 2015

Choose a reason for hiding this comment

diosmosis commented Mar 16, 2015

mattab commented Mar 16, 2015

diosmosis commented Mar 16, 2015

mattab commented Mar 17, 2015