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.

Sign up for GitHub

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

Validate report period for scheduled reports #21043

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Validate report period for scheduled reports #21043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions plugins/ScheduledReports/API.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ public function addReport(

self::validateCommonReportAttributes($period, $hour, $description, $idSegment, $reportType, $reportFormat, $evolutionPeriodFor, $evolutionPeriodN);

if (null !== $periodParam) {
Copy link
Contributor

@michalkleiner michalkleiner Jul 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we adopting the Yoda style? :)

Strictly technically speaking — it is safer and can be checked by a linter, so why not...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer using yoda style, even though I sometimes miss to use it myself 🙈
But at some point we can consider to force that using a linter maybe.

self::validatePeriodParam($periodParam);
}

// report parameters validations
$parameters = self::validateReportParameters($reportType, $parameters);

Expand Down Expand Up @@ -195,6 +199,10 @@ public function updateReport(

self::validateCommonReportAttributes($period, $hour, $description, $idSegment, $reportType, $reportFormat, $evolutionPeriodFor, $evolutionPeriodN);

if (null !== $periodParam) {
self::validatePeriodParam($periodParam);
}

// report parameters validations
$parameters = self::validateReportParameters($reportType, $parameters);

Expand Down Expand Up @@ -621,6 +629,7 @@ public function sendReport($idReport, $period = false, $date = false, $force = f
$report = reset($reports);

if (!empty($period)) {
self::validatePeriodParam($period);
$report['period_param'] = $period;
}

Expand Down Expand Up @@ -845,6 +854,17 @@ private static function validateReportPeriod($period)
}
}

private static function validatePeriodParam($period)
{
$periodValidator = new Period\PeriodValidator();
$allowedPeriods = array_flip($periodValidator->getPeriodsAllowedForAPI());
unset($allowedPeriods['range']);

if (!array_key_exists($period, $allowedPeriods)) {
throw new Exception('Report period must be one of the following: ' . implode(', ', array_keys($allowedPeriods)) . ' (got ' . $period . ')');
}
}

private static function validateReportHour($hour)
{
if (!is_numeric($hour) || $hour < 0 || $hour > 23) {
Expand Down
7 changes: 6 additions & 1 deletion plugins/ScheduledReports/Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Piwik\API\Request;
use Piwik\Common;
use Piwik\Nonce;
use Piwik\Period\PeriodValidator;
use Piwik\Piwik;
use Piwik\Plugins\ImageGraph\ImageGraph;
use Piwik\Plugins\LanguagesManager\LanguagesManager;
Expand Down Expand Up @@ -46,7 +47,11 @@ public function index()
$view->displayFormats = ScheduledReports::getDisplayFormats();

$view->paramPeriods = [];
foreach (Piwik::$idPeriods as $label => $id) {

$periodValidator = new PeriodValidator();
$allowedPeriods = $periodValidator->getPeriodsAllowedForAPI();

foreach ($allowedPeriods as $label) {
if ($label === 'range') {
continue;
}
Expand Down
78 changes: 77 additions & 1 deletion plugins/ScheduledReports/tests/Integration/ApiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ public function testGetReportSubjectAndReportTitle($expectedReportSubject, $expe
);
$getReportSubjectAndReportTitle->setAccessible(true);

list($reportSubject, $reportTitle) = $getReportSubjectAndReportTitle->invoke( APIScheduledReports::getInstance(), $websiteName, $reports);
[$reportSubject, $reportTitle] = $getReportSubjectAndReportTitle->invoke( APIScheduledReports::getInstance(), $websiteName, $reports);
$this->assertEquals($expectedReportSubject, $reportSubject);
$this->assertEquals($expectedReportTitle, $reportTitle);
}
Expand Down Expand Up @@ -584,6 +584,37 @@ public function test_addReport_validatesEvolutionPeriodForParam()
);
}

public function testAddReportValidatesPeriodParam()
{
$invalidPeriod = 'tomorrow';

$this->expectException(\Exception::class);
$this->expectExceptionMessage(
'Report period must be one of the following: day, week, month, year (got ' . $invalidPeriod . ')'
);

self::setSuperUser();

APIScheduledReports::getInstance()->addReport(
1,
'',
Schedule::PERIOD_DAY,
0,
ScheduledReports::EMAIL_TYPE,
ReportRenderer::HTML_FORMAT,
[
'VisitsSummary_get',
'UserCountry_getCountry',
'Referrers_getWebsites',
],
[ScheduledReports::DISPLAY_FORMAT_PARAMETER => ScheduledReports::DISPLAY_FORMAT_TABLES_ONLY],
false,
'each',
null,
$invalidPeriod
);
}

public function test_addReport_validatesEvolutionPeriodNParam()
{
$this->expectException(\Exception::class);
Expand Down Expand Up @@ -636,6 +667,51 @@ public function test_addReport_throwsIfEvolutionPeriodNParamIsEach_AndLastNSuppl
);
}

public function testUpdateReportValidatesPeriodParam()
{
$invalidPeriod = 'tomorrow';

$this->expectException(\Exception::class);
$this->expectExceptionMessage(
'Report period must be one of the following: day, week, month, year (got ' . $invalidPeriod . ')'
);

self::setSuperUser();

$idReport = APIScheduledReports::getInstance()->addReport(
1,
'',
Schedule::PERIOD_DAY,
0,
ScheduledReports::EMAIL_TYPE,
ReportRenderer::HTML_FORMAT,
[
'VisitsSummary_get',
],
[ScheduledReports::DISPLAY_FORMAT_PARAMETER => ScheduledReports::DISPLAY_FORMAT_TABLES_ONLY]
);

APIScheduledReports::getInstance()->updateReport(
$idReport,
1,
'',
Schedule::PERIOD_DAY,
0,
ScheduledReports::EMAIL_TYPE,
ReportRenderer::HTML_FORMAT,
[
'VisitsSummary_get',
'UserCountry_getCountry',
'Referrers_getWebsites',
],
[ScheduledReports::DISPLAY_FORMAT_PARAMETER => ScheduledReports::DISPLAY_FORMAT_TABLES_ONLY],
false,
'each',
null,
$invalidPeriod
);
}

public function test_updateReport_validatesEvolutionPeriodForParam()
{
$this->expectException(\Exception::class);
Expand Down