@diosmosis opened this Pull Request on September 29th 2018 Member

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.

@tsteur commented on October 2nd 2018 Member

Can we move this to 3.7.0 maybe?

@diosmosis commented on October 2nd 2018 Member

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 commented on November 8th 2018 Member

@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 commented on November 8th 2018 Member

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 commented on November 11th 2018 Member

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

@tsteur commented on November 11th 2018 Member

@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 commented on November 11th 2018 Member

Ok, will merge after Config::setSetting change.

@tsteur commented on November 12th 2018 Member

LGTM

This Pull Request was closed on November 12th 2018
Powered by GitHub Issue Mirror