From 4a68972a4afffc9bc57b1e7d2045a2f80997d43e Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 17 Sep 2024 10:54:43 +0100 Subject: [PATCH] MDL-83181 reportbuilder: ensure audience correctness before delete. --- .../classes/external/audiences/delete.php | 11 +---- .../classes/local/helpers/audience.php | 25 ++++++++++ .../tests/local/helpers/audience_test.php | 49 +++++++++++++++++++ 3 files changed, 76 insertions(+), 9 deletions(-) diff --git a/reportbuilder/classes/external/audiences/delete.php b/reportbuilder/classes/external/audiences/delete.php index 469d958764b..0c5331f4e1b 100644 --- a/reportbuilder/classes/external/audiences/delete.php +++ b/reportbuilder/classes/external/audiences/delete.php @@ -18,10 +18,10 @@ declare(strict_types=1); namespace core_reportbuilder\external\audiences; -use core_reportbuilder\local\audiences\base; use core_external\external_api; use core_external\external_value; use core_external\external_function_parameters; +use core_reportbuilder\local\helpers\audience; use core_reportbuilder\manager; use core_reportbuilder\permission; @@ -69,14 +69,7 @@ class delete extends external_api { self::validate_context($report->get_context()); permission::require_can_edit_report($report->get_report_persistent()); - $baseinstance = base::instance($instanceid); - if ($baseinstance && $baseinstance->user_can_edit()) { - $persistent = $baseinstance->get_persistent(); - $persistent->delete(); - return true; - } - - return false; + return audience::delete_report_audience($reportid, $instanceid); } /** diff --git a/reportbuilder/classes/local/helpers/audience.php b/reportbuilder/classes/local/helpers/audience.php index d3349137ff6..ebbb4024d37 100644 --- a/reportbuilder/classes/local/helpers/audience.php +++ b/reportbuilder/classes/local/helpers/audience.php @@ -25,6 +25,7 @@ use core_collator; use core_component; use core_reportbuilder\local\audiences\base; use core_reportbuilder\local\models\{audience as audience_model, schedule}; +use invalid_parameter_exception; /** * Class containing report audience helper methods @@ -254,6 +255,30 @@ class audience { return array_unique($audienceids, SORT_NUMERIC); } + /** + * Delete given audience from report + * + * @param int $reportid + * @param int $audienceid + * @return bool + * @throws invalid_parameter_exception + */ + public static function delete_report_audience(int $reportid, int $audienceid): bool { + $audience = audience_model::get_record(['id' => $audienceid, 'reportid' => $reportid]); + if ($audience === false) { + throw new invalid_parameter_exception('Invalid audience'); + } + + $instance = base::instance(0, $audience->to_record()); + if ($instance && $instance->user_can_edit()) { + $persistent = $instance->get_persistent(); + $persistent->delete(); + return true; + } + + return false; + } + /** * Returns the list of audiences types in the system. * diff --git a/reportbuilder/tests/local/helpers/audience_test.php b/reportbuilder/tests/local/helpers/audience_test.php index fcd20a33e96..1ad90c64669 100644 --- a/reportbuilder/tests/local/helpers/audience_test.php +++ b/reportbuilder/tests/local/helpers/audience_test.php @@ -22,7 +22,9 @@ use advanced_testcase; use context_system; use core_reportbuilder_generator; use core_reportbuilder\reportbuilder\audience\manual; +use core_reportbuilder\local\models\audience as audience_model; use core_user\reportbuilder\datasource\users; +use invalid_parameter_exception; /** * Unit tests for audience helper @@ -362,4 +364,51 @@ class audience_test extends advanced_testcase { $audiencetwo->get_persistent()->get('id'), ], $audiences); } + + /** + * Testing deleting report audience + */ + public function test_delete_report_audience(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + + $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); + $audienceone = $generator->create_audience(['reportid' => $report->get('id'), 'configdata' => []]); + $audiencetwo = $generator->create_audience(['reportid' => $report->get('id'), 'configdata' => []]); + + $audiences = audience_model::get_records(['reportid' => $report->get('id')]); + $this->assertCount(2, $audiences); + + // Delete the first audience. + $result = audience::delete_report_audience($report->get('id'), $audienceone->get_persistent()->get('id')); + $this->assertTrue($result); + + // We should be left with only the second audience. + $audiences = audience_model::get_records(['reportid' => $report->get('id')]); + $this->assertCount(1, $audiences); + $this->assertEquals($audiencetwo->get_persistent()->get('id'), reset($audiences)->get('id')); + } + + /** + * Testing deleting invalid report audience + */ + public function test_delete_report_audience_invalid(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + + $reportone = $generator->create_report(['name' => 'Report one', 'source' => users::class, 'default' => false]); + $audienceone = $generator->create_audience(['reportid' => $reportone->get('id'), 'configdata' => []]); + + $reporttwo = $generator->create_report(['name' => 'Report two', 'source' => users::class, 'default' => false]); + + $this->expectException(invalid_parameter_exception::class); + $this->expectExceptionMessage('Invalid audience'); + audience::delete_report_audience($reporttwo->get('id'), $audienceone->get_persistent()->get('id')); + } }