Opened 2 years ago

Closed 2 years ago

#2788 closed Bug (fixed)

Static Graph: better graphs when date range period parameter

Reported by: matt Owned by: JulienM
Priority: major Milestone: 1.7 Piwik 1.7
Component: Core Keywords:
Cc: Sensitive: no

Description

  • when period=range, should be rewritten to period=day in evolution graphs
    • Do we add a new parameter to decide which subperiod to plot, or default to "day" (configuration via config file?)
  • when period=month, should not be changed to period=range! but instead plot last N months

Cf: http://demo.piwik.org/index.php?module=API&method=API.getReportMetadata&period=range&date=2011-01-01,2012-01-01&format=xml&token_auth=x&idSites=1

<imageGraphUrl>
index.php?module=API&method=ImageGraph.get&idSite=1&apiModule=VisitsSummary&apiAction=get&token_auth=x&graphType=evolution&period=range&date=2011-01-01,2012-01-01
</imageGraphUrl>

at line: http://dev.piwik.org/trac/browser/trunk/plugins/ImageGraph/ImageGraph.php#L68

Change History (6)

comment:1 Changed 2 years ago by JulienM (JulienMoumne)

With the addition of a new graph type in ticket:2704#comment:11 and the refactoring of ImageGraph plugin, I would like to precisely define the following points:

  • What are the default graph types for each report and each period type (single period and multiple periods)
  • What are the applicable graph types for each report and each period type. As I've understood, evolution is not applicable to reports with dimension. In the ImageGraph Plugin, this logic is buried way deep in the code at line http://dev.piwik.org/trac/browser/trunk/plugins/ImageGraph/API.php#L301. I would like to recode and move-up that logic to make it clearer and more maintainable.

comment:2 Changed 2 years ago by matt (mattab)

What are the default graph types for each report and each period type (single period and multiple periods)

pretty much all that is currently, except the 2 bugs in this ticket: when period=range

However on trunk, the following debug URL will show all graphs as per imageGraphUrl:

http://localhost/trunk/index.php?module=ImageGraph&action=index&idSite=1&period=month&date=2011-10-10

You can see that it loads the Visits summary graph rewritten as: &period=month&date=2009-05-01,2011-10-31

Which seems to look good?

So is there really a bug with period=month?

What are the applicable graph types for each report and each period type. As I've understood, evolution is not applicable to reports with dimension. In the ImageGraph Plugin, this logic is buried way deep in the code at line http://dev.piwik.org/trac/browser/trunk/plugins/ImageGraph/API.php#L301. I would like to recode and move-up that logic to make it clearer and more maintainable.

The logic seems OK in this function, however this function is bloated and getting complicated. Re-factor sounds good

The default graph type is currently part of the ImageGraphUrl string within reports metadata. Wouldn't it be more suitable to remove it from there and add the 'default graph type' logic in http://dev.piwik.org/trac/browser/trunk/plugins/ImageGraph/API.php#L74 when the type is not specified as a parameter ? Currently, the logic is broken see : http://dev.piwik.org/trac/browser/trunk/plugins/ImageGraph/API.php#L105

If it's easier to have the logic in the get method then sure, +1 to remove the parameter from imageGraphUrl and put it to default to false in the API function parameter list

refactoring and simplicity are good ;)

comment:3 Changed 2 years ago by JulienM (JulienMoumne)

  • Owner set to JulienM
  • Status changed from new to assigned

comment:4 Changed 2 years ago by JulienM (JulienMoumne)

Concerning

when period=range, should be rewritten to period=day in evolution graphs

    * Do we add a new parameter to decide which subperiod to plot, or default to "day" (configuration via config file?)

I've currently set period='day' which seems to me a reasonable default.

Shall I add a new entry in the configuration file ?

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

Shall I add a new entry in the configuration file ?

Yes please it sounds the best approach (graphs_default_period_to_plot_when_period_range ?)

comment:6 Changed 2 years ago by JulienM (JulienMoumne)

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

(In [5582]) * fixes #2706, #2828, #2704, refs #1721, #2637, #2711, #2318, #71 : horizontal static graph implemented

  • fixes #2788, refs #2670, #1721, #2637, #2711 : default graph type logic moved to ImageGraph API, improved date/period logic, new parameter graphs_default_period_to_plot_when_period_range
  • fixes #2704, #2804, refs #1721 : pChart updated to 2.1.3, pChart code removed from Piwik code, OOP refactoring, support for unifont.ttf if present in ImageGraph/fonts, testAllSizes now uses report metadata ImageGraphUrl
  • refs #71 : space between report title and report table reduced to avoid page overflow
  • refs #2829 : TODO display percentages
  • r5544, r5547, r5549 merged
Note: See TracTickets for help on using tickets.