Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add support for segmentation on "page" scope Custom Variables #2633

Closed
anonymous-piwik-user opened this Issue · 17 comments

3 participants

@anonymous-piwik-user

When you pass in a custom variable that is scoped to a page instead of visit, no results are returned. Looking in the database in log_link_visit_action I can see the custom variables are recorded.

The solution I came up with was to join log_visit with log_link_visit_action to check if the custom variable is for a visit or page. I have created a patch. It might not be the best answer, but the code should explain what is going on.

@anonymous-piwik-user

Attachment: Patch to join log_link_visit_action when calculating number of visits
diff.txt

@mattab
Owner

Thanks for the patch. We will try and investigate for the next release

@BeezyT
Collaborator

(In [5115]) Refs #2633 API can handle segmentation by different scopes (page, visit...)

@BeezyT
Collaborator

(In [5116]) Refs #2633 Removed recognition of available segments on visits

@BeezyT
Collaborator

(In [5117]) Refs #2633 Another attempt at segment scope recognition

@mattab
Owner

(In [5129]) Refs #2633 - revert previous commits since they are temporary solution (final patch in the works by Timo)

@BeezyT
Collaborator

Here's a very short explination of the patch...

Result

  • All reports can use all available segments
  • CustomVars per page are referenced using CustomVariablePage(Name|Value)[1-5]
  • CustomVars per visit are referenced as before
  • New segments: goalConversion==idgoal, pageUrl, pageTitle

Implementation

  • If segements are based on other scopes than the report, the segmentation API handles the joins
  • Plugins need to supply the table of the segments as well (not only column name) via the Metdata API
  • Segment.test.php, test_getSelectQuery_* are a good starting point to understand the way the new segmentation API has to be used. $segment->getSqlQuery() gets the custom parts of the query and does all the magic based on the requested segment.

I did manual tests on all API methods with different segments (often joining all three tables). If anybody would like to do some further testing, that would be good, since this is a core part of Piwik.

PS: Somehow revision [5138] is missing in the ticket. That's the related changeset.

@BeezyT
Collaborator

(In [5139]) Refs #2633 updated unit test

@BeezyT
Collaborator

Attachment: Archives for failing unit test
archives.png

@BeezyT
Collaborator

In [5139], I fixed a failing unit test.

The integration test "test_oneVisitor_oneWebsite_severalDays_DateRange" was expecting 22 blobs for archive_numeric_2010_12 but there were 28. I'm not sure whether this is expected behavior or not. The changeset is just a quick and dirty fix, please review and explain what the expected number of blobs is in this case. I added a screenshot of the actual archives in the table. Maybe someone can spot the ones that shouldn't be there.

@BeezyT
Collaborator

(In [5140]) Refs #2633 fixed table prefix in Segment.php unit tests. This should also fix the webtests.

@BeezyT
Collaborator

(In [5141]) Refs #2633 fixing impact of lang changes

@mattab
Owner

(In [5148]) Refs #2633 Adding one test + mysql function replace also replaces _ (for functions such as UNIX_TIMESTAMP)

@mattab
Owner

(In [5150]) Refs #2633
Number of records in archive_numeric increased, because Timo added segmentation support to Frequency API

@mattab
Owner

(In [5151]) Refs #2633

  • Code review I noticed one line was removed from ArchiveProcessing/Day.php - restoring
  • Fixing documentation output displayed at index.php?module=API&action=listSegments which is in turn used for the page: http://piwik.org/docs/analytics-api/segmentation/ The fix is that now, custom variable of scope "page" work!!!! :) Great stuff Timo!
@BeezyT
Collaborator

Thanks for the updates, Matt.

Only one comment: I think my condition in ArchiveProcessing_Day was correct.

This is what it was before (at revision 5114):

$segmentSql = $this->getSegment()->getSql();
$sqlSegment = $segmentSql['sql'];
if(empty($sqlSegment) 
    || self::getPluginBeingProcessed($this->getRequestedReport()) == 'VisitsSummary')

This is my version (at revision 5141):

$segment = $this->getSegment();
$segmentation = !$segment->isEmpty();
$reportType = self::getPluginBeingProcessed($this->getRequestedReport());
if (!$segmentation || ($reportType == 'VisitsSummary'))

I did change empty($segment->getSql()['sql']) into $segment->isEmpty(), but that's on purpose, since getSql() has been removed. Apart from that, it should be the same condition.

This is your fix (at revision 5151):

$reportType = self::getPluginBeingProcessed($this->getRequestedReport());
if ($this->shouldProcessReportsAllPlugins($this->getSegment(), $this->period)
    || ($reportType == 'VisitsSummary'))

IMHO this is different to the original condition.

Please review.

@mattab
Owner

(In [5160]) Refs #2633 - confirmed code is correct

@anonymous-piwik-user anonymous-piwik-user added this to the 1.6 Piwik 1.6 milestone
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.