@mnapoli opened this Pull Request on March 11th 2015 Member

References #6933

E.g. 'iPad' would become 'Apple - iPad' in the Device model report.

This was the occasion for me to dive a bit into the reports (still very confused) so this is just a try. I'm not sure it's a really good implementation because it would break the continuity with previous reports.

Feedback appreciated, maybe there's a way to concatenate the brand and model name when generating the report (so that we keep continuity with previous reports). I couldn't succeed in doing that because I couldn't find any link between brands and models… E.g. how do I retrieve "Apple" when I have "iPad"…

@mattab commented on March 11th 2015 Owner

Maybe @sgiehl you could help review this?

@sgiehl commented on March 11th 2015 Member

That's quite a simple change and would do the trick for sure, but I'm not sure if that would be the correct/best way. Changing it that way would change the database record, and the field config_device_model would also hold the brand.

I've thought on combining brand and model on archiving only. That is also done for browser + version / os + version. See https://github.com/piwik/piwik/blob/master/plugins/DevicesDetection/Archiver.php#L30 on how it's done there. I'm not sure if there isn't a better way on doing that. @mattab Imho it would be best to have more than only on field the archives are build for.
In that case it would be best to build the archives based on brand and model. Maybe we could also add the device type as it would imho be cool to have the device type icon within the models report.

Btw. there is no way to retrieve the brand having the model only. That's because model names are not unambiguous.

@mnapoli commented on March 11th 2015 Member

If I concatenate at SQL level I end up with the brand code name, e.g. AP - iPod Touch. To have the real brand name I would have to split the string, replace and re-concatenate. Does that sound acceptable? It's a bit dirty, but I don't see a better way.

@sgiehl commented on March 11th 2015 Member

I would love to have that report not only based on one column. Having one column for brand and one for model would be cool. As so it might be possible to sort by brand OR model, and not only the combination. But afaik that is currently not possible, or is it @mattab?
@mnapoli It's the same with browser/os + version. In the database browsers and os are stored as shortcodes like FF for Firefox or AND for Android. Those are replaced when building the API response. See https://github.com/piwik/piwik/blob/master/plugins/DevicesDetection/API.php#L262

@mnapoli commented on March 15th 2015 Member

I've pushed an update following what is done for other reports (i.e. the CONCAT thing).

However I did not add the AddSegmentByLabel filter I'm not sure if it's needed here (I can add it if needed without problem).

Having one column for brand and one for model would be cool. […] But afaik that is currently not possible, or is it @mattab?

I have no idea about this.

Good for review, maybe merge if there is no better way.

@mattab commented on March 30th 2015 Owner

Having one column for brand and one for model would be cool. […] But afaik that is currently not possible, or is it @mattab?

It's possible to store metadata columns in the row before archiving, but I don't think we do it already so maybe better not to do this.

Good for review, maybe merge if there is no better way.

It looks good to me, besides the fact that we will lose the "Row Evolution" data for the report.

@mnapoli does the "Segmented Visitor Log" icon display and work correctly with this change?

@mattab commented on March 30th 2015 Owner

@mnapoli does the "Segmented Visitor Log" icon display and work correctly with this change?

Actually the icon does not currently display for this report, so don't worry about it. Covered in: #7212

@mattab commented on April 1st 2015 Owner

Merging for the beta phase - I think this is a good improvement and only known issue is that it will break Row Evolution for this report. But I don't think we care considering the added value!

Please comment if you have any further thought on this PR

This Pull Request was closed on April 1st 2015
Powered by GitHub Issue Mirror