From 8fe1f83fe1cefbe48f34a5192fc9f0aafbbf16b5 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Wed, 28 Sep 2022 18:00:50 +0100 Subject: [PATCH] MDL-75855 reportbuilder: don't allow condition/filter duplication. Custom reports shouldn't allow the same condition and/or filter instance to exist more that once per report. --- lib/db/install.xml | 3 ++ lib/db/upgrade.php | 30 +++++++++++++++ .../classes/local/helpers/report.php | 16 +++++++- .../tests/external/conditions/delete_test.php | 2 +- .../external/conditions/reorder_test.php | 2 +- .../tests/external/conditions/reset_test.php | 2 +- .../tests/external/filters/delete_test.php | 2 +- .../tests/external/filters/reorder_test.php | 2 +- .../tests/local/helpers/report_test.php | 38 +++++++++++++++++++ .../tests/task/send_schedule_test.php | 4 +- reportbuilder/upgrade.txt | 2 + version.php | 2 +- 12 files changed, 96 insertions(+), 9 deletions(-) diff --git a/lib/db/install.xml b/lib/db/install.xml index e18b9a07e03..4db36cf9eae 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -4610,6 +4610,9 @@ + + + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index b09d8c85aa2..69e3773c22a 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -3383,5 +3383,35 @@ privatefiles,moodle|/user/files.php'; upgrade_main_savepoint(true, 2023081800.01); } + if ($oldversion < 2023082200.01) { + + // Remove any non-unique filters/conditions. + $duplicates = $DB->get_records_sql(" + SELECT MIN(id) AS id, reportid, uniqueidentifier, iscondition + FROM {reportbuilder_filter} + GROUP BY reportid, uniqueidentifier, iscondition + HAVING COUNT(*) > 1"); + + foreach ($duplicates as $duplicate) { + $DB->delete_records_select( + 'reportbuilder_filter', + 'id <> :id AND reportid = :reportid AND uniqueidentifier = :uniqueidentifier AND iscondition = :iscondition', + (array) $duplicate + ); + } + + // Define index report-filter (unique) to be added to reportbuilder_filter. + $table = new xmldb_table('reportbuilder_filter'); + $index = new xmldb_index('report-filter', XMLDB_INDEX_UNIQUE, ['reportid', 'uniqueidentifier', 'iscondition']); + + // Conditionally launch add index report-filter. + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2023082200.01); + } + return true; } diff --git a/reportbuilder/classes/local/helpers/report.php b/reportbuilder/classes/local/helpers/report.php index 7ee0a080265..b979443896e 100644 --- a/reportbuilder/classes/local/helpers/report.php +++ b/reportbuilder/classes/local/helpers/report.php @@ -110,6 +110,7 @@ class report { public static function add_report_column(int $reportid, string $uniqueidentifier): column { $report = manager::get_report_from_id($reportid); + // Ensure column is available. if (!array_key_exists($uniqueidentifier, $report->get_columns())) { throw new invalid_parameter_exception('Invalid column'); } @@ -237,10 +238,16 @@ class report { public static function add_report_condition(int $reportid, string $uniqueidentifier): filter { $report = manager::get_report_from_id($reportid); + // Ensure condition is available. if (!array_key_exists($uniqueidentifier, $report->get_conditions())) { throw new invalid_parameter_exception('Invalid condition'); } + // Ensure condition wasn't already added. + if (array_key_exists($uniqueidentifier, $report->get_active_conditions())) { + throw new invalid_parameter_exception('Duplicate condition'); + } + $condition = new filter(0, (object) [ 'reportid' => $reportid, 'uniqueidentifier' => $uniqueidentifier, @@ -317,11 +324,16 @@ class report { public static function add_report_filter(int $reportid, string $uniqueidentifier): filter { $report = manager::get_report_from_id($reportid); - $reportfilters = $report->get_filters(); - if (!array_key_exists($uniqueidentifier, $reportfilters)) { + // Ensure filter is available. + if (!array_key_exists($uniqueidentifier, $report->get_filters())) { throw new invalid_parameter_exception('Invalid filter'); } + // Ensure filter wasn't already added. + if (array_key_exists($uniqueidentifier, $report->get_active_filters())) { + throw new invalid_parameter_exception('Duplicate filter'); + } + $filter = new filter(0, (object) [ 'reportid' => $reportid, 'uniqueidentifier' => $uniqueidentifier, diff --git a/reportbuilder/tests/external/conditions/delete_test.php b/reportbuilder/tests/external/conditions/delete_test.php index 9ef9bbc6e78..dc236aac8f0 100644 --- a/reportbuilder/tests/external/conditions/delete_test.php +++ b/reportbuilder/tests/external/conditions/delete_test.php @@ -84,7 +84,7 @@ class delete_test extends externallib_advanced_testcase { /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); - $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); $condition = $generator->create_condition(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:email']); $user = $this->getDataGenerator()->create_user(); diff --git a/reportbuilder/tests/external/conditions/reorder_test.php b/reportbuilder/tests/external/conditions/reorder_test.php index bb0a5e648a9..f87513d7893 100644 --- a/reportbuilder/tests/external/conditions/reorder_test.php +++ b/reportbuilder/tests/external/conditions/reorder_test.php @@ -92,7 +92,7 @@ class reorder_test extends externallib_advanced_testcase { /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); - $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); $condition = $generator->create_condition(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:email']); $user = $this->getDataGenerator()->create_user(); diff --git a/reportbuilder/tests/external/conditions/reset_test.php b/reportbuilder/tests/external/conditions/reset_test.php index 1538583c1ba..de423e2a5f7 100644 --- a/reportbuilder/tests/external/conditions/reset_test.php +++ b/reportbuilder/tests/external/conditions/reset_test.php @@ -49,7 +49,7 @@ class reset_test extends externallib_advanced_testcase { /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); - $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); $generator->create_condition(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:fullname']); $instance = manager::get_report_from_persistent($report); diff --git a/reportbuilder/tests/external/filters/delete_test.php b/reportbuilder/tests/external/filters/delete_test.php index 249270592b6..ec51832bf02 100644 --- a/reportbuilder/tests/external/filters/delete_test.php +++ b/reportbuilder/tests/external/filters/delete_test.php @@ -85,7 +85,7 @@ class delete_test extends externallib_advanced_testcase { /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); - $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); $filter = $generator->create_filter(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:email']); $user = $this->getDataGenerator()->create_user(); diff --git a/reportbuilder/tests/external/filters/reorder_test.php b/reportbuilder/tests/external/filters/reorder_test.php index faa60a308de..a05c52dd2c3 100644 --- a/reportbuilder/tests/external/filters/reorder_test.php +++ b/reportbuilder/tests/external/filters/reorder_test.php @@ -95,7 +95,7 @@ class reorder_test extends externallib_advanced_testcase { /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); - $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); $filter = $generator->create_filter(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:email']); $user = $this->getDataGenerator()->create_user(); diff --git a/reportbuilder/tests/local/helpers/report_test.php b/reportbuilder/tests/local/helpers/report_test.php index 8bc6fd34335..1eceae0f5ce 100644 --- a/reportbuilder/tests/local/helpers/report_test.php +++ b/reportbuilder/tests/local/helpers/report_test.php @@ -386,6 +386,25 @@ class report_test extends advanced_testcase { report::add_report_condition($report->get('id'), 'user:invalid'); } + /** + * Test adding duplicate report condition + */ + public function test_add_report_condition_duplicate(): 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]); + + // First one is fine. + report::add_report_condition($report->get('id'), 'user:email'); + + $this->expectException(invalid_parameter_exception::class); + $this->expectExceptionMessage('Duplicate condition'); + report::add_report_condition($report->get('id'), 'user:email'); + } + /** * Test deleting report condition */ @@ -536,6 +555,25 @@ class report_test extends advanced_testcase { report::add_report_filter($report->get('id'), 'user:invalid'); } + /** + * Test adding duplicate report filter + */ + public function test_add_report_filter_duplicate(): 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]); + + // First one is fine. + report::add_report_filter($report->get('id'), 'user:email'); + + $this->expectException(invalid_parameter_exception::class); + $this->expectExceptionMessage('Duplicate filter'); + report::add_report_filter($report->get('id'), 'user:email'); + } + /** * Test deleting report filter */ diff --git a/reportbuilder/tests/task/send_schedule_test.php b/reportbuilder/tests/task/send_schedule_test.php index 7597244733b..7f53bd7ea00 100644 --- a/reportbuilder/tests/task/send_schedule_test.php +++ b/reportbuilder/tests/task/send_schedule_test.php @@ -157,9 +157,11 @@ class send_schedule_test extends advanced_testcase { $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); // Create a report that won't return any data. - $report = $generator->create_report(['name' => 'Myself', 'source' => users::class]); + $report = $generator->create_report(['name' => 'Myself', 'source' => users::class, 'default' => false]); + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:username']); $generator->create_condition(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:username']); + manager::get_report_from_persistent($report)->set_condition_values([ 'user:username_operator' => text::IS_EQUAL_TO, 'user:username_value' => 'baconlettucetomato', diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt index 410ebc3d168..5648ff04d22 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -23,6 +23,8 @@ Information provided here is intended especially for developers. * If a non-default column is specified in a datasource `get_default_column_sorting` method, a coding exception will be thrown * Trying to add/annotate duplicate entity names to a report will now throw a coding exception * The `get_default_entity_name` method of the base entity class is now private, and shouldn't be overridden in extending classes +* The report helper methods `add_report_[condition|filter]` now throw an exception when trying to add duplicate conditions or + filters to a report * Two new methods: - `get_default_no_results_notice` and - `set_default_no_results_notice` diff --git a/version.php b/version.php index 0bb712c721a..af1a3b30d5f 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2023082200.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2023082200.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '4.3dev+ (Build: 20230822)'; // Human-friendly version name