Ability to change periods over which evolution graphs display in scheduled reports #13501

diosmosis · 2018-09-29T05:38:11Z

Changes:

Add new INI config param [General] graphs_default_evolution_graph_last_days_amount to match other params.
Add new option to scheduled reports to allow specifying per report, what periods are displayed in evolution graphs.

Tests don't pass just yet, need to add a new file.

… to control lastN used in ImageGraph evolution.

tsteur · 2018-10-02T19:20:49Z

Can we move this to 3.7.0 maybe?

diosmosis · 2018-10-02T19:37:49Z

It's required for a cloud report, so if moved to 3.7.0, then we'd have to wait until 3.7.0 before it can be used.

CC @mattab

tsteur · 2018-11-08T02:13:29Z

plugins/ImageGraph/ImageGraph.php

+
+    public static function getDefaultGraphEvolutionLastPeriods()
+    {
+        $lastPeriods = Config::getInstance()->General['graphs_default_evolution_graph_last_days_amount'];


should we maybe fail if someone configures a value <= 0? so the user can see it was not correctly changed? not needed though...

plugins/ScheduledReports/API.php

tsteur · 2018-11-08T02:14:51Z

plugins/ScheduledReports/API.php

@@ -86,18 +87,22 @@ public function __construct(LoggerInterface $logger)
     * @param array $reports array of reports
     * @param array $parameters array of parameters
     * @param bool|int $idSegment Segment Identifier
+     * @param bool $evolutionPeriodFor If true, evolution graphs cover each day within the period. If false, evolution graphs


I'm actually not quite understanding the difference between the two but reckon will get more clear once having a look at it.

$evolutionPeriodFor determines if looking within range or using lastN value. I originally only had one parameter $evolutionPeriodLastN where if it was <= 0, it would look at days within the range, but that's not really clear w/ just one parameter, so I opted for being explicit.

tsteur · 2018-11-08T02:20:25Z

plugins/ScheduledReports/API.php

+        $originalShowEvolutionWithinSelectedPeriod = Config::getInstance()->General['graphs_show_evolution_within_selected_period'];
+        $originalDefaultEvolutionGraphLastPeriodsAmount = Config::getInstance()->General['graphs_default_evolution_graph_last_days_amount'];
+        try {
+            Config::getInstance()->General['graphs_show_evolution_within_selected_period'] = (bool)$report['evolution_graph_within_period'];


I haven't tested it yet but are you sure this works? I think in the past usually it was often needed to do

$general = $config->General;$general['...']=(bool)....; $config->General = $general;. Not sure if this is still a thing...

Also I'm not sure this is actually applied... without testing or looking further I would have expected this needs to be set before calling \Piwik\Plugins\API\API::getInstance()->getReportMetadata($idSite);

I remember that was for specific versions of PHP. I can do that to be safe.

Note: it's used in API.getProcessedReport, but can set it before all the API calls.

I remember that was for specific versions of PHP. I can do that to be safe.

might be good. I don't really remember why it was needed or whether it's still the case. Haven't tested to write it the normal way in a while as gotten used to it :)

tsteur · 2018-11-08T02:22:28Z

plugins/ScheduledReports/templates/_addReport.twig

+
+            <div
+                class="row evolution-graph-period"
+                ng-show="manageScheduledReport.report.displayFormat == '2' || manageScheduledReport.report.displayFormat == '3'"


should this be also available for "Default) Display Report tables (Graphs only for key metrics)"?

It has an effect, so I added it.

tsteur · 2018-11-08T02:23:07Z

plugins/ScheduledReports/lang/en.json

@@ -53,6 +53,8 @@
        "SuccessfullyUnsubscribed": "You have been successfully unsubscribed from the report %1$s.",
        "UnsubscribeFooter": "To unsubscribe from this report please follow this link: %1$s",
        "NoTokenProvided": "No token was provided in the URL",
-        "NoSubscriptionFound": "No subscription found. Maybe the report was already unsubscribed or removed."
+        "NoSubscriptionFound": "No subscription found. Maybe the report was already unsubscribed or removed.",
+        "EvolutionGraphsShowForEachInPeriod": "Evolution graphs show the evolution for %1$seach day%2$s in the last %3$s",


should it be " shows the evolution"?

should be 'show' since 'evolution graphs' is plural (in this case the option is for all evolution graphs in the report, which is why it's plural in the text)

tsteur · 2018-11-08T02:26:04Z

@diosmosis I've just tried it and first error I got was The evolutionPeriodN param has no effect when evolutionPeriodFor is "each".. If I understand this correct, maybe we could just ignore the number of days when each day is selected or so?

tsteur · 2018-11-08T02:27:10Z

this seems to be the default actually and as soon as I select the first option and then click create, the above error is shown.

diosmosis · 2018-11-11T03:01:55Z

@tsteur Updated the PR and fixed the issue you noticed.

tsteur · 2018-11-11T19:38:58Z

@diosmosis looks good. Be maybe great to change the config access... I reckon it be good to have a method Config::setSetting($category, $name, $value) which sets it in a save way that works cross PHP versions? Seeing myself repeat that code quite a few times...

Otherwise: I've updated to Mac OS Mojave and GD+freetype isn't working anymore so cannot test the graphs but otherwise looks good 👍

diosmosis · 2018-11-11T21:29:01Z

Ok, will merge after Config::setSetting change.

tsteur · 2018-11-12T00:37:42Z

LGTM

Add options to UI and start on backend code.

d42bcc1

diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 29, 2018

diosmosis added this to the 3.6.1 milestone Sep 29, 2018

diosmosis added 2 commits September 29, 2018 20:37

Handle new evolution feature in ScheduledReports & add new INI config…

3242f44

… to control lastN used in ImageGraph evolution.

Add tests for new feature.

578f3c7

diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Sep 30, 2018

diosmosis added 4 commits October 1, 2018 17:05

Give new scheduled reports different names.

b1226ab

Add tests to SystemTestCase.

939fce5

Add test.

3a97e4f

Update expected system test files.

d6414be

mattab modified the milestones: 3.6.1, 3.6.2 Oct 14, 2018

Merge branch '3.x-dev' into scheduled-reports-evo-period

60470f3

diosmosis force-pushed the scheduled-reports-evo-period branch from 8f7d046 to 60470f3 Compare November 4, 2018 23:16

mattab requested a review from tsteur November 7, 2018 21:38

tsteur reviewed Nov 8, 2018

View reviewed changes

plugins/ScheduledReports/API.php Show resolved Hide resolved

tsteur reviewed Nov 8, 2018

View reviewed changes

Apply PR review & fix issue pointed out.

3ca3df5

Add Config::setSetting() convenience method & use.

6ff3f6e

diosmosis added 3 commits November 11, 2018 17:01

Merge branch '3.x-dev' into scheduled-reports-evo-period

8aeccd0

fix some tests

c86062a

fix test

de56dca

diosmosis merged commit 66148cc into 3.x-dev Nov 12, 2018

diosmosis deleted the scheduled-reports-evo-period branch November 12, 2018 02:20

mattab mentioned this pull request Mar 1, 2019

Evolution graphs: "Change period" feature to show historical up to the very last period within the selected date range #14149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to change periods over which evolution graphs display in scheduled reports #13501

Ability to change periods over which evolution graphs display in scheduled reports #13501

diosmosis commented Sep 29, 2018 •

edited

tsteur commented Oct 2, 2018

diosmosis commented Oct 2, 2018

tsteur Nov 8, 2018

tsteur Nov 8, 2018

diosmosis Nov 11, 2018

tsteur Nov 8, 2018

diosmosis Nov 8, 2018

diosmosis Nov 11, 2018

tsteur Nov 11, 2018

tsteur Nov 8, 2018

diosmosis Nov 11, 2018

tsteur Nov 8, 2018

diosmosis Nov 11, 2018

tsteur commented Nov 8, 2018

tsteur commented Nov 8, 2018

diosmosis commented Nov 11, 2018

tsteur commented Nov 11, 2018

diosmosis commented Nov 11, 2018

tsteur commented Nov 12, 2018

Ability to change periods over which evolution graphs display in scheduled reports #13501

Ability to change periods over which evolution graphs display in scheduled reports #13501

Conversation

diosmosis commented Sep 29, 2018 • edited

tsteur commented Oct 2, 2018

diosmosis commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Nov 8, 2018

tsteur commented Nov 8, 2018

diosmosis commented Nov 11, 2018

tsteur commented Nov 11, 2018

diosmosis commented Nov 11, 2018

tsteur commented Nov 12, 2018

diosmosis commented Sep 29, 2018 •

edited