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

Provide possibility to configure referrer exclusion list #19302

Merged
merged 26 commits into from Jun 30, 2022

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jun 1, 2022

Description:

This PR provides the possibility to configure a list of host excluded from referrer detection.
The list can be maintained globally, as well as enriched per site.

Referrer on that exclusion list will be ignored on PHP side and such hits will be handled as DIRECT entry.

In addition it will be possible to define ignored referrers within the javascript tracker, so they are ignored for setting the referrer cookies and also not sent with tracking requests.

refs #18612

Review

@sgiehl sgiehl force-pushed the referrerexclusion branch 5 times, most recently from 6f39dd0 to dc81b49 Compare June 8, 2022 07:30
@sgiehl sgiehl added this to the 4.12.0 milestone Jun 8, 2022
@sgiehl sgiehl added the Enhancement For new feature suggestions that for example enhance Matomo's cabapilities.. label Jun 8, 2022
@sgiehl sgiehl added the Needs Review For pull requests that need a code review. label Jun 9, 2022
@sgiehl sgiehl marked this pull request as ready for review June 9, 2022 15:02
@mattab
Copy link
Member

mattab commented Jun 28, 2022

@sgiehl I haven't reviewed yet, but based on your comment in the other issue, can we add paypal.com to the "global" list of excluded referrers so that users don't have to configure it and it just works for people out of the box?

@sgiehl
Copy link
Member Author

sgiehl commented Jun 28, 2022

I'm not able to say if there might be reasons for someone to not having it excluded, but yes, I can let it be added to the list once on update or install.

Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

I've noted a couple of minor issues, but overall this seems to be working well. UI works to set the excluded referrers, the tracker code is generated with the exclusions, any excluded referrers are ignored and the tests all pass. 👍

CHANGELOG.md Outdated Show resolved Hide resolved
plugins/Referrers/Columns/Base.php Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Jun 29, 2022

@bx80 applied some fixes

CHANGELOG.md Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Jun 30, 2022

@bx80 I've created a PR for the developer docs, so the new methods will be documented. Feel free to review and merge that one.
In addition I've updated the FAQ: https://matomo.org/faq/how-to/how-do-i-add-a-referral-exclusion-in-matomo/

@sgiehl sgiehl merged commit 92896ae into 4.x-dev Jun 30, 2022
@sgiehl sgiehl deleted the referrerexclusion branch June 30, 2022 09:58
@mattab
Copy link
Member

mattab commented Jul 4, 2022

@sgiehl didn't check the UI, but was checking the UI tests and noticed in https://github.com/matomo-org/matomo/blob/6f76e4fdbd183f729900d5a7286d5e281ac7d22a/plugins/SitesManager/tests/UI/expected-screenshots/SitesManager_global_settings.png that it doesn't show "paypal.com" in the field - i would have expected the screenshot test to show 'paypal.com' so it doesn't regress in the future?

@sgiehl
Copy link
Member Author

sgiehl commented Jul 4, 2022

Haven't added that yet, but still having that in mind. The discussion in the other issue is still ongoing and I'm not sure if adding paypal by default is that much of help

@mattab
Copy link
Member

mattab commented Jul 4, 2022

Yes it is of much help and is needed, so let's add it so it actually fixes the issue for people 👍

@sgiehl
Copy link
Member Author

sgiehl commented Jul 5, 2022

@mattab This will only have an impact on the PHP side of tracking. The tracking code would need to be updated by everyone to also reflect that on javascript side, which would be the more important part. Also I guess this might currently not be easily possible with TagManager.

@mattab
Copy link
Member

mattab commented Jul 5, 2022

Let's add Paypal.com to our core JS tracker code so it's not an issue for Paypal - that's a big reason of why we do this work.
(by adding to the core code, then we don't make the JS tracker embedded in users site "ugly" and showing paypal - instead it's hidden in the big minified JS code)

@sgiehl
Copy link
Member Author

sgiehl commented Jul 5, 2022

@mattab There are so many other payment providers and other services like SSOs, that might then still produce the same issue. Also having a hard coded list, might in the end cause problems for users that might expect e.g. paypal from referring users to their site.
It still feels a bit like trying to provide a workaround for an issue we might not yet have identified correctly.

@mattab
Copy link
Member

mattab commented Jul 5, 2022

Let's start with the workaround and see how it goes (pragmatic) so we can move on from this and fix the high impact issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that for example enhance Matomo's cabapilities.. Needs Review For pull requests that need a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants