Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#2633 closed New feature (fixed)

Add support for segmentation on "page" scope Custom Variables

Reported by: daveb Owned by:
Priority: critical Milestone: 1.6 Piwik 1.6
Component: Core Keywords:
Cc: Sensitive: no

Description

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.

Attachments (2)

diff.txt (3.2 KB) - added by daveb 3 years ago.
Patch to join log_link_visit_action when calculating number of visits
archives.png (128.8 KB) - added by EZdesign 3 years ago.
Archives for failing unit test

Download all attachments as: .zip

Change History (21)

Changed 3 years ago by daveb

Patch to join log_link_visit_action when calculating number of visits

comment:1 Changed 3 years ago by matt (mattab)

  • Milestone set to 1.6 Piwik 1.6
  • Summary changed from Segmenting by Page Scope Custom Variable Not Working to [patch] Segmenting by Page Scope Custom Variable Not Working

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

comment:2 Changed 3 years ago by EZdesign (BeezyT)

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

comment:3 Changed 3 years ago by EZdesign (BeezyT)

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

comment:4 Changed 3 years ago by EZdesign (BeezyT)

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

comment:5 Changed 3 years ago by matt (mattab)

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

comment:6 Changed 3 years ago by EZdesign (BeezyT)

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.

comment:7 Changed 3 years ago by EZdesign (BeezyT)

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

Changed 3 years ago by EZdesign (BeezyT)

Archives for failing unit test

comment:8 Changed 3 years ago by EZdesign (BeezyT)

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.

comment:9 Changed 3 years ago by EZdesign (BeezyT)

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

comment:10 Changed 3 years ago by EZdesign (BeezyT)

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

comment:11 Changed 3 years ago by matt (mattab)

  • Priority changed from normal to critical

comment:12 Changed 3 years ago by matt (mattab)

  • Summary changed from [patch] Segmenting by Page Scope Custom Variable Not Working to Segmenting by "page" scope Custom Variable should be possible
  • Type changed from Bug to New feature

comment:13 Changed 3 years ago by matt (mattab)

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

comment:14 Changed 3 years ago by matt (mattab)

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

comment:15 Changed 3 years ago by matt (mattab)

(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!

comment:16 Changed 3 years ago by EZdesign (BeezyT)

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.

comment:17 Changed 3 years ago by matt (mattab)

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

comment:18 Changed 3 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

comment:19 Changed 3 years ago by matt (mattab)

  • Summary changed from Segmenting by "page" scope Custom Variable should be possible to Add support for segmentation on "page" scope Custom Variables
Note: See TracTickets for help on using tickets.