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

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

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

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

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

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

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.

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

Attachment: Archives for failing unit test
archives.png

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.

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

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

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

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

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

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.

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