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 ffc4baecee5..6dc52e62102 100644
--- a/lib/db/upgrade.php
+++ b/lib/db/upgrade.php
@@ -3471,5 +3471,35 @@ privatefiles,moodle|/user/files.php';
upgrade_main_savepoint(true, 2023082200.02);
}
+ if ($oldversion < 2023082200.04) {
+
+ // 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.04);
+ }
+
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 cc7be4cb625..6df9844f42c 100644
--- a/reportbuilder/upgrade.txt
+++ b/reportbuilder/upgrade.txt
@@ -28,6 +28,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 cea96a68c54..130f453921b 100644
--- a/version.php
+++ b/version.php
@@ -29,7 +29,7 @@
defined('MOODLE_INTERNAL') || die();
-$version = 2023082200.03; // YYYYMMDD = weekly release date of this DEV branch.
+$version = 2023082200.04; // 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