From 9e5f911411c2a333f72ccc720a4e48166f213353 Mon Sep 17 00:00:00 2001
From: Paul Holden <paulh@moodle.com>
Date: Mon, 30 Sep 2024 12:14:16 +0100
Subject: [PATCH 1/2] MDL-82809 reportbuilder: load custom/profile fields
 consistently.

Ensure that provided columns and filters for each use appropriate
checks to determine their availability (based on user permissions,
etc), rather than conditionally adding them to the entity.
---
 .../classes/local/entities/course.php         |  25 ++-
 .../classes/local/helpers/custom_fields.php   |  11 +-
 .../local/helpers/user_profile_fields.php     |  18 +-
 .../local/helpers/custom_fields_test.php      |  39 ++++-
 .../helpers/user_profile_fields_test.php      | 154 ++++++++++++------
 5 files changed, 166 insertions(+), 81 deletions(-)

diff --git a/reportbuilder/classes/local/entities/course.php b/reportbuilder/classes/local/entities/course.php
index 7d4586c045e..a21ea0a51f4 100644
--- a/reportbuilder/classes/local/entities/course.php
+++ b/reportbuilder/classes/local/entities/course.php
@@ -74,30 +74,27 @@ class course extends base {
     }
 
     /**
-     * Get custom fields helper
-     *
-     * @return custom_fields
-     */
-    protected function get_custom_fields(): custom_fields {
-        $customfields = new custom_fields($this->get_table_alias('course') . '.id', $this->get_entity_name(),
-            'core_course', 'course');
-        $customfields->add_joins($this->get_joins());
-        return $customfields;
-    }
-
-    /**
-     * Initialise the entity, adding all course and custom course fields
+     * Initialise the entity
      *
      * @return base
      */
     public function initialise(): base {
-        $customfields = $this->get_custom_fields();
+        $tablealias = $this->get_table_alias('course');
+
+        $customfields = (new custom_fields(
+            "{$tablealias}.id",
+            $this->get_entity_name(),
+            'core_course',
+            'course',
+        ))
+            ->add_joins($this->get_joins());
 
         $columns = array_merge($this->get_all_columns(), $customfields->get_columns());
         foreach ($columns as $column) {
             $this->add_column($column);
         }
 
+        // All the filters defined by the entity can also be used as conditions.
         $filters = array_merge($this->get_all_filters(), $customfields->get_filters());
         foreach ($filters as $filter) {
             $this
diff --git a/reportbuilder/classes/local/helpers/custom_fields.php b/reportbuilder/classes/local/helpers/custom_fields.php
index 5b3e3525a61..da0e67b5ebc 100644
--- a/reportbuilder/classes/local/helpers/custom_fields.php
+++ b/reportbuilder/classes/local/helpers/custom_fields.php
@@ -257,13 +257,12 @@ class custom_fields {
                     $customdataparams,
                 ))
                     ->add_joins($this->get_joins())
-                    ->add_join($this->get_table_join($field));
+                    ->add_join($this->get_table_join($field))
+                    ->set_is_available($this->handler->can_view($field, 0));
 
-                // Options are stored inside configdata json string and we need to convert it to array.
-                if ($field->get('type') === 'select') {
-                    $filter->set_options_callback(static function() use ($field): array {
-                        return $field->get_options();
-                    });
+                // If using a select filter, then populate the options.
+                if ($filter->get_filter_class() === select::class) {
+                    $filter->set_options_callback(fn(): array => $field->get_options());
                 }
 
                 $filters[] = $filter;
diff --git a/reportbuilder/classes/local/helpers/user_profile_fields.php b/reportbuilder/classes/local/helpers/user_profile_fields.php
index 420885968db..2f1fe1ae2c1 100644
--- a/reportbuilder/classes/local/helpers/user_profile_fields.php
+++ b/reportbuilder/classes/local/helpers/user_profile_fields.php
@@ -56,11 +56,7 @@ class user_profile_fields {
         /** @var string The entity name used when adding columns and filters */
         private readonly string $entityname,
     ) {
-        // Retrieve the list of available/visible user profile fields.
-        $this->userprofilefields = array_filter(
-            profile_get_user_fields_with_data(0),
-            fn(profile_field_base $field) => $field->is_visible(),
-        );
+        $this->userprofilefields = profile_get_user_fields_with_data(0);
     }
 
     /**
@@ -152,7 +148,8 @@ class user_profile_fields {
                     );
 
                     return $field->display_data();
-                }, $profilefield);
+                }, $profilefield)
+                ->set_is_available($profilefield->is_visible());
         }
 
         return $columns;
@@ -222,11 +219,12 @@ class user_profile_fields {
                 $userinfoparams,
             ))
                 ->add_joins($this->get_joins())
-                ->add_join($this->get_table_join($profilefield));
+                ->add_join($this->get_table_join($profilefield))
+                ->set_is_available($profilefield->is_visible());
 
-            // If menu type then set filter options as appropriate.
-            if ($profilefield->field->datatype === 'menu') {
-                $filter->set_options($profilefield->options);
+            // If using a select filter, then populate the options.
+            if ($filter->get_filter_class() === select::class) {
+                $filter->set_options_callback(fn(): array => $profilefield->options);
             }
 
             $filters[] = $filter;
diff --git a/reportbuilder/tests/local/helpers/custom_fields_test.php b/reportbuilder/tests/local/helpers/custom_fields_test.php
index 2e03c3cb262..538e7f288d1 100644
--- a/reportbuilder/tests/local/helpers/custom_fields_test.php
+++ b/reportbuilder/tests/local/helpers/custom_fields_test.php
@@ -65,9 +65,10 @@ final class custom_fields_test extends core_reportbuilder_testcase {
             ['categoryid' => $category->get('id'), 'type' => 'textarea', 'name' => 'Textarea', 'shortname' => 'textarea',
                 'configdata' => ['defaultvalue' => 'Default']]);
 
+        // This field is available only to course teachers.
         $generator->create_field(
             ['categoryid' => $category->get('id'), 'type' => 'checkbox', 'name' => 'Checkbox', 'shortname' => 'checkbox',
-                'configdata' => ['checkbydefault' => 1]]);
+                'configdata' => ['checkbydefault' => 1, 'visibility' => 1]]);
 
         $generator->create_field(
             ['categoryid' => $category->get('id'), 'type' => 'date', 'name' => 'Date', 'shortname' => 'date']);
@@ -88,10 +89,11 @@ final class custom_fields_test extends core_reportbuilder_testcase {
      */
     public function test_get_columns(): void {
         $this->resetAfterTest();
+        $this->setAdminUser();
 
         $customfields = $this->generate_customfields();
-        $columns = $customfields->get_columns();
 
+        $columns = $customfields->get_columns();
         $this->assertCount(5, $columns);
         $this->assertContainsOnlyInstancesOf(column::class, $columns);
 
@@ -112,6 +114,19 @@ final class custom_fields_test extends core_reportbuilder_testcase {
             [true, false, true, true, true],
             array_map(fn(column $column) => $column->get_is_sortable(), $columns)
         );
+
+        // Column available.
+        $this->assertEquals(
+            [true, true, true, true, true],
+            array_map(fn(column $column) => $column->get_is_available(), $columns),
+        );
+
+        // Column available, for non-privileged user.
+        $this->setUser(null);
+        $this->assertEquals(
+            [true, true, false, true, true],
+            array_map(fn(column $column) => $column->get_is_available(), $customfields->get_columns()),
+        );
     }
 
     /**
@@ -150,18 +165,32 @@ final class custom_fields_test extends core_reportbuilder_testcase {
      */
     public function test_get_filters(): void {
         $this->resetAfterTest();
+        $this->setAdminUser();
 
         $customfields = $this->generate_customfields();
-        $filters = $customfields->get_filters();
 
+        $filters = $customfields->get_filters();
         $this->assertCount(5, $filters);
         $this->assertContainsOnlyInstancesOf(filter::class, $filters);
 
-        // Filter titles.
+        // Filter headers.
         $this->assertEquals(
             ['Text', 'Textarea', 'Checkbox', 'Date', 'Select'],
             array_map(fn(filter $filter) => $filter->get_header(), $filters)
         );
+
+        // Filter available.
+        $this->assertEquals(
+            [true, true, true, true, true],
+            array_map(fn(filter $filter) => $filter->get_is_available(), $filters),
+        );
+
+        // Filter available, for non-privileged user.
+        $this->setUser(null);
+        $this->assertEquals(
+            [true, true, false, true, true],
+            array_map(fn(filter $filter) => $filter->get_is_available(), $customfields->get_filters()),
+        );
     }
 
     /**
@@ -169,6 +198,7 @@ final class custom_fields_test extends core_reportbuilder_testcase {
      */
     public function test_custom_report_content(): void {
         $this->resetAfterTest();
+        $this->setAdminUser();
 
         $category = $this->getDataGenerator()->create_category(['name' => 'Zebras']);
         $courseone = $this->getDataGenerator()->create_course(['category' => $category->id, 'fullname' => 'C1']);
@@ -301,6 +331,7 @@ final class custom_fields_test extends core_reportbuilder_testcase {
      */
     public function test_custom_report_filter(string $filtername, array $filtervalues, ?string $expectmatch = null): void {
         $this->resetAfterTest();
+        $this->setAdminUser();
 
         $this->getDataGenerator()->create_course(['fullname' => 'C1']);
 
diff --git a/reportbuilder/tests/local/helpers/user_profile_fields_test.php b/reportbuilder/tests/local/helpers/user_profile_fields_test.php
index bdd7874c3e2..7bd83aab232 100644
--- a/reportbuilder/tests/local/helpers/user_profile_fields_test.php
+++ b/reportbuilder/tests/local/helpers/user_profile_fields_test.php
@@ -49,9 +49,10 @@ final class user_profile_fields_test extends core_reportbuilder_testcase {
         $this->getDataGenerator()->create_custom_profile_field([
             'shortname' => 'checkbox', 'name' => 'Checkbox field', 'datatype' => 'checkbox', 'defaultdata' => 1]);
 
+        // This field is available only to admins.
         $this->getDataGenerator()->create_custom_profile_field([
             'shortname' => 'datetime', 'name' => 'Date field', 'datatype' => 'datetime', 'param2' => 2022, 'param3' => 0,
-                'defaultdata' => 0]);
+                'defaultdata' => 0, 'visible' => PROFILE_VISIBLE_NONE]);
 
         $this->getDataGenerator()->create_custom_profile_field([
             'shortname' => 'menu', 'name' => 'Menu field', 'datatype' => 'menu', 'param1' => "Cat\nDog", 'defaultdata' => 'Cat']);
@@ -77,55 +78,88 @@ final class user_profile_fields_test extends core_reportbuilder_testcase {
      */
     public function test_get_columns(): void {
         $this->resetAfterTest();
-
-        $userentity = new user();
-        $useralias = $userentity->get_table_alias('user');
+        $this->setAdminUser();
 
         // Get pre-existing user profile fields.
-        $initialuserprofilefields = new user_profile_fields("$useralias.id", $userentity->get_entity_name());
-        $initialcolumns = $initialuserprofilefields->get_columns();
-        $initialcolumntitles = array_map(static function(column $column): string {
-            return $column->get_title();
-        }, $initialcolumns);
-        $initialcolumntypes = array_map(static function(column $column): int {
-            return $column->get_type();
-        }, $initialcolumns);
+        $userentity = new user();
+        $initialcolumns = (new user_profile_fields(
+            $userentity->get_table_alias('user') . '.id',
+            $userentity->get_entity_name(),
+        ))->get_columns();
 
         // Add new custom profile fields.
         $userprofilefields = $this->generate_userprofilefields();
-        $columns = $userprofilefields->get_columns();
 
-        // Columns count should be equal to start + 6.
-        $this->assertCount(count($initialcolumns) + 6, $columns);
+        // Ensure pre-existing fields are ignored in subsequent assertions.
+        $columns = array_slice($userprofilefields->get_columns(), count($initialcolumns));
+        $this->assertCount(6, $columns);
         $this->assertContainsOnlyInstancesOf(column::class, $columns);
 
-        // Assert column titles.
-        $columntitles = array_map(static function(column $column): string {
-            return $column->get_title();
-        }, $columns);
-        $expectedcolumntitles = array_merge($initialcolumntitles, [
+        // Column titles.
+        $this->assertEquals([
             'Checkbox field',
             'Date field',
             'Menu field',
             'MSN ID',
             'Text field',
             'Textarea field',
-        ]);
-        $this->assertEquals($expectedcolumntitles, $columntitles);
+        ], array_map(
+            fn(column $column): string => $column->get_title(),
+            $columns,
+        ));
 
-        // Assert column types.
-        $columntypes = array_map(static function(column $column): int {
-            return $column->get_type();
-        }, $columns);
-        $expectedcolumntypes = array_merge($initialcolumntypes, [
+        // Column types.
+        $this->assertEquals([
             column::TYPE_BOOLEAN,
             column::TYPE_TIMESTAMP,
             column::TYPE_TEXT,
             column::TYPE_TEXT,
             column::TYPE_TEXT,
             column::TYPE_LONGTEXT,
-        ]);
-        $this->assertEquals($expectedcolumntypes, $columntypes);
+        ], array_map(
+            fn(column $column): int => $column->get_type(),
+            $columns,
+        ));
+
+        // Column sortable.
+        $this->assertEquals([
+            true,
+            true,
+            true,
+            true,
+            true,
+            false,
+        ], array_map(
+            fn(column $column): bool => $column->get_is_sortable(),
+            $columns,
+        ));
+
+        // Column available.
+        $this->assertEquals([
+            true,
+            true,
+            true,
+            true,
+            true,
+            true,
+        ], array_map(
+            fn(column $column): bool => $column->get_is_available(),
+            $columns,
+        ));
+
+        // Column available, for non-privileged user.
+        $this->setUser(null);
+        $this->assertEquals([
+            true,
+            false,
+            true,
+            true,
+            true,
+            true,
+        ], array_map(
+            fn(column $column): bool => $column->get_is_available(),
+            array_slice($userprofilefields->get_columns(), count($initialcolumns)),
+        ));
     }
 
     /**
@@ -164,38 +198,62 @@ final class user_profile_fields_test extends core_reportbuilder_testcase {
      */
     public function test_get_filters(): void {
         $this->resetAfterTest();
-
-        $userentity = new user();
-        $useralias = $userentity->get_table_alias('user');
+        $this->setAdminUser();
 
         // Get pre-existing user profile fields.
-        $initialuserprofilefields = new user_profile_fields("$useralias.id", $userentity->get_entity_name());
-        $initialfilters = $initialuserprofilefields->get_filters();
-        $initialfilterheaders = array_map(static function(filter $filter): string {
-            return $filter->get_header();
-        }, $initialfilters);
+        $userentity = new user();
+        $initialfilters = (new user_profile_fields(
+            $userentity->get_table_alias('user') . '.id',
+            $userentity->get_entity_name(),
+        ))->get_filters();
 
         // Add new custom profile fields.
         $userprofilefields = $this->generate_userprofilefields();
-        $filters = $userprofilefields->get_filters();
 
-        // Filters count should be equal to start + 6.
-        $this->assertCount(count($initialfilters) + 6, $filters);
+        // Ensure pre-existing fields are ignored in subsequent assertions.
+        $filters = array_slice($userprofilefields->get_filters(), count($initialfilters));
+        $this->assertCount(6, $filters);
         $this->assertContainsOnlyInstancesOf(filter::class, $filters);
 
-        // Assert filter headers.
-        $filterheaders = array_map(static function(filter $filter): string {
-            return $filter->get_header();
-        }, $filters);
-        $expectedfilterheaders = array_merge($initialfilterheaders, [
+        // Filter headers.
+        $this->assertEquals([
             'Checkbox field',
             'Date field',
             'Menu field',
             'MSN ID',
             'Text field',
             'Textarea field',
-        ]);
-        $this->assertEquals($expectedfilterheaders, $filterheaders);
+        ], array_map(
+            fn(filter $filter): string => $filter->get_header(),
+            $filters,
+        ));
+
+        // Filter available.
+        $this->assertEquals([
+            true,
+            true,
+            true,
+            true,
+            true,
+            true,
+        ], array_map(
+            fn(filter $filter): bool => $filter->get_is_available(),
+            $filters,
+        ));
+
+        // Filter available, for non-privileged user.
+        $this->setUser(null);
+        $this->assertEquals([
+            true,
+            false,
+            true,
+            true,
+            true,
+            true,
+        ], array_map(
+            fn(filter $filter): bool => $filter->get_is_available(),
+            array_slice($userprofilefields->get_filters(), count($initialfilters)),
+        ));
     }
 
     /**
@@ -203,6 +261,7 @@ final class user_profile_fields_test extends core_reportbuilder_testcase {
      */
     public function test_custom_report_content(): void {
         $this->resetAfterTest();
+        $this->setAdminUser();
 
         // Create test subject with user profile fields content.
         $this->generate_userprofilefields();
@@ -330,6 +389,7 @@ final class user_profile_fields_test extends core_reportbuilder_testcase {
      */
     public function test_custom_report_filter(string $filtername, array $filtervalues, ?string $expectmatch = null): void {
         $this->resetAfterTest();
+        $this->setAdminUser();
 
         // Create test subject with user profile fields content.
         $this->generate_userprofilefields();

From 26bb378e88d1a093abe96b06b594f176c2aa76fd Mon Sep 17 00:00:00 2001
From: Paul Holden <paulh@moodle.com>
Date: Tue, 15 Oct 2024 20:42:31 +0100
Subject: [PATCH 2/2] MDL-82809 reportbuilder: always apply conditions to
 custom report.

Including those that are otherwise unavailable to the current user,
ensuring that they are always applied when viewing report output.
---
 .upgradenotes/MDL-82809-2024101521112024.yml  |  8 +++++
 reportbuilder/classes/datasource.php          | 13 ++++---
 reportbuilder/classes/local/report/base.php   |  3 +-
 .../classes/table/base_report_table.php       |  2 +-
 reportbuilder/tests/datasource_test.php       | 34 +++++++++++++++++++
 5 files changed, 51 insertions(+), 9 deletions(-)
 create mode 100644 .upgradenotes/MDL-82809-2024101521112024.yml

diff --git a/.upgradenotes/MDL-82809-2024101521112024.yml b/.upgradenotes/MDL-82809-2024101521112024.yml
new file mode 100644
index 00000000000..2feb3d2ff0a
--- /dev/null
+++ b/.upgradenotes/MDL-82809-2024101521112024.yml
@@ -0,0 +1,8 @@
+issueNumber: MDL-82809
+notes:
+  core_reportbuilder:
+    - message: >-
+        The `get_active_conditions` method of the base report class has a new
+        `$checkavailable` parameter to determine whether to check the returned
+        conditions availability
+      type: changed
diff --git a/reportbuilder/classes/datasource.php b/reportbuilder/classes/datasource.php
index f25e4949831..3aec964d30b 100644
--- a/reportbuilder/classes/datasource.php
+++ b/reportbuilder/classes/datasource.php
@@ -194,8 +194,7 @@ abstract class datasource extends base {
                         " {$instance->get_is_deprecated_message()}", DEBUG_DEVELOPER);
                 }
 
-                $this->activefilters['values'][$instance->get_unique_identifier()] =
-                    $instance->set_persistent($filter);
+                $this->activefilters['values'][$instance->get_unique_identifier()] = $instance->set_persistent($filter);
             }
         }
 
@@ -277,9 +276,10 @@ abstract class datasource extends base {
      * Override parent method, returning only those conditions specifically added to the custom report (rather than all that are
      * available)
      *
+     * @param bool $checkavailable
      * @return filter[]
      */
-    public function get_active_conditions(): array {
+    public function get_active_conditions(bool $checkavailable = true): array {
         $reportid = $this->get_report_persistent()->get('id');
 
         // Determine whether we already retrieved the conditions since the report was last modified.
@@ -294,15 +294,14 @@ abstract class datasource extends base {
         foreach ($activeconditions as $condition) {
             $instance = $this->get_condition($condition->get('uniqueidentifier'));
 
-            // Ensure the condition is still present and available.
-            if ($instance !== null && $instance->get_is_available()) {
+            // Ensure the condition is still present and available (if checking available status).
+            if ($instance !== null && (!$checkavailable || $instance->get_is_available())) {
                 if ($instance->get_is_deprecated()) {
                     debugging("The condition '{$instance->get_unique_identifier()}' is deprecated, please do not use it any more." .
                         " {$instance->get_is_deprecated_message()}", DEBUG_DEVELOPER);
                 }
 
-                $this->activeconditions['values'][$instance->get_unique_identifier()] =
-                    $instance->set_persistent($condition);
+                $this->activeconditions['values'][$instance->get_unique_identifier()] = $instance->set_persistent($condition);
             }
         }
 
diff --git a/reportbuilder/classes/local/report/base.php b/reportbuilder/classes/local/report/base.php
index 4b9c9bf4aa2..0a335f78bfe 100644
--- a/reportbuilder/classes/local/report/base.php
+++ b/reportbuilder/classes/local/report/base.php
@@ -553,9 +553,10 @@ abstract class base {
     /**
      * Return all active report conditions (by default, all available conditions)
      *
+     * @param bool $checkavailable
      * @return filter[]
      */
-    public function get_active_conditions(): array {
+    public function get_active_conditions(bool $checkavailable = true): array {
         $conditions = $this->get_conditions();
         foreach ($conditions as $condition) {
             if ($condition->get_is_deprecated()) {
diff --git a/reportbuilder/classes/table/base_report_table.php b/reportbuilder/classes/table/base_report_table.php
index d33f8167bf1..3ca83e63b27 100644
--- a/reportbuilder/classes/table/base_report_table.php
+++ b/reportbuilder/classes/table/base_report_table.php
@@ -79,7 +79,7 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl
 
         // For each condition, we need to ensure their values are always accounted for in the report.
         $conditionvalues = $this->report->get_condition_values();
-        foreach ($this->report->get_active_conditions() as $condition) {
+        foreach ($this->report->get_active_conditions(false) as $condition) {
             [$conditionsql, $conditionparams] = $this->get_filter_sql($condition, $conditionvalues, 'c' . $conditionindex++);
             if ($conditionsql !== '') {
                 $joins = array_merge($joins, $condition->get_joins());
diff --git a/reportbuilder/tests/datasource_test.php b/reportbuilder/tests/datasource_test.php
index 58349409f24..f7c34cdcc24 100644
--- a/reportbuilder/tests/datasource_test.php
+++ b/reportbuilder/tests/datasource_test.php
@@ -288,6 +288,40 @@ final class datasource_test extends advanced_testcase {
         $this->assertCount($expectedcountconditions, $instance->get_conditions());
     }
 
+    /**
+     * Test getting active conditions
+     *
+     * @covers ::get_active_conditions
+     */
+    public function test_get_active_conditions(): void {
+        $instance = $this->get_datasource_test_source();
+
+        $method = (new ReflectionClass($instance))->getMethod('add_conditions_from_entity');
+        $method->invoke($instance, 'datasource_test_entity');
+
+        /** @var core_reportbuilder_generator $generator */
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder');
+
+        $reportid = $instance->get_report_persistent()->get('id');
+        $generator->create_condition(['reportid' => $reportid, 'uniqueidentifier' => 'datasource_test_entity:first']);
+        $generator->create_condition(['reportid' => $reportid, 'uniqueidentifier' => 'datasource_test_entity:second']);
+
+        // Set the second condition as unavailable.
+        $instance->get_condition('datasource_test_entity:second')->set_is_available(false);
+
+        $this->assertEquals([
+            'datasource_test_entity:first',
+        ], array_keys($instance->get_active_conditions(true)));
+
+        // Ensure report elements are reloaded.
+        $instance::report_elements_modified($reportid);
+
+        $this->assertEquals([
+            'datasource_test_entity:first',
+            'datasource_test_entity:second',
+        ], array_keys($instance->get_active_conditions(false)));
+    }
+
     /**
      * Create and return our test datasource instance
      *