L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-79574 reportbuilder: consistent application of column indexes.

Browse Source 
No longer reliant on persistent record indexing (whose behaviour
has now changed), we are now explicit in setting each column index
within reports.
This commit is contained in:
Paul Holden
2026-02-27 10:30:09 +00:00
parent 1b92a95ab7
commit b2416de7c8
9 changed files with 54 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
public/reportbuilder/classes/datasource.php
+4 -3
View File
@@ -122,8 +122,9 @@ abstract class datasource extends base {
$this->activecolumns = ['builttime' => microtime(true), 'values' => []];
$activecolumns = column_model::get_records(['reportid' => $reportid], 'columnorder');
foreach ($activecolumns as $index => $column) {
$columnindex = 0;
$columns = column_model::get_records(['reportid' => $reportid], 'columnorder');
foreach ($columns as $column) {
$instance = $this->get_column($column->get('uniqueidentifier'));
// Ensure the column is still present and available.
@@ -137,7 +138,7 @@ abstract class datasource extends base {
// We should clone the report column to ensure if it's added twice to a report, each operates independently.
$this->activecolumns['values'][] = clone $instance
->set_index($index)
->set_index($columnindex++)
->set_persistent($column)
->set_aggregation($columnaggregation, $instance->get_aggregation_options($columnaggregation));
}
public/reportbuilder/classes/local/report/base.php
+2
View File
@@ -455,8 +455,10 @@ abstract class base {
* @return column[]
*/
public function get_active_columns(): array {
$columnindex = 0;
$columns = $this->get_columns();
foreach ($columns as $column) {
$column->set_index($columnindex++);
if ($column->get_is_deprecated()) {
debugging("The column '{$column->get_unique_identifier()}' is deprecated, please do not use it any more." .
" {$column->get_is_deprecated_message()}", DEBUG_DEVELOPER);
public/reportbuilder/classes/local/report/column.php
+1 -1
View File
@@ -55,7 +55,7 @@ final class column {
public const TYPE_LONGTEXT = 6;
/** @var int $index Column index within a report */
private $index;
private int $index = 0;
/** @var bool $hascustomcolumntitle Used to store if the column has been given a custom title */
private $hascustomcolumntitle = false;
public/reportbuilder/classes/table/system_report_table.php
+1 -4
View File
@@ -117,10 +117,7 @@ class system_report_table extends base_report_table {
$this->no_sorting('selectall');
}
$columnindex = 1;
foreach ($columns as $identifier => $column) {
$column->set_index($columnindex++);
foreach ($columns as $column) {
$columnheaders[$column->get_column_alias()] = $column->get_title();
// Specify whether column should behave as a user fullname column unless the column has a custom title set.
public/reportbuilder/tests/external/columns/reorder_test.php
Vendored
+5 -6
View File
@@ -61,18 +61,17 @@ final class reorder_test extends \core_external\tests\externallib_testcase {
$this->assertTrue($result);
// Assert report columns order.
$columns = column::get_records(['reportid' => $report->get('id')], 'columnorder');
$columnidentifiers = array_map(static function(column $column): string {
return $column->get('uniqueidentifier');
}, $columns);
$columnidentifiers = array_map(
fn(column $column): string => $column->get('uniqueidentifier'),
column::get_records(['reportid' => $report->get('id')], 'columnorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $columnidentifiers);
], array_values($columnidentifiers));
}
/**
public/reportbuilder/tests/external/columns/sort/reorder_test.php
Vendored
+5 -6
View File
@@ -65,18 +65,17 @@ final class reorder_test extends \core_external\tests\externallib_testcase {
$this->assertArrayHasKey('sorticon', $sortablecolumn);
// Assert report column sort order.
$columns = column::get_records(['reportid' => $report->get('id')], 'sortorder');
$columnidentifiers = array_map(static function(column $column): string {
return $column->get('uniqueidentifier');
}, $columns);
$columnidentifiers = array_map(
fn(column $column): string => $column->get('uniqueidentifier'),
column::get_records(['reportid' => $report->get('id')], 'sortorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $columnidentifiers);
], array_values($columnidentifiers));
}
/**
public/reportbuilder/tests/external/conditions/reorder_test.php
Vendored
+5 -6
View File
@@ -62,18 +62,17 @@ final class reorder_test extends \core_external\tests\externallib_testcase {
$this->assertNotEmpty($result['activeconditionsform']);
// Assert report conditions order.
$conditions = filter::get_condition_records($report->get('id'), 'filterorder');
$conditionidentifiers = array_map(static function(filter $condition): string {
return $condition->get('uniqueidentifier');
}, $conditions);
$conditionidentifiers = array_map(
fn(filter $condition): string => $condition->get('uniqueidentifier'),
filter::get_condition_records($report->get('id'), 'filterorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $conditionidentifiers);
], array_values($conditionidentifiers));
}
/**
public/reportbuilder/tests/external/filters/reorder_test.php
Vendored
+5 -6
View File
@@ -65,18 +65,17 @@ final class reorder_test extends \core_external\tests\externallib_testcase {
$this->assertCount(4, $result['activefilters']);
// Assert report filters order.
$filters = filter::get_filter_records($report->get('id'), 'filterorder');
$filteridentifiers = array_map(static function(filter $filter): string {
return $filter->get('uniqueidentifier');
}, $filters);
$filteridentifiers = array_map(
fn(filter $filter): string => $filter->get('uniqueidentifier'),
filter::get_filter_records($report->get('id'), 'filterorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $filteridentifiers);
], array_values($filteridentifiers));
}
/**
public/reportbuilder/tests/local/helpers/report_test.php
+26 -30
View File
@@ -141,34 +141,34 @@ final class report_test extends advanced_testcase {
// Assert new report columns.
$newcolumns = column::get_records(['reportid' => $newreport->get('id')]);
$this->assertCount(1, $newcolumns);
[$newcolumn] = $newcolumns;
$newcolumn = reset($newcolumns);
$this->assertNotEquals($column->get('id'), $newcolumn->get('id'));
$this->assertEquals('user:lastname', $newcolumn->get('uniqueidentifier'));
// Assert new report conditions.
$newconditions = filter::get_condition_records($newreport->get('id'));
$this->assertCount(1, $newconditions);
[$newcondition] = $newconditions;
$newcondition = reset($newconditions);
$this->assertNotEquals($condition->get('id'), $newcondition->get('id'));
$this->assertEquals('user:firstname', $newcondition->get('uniqueidentifier'));
// Assert new report filters.
$newfilters = filter::get_filter_records($newreport->get('id'));
$this->assertCount(1, $newfilters);
[$newfilter] = $newfilters;
$newfilter = reset($newfilters);
$this->assertNotEquals($filter->get('id'), $newfilter->get('id'));
$this->assertEquals('user:email', $newfilter->get('uniqueidentifier'));
// Assert new report audiences.
$newaudiences = audience::get_records(['reportid' => $newreport->get('id')]);
$this->assertCount(1, $newaudiences);
[$newaudience] = $newaudiences;
$newaudience = reset($newaudiences);
$this->assertNotEquals($audience->get_persistent()->get('id'), $newaudience->get('id'));
// Assert new report schedules.
$newschedules = schedule::get_records(['reportid' => $newreport->get('id')]);
$this->assertCount(1, $newschedules);
[$newschedule] = $newschedules;
$newschedule = reset($newschedules);
$this->assertNotEquals($schedule->get('id'), $newschedule->get('id'));
$this->assertEquals([
$newaudience->get('id'),
@@ -369,18 +369,17 @@ final class report_test extends advanced_testcase {
$this->assertTrue($result);
// Assert report columns order.
$columns = column::get_records(['reportid' => $report->get('id')], 'columnorder');
$columnidentifiers = array_map(static function(column $column): string {
return $column->get('uniqueidentifier');
}, $columns);
$columnidentifiers = array_map(
fn(column $column): string => $column->get('uniqueidentifier'),
column::get_records(['reportid' => $report->get('id')], 'columnorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $columnidentifiers);
], array_values($columnidentifiers));
}
/**
@@ -425,19 +424,18 @@ final class report_test extends advanced_testcase {
$result = report::reorder_report_column_sorting($report->get('id'), $columncity->get('id'), 2);
$this->assertTrue($result);
// Assert report columns order.
$columns = column::get_records(['reportid' => $report->get('id')], 'sortorder');
$columnidentifiers = array_map(static function(column $column): string {
return $column->get('uniqueidentifier');
}, $columns);
// Assert report column sort order.
$columnidentifiers = array_map(
fn(column $column): string => $column->get('uniqueidentifier'),
column::get_records(['reportid' => $report->get('id')], 'sortorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $columnidentifiers);
], array_values($columnidentifiers));
}
/**
@@ -633,18 +631,17 @@ final class report_test extends advanced_testcase {
$this->assertTrue($result);
// Assert report conditions order.
$conditions = filter::get_condition_records($report->get('id'), 'filterorder');
$conditionidentifiers = array_map(static function(filter $condition): string {
return $condition->get('uniqueidentifier');
}, $conditions);
$conditionidentifiers = array_map(
fn(filter $condition): string => $condition->get('uniqueidentifier'),
filter::get_condition_records($report->get('id'), 'filterorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $conditionidentifiers);
], array_values($conditionidentifiers));
}
/**
@@ -799,18 +796,17 @@ final class report_test extends advanced_testcase {
$this->assertTrue($result);
// Assert report filters order.
$filters = filter::get_filter_records($report->get('id'), 'filterorder');
$filteridentifiers = array_map(static function(filter $filter): string {
return $filter->get('uniqueidentifier');
}, $filters);
$filteridentifiers = array_map(
fn(filter $filter): string => $filter->get('uniqueidentifier'),
filter::get_filter_records($report->get('id'), 'filterorder'),
);
$this->assertEquals([
'user:fullname',
'user:city',
'user:email',
'user:country',
], $filteridentifiers);
], array_values($filteridentifiers));
}
/**