Ensure insight reports uses the correct compare period number for periods other than day #15103
Conversation
sgiehl
added
Bug
sgiehl
force-pushed
the
fixinsightreport
branch
from
1a59ded
to
c1ed74d
Compare
| @@ -59,6 +59,17 @@ public function beforeLoadDataTable() | |||
| ); | |||
| } | |||
|
|
|||
| private function getComparedToXPeriodsAgo() | |||
| { | |||
| $period = Common::getRequestVar('period', null, 'string'); | |||
There was a problem hiding this comment.
@sgiehl took me a while to understand what is happening here and seeing no Range::getDateXPeriodsAgo() is converting eg indeed 365 to 365 months ago. From what I see week and year only can compare to the previous period, but compared_to_x_periods_ago can be 1 or 12 for months. Would we need to convert eg a number 365 to 12 instead of 1 when the period is month?
Also probably not needed but could maybe have a side effect... (probs not and only if plugins where to maybe hook into it somehow or so)... Would we maybe need to change $this->requestConfig->compared_to_x_periods_ago in above code and set the updated value to comparedToXPeriods? I don't think it's actually used in any way but may be better.
There was a problem hiding this comment.
added another condition for month to fix that
…iods other than day
sgiehl
force-pushed
the
fixinsightreport
branch
from
c1ed74d
to
11130c4
Compare
When viewing a insight report for a period other than day it might happen, that the report shows an error like this:
This happens when someone chooses to compare to same day in last year when viewing a day report and switching to another period afterwards.
When switching the compared date this setting is stored in the report options. When switching to another period like month, it tries to compare the current month to the month 365 months ago, as the stored option will be used.
As all periods except day should always compare to the last period before. This fix should solve this issue.