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

Implementation of new Browser Reports by Language and Language code #6199

Merged
merged 15 commits into from Sep 23, 2014

Conversation

andrzejewsky
Copy link

@mattab
Copy link
Member

mattab commented Sep 17, 2014

Nice pull request and good shot at trying to keep tests green! Here is my feedback:

  • The language country from the Language report is detected from the language code itself rather than the actual country of the user. So the data already archived in Piwik is good enough: it gives us the full list of all accept languages including language country codes.
    • Changes in Archiver.php are not needed and can be removed.
  • From the existing archived data you can run the datatable GroupBy filter and then group by detected { language code, language country }
    • eg. the GroupBy filter would call a function to extra language en and country bb from the accept language en-bb
    • in case there is no country detail eg. language code is pl then simply write Polish (pl) (instead of current Polish - Poland - (pl-pl))
    • eg. if we have both fr and fr-be then the datatable will show both rows French (fr) and French - Belgium (fr-be) (and not French - France (fr))
  • in label column to keep it simple, write Arabic - Qatar (ar-qa) instead of Arabic - Qatar - (ar-qa)
  • Integration test look good! some feedback:
    • maybe generate more than 1 visit for some languages so we confirm that algorithms work when nb_visits > 1
    • since we don't use the user country location code in the code, it can be removed from the test as well

let me know if you need help with making build green, travis uploads all processed artifacts which you can download and put in expected/ folder in case it's slow to run tests locally. See https://github.com/piwik/piwik/blob/master/tests/README.md#build-artifacts

as soon as you make those changes and build is green I'll be able to merge. Cheers!

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 17, 2014
@mattab mattab added this to the Piwik 2.7.0 milestone Sep 17, 2014
@mattab mattab changed the title Resolve issue #6097 Implementation of new Browser Reports by Language and Language code Sep 22, 2014
@andrzejewsky
Copy link
Author

Ok, all tests are green. Could you look at this ?

$metricsByLanguage->sumMetricsVisits($code, $row);
$langCode = Common::extractLanguageCodeFromBrowserLanguage($row['label'], $languageCodes);
$countryCode = Common::extractCountryCodeFromBrowserLanguage($row['label'], $countryCodes, $enableLanguageToCountryGuess = true);
$label = $countryCode == 'xx' || $countryCode == $langCode ? $langCode : $langCode . '-' . $countryCode;
Copy link
Member

Choose a reason for hiding this comment

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

can you deconstruct the if statement here: it will be easier to read 👍

@mattab
Copy link
Member

mattab commented Sep 23, 2014

Review

  • excellent! the code is more simple now and just makes sense.
  • the PR includes the file chmod changes from 644 to 755 -> can you revert the permission changes somehow, configure git with git config core.fileMode false (source)
  • otherwise looks good and happy to merge!

mattab pushed a commit that referenced this pull request Sep 23, 2014
Implementation of new Browser Reports by Language and Language code fixes #6097
@mattab mattab merged commit 9e89d84 into matomo-org:master Sep 23, 2014
@mattab
Copy link
Member

mattab commented Sep 23, 2014

Kuddos @vox3r for this work.
It will be released tomorrow in 2.7.0 to thousands of users. this is a very good start to the project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants