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

Jump to bottom

Add option to track only website urls #8345

Merged
merged 9 commits into from Aug 26, 2015
Merged

Add option to track only website urls #8345

merged 9 commits into from Aug 26, 2015

Conversation

barbushin
Copy link
Contributor

Closes #588

@barbushin barbushin added the Needs Review PRs that need a code review label Jul 13, 2015
@barbushin barbushin mentioned this pull request Jul 13, 2015
Closed
@barbushin barbushin added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Jul 13, 2015
@barbushin
Copy link
Contributor Author

There is a diff in UI tests that I can't understand:
image in http://builds-artifacts.piwik.org/ui-tests.master/14165.7/screenshot-diffs/singlediff.html?processed=../processed-ui-screenshots/VisitorMap_regions.png&expected=VisitorMap_regions.png&github=VisitorMap_regions.png

Anybody have ideas why views count on map is changed if all Unit and System tests are passed? Can I push that screenshots as correct?

@barbushin barbushin removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jul 14, 2015
@mattab
Copy link
Member

mattab commented Jul 14, 2015

@barbushin I wrote about this issue in #6693 (comment) - @diosmosis will investigate it in coming days, but he's busy working on Funnel plugin, so if you want to try to investigate this random failure, you're welcome to

mattab
mattab reviewed Jul 14, 2015
View reviewed changes
core/Updates/2.14.1-b2.php
static function getSql()
{
$updateSql = array(
'ALTER TABLE `' . Common::prefixTable('site') . '` ADD COLUMN `exclude_unknown_urls` TINYINT(1) DEFAULT 0 AFTER `currency`' => array(1060)
Copy link
Member

Choose a reason for hiding this comment

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

getSql() was deprecated in 2.12.0 - use getMigrationQueries() instead

See example of new use case in https://github.com/piwik/piwik/blob/master/core/Updates/2.14.0-b2.php#L25-24

Ideally we would update all old core/Updates/* to have consistent use of this new function (feel free to do it in new PR if you want)

@barbushin barbushin mentioned this pull request Jul 14, 2015
Merged
@barbushin barbushin added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Jul 14, 2015
@mattab mattab modified the milestone: 2.15.0 Jul 16, 2015
@mattab mattab 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 9, 2015
@diosmosis
Copy link
Member

@mattab Can you tell me if this exclusion should be enabled by default or not? Seems like it would be useful to enable it by default.

Add tracker requests excluded due to unknown URLs to NoVisitsTest fix…
ecd01a2 
…ture so it will be tested in system tests.
@gaumondp
Copy link

May I suggest we modify the title of this ticket? Right now I think no one would be able to guess what it really adds.

The original #588 title was way more descriptive. Maybe change for :
"Set website URLs or hosts allowed to be tracked" ?

Thanks @barbushin and all for making Piwik less prone to stats poisoning !

@diosmosis diosmosis added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Aug 26, 2015
@diosmosis
Copy link
Member

@gaumondp I made sure #588 will appear in the changelog, not this PR so the name shouldn't be an issue. When this is merged, #588 will be closed w/ it.

diosmosis added a commit that referenced this pull request Aug 26, 2015
@diosmosis
Merge pull request #8345 from piwik/588_urls_whitelist_2
0f01662 
Fixes #588, add option to ignore actions w/ URLs that are not for the website during tracking.
@diosmosis diosmosis merged commit 0f01662 into master Aug 26, 2015
@diosmosis diosmosis deleted the 588_urls_whitelist_2 branch August 26, 2015 19:35
diosmosis pushed a commit that referenced this pull request Aug 26, 2015
Update expected screenshots for #8345
96ae06b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@barbushin @mattab @diosmosis @gaumondp