Skip to content

Update resolution in subsequent tracking requests if previously unknown #22080

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.

Sign up for GitHub

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
Merged
merged 5 commits into from
Apr 8, 2024
Merged

Update resolution in subsequent tracking requests if previously unknown #22080

merged 5 commits into from
Apr 8, 2024

Conversation

seb303
Copy link
Contributor

@seb303 seb303 commented Apr 7, 2024

Fixes the issue mentioned here:
#22079

Opted for suggestion (2):
If the resolution happens to be "unknown" in the first page impression of a visit, then when a specific value is available from a subsequent page impression, this value will replace the "unknown" value. For backwards compatibility, once a specific value is set, it will not be overridden by a different value from a subsequent page impression.

Review

This was referenced Apr 7, 2024
Closed
Open
Open
@MatomoForumNotifications
Copy link

This pull request has been mentioned on Matomo forums. There might be relevant details there:

https://forum.matomo.org/t/resolution-get-stuck-as-unknown-if-not-set-with-first-tracking-event/56079/9

@sgiehl sgiehl linked an issue Apr 8, 2024 that may be closed by this pull request
Option to record device resolution onExistingVisit #22079
Closed
@sgiehl sgiehl added the Needs Review PRs that need a code review label Apr 8, 2024
sgiehl
sgiehl requested changes Apr 8, 2024
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left some comments for smaller tweaks, but in general I think this should be good to merge then.

seb303 and others added 4 commits April 8, 2024 13:42
@seb303 @sgiehl
Update plugins/Resolution/Columns/Resolution.php
ad3e877 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@seb303 @sgiehl
Update plugins/Resolution/Columns/Resolution.php
5cad23b 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@seb303 @sgiehl
Update plugins/Resolution/Columns/Resolution.php
92c0d3e 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@sgiehl
Adds Integration test
1bcd59d
sgiehl
sgiehl approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

@seb303 I've just added a simple Integration test, to ensure this behavior doesn't change by accident in the future. Will merge the PR once the tests have passed. Thanks again for your contribution 🎉

@sgiehl sgiehl added this to the 5.1.0 milestone Apr 8, 2024
@sgiehl sgiehl added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Tracking For issues related to getting tracking data into Matomo. and removed Needs Review PRs that need a code review labels Apr 8, 2024
@sgiehl sgiehl changed the title Record resolution even if not present at start of visit Update resolution in subsequent tracking requests if previously unknown Apr 8, 2024
@sgiehl sgiehl merged commit cc0c87b into matomo-org:5.x-dev Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
c: Tracking For issues related to getting tracking data into Matomo. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.

Option to record device resolution onExistingVisit
3 participants
@seb303 @MatomoForumNotifications @sgiehl