Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Metadata support for lastX & previousX #2414

Closed
JulienMoumne opened this Issue · 12 comments

2 participants

Julien Moumné Matthieu Aubry
Julien Moumné
Collaborator

If you try to set date=last10&period=day as in /docs/analytics-api/reference/ you get

Date format must be: YYYY-MM-DD, or 'today' or 'yesterday' or any keyword supported by the strtotime function (see http://php.net/strtotime for more information)

Reproducible on the demo http://demo.piwik.org/index.php?module=API&method=API.getProcessedReport&idSite=7&period=day&date=last10&apiModule=UserCountry&apiAction=getCountry&format=xml&token_auth=anonymous

Matthieu Aubry
Owner

Metadata lack of support of previous|last is a known limitation but we would like to fix it, because it would be useful for the ImageGraph plugin #1721 (to plot evolution graph).

Julien Moumné
Collaborator

and a requirement for #151

Julien Moumné
Collaborator

Concerning metadata <prettyDate/> please confirm output for the following cases:

1.
&period=day
$date=last10
<prettyDate>Last 10 days</prettyDate>
2.
&period=month
$date=previous10
<prettyDate>Previous 10 months</prettyDate>
3.
&period=month
$date=2011-01-01,2011-04-01
<prettyDate>Months included in 1 Jan 11 - 1 Apr 11</prettyDate>
Julien Moumné
Collaborator

Concerning <reportData/>, what is the best option

1.
<reportData>
    <row period="2011-05-16_to_2011-05-22">
        <row><row/>
        <row><row/>
        [...]
    <row/>
    [...]
<reportData/>
2.
<reportData>
    <row>
        <row><row/>
        <row><row/>
        [...]
    <row/>
    [...]
<reportData/>

+

<reportMetadata>
    <row>
        <period>2011-05-16_to_2011-05-22</period>
        <row>
           <idsubdatatable>1739</idsubdatatable>
        </row>
        <row>
           <idsubdatatable>1739</idsubdatatable>
        </row>
        [...]
    </row>
    [...]

Something completely different or a variant of above ideas ?

Matthieu Aubry
Owner
  • For <prettyDate>, you can use the same output as Period=range. For example, last10 will be: 20 May 11 - 29 May 11

    For last3&period=month, it will be: March 2011 - May 2011

    For period=week: 9 May 11 - 29 May 11

  • for <reportData> and <reportMetadata>, I think solution 1) is best. But, maybe we can stay consistent with existing output for last10

    Day: <result date="2011-04-30">

    week: <result date="2010-11-01 to 2010-11-07">

The code writing <result date=""> is in renderDataTableArray() in Xml renderer. It works on a Piwik_DataTable_Array. Maybe the metadata for last10&period=day for example, would build a Piwik_DataTable_Array? Or maybe you have a different plan (which might be OK!)?

Julien Moumné
Collaborator

I like the idea of staying consistent with the general API.

Concerning <prettyDate>, it means we have the same output for two quite different calls:

1.
&period=day
&date=last10
<prettyDate>20 May 11 - 29 May 11</prettyDate>
2.
&period=range
&date=2011-05-20,2011-05-29
<prettyDate>20 May 11 - 29 May 11</prettyDate>

In the first case we give no indication there are multiple periods in the output.

Matthieu Aubry
Owner

I think it's fine having same string for these 2 outputs, because the consumers (RSS & Mobile App) will not I think use this prettyDate? Also, better not introduce new dates until we are sure we need them. :)

Julien Moumné
Collaborator
Julien Moumné
Collaborator

Here is the patch for review.

Info:

1) There is a change of output for dimension-less report both for JSON & XML formats. (from http://pastebin.com/tD4JY6V2 to http://pastebin.com/2QzDnqpq see 'reportData'). This has been done to unify the general API and the metadata API,

2) The metadata getProcessedReport API returning values have changed. Instead of returning PHP arrays for reportData and reportMetadata, it now uses Piwik_DataTable (for a single period) and Piwik_DataTable_Array (for multiple periods). This has been done to leverage Piwik_DataTable_Renderer (see changes in Piwik_API_ResponseBuilder),

3) Because of second point, both PDF and HTML report renderers have been updated,

4) Also a side effect of second point, some existing integration test XML files (concerning reports with dimension) have been updated because Piwik_DataTable_Renderer_Xml does not append an additional EOL caracter before </row>,

5) I reused an integration test scenario to test multiple period metadata reports. It only tests for period="day". If need be, I am willing to add more scenarios,

6) @review annotations are questions left for the reviewers.

Matthieu Aubry
Owner

Great stuff!! :)

  • I think tests are covering all use cases? so it's great > @review is this the correct place for this factoring ? Looks good

@review, should renderers provide a way to directly access the rendered body ?
Maybe if it makes the code easier... but not required, when there is only 1 use case
@review does $key need a particular treatment ?
Don't think so.. technically it should be escaped, but we will only set safe values so its fine
// @review does $value need a particular treatment ?
I'd be tempted to json_encode the value, but maybe it doesn't work as expected? this would be safer here

Julien Moumné
Collaborator

(In [4879]) fixes #2414 refs piwik/piwik-mobile-2#2350 refs #5571

Julien Moumné
Collaborator

(In [4900]) refs #2414 #2350 #151 - fix for bug introduced on json arrays and objects + adding first layer of unit tests

Julien Moumné JulienMoumne added this to the 1.5 - Piwik 1.5 milestone
Julien Moumné JulienMoumne self-assigned this
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.