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

Added feature Custom Dimensions #9217

Merged
merged 7 commits into from Nov 25, 2015
Merged

Added feature Custom Dimensions #9217

merged 7 commits into from Nov 25, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Nov 16, 2015

refs #9129
fixes #9201 It should only use a request processor of an actually installed plugin
refs #9256 Overlay now show the Segment name

refs #9216 It should now only use a plugin during tracking when it is actually installed

@mattab can you have a look at https://github.com/piwik/plugin-CustomDimensions/blob/master/lang/en.json#L2 and provide useful texts? I did sometimes not know what to write and how to describe. Ideally we have a user guide on piwik.org soon that describes it a bit better and ideally as well the difference between Custom Variables and Custom Dimensions.

@diosmosis I had to make several changes to tests as this https://github.com/piwik/piwik/blob/2.15.1-b1/tests/PHPUnit/Framework/Mock/TestConfig.php#L73 (setting PluginsInstalled = array()) caused a lot of pain. After 1.5 days I made all the tests in core work apart from one test Theme_Home in https://github.com/piwik/piwik/blob/9129_2/tests/UI/specs/Theme_spec.js#L34 that still fails for some reasons but works locally on my installation. Maybe you have an idea? I thought it's maybe due to some TestEnvironment variables that were set in a previous test but this seems to be not the case see https://travis-ci.org/piwik/piwik/jobs/91299185#L1135 .

The changes in the tests will cause some changes to be made in other plugins most likely. Whenever we set configOverride = array('Group' => array('name' => '1')) in PHP or configOverride = {group: {name: 1}} in JS we should instead use configOverride['Group']['name'] = '1' as we otherwise overwrite a previously set PluginsInstalled entry. I could not find a better solution for this as saving the list of installed plugins in TestEnvironment. Usages of configOverride that maybe has to be changed: https://github.com/search?l=&q=configoverride+user%3Apiwik+user%3Apiwikpro&ref=advsearch&type=Code&utf8=%E2%9C%93

This PR also shows that it is not yet possible to build a plugin that is based on an ID (like idDimension, idGoal, ...) and it won't even be easily possible to build this in core since those IDs are needed in API methods. Currently we need to mention $idDimension and $idGoal as parameters in some API methods.

Apart from that:

  • I also added segment support for overlays.
  • SegmentSelector shows on hover a description of the selected segment now

@tsteur tsteur added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Nov 16, 2015
@tsteur tsteur added this to the 2.15.1 milestone Nov 16, 2015
@@ -80,7 +80,7 @@ public static function makeEmptyRow()
*
* @return void
*/
protected function doSumVisitsMetrics($newRowToAdd, &$oldRowToUpdate, $onlyMetricsAvailableInActionsTable = false)
protected function doSumVisitsMetrics($newRowToAdd, &$oldRowToUpdate)
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of a boolean parameter splitted logic into doSumVisitsMetrics and doSumActionMetrics

@tsteur tsteur added Needs Review PRs that need a code review Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Nov 16, 2015
@tsteur
Copy link
Member Author

tsteur commented Nov 19, 2015

I'm adding WIP + needs review as it would be good to already have a look at it while I add a few more things

@tsteur tsteur force-pushed the 9129_2 branch 4 times, most recently from d79b2d3 to dec8db7 Compare November 23, 2015 01:50
@tsteur tsteur removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Nov 23, 2015
@mattab
Copy link
Member

mattab commented Nov 24, 2015

Big Pull request, well done @tsteur for another major new feature that will deliver amazing value to users 👍

@mattab can you have a look at https://github.com/piwik/plugin-CustomDimensions/blob/master/lang/en.json#L2 and provide useful texts? I did sometimes not know what to write and how to describe.

Done in matomo-org/plugin-CustomDimensions@2ffaa0d

Ideally we have a user guide on piwik.org soon that describes it a bit better and ideally as well the difference between Custom Variables and Custom Dimensions.

👍 user guide is a must for this powerful feature. will be covered in https://github.com/piwik/plugin-CustomDimensions/issues/3

@mattab
Copy link
Member

mattab commented Nov 24, 2015

There is a blocker before this can be merged:

@tsteur let's take a look tomorrow morning at this one as i'd love to merge PR so @zawadzinski can demo Custom Dimensions!

mattab pushed a commit that referenced this pull request Nov 25, 2015
Added feature Custom Dimensions
@mattab mattab merged commit 01137cb into master Nov 25, 2015
@mattab mattab deleted the 9129_2 branch November 25, 2015 01:48
@mattab
Copy link
Member

mattab commented Nov 25, 2015

well done @tsteur ! Next step: publish CustomDimensions plugin on Marketplace

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 not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants