From 5478e8b4ca1f2f0a9c77af02d544b8abac35399c Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Mon, 6 Jan 2025 16:24:56 +0000 Subject: [PATCH] MDL-84080 question: Prevent overly-long qbank names When a question bank is created by the system, it's name may be created by combining a course of category name with a language string. If the course name is already near the limit for its database field, the resulting question bank name may be too long for that field, and cause an error. This implements a function for generating question bank names which will trim string parameters down until the resulting name is below the allowed length. It also implements some validation to prevent manually-created banks with names that are too long. --- .../task/transfer_question_categories.php | 6 +- mod/qbank/mod_form.php | 1 + question/banks.php | 2 +- .../local/bank/question_bank_helper.php | 59 ++++++++++- .../local/bank/question_bank_helper_test.php | 98 +++++++++++++++++++ 5 files changed, 160 insertions(+), 6 deletions(-) diff --git a/mod/qbank/classes/task/transfer_question_categories.php b/mod/qbank/classes/task/transfer_question_categories.php index cc486249264..958e0b0a3be 100644 --- a/mod/qbank/classes/task/transfer_question_categories.php +++ b/mod/qbank/classes/task/transfer_question_categories.php @@ -105,17 +105,17 @@ class transfer_question_categories extends adhoc_task { switch ($oldcontext->contextlevel) { case CONTEXT_SYSTEM: $course = get_site(); - $bankname = get_string('systembank', 'question'); + $bankname = question_bank_helper::get_bank_name_string('systembank', 'question'); break; case CONTEXT_COURSECAT: $coursecategory = core_course_category::get($oldcontext->instanceid); $courseshortname = "{$coursecategory->name}-{$coursecategory->id}"; $course = $this->create_course($coursecategory, $courseshortname); - $bankname = get_string("sharedbank", "mod_qbank", $coursecategory->name); + $bankname = question_bank_helper::get_bank_name_string('sharedbank', 'mod_qbank', $coursecategory->name); break; case CONTEXT_COURSE: $course = get_course($oldcontext->instanceid); - $bankname = get_string("sharedbank", "mod_qbank", $course->shortname); + $bankname = question_bank_helper::get_bank_name_string('sharedbank', 'mod_qbank', $course->shortname); break; default: // This shouldn't be possible, so we can't really transfer it. diff --git a/mod/qbank/mod_form.php b/mod/qbank/mod_form.php index 792755efb31..465a6f59572 100644 --- a/mod/qbank/mod_form.php +++ b/mod/qbank/mod_form.php @@ -55,6 +55,7 @@ class mod_qbank_mod_form extends moodleform_mod { } $mform->addHelpButton('name', 'qbankname', 'mod_qbank'); $mform->addRule('name', null, 'required', null, 'client'); + $mform->addRule('name', null, 'maxlength', \core_question\local\bank\question_bank_helper::BANK_NAME_MAX_LENGTH, 'client'); // Add intro editor. $mform->addElement('editor', 'introeditor', get_string('moduleintro'), ['rows' => 10], [ diff --git a/question/banks.php b/question/banks.php index d28e2daf837..d86f0aac41b 100644 --- a/question/banks.php +++ b/question/banks.php @@ -57,7 +57,7 @@ if ($createdefault) { require_sesskey(); question_bank_helper::create_default_open_instance( $course, - get_string('defaultbank', 'core_question', ['coursename' => $course->fullname]) + question_bank_helper::get_bank_name_string('defaultbank', 'core_question', ['coursename' => $course->fullname]), ); \core\notification::add(get_string('defaultcreated', 'question'), \core\notification::SUCCESS); redirect($pageurl); diff --git a/question/classes/local/bank/question_bank_helper.php b/question/classes/local/bank/question_bank_helper.php index b0c78999116..6470692fdae 100644 --- a/question/classes/local/bank/question_bank_helper.php +++ b/question/classes/local/bank/question_bank_helper.php @@ -65,6 +65,11 @@ class question_bank_helper { */ private const CATEGORY_DELIMITER = '<->'; + /** + * Maximum length for the question bank name database field. + */ + public const BANK_NAME_MAX_LENGTH = 255; + /** * Modules that share questions via FEATURE_PUBLISHES_QUESTIONS. * @@ -411,7 +416,11 @@ class question_bank_helper { } if (!$systembank && $createifnotexists) { - $systembank = self::create_default_open_instance($course, get_string('systembank', 'question'), self::TYPE_SYSTEM); + $systembank = self::create_default_open_instance( + $course, + self::get_bank_name_string('systembank', 'question'), + self::TYPE_SYSTEM, + ); } return $systembank; @@ -440,7 +449,11 @@ class question_bank_helper { } if (!$previewbank && $createifnotexists) { - $previewbank = self::create_default_open_instance($site, get_string('previewbank', 'question'), self::TYPE_PREVIEW); + $previewbank = self::create_default_open_instance( + $site, + self::get_bank_name_string('previewbank', 'question'), + self::TYPE_PREVIEW + ); } return $previewbank; @@ -521,6 +534,13 @@ class question_bank_helper { } } + if (strlen($bankname) > self::BANK_NAME_MAX_LENGTH) { + throw new \coding_exception( + 'The provided bankname is too long for the database field.', + 'Use question_bank_helper::get_bank_name_string to get a suitably truncated name.', + ); + } + $data = new stdClass(); $data->section = 0; $data->visible = 0; @@ -585,4 +605,39 @@ class question_bank_helper { global $CFG; return $CFG->corequestion_defaultqbankmod ?? 'qbank'; } + + /** + * Get the requested language string, with parameters truncated to ensure the result fits in the database. + * + * Since we may be generating a question bank name based on an existing course or category name, we need to ensure + * that the resulting string isn't longer than the maximum module name. + * + * @param string $identifier The string identifier + * @param string $component The string component + * @param mixed|null $params The string parameters (a single string, array or object as accepted by get_string) + * @return string The string truncated to a length that will fit in the database. + */ + public static function get_bank_name_string(string $identifier, string $component, mixed $params = null): string { + if (is_object($params)) { + $shortparams = (array) $params; + } else { + $shortparams = $params; + } + $bankname = get_string($identifier, $component, $shortparams); + if (!is_null($shortparams)) { + $trimlength = self::BANK_NAME_MAX_LENGTH - 4; + while (\core_text::strlen($bankname) > self::BANK_NAME_MAX_LENGTH && $trimlength > 0) { + // Gradually shorten the string parameters until the resulting string is short enough. + if (is_array($shortparams)) { + $shortparams = array_map(fn($param) => shorten_text(trim($param), $trimlength), $shortparams); + } else { + $shortparams = shorten_text(trim($shortparams), $trimlength); + } + $bankname = get_string($identifier, $component, $shortparams); + $trimlength -= 10; + } + } + // As a failsafe, limit the length of the final string in case the lang string is too long. + return shorten_text($bankname, self::BANK_NAME_MAX_LENGTH); + } } diff --git a/question/tests/local/bank/question_bank_helper_test.php b/question/tests/local/bank/question_bank_helper_test.php index 13bbe1b7b34..9a4e5b00724 100644 --- a/question/tests/local/bank/question_bank_helper_test.php +++ b/question/tests/local/bank/question_bank_helper_test.php @@ -200,6 +200,28 @@ final class question_bank_helper_test extends \advanced_testcase { $this->assertEquals(1, $cminfo->showdescription); } + /** + * Create a default instance, passing a name that is too long for the database. + * + * @return void + * @throws \coding_exception + * @throws \dml_exception + * @throws \moodle_exception + */ + public function test_create_default_open_instance_with_long_name(): void { + $this->resetAfterTest(); + self::setAdminUser(); + + $coursename = random_string(question_bank_helper::BANK_NAME_MAX_LENGTH); + $course = self::getDataGenerator()->create_course(['shortname' => $coursename]); + + $this->expectExceptionMessage('The provided bankname is too long for the database field.'); + question_bank_helper::create_default_open_instance( + $course, + get_string('defaultbank', 'core_question', ['coursename' => $coursename]), + ); + } + /** * Assert that viewing a question bank logs the view for that user up to a maximum of 5 unique bank views. * @@ -310,4 +332,80 @@ final class question_bank_helper_test extends \advanced_testcase { $modrecord = $DB->get_record('qbank', ['id' => $qbank->instance]); $this->assertEquals(question_bank_helper::TYPE_SYSTEM, $modrecord->type); } + + /** + * Assert that get_bank_name_string returns suitably truncated strings. + * + * @dataProvider bank_name_strings + * @param string $identifier + * @param string $component + * @param mixed $params + * @param string $expected + */ + public function test_get_bank_name_string(string $identifier, string $component, mixed $params, string $expected): void { + $this->assertEquals($expected, question_bank_helper::get_bank_name_string($identifier, $component, $params)); + } + + /** + * Get string examples with different parameter types and lengths. + * + * @return array[] + */ + public static function bank_name_strings(): array { + $longname = 'One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen seventeen ' . + 'eighteen nineteen twenty twenty-one twenty-two twenty-three twenty-four twenty-five twenty-six twenty-seven ' . + 'twenty-eight twenty-nine thirty thirty-one'; + return [ + 'String with no parameters' => [ + 'systembank', + 'question', + null, + 'System shared question bank', + ], + 'String with short string parameter' => [ + 'topfor', + 'question', + 'Test course', + 'Top for Test course', + ], + 'String with long string parameter' => [ + 'topfor', + 'question', + $longname, + 'Top for One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen ' . + 'seventeen eighteen nineteen twenty twenty-one twenty-two twenty-three twenty-four twenty-five twenty-six ' . + 'twenty-seven twenty-eight ...', + ], + 'String with short array parameter' => [ + 'defaultbank', + 'question', + ['coursename' => 'Test course'], + 'Test course course question bank', + ], + 'String with long array parameter' => [ + 'defaultbank', + 'question', + ['coursename' => $longname], + 'One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen seventeen ' . + 'eighteen nineteen twenty twenty-one twenty-two twenty-three twenty-four twenty-five twenty-six ' . + 'twenty-seven twenty-eight ... course question bank', + ], + 'String with multiple long array parameters' => [ + 'markoutofmax', + 'question', + ['mark' => $longname, 'max' => $longname], + 'Mark One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen seventeen ' . + 'eighteen ... out of One two three four five six seven eight nine ten eleven twelve thirteen fourteen ' . + 'fifteen sixteen seventeen eighteen ...', + ], + 'Long lang string' => [ + 'howquestionsbehave_help', + 'question', + null, + 'Students can interact with the questions in the quiz in various different ways. For example, you may wish the ' . + 'students to enter an answer to each question and then submit the entire quiz, before anything is graded or ' . + 'they get any feedback. That would ...', + ], + ]; + } }