Re-Think skip tracking filter

[Zodiac1978]

I'm not convinced of the usage of the skip tracking filter.

For example: If someone wants to track a Membership site (or community site with BuddyPress) it would be necessary to track logged in visitors. But our approach is to exit the skip_tracking function if the filter is not null.

The user needs to set the filter and he/she needs to copy all the other code lines to just remove is_user_logged_in() which is not as easy as it could be.

https://github.com/pluginkollektiv/statify/blob/master/inc/class-statify-frontend.php#L139-L176

For example:

add_filter(
    'statify__skip_tracking',
    function() {
        if ( is_logged_in() ) {
            // Track logged in users, excluding feed, preview, error and search (skip otherwise)
            return !is_feed() && !is_preview() && !is_404() && !is_search();
        }
	
        // Continue regular processing.
        return null;
    }
);

[Zodiac1978]

More context: https://wordpress.org/support/topic/logged-in-users-12/

[2ndkauboy]

I agree that the function should be rewritten. There should only be one condition or conditional function in each if block and all of then should be able to be disabled using a filter. And additional filter at the end should allow skipping the teaching due to other conditions.

[stklcode]

Agree on that, however a user should be able to white and blacklist on arbitrary conditions, that's what the hook is for in my opinion.

To not overload the existing hook, we might consider adding a second hook to filter the default settings. like statify__filter_opts( array $opts ).

Thinking further, an array of 9+ booleans is not what comes to my mind when I think of comfort. What about adding a settings page checkboxes instead?
This would really enable the average user to modify the behavior, brings transparency and we can add short info texts to explain each condition.

[Zodiac1978]

What about adding a settings page checkboxes instead?

I think that's a great idea!

I stumbled upon another related issue in the support forums (about the first block "Skip tracking via User Agent."):

Statify comes with a default filter that whitelists Windows, Linux, iPhone, etc.
The mobile bots however match this to force mobile display instead of desktop version.

Mozilla/5.0 (iPhone; CPU iPhone OS 8_1 like Mac OS X) AppleWebKit/600.1.4 (KHTML, like Gecko) Version/8.0 Mobile/12B411 Safari/600.1.4 (compatible; YandexBot/3.0; +http://yandex.com/bots) — Indexing robot.

This behavior should probably be changed in future versions to a more refined blacklist.

Currently the only chance is a custom skip hook or a blacklist plugin to filter their IP subnet. (my blacklist extension does not feature user agent filter either at the moment)

Then there is a block about "Skip tracking via Referrer check and Conditional_Tags.":
is_trackback is not useful, as this is the trackback call itself and no view.
is_robots is not useful, as this the request for the robots.txt file and no page view.
check_referrer is the blacklist check. Making this optional is obviously not useful ;)
is_user_logged_in is useful for a checkbox (see explanation above).
_is_internal is a special case - this function is summarizing four conditionals: is_feed(), is_preview(), is_404() and is_search()

I think is_preview should be already covered through is_user_logged_in, or not?

I am not sure about is_feed(), is_404() and is_search() - should this counted as views? And should we add those metadata for showing counts for Feed, Search and 404s?

[Zodiac1978]

@2ndkauboy I am not sure if you read the two last comments on this ticket.

What about using a user faced setting instead of a filter?

Additionally I don't think that every filter is necessary here. Why do you want to track the visit of the robots file or a trackback call?

And Feed, Search or 404 views should add those info in the database IMHO, so that we can show this info in the dashboard, like the mobile usage mentioned in #104

Maybe we could extend this with more boolean values, like ismobile, issearch and is404.

[2ndkauboy]

Sorry, I have not seen your latest comments, before I worked on the patches. We could easily remove some of the filters/conditions.

A settings page would be nice, but is also quite a lot of work to be done.

[stklcode]

I think is_preview should be already covered through is_user_logged_in, or not?

Yes and No.
Logged in users include the JS snippet (for Caching purposes) and this should not happen on preview pages . That's why we split up the checks in two methods a version or two ago.

Sorry, I have not seen your latest comments, before I worked on the patches.

No problem. Your solution is great, if we decide against a settings page.

A settings page would be nice, but is also quite a lot of work to be done.

Hand full of settings API calls and a common Boolean sanitizer is not what I would call "a lot", more of a copy&paste orgy, but you're right. We should evaluate if the effort would bring benefit for more than a few users.

[Zodiac1978]

@stklcode This ticket can be closed after merging #111 - correct?

[stklcode]

I think so.

Although we probably should either revert the settings for feed and search tracking or document it. JS snippet is not included there, so if those pages are cached, it is far from trivial to consistently track users there. And even if the description does claim that, it would always raise a "why"...

[patrickrobrecht]

Moved the remaining problems to #137 (feed tracking, not fixable for the JavaScript tracking case) and #138 (search result pages - actually counted, but without the query parameter).