From e4e7e59d907017ed879d30c05ed3839bc2f9c8da Mon Sep 17 00:00:00 2001
From: Paul Holden <paulh@moodle.com>
Date: Mon, 24 Jul 2023 16:55:01 +0100
Subject: [PATCH] MDL-78807 reportbuilder: ensure report base conditions are
 non-empty.

---
 reportbuilder/classes/local/report/base.php   | 10 ++-
 .../tests/local/report/base_test.php          | 75 +++++++++++++++++++
 reportbuilder/upgrade.txt                     |  1 +
 3 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/reportbuilder/classes/local/report/base.php b/reportbuilder/classes/local/report/base.php
index 9679c020ae8..deeebb8076b 100644
--- a/reportbuilder/classes/local/report/base.php
+++ b/reportbuilder/classes/local/report/base.php
@@ -225,16 +225,20 @@ abstract class base {
     }
 
     /**
-     * Define more complex clause that will always be applied to the report query
+     * Define more complex/non-empty clause to apply to the report query
      *
      * @param string $where
      * @param array $params Note that the param names should be generated by {@see database::generate_param_name}
      */
     final public function add_base_condition_sql(string $where, array $params = []): void {
+
+        // Validate parameters always, so that potential errors are caught early.
         database::validate_params($params);
 
-        $this->sqlwheres[] = trim($where);
-        $this->sqlparams = $params + $this->sqlparams;
+        if ($where !== '') {
+            $this->sqlwheres[] = trim($where);
+            $this->sqlparams = $params + $this->sqlparams;
+        }
     }
 
     /**
diff --git a/reportbuilder/tests/local/report/base_test.php b/reportbuilder/tests/local/report/base_test.php
index 5f04428a59f..63ea6aa792d 100644
--- a/reportbuilder/tests/local/report/base_test.php
+++ b/reportbuilder/tests/local/report/base_test.php
@@ -21,6 +21,7 @@ namespace core_reportbuilder\local\report;
 use advanced_testcase;
 use coding_exception;
 use context_system;
+use core_reportbuilder\local\helpers\database;
 use core_reportbuilder\system_report_available;
 use core_reportbuilder\system_report_factory;
 use lang_string;
@@ -70,6 +71,80 @@ class base_test extends advanced_testcase {
         $this->assertEmpty($params);
     }
 
+    /**
+     * Test for adding SQL base condition to a report
+     */
+    public function test_add_base_condition_sql(): void {
+        $this->resetAfterTest();
+
+        $parameter = database::generate_param_name();
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+        $systemreport->add_base_condition_sql("username = :{$parameter}", [$parameter => 'admin']);
+
+        [$where, $params] = $systemreport->get_base_condition();
+        $this->assertEquals("username = :{$parameter}", $where);
+        $this->assertEquals([$parameter => 'admin'], $params);
+    }
+
+    /**
+     * Test for adding multiple SQL base condition to a report
+     */
+    public function test_add_base_condition_sql_multiple(): void {
+        $this->resetAfterTest();
+
+        [$paramusername, $paramemail] = database::generate_param_names(2);
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+        $systemreport->add_base_condition_sql("username = :{$paramusername}", [$paramusername => 'admin']);
+        $systemreport->add_base_condition_sql("email = :{$paramemail}", [$paramemail => 'admin@example.com']);
+
+        [$where, $params] = $systemreport->get_base_condition();
+        $this->assertEquals("username = :{$paramusername} AND email = :{$paramemail}", $where);
+        $this->assertEquals([$paramusername => 'admin', $paramemail => 'admin@example.com'], $params);
+    }
+
+    /**
+     * Test for adding empty SQL base condition to a report
+     */
+    public function test_add_base_condition_sql_empty_clause(): void {
+        $this->resetAfterTest();
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+        $systemreport->add_base_condition_sql('username IS NOT NULL');
+        $systemreport->add_base_condition_sql('');
+
+        [$where, $params] = $systemreport->get_base_condition();
+        $this->assertEquals("username IS NOT NULL", $where);
+        $this->assertEmpty($params);
+    }
+
+    /**
+     * Test for adding SQL base condition to a report with invalid parameter
+     */
+    public function test_add_base_condition_sql_invalid_parameter(): void {
+        $this->resetAfterTest();
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+
+        $this->expectException(coding_exception::class);
+        $this->expectExceptionMessage('Invalid parameter names');
+        $systemreport->add_base_condition_sql("username = :param", ['param' => 'admin']);
+    }
+
+    /**
+     * Test getting report base conditions, where none have been set
+     */
+    public function test_get_base_condition_default(): void {
+        $this->resetAfterTest();
+
+        $systemreport = system_report_factory::create(system_report_available::class, context_system::instance());
+
+        [$where, $params] = $systemreport->get_base_condition();
+        $this->assertEmpty($where);
+        $this->assertEmpty($params);
+    }
+
     /**
      * Test for get_filter_instances
      */
diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt
index 223a0df266d..a2bb206b6d6 100644
--- a/reportbuilder/upgrade.txt
+++ b/reportbuilder/upgrade.txt
@@ -11,6 +11,7 @@ Information provided here is intended especially for developers.
   - 'enrolment:role` => `role:name`
 * The following report entity filters/conditions have been deprecated, with replacements as follows:
   - `enrolment:method` => `enrol:plugin`
+* The `add_base_condition_sql` method of the base report class will now ignore empty where clauses
 * 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