From bfb5197738f5df58cf01c6a2eaaddf1b61264d5a Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Mon, 16 Jan 2023 18:15:41 +0000 Subject: [PATCH] MDL-76901 reportbuilder: cache custom report element persistents. During request lifecycle ensure that column, filter and condition persistents are only loaded a single time to prevent additional database reads. Provide invalidation method, used by the report helpers, to ensure that the persistents are re-loaded appropriately. Co-authored-by: Marina Glancy --- reportbuilder/classes/datasource.php | 80 +++++++++++++++---- ...custom_report_columns_sorting_exporter.php | 23 +++--- .../custom_report_conditions_exporter.php | 15 ++-- .../custom_report_filters_exporter.php | 21 +++-- reportbuilder/classes/form/card_view.php | 19 ++--- reportbuilder/classes/form/condition.php | 14 +--- reportbuilder/classes/local/models/column.php | 30 +++++++ reportbuilder/classes/local/models/filter.php | 30 +++++++ 8 files changed, 164 insertions(+), 68 deletions(-) diff --git a/reportbuilder/classes/datasource.php b/reportbuilder/classes/datasource.php index d726f9985b1..a0250e22642 100644 --- a/reportbuilder/classes/datasource.php +++ b/reportbuilder/classes/datasource.php @@ -35,6 +35,18 @@ use core_reportbuilder\local\report\filter; */ abstract class datasource extends base { + /** @var float[] $elementsmodified Track the time elements of specific reports have been added, updated, removed */ + private static $elementsmodified = []; + + /** @var array $activecolumns */ + private $activecolumns; + + /** @var array $activefilters */ + private $activefilters; + + /** @var array $activeconditions */ + private $activeconditions; + /** * Return user friendly name of the datasource * @@ -129,22 +141,32 @@ abstract class datasource extends base { * @return column[] */ public function get_active_columns(): array { - $columns = []; + $reportid = $this->get_report_persistent()->get('id'); - $activecolumns = column_model::get_records(['reportid' => $this->get_report_persistent()->get('id')], 'columnorder'); + // Determine whether we already retrieved the columns since the report was last modified. + self::$elementsmodified += [$reportid => -1]; + if ($this->activecolumns !== null && $this->activecolumns['builttime'] > self::$elementsmodified[$reportid]) { + return $this->activecolumns['values']; + } + + $this->activecolumns = ['builttime' => microtime(true), 'values' => []]; + + $activecolumns = column_model::get_records(['reportid' => $reportid], 'columnorder'); foreach ($activecolumns as $index => $column) { $instance = $this->get_column($column->get('uniqueidentifier')); + + // Ensure the column is still present and available. if ($instance !== null && $instance->get_is_available()) { - $instance->set_persistent($column); // We should clone the report column to ensure if it's added twice to a report, each operates independently. - $columns[] = clone $instance + $this->activecolumns['values'][] = clone $instance ->set_index($index) + ->set_persistent($column) ->set_aggregation($column->get('aggregation')); } } - return $columns; + return $this->activecolumns['values']; } /** @@ -207,18 +229,28 @@ abstract class datasource extends base { * @return filter[] */ public function get_active_filters(): array { - $filters = []; + $reportid = $this->get_report_persistent()->get('id'); - $activefilters = filter_model::get_filter_records($this->get_report_persistent()->get('id'), 'filterorder'); + // Determine whether we already retrieved the filters since the report was last modified. + self::$elementsmodified += [$reportid => -1]; + if ($this->activefilters !== null && $this->activefilters['builttime'] > self::$elementsmodified[$reportid]) { + return $this->activefilters['values']; + } + + $this->activefilters = ['builttime' => microtime(true), 'values' => []]; + + $activefilters = filter_model::get_filter_records($reportid, 'filterorder'); foreach ($activefilters as $filter) { $instance = $this->get_filter($filter->get('uniqueidentifier')); + + // Ensure the filter is still present and available. if ($instance !== null && $instance->get_is_available()) { - $filters[$instance->get_unique_identifier()] = $instance - ->set_persistent($filter); + $this->activefilters['values'][$instance->get_unique_identifier()] = + $instance->set_persistent($filter); } } - return $filters; + return $this->activefilters['values']; } /** @@ -296,17 +328,28 @@ abstract class datasource extends base { * @return filter[] */ public function get_active_conditions(): array { - $conditions = []; + $reportid = $this->get_report_persistent()->get('id'); - $activeconditions = filter_model::get_condition_records($this->get_report_persistent()->get('id'), 'filterorder'); + // Determine whether we already retrieved the conditions since the report was last modified. + self::$elementsmodified += [$reportid => -1]; + if ($this->activeconditions !== null && $this->activeconditions['builttime'] > self::$elementsmodified[$reportid]) { + return $this->activeconditions['values']; + } + + $this->activeconditions = ['builttime' => microtime(true), 'values' => []]; + + $activeconditions = filter_model::get_condition_records($reportid, 'filterorder'); 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()) { - $conditions[$instance->get_unique_identifier()] = $instance->set_persistent($condition); + $this->activeconditions['values'][$instance->get_unique_identifier()] = + $instance->set_persistent($condition); } } - return $conditions; + return $this->activeconditions['values']; } /** @@ -328,4 +371,13 @@ abstract class datasource extends base { $this->add_all_from_entity($entity->get_entity_name()); } } + + /** + * Indicate that report elements have been modified, e.g. columns/filters/conditions have been added, removed or updated + * + * @param int $reportid + */ + final public static function report_elements_modified(int $reportid): void { + self::$elementsmodified[$reportid] = microtime(true); + } } diff --git a/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php b/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php index 62619409a83..2ba1a8a93b6 100644 --- a/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php +++ b/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php @@ -18,11 +18,12 @@ declare(strict_types=1); namespace core_reportbuilder\external; +use core_collator; use pix_icon; use renderer_base; use core\external\exporter; use core_reportbuilder\datasource; -use core_reportbuilder\local\models\column; +use core_reportbuilder\local\report\column; /** * Custom report columns sorting exporter class @@ -90,19 +91,15 @@ class custom_report_columns_sorting_exporter extends exporter { /** @var datasource $report */ $report = $this->related['report']; - $reportid = $report->get_report_persistent()->get('id'); - $activecolumns = column::get_records(['reportid' => $reportid], 'sortorder'); - $sortablecolumns = array_filter($activecolumns, function(column $persistent) use($report): bool { - $column = $report->get_column($persistent->get('uniqueidentifier')); - if ($column === null) { - return false; - } - - return $column->set_aggregation($persistent->get('aggregation'))->get_is_sortable(); + // Filter/retrieve all "sortable" active columns. + $sortablecolumns = array_filter($report->get_active_columns(), function(column $column): bool { + return $column->get_is_sortable(); }); - $sortablecolumns = array_map(function(column $persistent) use ($report) { - $columntitle = $report->get_column($persistent->get('uniqueidentifier'))->get_title(); + $sortablecolumns = array_map(function(column $column) use ($report): array { + $persistent = $column->get_persistent(); + + $columntitle = $column->get_title(); $columnheading = $persistent->get_formatted_heading($report->get_context()); $columnsortascending = ($persistent->get('sortdirection') == SORT_ASC); @@ -125,6 +122,8 @@ class custom_report_columns_sorting_exporter extends exporter { ]; }, $sortablecolumns); + core_collator::asort_array_of_arrays_by_key($sortablecolumns, 'sortorder', core_collator::SORT_NUMERIC); + return [ 'hassortablecolumns' => !empty($sortablecolumns), 'sortablecolumns' => array_values($sortablecolumns), diff --git a/reportbuilder/classes/external/custom_report_conditions_exporter.php b/reportbuilder/classes/external/custom_report_conditions_exporter.php index ecce5acf5ef..65be27db618 100644 --- a/reportbuilder/classes/external/custom_report_conditions_exporter.php +++ b/reportbuilder/classes/external/custom_report_conditions_exporter.php @@ -22,7 +22,7 @@ use renderer_base; use core\external\exporter; use core_reportbuilder\datasource; use core_reportbuilder\form\condition; -use core_reportbuilder\local\models\filter; +use core_reportbuilder\local\report\filter; /** * Custom report conditions exporter class @@ -96,13 +96,12 @@ class custom_report_conditions_exporter extends exporter { protected function get_other_values(renderer_base $output): array { /** @var datasource $report */ $report = $this->related['report']; - $reportid = $report->get_report_persistent()->get('id'); // Current condition instances contained in the report. - $conditioninstances = filter::get_condition_records($reportid, 'filterorder'); + $conditions = $report->get_active_conditions(); $conditionidentifiers = array_map(static function(filter $condition): string { - return $condition->get('uniqueidentifier'); - }, $conditioninstances); + return $condition->get_unique_identifier(); + }, $conditions); $availableconditions = []; @@ -128,11 +127,11 @@ class custom_report_conditions_exporter extends exporter { ]; } - // Generate filters form if report contains any filters. - $conditionspresent = !empty($conditioninstances); + // Generate conditions form if any present. + $conditionspresent = !empty($conditions); if ($conditionspresent) { $conditionsform = new condition(null, null, 'post', '', [], true, [ - 'reportid' => $reportid, + 'reportid' => $report->get_report_persistent()->get('id'), ]); $conditionsform->set_data_for_dynamic_submission(); } diff --git a/reportbuilder/classes/external/custom_report_filters_exporter.php b/reportbuilder/classes/external/custom_report_filters_exporter.php index 7d0e388f954..1d5c6358d0a 100644 --- a/reportbuilder/classes/external/custom_report_filters_exporter.php +++ b/reportbuilder/classes/external/custom_report_filters_exporter.php @@ -21,7 +21,7 @@ namespace core_reportbuilder\external; use renderer_base; use core\external\exporter; use core_reportbuilder\datasource; -use core_reportbuilder\local\models\filter; +use core_reportbuilder\local\report\filter; use core_reportbuilder\output\filter_heading_editable; /** @@ -101,10 +101,10 @@ class custom_report_filters_exporter extends exporter { /** @var datasource $report */ $report = $this->related['report']; - // Current filters added to the report. - $filters = filter::get_filter_records($report->get_report_persistent()->get('id'), 'filterorder'); + // Current filter instances contained in the report. + $filters = $report->get_active_filters(); $filteridentifiers = array_map(static function(filter $filter): string { - return $filter->get('uniqueidentifier'); + return $filter->get_unique_identifier(); }, $filters); $availablefilters = $activefilters = []; @@ -133,22 +133,19 @@ class custom_report_filters_exporter extends exporter { // Populate active filters. $filterinstances = $report->get_filter_instances(); - foreach ($filters as $filter) { - $filterinstance = $filterinstances[$filter->get('uniqueidentifier')] ?? null; - if ($filterinstance === null) { - continue; - } + foreach ($filterinstances as $filterinstance) { + $persistent = $filterinstance->get_filter_persistent(); $entityname = $filterinstance->get_entity_name(); $displayvalue = $filterinstance->get_header(); - $editable = new filter_heading_editable(0, $filter); + $editable = new filter_heading_editable(0, $persistent); $activefilters[] = [ - 'id' => $filter->get('id'), + 'id' => $persistent->get('id'), 'entityname' => $report->get_entity_title($entityname)->out(), 'heading' => $displayvalue, 'headingeditable' => $editable->render($output), - 'sortorder' => $filter->get('filterorder'), + 'sortorder' => $persistent->get('filterorder'), 'movetitle' => get_string('movefilter', 'core_reportbuilder', $displayvalue), ]; } diff --git a/reportbuilder/classes/form/card_view.php b/reportbuilder/classes/form/card_view.php index 4c5183947cb..789696a4fe3 100644 --- a/reportbuilder/classes/form/card_view.php +++ b/reportbuilder/classes/form/card_view.php @@ -25,7 +25,6 @@ use core_reportbuilder\manager; use core_reportbuilder\permission; use core_reportbuilder\local\report\base; use core_reportbuilder\local\models\report; -use core_reportbuilder\local\models\column; /** * Card view dynamic form @@ -85,11 +84,11 @@ class card_view extends dynamic_form { */ public function set_data_for_dynamic_submission(): void { $report = $this->get_report(); - $totalcolumns = column::count_records(['reportid' => $report->get_report_persistent()->get('id')]); $settings = $report->get_settings_values(); + $defaults = [ // Maximum value for 'cardview_visiblecolumns' should be the report total number of columns. - 'visiblecolumns' => min($settings['cardview_visiblecolumns'] ?? 1, $totalcolumns), + 'visiblecolumns' => min($settings['cardview_visiblecolumns'] ?? 1, count($report->get_active_columns())), 'showfirsttitle' => $settings['cardview_showfirsttitle'] ?? 0, ]; $this->set_data(array_merge($defaults, $this->_ajaxformdata)); @@ -108,21 +107,17 @@ class card_view extends dynamic_form { * Card view form definition */ public function definition(): void { - $mform = $this->_form; + $report = $this->get_report(); - $reportid = $this->optional_param('reportid', 0, PARAM_INT); - $totalcolumns = column::count_records(['reportid' => $reportid]); - $visibilityarray = []; - // Generate select options from 1 to report total number of columns. - for ($i = 1; $i <= max($totalcolumns, 1); $i++) { - $visibilityarray[$i] = $i; - } + $mform = $this->_form; $mform->addElement('hidden', 'reportid'); $mform->setType('reportid', PARAM_INT); + // Generate select options from 1 to report total number of columns. + $visiblecolumns = range(1, max(count($report->get_active_columns()), 1)); $mform->addElement('select', 'visiblecolumns', get_string('cardviewvisiblecolumns', 'core_reportbuilder'), - $visibilityarray); + array_combine($visiblecolumns, $visiblecolumns)); $mform->setType('visiblecolumns', PARAM_INT); $mform->addElement('selectyesno', 'showfirsttitle', get_string('cardviewfirstcolumntitle', 'core_reportbuilder')); diff --git a/reportbuilder/classes/form/condition.php b/reportbuilder/classes/form/condition.php index 867c73f340f..7d6f8b5dd45 100644 --- a/reportbuilder/classes/form/condition.php +++ b/reportbuilder/classes/form/condition.php @@ -24,7 +24,6 @@ use core_form\dynamic_form; use core_reportbuilder\manager; use core_reportbuilder\permission; use core_reportbuilder\local\report\base; -use core_reportbuilder\local\models\filter; use core_reportbuilder\local\models\report; /** @@ -121,22 +120,17 @@ class condition extends dynamic_form { // Allow each condition instance to add itself to this form, wrapping each inside custom header/footer template. $conditioninstances = $this->get_report()->get_condition_instances(); - - $conditions = filter::get_condition_records($this->get_report()->get_report_persistent()->get('id'), 'filterorder'); - foreach ($conditions as $condition) { - $conditioninstance = $conditioninstances[$condition->get('uniqueidentifier')] ?? null; - if ($conditioninstance === null) { - continue; - } + foreach ($conditioninstances as $conditioninstance) { + $persistent = $conditioninstance->get_filter_persistent(); $entityname = $conditioninstance->get_entity_name(); $displayvalue = $conditioninstance->get_header(); $mform->addElement('html', $OUTPUT->render_from_template('core_reportbuilder/local/conditions/header', [ - 'id' => $condition->get('id'), + 'id' => $persistent->get('id'), 'entityname' => $this->get_report()->get_entity_title($entityname), 'heading' => $displayvalue, - 'sortorder' => $condition->get('filterorder'), + 'sortorder' => $persistent->get('filterorder'), 'movetitle' => get_string('movecondition', 'core_reportbuilder', $displayvalue), ])); diff --git a/reportbuilder/classes/local/models/column.php b/reportbuilder/classes/local/models/column.php index f8a19aca8a3..f654d15d858 100644 --- a/reportbuilder/classes/local/models/column.php +++ b/reportbuilder/classes/local/models/column.php @@ -21,6 +21,7 @@ namespace core_reportbuilder\local\models; use context; use lang_string; use core\persistent; +use core_reportbuilder\datasource; /** * Persistent class to represent a report column @@ -108,6 +109,35 @@ class column extends persistent { return new report($this->get('reportid')); } + /** + * Ensure report source is notified of new column + */ + protected function after_create(): void { + datasource::report_elements_modified($this->get('reportid')); + } + + /** + * Ensure report source is notified of updated column + * + * @param bool $result + */ + protected function after_update($result): void { + if ($result) { + datasource::report_elements_modified($this->get('reportid')); + } + } + + /** + * Ensure report source is notified of deleted column + * + * @param bool $result + */ + protected function after_delete($result): void { + if ($result) { + datasource::report_elements_modified($this->get('reportid')); + } + } + /** * Helper method to return the current maximum column order value for a report * diff --git a/reportbuilder/classes/local/models/filter.php b/reportbuilder/classes/local/models/filter.php index 8457ce65341..eca978582bc 100644 --- a/reportbuilder/classes/local/models/filter.php +++ b/reportbuilder/classes/local/models/filter.php @@ -21,6 +21,7 @@ namespace core_reportbuilder\local\models; use context; use lang_string; use core\persistent; +use core_reportbuilder\datasource; /** * Persistent class to represent a report filter/condition @@ -84,6 +85,35 @@ class filter extends persistent { return true; } + /** + * Ensure report source is notified of new filter + */ + protected function after_create(): void { + datasource::report_elements_modified($this->get('reportid')); + } + + /** + * Ensure report source is notified of updated filter + * + * @param bool $result + */ + protected function after_update($result): void { + if ($result) { + datasource::report_elements_modified($this->get('reportid')); + } + } + + /** + * Ensure report source is notified of deleted filter + * + * @param bool $result + */ + protected function after_delete($result): void { + if ($result) { + datasource::report_elements_modified($this->get('reportid')); + } + } + /** * Return the report this filter belongs to *