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

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

Merged
merged 13 commits into from Nov 12, 2018

Conversation

Projects
None yet
3 participants
@diosmosis
Member

diosmosis commented Sep 29, 2018

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.

@diosmosis diosmosis added this to the 3.6.1 milestone Sep 29, 2018

diosmosis added some commits Sep 30, 2018

Handle new evolution feature in ScheduledReports & add new INI config…
… to control lastN used in ImageGraph evolution.

diosmosis added some commits Oct 2, 2018

@tsteur

This comment has been minimized.

Member

tsteur commented Oct 2, 2018

Can we move this to 3.7.0 maybe?

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 2, 2018

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

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

@diosmosis diosmosis force-pushed the scheduled-reports-evo-period branch from 8f7d046 to 60470f3 Nov 4, 2018

@mattab mattab requested a review from tsteur Nov 7, 2018

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

This comment has been minimized.

@tsteur

tsteur Nov 8, 2018

Member

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

@@ -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

This comment has been minimized.

@tsteur

tsteur Nov 8, 2018

Member

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

This comment has been minimized.

@diosmosis

diosmosis Nov 11, 2018

Member

$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.

$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'];

This comment has been minimized.

@tsteur

tsteur Nov 8, 2018

Member

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

This comment has been minimized.

@diosmosis

diosmosis Nov 8, 2018

Member

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

This comment has been minimized.

@diosmosis

diosmosis Nov 11, 2018

Member

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

This comment has been minimized.

@tsteur

tsteur Nov 11, 2018

Member

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

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

This comment has been minimized.

@tsteur

tsteur Nov 8, 2018

Member

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

This comment has been minimized.

@diosmosis

diosmosis Nov 11, 2018

Member

It has an effect, so I added it.

@@ -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",

This comment has been minimized.

@tsteur

tsteur Nov 8, 2018

Member

should it be " shows the evolution"?

This comment has been minimized.

@diosmosis

diosmosis Nov 11, 2018

Member

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

This comment has been minimized.

Member

tsteur commented Nov 8, 2018

@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

This comment has been minimized.

Member

tsteur commented Nov 8, 2018

image

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

This comment has been minimized.

Member

diosmosis commented Nov 11, 2018

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

@tsteur

This comment has been minimized.

Member

tsteur commented Nov 11, 2018

@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

This comment has been minimized.

Member

diosmosis commented Nov 11, 2018

Ok, will merge after Config::setSetting change.

@tsteur

This comment has been minimized.

Member

tsteur commented Nov 12, 2018

LGTM

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

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@diosmosis diosmosis deleted the scheduled-reports-evo-period branch Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment