From a3c386af933a00b9126b7be612b42dde4ec87cb7 Mon Sep 17 00:00:00 2001
From: Paul Holden <paulh@moodle.com>
Date: Fri, 12 Jul 2024 17:17:57 +0100
Subject: [PATCH] MDL-82463 reportbuilder: change non-numeric entity column
 types.

Where columns were previously of type `TYPE_INTEGER` or `TYPE_FLOAT`
but did not provide numeric data on output, we should change their
type to `TYPE_TEXT` (i.e. the default) to ensure that future work on
numeric aggregation doesn't affect them.
---
 .../reportbuilder/local/entities/task_log.php        | 12 +++---------
 .../reportbuilder/local/entities/role_assignment.php |  2 --
 .../classes/reportbuilder/local/entities/badge.php   | 10 ++++------
 .../classes/reportbuilder/local/entities/comment.php |  4 +---
 files/classes/reportbuilder/local/entities/file.php  |  4 +---
 group/classes/reportbuilder/local/entities/group.php | 12 +++---------
 lib/classes/reportbuilder/local/entities/context.php |  7 ++-----
 reportbuilder/classes/local/entities/user.php        |  5 +----
 reportbuilder/classes/local/report/column.php        |  7 ++++---
 .../reportbuilder/local/entities/instance.php        |  4 +---
 10 files changed, 20 insertions(+), 47 deletions(-)

diff --git a/admin/classes/reportbuilder/local/entities/task_log.php b/admin/classes/reportbuilder/local/entities/task_log.php
index 8cc6cdf2bfc..dc541c958b1 100644
--- a/admin/classes/reportbuilder/local/entities/task_log.php
+++ b/admin/classes/reportbuilder/local/entities/task_log.php
@@ -211,11 +211,8 @@ class task_log extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_field("{$tablealias}.pid")
-            ->set_is_sortable(true)
-            // Although this is an integer column, it doesn't make sense to perform numeric aggregation on it.
-            ->set_disabled_aggregation(['avg', 'count', 'countdistinct', 'max', 'min', 'sum']);
+            ->set_is_sortable(true);
 
         // Database column.
         $columns[] = (new column(
@@ -224,17 +221,14 @@ class task_log extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$tablealias}.dbreads, {$tablealias}.dbwrites")
             ->set_is_sortable(true, ["{$tablealias}.dbreads", "{$tablealias}.dbwrites"])
-            ->add_callback(static function(int $value, stdClass $row): string {
+            ->add_callback(static function($value, stdClass $row): string {
                 $output = '';
                 $output .= \html_writer::div(get_string('task_stats:dbreads', 'admin', $row->dbreads));
                 $output .= \html_writer::div(get_string('task_stats:dbwrites', 'admin', $row->dbwrites));
                 return $output;
-            })
-            // Although this is an integer column, it doesn't make sense to perform numeric aggregation on it.
-            ->set_disabled_aggregation(['avg', 'count', 'countdistinct', 'max', 'min', 'sum']);
+            });
 
         // Database reads column.
         $columns[] = (new column(
diff --git a/admin/roles/classes/reportbuilder/local/entities/role_assignment.php b/admin/roles/classes/reportbuilder/local/entities/role_assignment.php
index fda44a9c73a..55eda7c9f61 100644
--- a/admin/roles/classes/reportbuilder/local/entities/role_assignment.php
+++ b/admin/roles/classes/reportbuilder/local/entities/role_assignment.php
@@ -102,7 +102,6 @@ class role_assignment extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_TEXT)
             ->add_field("{$raalias}.component")
             ->set_is_sortable(true);
 
@@ -113,7 +112,6 @@ class role_assignment extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_field("{$raalias}.itemid")
             ->set_is_sortable(true);
 
diff --git a/badges/classes/reportbuilder/local/entities/badge.php b/badges/classes/reportbuilder/local/entities/badge.php
index 46d31a1da43..526c441382c 100644
--- a/badges/classes/reportbuilder/local/entities/badge.php
+++ b/badges/classes/reportbuilder/local/entities/badge.php
@@ -175,13 +175,11 @@ class badge extends base {
             ->add_join("LEFT JOIN {context} {$contextalias}
                     ON {$contextalias}.contextlevel = " . CONTEXT_COURSE . "
                    AND {$contextalias}.instanceid = {$badgealias}.courseid")
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$badgealias}.id, {$badgealias}.type, {$badgealias}.courseid")
             ->add_field($DB->sql_cast_to_char("{$badgealias}.imagecaption"), 'imagecaption')
             ->add_fields(context_helper::get_preload_record_columns_sql($contextalias))
-            ->set_disabled_aggregation_all()
-            ->add_callback(static function(?int $badgeid, stdClass $badge): string {
-                if (!$badgeid) {
+            ->add_callback(static function($value, stdClass $badge): string {
+                if ($badge->id === null) {
                     return '';
                 }
                 if ($badge->type == BADGE_TYPE_SITE) {
@@ -191,7 +189,7 @@ class badge extends base {
                     $context = context_course::instance($badge->courseid);
                 }
 
-                $badgeimage = moodle_url::make_pluginfile_url($context->id, 'badges', 'badgeimage', $badgeid, '/', 'f2');
+                $badgeimage = moodle_url::make_pluginfile_url($context->id, 'badges', 'badgeimage', $badge->id, '/', 'f2');
                 return html_writer::img($badgeimage, $badge->imagecaption);
             });
 
@@ -207,7 +205,7 @@ class badge extends base {
             ->set_is_sortable(true)
             ->add_callback(static function($language): string {
                 $languages = get_string_manager()->get_list_of_languages();
-                return $languages[$language] ?? $language ?? '';
+                return (string) ($languages[$language] ?? $language);
             });
 
         // Version.
diff --git a/comment/classes/reportbuilder/local/entities/comment.php b/comment/classes/reportbuilder/local/entities/comment.php
index 659035e8b8d..cd34cb4c8dc 100644
--- a/comment/classes/reportbuilder/local/entities/comment.php
+++ b/comment/classes/reportbuilder/local/entities/comment.php
@@ -193,10 +193,8 @@ class comment extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$commentalias}.itemid")
-            ->set_is_sortable(true)
-            ->set_disabled_aggregation_all();
+            ->set_is_sortable(true);
 
         // Time created.
         $columns[] = (new column(
diff --git a/files/classes/reportbuilder/local/entities/file.php b/files/classes/reportbuilder/local/entities/file.php
index ea6fd5a5eb7..f378f784c12 100644
--- a/files/classes/reportbuilder/local/entities/file.php
+++ b/files/classes/reportbuilder/local/entities/file.php
@@ -306,10 +306,8 @@ class file extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$filesalias}.itemid")
-            ->set_is_sortable(true)
-            ->set_disabled_aggregation_all();
+            ->set_is_sortable(true);
 
         // Time created.
         $columns[] = (new column(
diff --git a/group/classes/reportbuilder/local/entities/group.php b/group/classes/reportbuilder/local/entities/group.php
index ad0097ca1c5..5348f4e672b 100644
--- a/group/classes/reportbuilder/local/entities/group.php
+++ b/group/classes/reportbuilder/local/entities/group.php
@@ -192,12 +192,9 @@ class group extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$groupsalias}.visibility")
             ->set_is_sortable(true)
-            // It doesn't make sense to offer integer aggregation methods for this column.
-            ->set_disabled_aggregation(['avg', 'max', 'min', 'sum'])
-            ->set_callback(static function(?int $visibility): string {
+            ->set_callback(static function(?string $visibility): string {
                 if ($visibility === null) {
                     return '';
                 }
@@ -209,7 +206,7 @@ class group extends base {
                     GROUPS_VISIBILITY_NONE => new lang_string('visibilitynone', 'core_group'),
                 ];
 
-                return (string) ($options[$visibility] ?? $visibility);
+                return (string) ($options[(int) $visibility] ?? $visibility);
             });
 
         // Participation column.
@@ -231,12 +228,9 @@ class group extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$groupsalias}.picture, {$groupsalias}.id, {$contextalias}.id AS contextid")
             ->set_is_sortable(false)
-            // It doesn't make sense to offer integer aggregation methods for this column.
-            ->set_disabled_aggregation(['avg', 'max', 'min', 'sum'])
-            ->set_callback(static function ($picture, stdClass $group): string {
+            ->set_callback(static function($value, stdClass $group): string {
                 if (empty($group->picture)) {
                     return '';
                 }
diff --git a/lib/classes/reportbuilder/local/entities/context.php b/lib/classes/reportbuilder/local/entities/context.php
index 9acf790ee68..d18c184707b 100644
--- a/lib/classes/reportbuilder/local/entities/context.php
+++ b/lib/classes/reportbuilder/local/entities/context.php
@@ -136,17 +136,14 @@ class context extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$contextalias}.contextlevel")
             ->set_is_sortable(true)
-            // It doesn't make sense to offer integer aggregation methods for this column.
-            ->set_disabled_aggregation(['avg', 'max', 'min', 'sum'])
-            ->add_callback(static function(?int $level): string {
+            ->add_callback(static function(?string $level): string {
                 if ($level === null) {
                     return '';
                 }
 
-                return context_helper::get_level_name($level);
+                return context_helper::get_level_name((int) $level);
             });
 
         // Path.
diff --git a/reportbuilder/classes/local/entities/user.php b/reportbuilder/classes/local/entities/user.php
index 4e1c112992e..1bde11b0d9d 100644
--- a/reportbuilder/classes/local/entities/user.php
+++ b/reportbuilder/classes/local/entities/user.php
@@ -282,11 +282,8 @@ class user extends base {
         ))
             ->add_joins($this->get_joins())
             ->add_fields($userpictureselect)
-            ->set_type(column::TYPE_INTEGER)
             ->set_is_sortable($this->is_sortable('picture'))
-            // It doesn't make sense to offer integer aggregation methods for this column.
-            ->set_disabled_aggregation(['avg', 'max', 'min', 'sum'])
-            ->add_callback(static function ($value, stdClass $row): string {
+            ->add_callback(static function($value, stdClass $row): string {
                 global $OUTPUT;
 
                 return !empty($row->id) ? $OUTPUT->user_picture($row, ['link' => false, 'alttext' => false]) : '';
diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php
index fa7e4e84f8e..54501bc91f5 100644
--- a/reportbuilder/classes/local/report/column.php
+++ b/reportbuilder/classes/local/report/column.php
@@ -213,7 +213,8 @@ final class column {
      * Set the column type, if not called then the type will be assumed to be {@see TYPE_TEXT}
      *
      * The type of a column is used to cast the first column field passed to any callbacks {@see add_callback} as well as the
-     * aggregation options available for the column
+     * aggregation options available for the column. It should represent how the column content is returned from callbacks
+     *
      *
      * @param int $type
      * @return self
@@ -643,11 +644,11 @@ final class column {
      */
     public function format_value(array $row) {
         $values = $this->get_values($row);
-        $value = self::get_default_value($values, $this->type);
+        $value = self::get_default_value($values, $this->get_type());
 
         // If column is being aggregated then defer formatting to them, otherwise loop through all column callbacks.
         if (!empty($this->aggregation)) {
-            $value = $this->aggregation::format_value($value, $values, $this->callbacks, $this->type);
+            $value = $this->aggregation::format_value($value, $values, $this->callbacks, $this->get_type());
         } else {
             foreach ($this->callbacks as $callback) {
                 [$callable, $arguments] = $callback;
diff --git a/tag/classes/reportbuilder/local/entities/instance.php b/tag/classes/reportbuilder/local/entities/instance.php
index 46cceeaa8be..6e772bd4430 100644
--- a/tag/classes/reportbuilder/local/entities/instance.php
+++ b/tag/classes/reportbuilder/local/entities/instance.php
@@ -184,10 +184,8 @@ class instance extends base {
             $this->get_entity_name()
         ))
             ->add_joins($this->get_joins())
-            ->set_type(column::TYPE_INTEGER)
             ->add_fields("{$instancealias}.itemid")
-            ->set_is_sortable(true)
-            ->set_disabled_aggregation_all();
+            ->set_is_sortable(true);
 
         // Time created.
         $columns[] = (new column(
-- 
2.43.0