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

Use generic device model names if device type is known #13015

Merged
merged 3 commits into from Jun 6, 2018

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented May 30, 2018

refs #12999

@sgiehl sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label May 30, 2018
@sgiehl sgiehl added this to the 3.6.0 milestone May 30, 2018
@sgiehl sgiehl force-pushed the genericdevicemodel branch 3 times, most recently from 24c975a to 118587e Compare May 30, 2018 16:15
@sgiehl sgiehl 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 May 30, 2018
@sgiehl sgiehl force-pushed the genericdevicemodel branch 2 times, most recently from c171f6e to 12e673a Compare May 31, 2018 20:25
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

Some minor comments, otherwise looks good

return 'generic mobile';
}

return '';
Copy link
Member

Choose a reason for hiding this comment

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

This will also only affect new data, right? Would it be possible to do this during archiving so it can be applied to old reports?

Don't think this is super important if not, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's currently only for new data. Doing that while archiving would be possible but maybe a bit slow, as we would need to run a custom archiving query having an CASE statement to set the generic name based on the device type field... Also it might be a bit weird to have reports showing a generic model, but none can be found with segmentation as the log tables were not updated.
So maybe the correct way would be to write some kind of reattribution script like for geolocation.

Copy link
Member

Choose a reason for hiding this comment

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

A reattribution script sounds good, though this can always be added later if users require it. Will merge as is.

}
if (!$brand) {
if (!$brand || $brand == Piwik::translate('General_Unknown')) {
Copy link
Member

Choose a reason for hiding this comment

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

Is $brand == Piwik::translate('General_Unknown' necessary? There's the same check above.

@diosmosis diosmosis merged commit 5076bd6 into 3.x-dev Jun 6, 2018
@diosmosis diosmosis deleted the genericdevicemodel branch June 6, 2018 22:29
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
)

* Use generic device model names

* updates tests

* Update screenshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants