From 6001ee3dfd4d7042b2ebc253ff99ee9ed4dfc260 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 22 Aug 2023 14:28:40 +0100 Subject: [PATCH] MDL-74054 core_question: Define unique question bank column IDs This also resolves MDL-78829. Some question bank plugins use a separate class for each plugin they define. However, qbank_customfields (and potentially others in the future) uses a single class to define multiple fields. Using the class name as an ID for the column doesn't give us a way of reliable instantiating an object for the column. Previously, qbank_customfields appended the field name as though it was a namespaced class, but this had to be manually constructed and deconstructed by detecting this particular column class. This change introduces a standard way of constructing a unique ID for each question bank column, in the form pluginname\columnclass-columnname. This ensures that the ID will be unique for each column, and the ID can be used to instatiate the column's object. --- .../classes/column_manager.php | 61 +++++------------ .../classes/custom_field_column.php | 14 +++- .../classes/local/bank/checkbox_column.php | 1 + question/classes/local/bank/column_base.php | 38 +++++++++++ question/classes/local/bank/view.php | 67 +++++++------------ question/templates/column_header.mustache | 7 +- 6 files changed, 101 insertions(+), 87 deletions(-) diff --git a/question/bank/columnsortorder/classes/column_manager.php b/question/bank/columnsortorder/classes/column_manager.php index 5491fa8a1de..f8f9289e851 100644 --- a/question/bank/columnsortorder/classes/column_manager.php +++ b/question/bank/columnsortorder/classes/column_manager.php @@ -21,7 +21,6 @@ defined('MOODLE_INTERNAL') || die(); require_once($CFG->libdir . '/questionlib.php'); use context_system; -use core_question\local\bank\column_action_base; use core_question\local\bank\column_base; use core_question\local\bank\column_manager_base; use core_question\local\bank\question_edit_contexts; @@ -29,6 +28,7 @@ use core_question\local\bank\view; use qbank_columnsortorder\local\bank\column_action_move; use qbank_columnsortorder\local\bank\column_action_remove; use qbank_columnsortorder\local\bank\column_action_resize; +use qbank_columnsortorder\local\bank\preview_view; use moodle_url; /** @@ -158,7 +158,8 @@ class column_manager extends column_manager_base { $context = context_system::instance(); $contexts = new question_edit_contexts($context); // Dummy call to get the objects without error. - $questionbank = new view($contexts, new moodle_url('/question/bank/columnsortorder/sortcolumns.php'), $course, null); + $questionbank = new preview_view($contexts, new moodle_url('/question/bank/columnsortorder/sortcolumns.php'), $course, + null); return $questionbank; } @@ -178,6 +179,7 @@ class column_manager extends column_manager_base { 'class' => get_class($column), 'name' => $column->get_title(), 'colname' => end($classelements), + 'id' => implode(self::ID_SEPARATOR, [$column::class, $column->get_column_name()]), ]; } return $columns; @@ -191,20 +193,12 @@ class column_manager extends column_manager_base { public function get_disabled_columns(): array { $disabled = []; if ($this->disabledcolumns) { - foreach ($this->disabledcolumns as $class => $value) { - if (strpos($class, 'qbank_customfields\custom_field_column') !== false) { - $class = explode('\\', $class); - $disabledname = array_pop($class); - $class = implode('\\', $class); - $disabled[] = (object) [ - 'disabledname' => $disabledname, - ]; - } else { - $columnobject = new $class($this->get_questionbank()); - $disabled[] = (object) [ - 'disabledname' => $columnobject->get_title(), - ]; - } + foreach (array_keys($this->disabledcolumns) as $column) { + [$classname, $columnname] = explode(self::ID_SEPARATOR, $column); + $columnobject = $classname::from_column_name($this->get_questionbank(), $columnname); + $disabled[] = (object) [ + 'disabledname' => $columnobject->get_title(), + ]; } } return $disabled; @@ -271,17 +265,10 @@ class column_manager extends column_manager_base { } foreach ($allcolumns as $column) { - if (strpos($column->class, $plugin) !== false) { - if ($column->class === 'qbank_customfields\custom_field_column') { - $disabledcolumns[$column->class . '\\' . $column->colname] = $column->class . '\\' . $column->colname; - if (isset($enabledcolumns[$column->class . '\\' . $column->colname])) { - unset($enabledcolumns[$column->class. '\\' . $column->colname]); - } - } else { - $disabledcolumns[$column->class] = $column->class; - if (isset($enabledcolumns[$column->class])) { - unset($enabledcolumns[$column->class]); - } + if (str_contains($column->class, $plugin)) { + $disabledcolumns[$column->id] = $column->id; + if (isset($enabledcolumns[$column->id])) { + unset($enabledcolumns[$column->id]); } } } @@ -301,21 +288,9 @@ class column_manager extends column_manager_base { $columnsortorder = $this->columnorder; asort($columnsortorder); $columnorder = []; - foreach ($columnsortorder as $classname => $colposition) { - $colname = explode('\\', $classname); - if (strpos($classname, 'qbank_customfields\custom_field_column') !== false) { - unset($colname[0]); - $classname = implode('\\', $colname); - // Checks if custom column still exists. - if (array_key_exists($classname, $ordertosort)) { - $columnorder[$classname] = $colposition; - } else { - $configtounset = str_replace('\\', '\\\\', $classname); - // Cleans config db. - unset_config($configtounset, 'column_sortorder'); - } - } else { - $columnorder[end($colname)] = $colposition; + foreach ($columnsortorder as $columnid => $colposition) { + if (array_key_exists($columnid, $ordertosort)) { + $columnorder[$columnid] = $colposition; } } $properorder = array_merge($columnorder, $ordertosort); @@ -375,7 +350,7 @@ class column_manager extends column_manager_base { $colsizemap = $this->get_colsize_map(); $columnclass = get_class($column); if (array_key_exists($columnclass, $colsizemap)) { - return $colsizemap[$columnclass]; + return $colsizemap[$columnclass] . 'px'; } return parent::get_column_width($column); } diff --git a/question/bank/customfields/classes/custom_field_column.php b/question/bank/customfields/classes/custom_field_column.php index 469d6f9fceb..6a21e10b77e 100644 --- a/question/bank/customfields/classes/custom_field_column.php +++ b/question/bank/customfields/classes/custom_field_column.php @@ -17,6 +17,8 @@ namespace qbank_customfields; use core_question\local\bank\column_base; +use core_question\local\bank\view; +use qbank_customfields\customfield\question_handler; /** * A column type for the name of the question creator. @@ -42,6 +44,16 @@ class custom_field_column extends column_base { $this->field = $field; } + public static function from_column_name(view $view, string $columnname): custom_field_column { + $handler = question_handler::create(); + foreach ($handler->get_fields() as $field) { + if ($field->get('shortname') == $columnname) { + return new static($view, $field); + } + } + throw new \coding_exception('Custom field ' . $columnname . ' does not exist.'); + } + /** * Get the internal name for this column. Used as a CSS class name, * and to store information about the current sort. Must match PARAM_ALPHA. @@ -60,7 +72,7 @@ class custom_field_column extends column_base { * @return string The unique name; */ public function get_column_name(): string { - return 'custom_field_column\\' . $this->field->get('shortname'); + return $this->field->get('shortname'); } /** diff --git a/question/classes/local/bank/checkbox_column.php b/question/classes/local/bank/checkbox_column.php index 97fe9646420..7d66e8d7ab3 100644 --- a/question/classes/local/bank/checkbox_column.php +++ b/question/classes/local/bank/checkbox_column.php @@ -70,6 +70,7 @@ class checkbox_column extends column_base { $data['tip'] = $this->get_title_tip(); $data['colname'] = $this->get_column_name(); + $data['columnid'] = $this->get_column_id(); $data['name'] = get_string('selectall'); $data['class'] = $name; $data['width'] = $width; diff --git a/question/classes/local/bank/column_base.php b/question/classes/local/bank/column_base.php index 9098c4667ed..73c449d4322 100644 --- a/question/classes/local/bank/column_base.php +++ b/question/classes/local/bank/column_base.php @@ -33,6 +33,11 @@ namespace core_question\local\bank; */ abstract class column_base extends view_component { + /** + * @const string A separator for joining column attributes together into a unique ID string. + */ + const ID_SEPARATOR = '-'; + /** * @var view $qbank the question bank view we are helping to render. */ @@ -44,6 +49,20 @@ abstract class column_base extends view_component { /** @var bool determine whether the column is visible */ public $isvisible = true; + /** + * Return an instance of this column, based on the column name. + * + * In the case of the base class, we don't actually use the column name since the class represents one specific column. + * However, sub-classes may use the column name as an additional constructor to the parameter. + * + * @param view $view Question bank view + * @param string $columnname The column name for this instance, as returned by {@see get_column_name()} + * @return column_base An instance of this class. + */ + public static function from_column_name(view $view, string $columnname): column_base { + return new static($view); + } + /** * Set the column as heading */ @@ -121,6 +140,7 @@ abstract class column_base extends view_component { } $data['colname'] = $this->get_column_name(); + $data['columnid'] = $this->get_column_id(); $data['name'] = $title; $data['class'] = $name; $data['width'] = $width; @@ -272,6 +292,24 @@ abstract class column_base extends view_component { return (new \ReflectionClass($this))->getShortName(); } + /** + * Return a unique ID for this column object. + * + * This is constructed using the class name and get_column_name(), which must be unique. + * + * The combination of these attributes allows the object to be reconstructed, by splitting the ID into its constituent + * parts then calling {@see from_column_name()}, like this: + * [$class, $columnname] = explode(column_base::ID_SEPARATOR, $columnid, 2); + * $column = $class::from_column_name($qbank, $columnname); + * Including 2 as the $limit parameter for explode() is a good idea for safely, in case a plugin defines a column with the + * ID_SEPARATOR in the column name. + * + * @return string The column ID. + */ + final public function get_column_id(): string { + return implode(self::ID_SEPARATOR, [static::class, $this->get_column_name()]); + } + /** * Any extra class names you would like applied to every cell in this column. * diff --git a/question/classes/local/bank/view.php b/question/classes/local/bank/view.php index ccb3743edc9..c004c2415ca 100644 --- a/question/classes/local/bank/view.php +++ b/question/classes/local/bank/view.php @@ -383,66 +383,49 @@ class view { * * @return array */ - protected function get_class_for_columns(): array { - $this->corequestionbankcolumns = [ - 'checkbox_column', - 'question_type_column', - 'question_name_idnumber_tags_column', - 'edit_menu_column', - 'export_xml_action_column', - 'question_status_column', - 'version_number_column', - 'creator_name_column', - 'comment_count_column', + protected function get_question_bank_plugins(): array { + $questionbankclasscolumns = []; + $newpluginclasscolumns = []; + $corequestionbankcolumns = [ + 'core_question\local\bank\checkbox_column' . column_base::ID_SEPARATOR . 'checkbox_column', + 'qbank_viewquestiontype\question_type_column' . column_base::ID_SEPARATOR . 'question_type_column', + 'qbank_viewquestionname\question_name_idnumber_tags_column' . column_base::ID_SEPARATOR . + 'question_name_idnumber_tags_column', + 'core_question\local\bank\edit_menu_column' . column_base::ID_SEPARATOR . 'edit_menu_column', + 'qbank_editquestion\question_status_column' . column_base::ID_SEPARATOR . 'question_status_column', + 'qbank_history\version_number_column' . column_base::ID_SEPARATOR . 'version_number_column', + 'qbank_viewcreator\creator_name_column' . column_base::ID_SEPARATOR . 'creator_name_column', + 'qbank_comment\comment_count_column' . column_base::ID_SEPARATOR . 'comment_count_column', ]; if (question_get_display_preference('qbshowtext', 0, PARAM_INT, new \moodle_url(''))) { - $this->corequestionbankcolumns[] = 'question_text_row'; + $corequestionbankcolumns[] = 'qbank_viewquestiontext\question_text_row' . column_base::ID_SEPARATOR . + 'question_text_row'; } - $questionbankclasscolumns = []; - foreach ($this->corequestionbankcolumns as $fullname) { - $shortname = $fullname; - if (class_exists('core_question\\local\\bank\\' . $fullname)) { - $fullname = 'core_question\\local\\bank\\' . $fullname; - $questionbankclasscolumns[$shortname] = new $fullname($this); - } else { - $questionbankclasscolumns[$shortname] = ''; + foreach ($corequestionbankcolumns as $columnid) { + [$columnclass, $columnname] = explode(column_base::ID_SEPARATOR, $columnid, 2); + if (class_exists($columnclass)) { + $questionbankclasscolumns[$columnid] = $columnclass::from_column_name($this, $columnname); } } - return $questionbankclasscolumns; - } - /** - * Get the list of qbank plugins with available objects for features. - * - * @return array - */ - protected function get_question_bank_plugins(): array { - $newpluginclasscolumns = []; - $questionbankclasscolumns = $this->get_class_for_columns(); - - $plugins = $this->plugins; - foreach ($this->plugins as $componentname => $plugin) { + foreach ($this->plugins as $plugin) { $plugincolumnobjects = $plugin->get_question_columns($this); foreach ($plugincolumnobjects as $columnobject) { - $columnname = $columnobject->get_column_name(); - foreach ($this->corequestionbankcolumns as $key => $corequestionbankcolumn) { - if (!\core\plugininfo\qbank::is_plugin_enabled($componentname)) { - unset($questionbankclasscolumns[$columnname]); - continue; - } + $columnid = $columnobject->get_column_id(); + foreach ($corequestionbankcolumns as $corequestionbankcolumn) { // Check if it has custom preference selector to view/hide. if ($columnobject->has_preference()) { if (!$columnobject->get_preference()) { continue; } } - if ($corequestionbankcolumn === $columnname) { - $questionbankclasscolumns[$columnname] = $columnobject; + if ($corequestionbankcolumn === $columnid) { + $questionbankclasscolumns[$columnid] = $columnobject; } else { // Any community plugin for column/action. - $newpluginclasscolumns[$columnname] = $columnobject; + $newpluginclasscolumns[$columnid] = $columnobject; } } } diff --git a/question/templates/column_header.mustache b/question/templates/column_header.mustache index 68a0ffcd26f..50558ca47a3 100644 --- a/question/templates/column_header.mustache +++ b/question/templates/column_header.mustache @@ -51,7 +51,12 @@ "name": "plugin_name" } }} - +
{{#title}}