Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static Graph: better graphs when date range period parameter #2788

Closed
mattab opened this issue Nov 20, 2011 · 5 comments
Closed

Static Graph: better graphs when date range period parameter #2788

mattab opened this issue Nov 20, 2011 · 5 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Nov 20, 2011

  • 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: https://github.com/piwik/piwik/blob/master/plugins/ImageGraph/ImageGraph.php#L68

@julienmoumne
Copy link
Member

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:

@mattab
Copy link
Member Author

mattab commented Nov 21, 2011

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 https://github.com/piwik/piwik/blob/master/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 https://github.com/piwik/piwik/blob/master/plugins/ImageGraph/API.php#L74 when the type is not specified as a parameter ? Currently, the logic is broken see : https://github.com/piwik/piwik/blob/master/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 ;)

@julienmoumne
Copy link
Member

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 ?

@mattab
Copy link
Member Author

mattab commented Nov 28, 2011

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 ?)

@julienmoumne
Copy link
Member

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

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

2 participants