From 2d6df85dea4fc5365b28781ce3cabc0fb7eeb3b5 Mon Sep 17 00:00:00 2001 From: Conn Warwicker Date: Tue, 21 Jan 2025 15:46:58 +0000 Subject: [PATCH] MDL-84302 mod_quiz: Improvements with invalid questions. - Improve visuals of invalid questions by highlighting and labelling. - Removes links to actions which will not work for invalid questions - Doesn't allow a preview/attempt of a quiz if it has invalid questions. This is an improvement over it throwing an exception. - Don't allow the adding of questions to a quiz if they have an invalid type. --- mod/quiz/classes/output/edit_renderer.php | 30 ++++++++++---- .../classes/question/bank/qbank_helper.php | 1 + mod/quiz/lang/en/quiz.php | 1 + mod/quiz/locallib.php | 6 +++ mod/quiz/styles.css | 8 ++++ mod/quiz/tests/behat/attempt_invalid.feature | 41 +++++++++++++++++++ mod/quiz/view.php | 10 ++++- .../local/bank/random_question_loader.php | 15 ++++--- .../tests/random_question_loader_test.php | 27 ++++++++++++ 9 files changed, 123 insertions(+), 16 deletions(-) create mode 100644 mod/quiz/tests/behat/attempt_invalid.feature diff --git a/mod/quiz/classes/output/edit_renderer.php b/mod/quiz/classes/output/edit_renderer.php index d17d33ef264..ef6cbdc98ae 100644 --- a/mod/quiz/classes/output/edit_renderer.php +++ b/mod/quiz/classes/output/edit_renderer.php @@ -28,6 +28,7 @@ use core_question\local\bank\question_version_status; use \mod_quiz\structure; use \html_writer; use qbank_previewquestion\question_preview_options; +use question_bank; use renderable; /** @@ -867,7 +868,7 @@ class edit_renderer extends \plugin_renderer_base { $qtype = $structure->get_question_type_for_slot($slot); $slotinfo = $structure->get_slot_by_number($slot); $questionicons = ''; - if ($qtype !== 'random') { + if ($qtype !== 'random' && question_bank::is_qtype_usable($qtype)) { $questionicons .= $this->question_preview_icon($structure->get_quiz(), $structure->get_question_in_slot($slot), null, null, $slotinfo->requestedversion ?: question_preview_options::ALWAYS_LATEST); @@ -924,7 +925,7 @@ class edit_renderer extends \plugin_renderer_base { public function question_preview_icon($quiz, $questiondata, $label = null, $variant = null, $restartversion = null) { $question = clone($questiondata); - if (!\question_bank::is_qtype_usable($question->qtype)) { + if (!question_bank::is_qtype_usable($question->qtype)) { return ''; } @@ -1062,7 +1063,7 @@ class edit_renderer extends \plugin_renderer_base { $instancename = quiz_question_tostring($question); - $qtype = \question_bank::get_qtype($question->qtype, false); + $qtype = question_bank::get_qtype($question->qtype, false); $namestr = $qtype->local_name(); $icon = $this->pix_icon('icon', $namestr, $qtype->plugin_name(), ['title' => $namestr, @@ -1073,10 +1074,21 @@ class edit_renderer extends \plugin_renderer_base { // Need plain question name without html tags for link title. $title = shorten_text(format_string($question->name), 100); - // Display the link itself. - $activitylink = $icon . html_writer::tag('span', $editicon . $instancename, ['class' => 'instancename']); - $output .= html_writer::link($editurl, $activitylink, - ['title' => get_string('editquestion', 'quiz').' '.$title]); + // If the question is invalid, don't show the link as it won't work. + if (!question_bank::is_qtype_usable($question->qtype)) { + $output .= html_writer::span($title); + $output .= html_writer::span( + get_string('invalidquestiontype', 'question', $question->originalqtype), + 'badge bg-danger text-white ml-3' + ); + } else { + + // Display the link itself. + $activitylink = $icon . html_writer::tag('span', $editicon . $instancename, ['class' => 'instancename']); + $output .= html_writer::link($editurl, $activitylink, + ['title' => get_string('editquestion', 'quiz') . ' ' . $title]); + + } return $output; } @@ -1110,7 +1122,7 @@ class edit_renderer extends \plugin_renderer_base { } $configuretitle = get_string('configurerandomquestion', 'quiz'); - $qtype = \question_bank::get_qtype($question->qtype, false); + $qtype = question_bank::get_qtype($question->qtype, false); $namestr = $qtype->local_name(); $icon = $this->pix_icon('icon', $namestr, $qtype->plugin_name(), ['class' => 'icon activityicon']); @@ -1294,7 +1306,7 @@ class edit_renderer extends \plugin_renderer_base { 'questiondependsonprevious', ], 'quiz'); - foreach (\question_bank::get_all_qtypes() as $qtype => $notused) { + foreach (question_bank::get_all_qtypes() as $qtype => $notused) { $this->page->requires->string_for_js('pluginname', 'qtype_' . $qtype); } diff --git a/mod/quiz/classes/question/bank/qbank_helper.php b/mod/quiz/classes/question/bank/qbank_helper.php index fbfc98c653f..098afdd6a08 100644 --- a/mod/quiz/classes/question/bank/qbank_helper.php +++ b/mod/quiz/classes/question/bank/qbank_helper.php @@ -189,6 +189,7 @@ class qbank_helper { $slot->length = 1; } else if (!\question_bank::qtype_exists($slot->qtype)) { // Question of unknown type found in the database. Set to placeholder question types instead. + $slot->originalqtype = $slot->qtype; $slot->qtype = 'missingtype'; } else { $slot->_partiallyloaded = 1; diff --git a/mod/quiz/lang/en/quiz.php b/mod/quiz/lang/en/quiz.php index 693d83f6e7f..f5bb443719e 100644 --- a/mod/quiz/lang/en/quiz.php +++ b/mod/quiz/lang/en/quiz.php @@ -807,6 +807,7 @@ $string['quiz:emailnotifyattemptgraded'] = 'Receive notification when your attem $string['quiz:emailwarnoverdue'] = 'Receive warning when your quiz attempt becomes overdue'; $string['quiz:grade'] = 'Grade quizzes manually'; $string['quiz:ignoretimelimits'] = 'Ignore quiz time limit'; +$string['quizinvalidquestions'] = 'This quiz has questions with invalid types. The missing question type must be reinstalled or the affected questions removed, before the quiz can be used.'; $string['quizisclosed'] = 'This quiz is closed'; $string['quizisopen'] = 'This quiz is open'; $string['quizisclosedwillopen'] = 'Quiz closed (opens {$a})'; diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index 925b9094c7c..344a2263964 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -1714,6 +1714,12 @@ function quiz_add_quiz_question($questionid, $quiz, $page = 0, $maxmark = null) ); } + // If the question type is invalid, we cannot add it to the quiz. It shouldn't be possible to get to this + // point without fiddling with the DOM so we can just throw an exception. + if (!\question_bank::is_qtype_installed($questiontype)) { + throw new coding_exception('Invalid question type: ' . $questiontype); + } + $trans = $DB->start_delegated_transaction(); $sql = "SELECT qbe.id diff --git a/mod/quiz/styles.css b/mod/quiz/styles.css index cc5f4914672..92b26589d03 100644 --- a/mod/quiz/styles.css +++ b/mod/quiz/styles.css @@ -644,6 +644,14 @@ table.quizreviewsummary td.cell { position: relative; } +#page-mod-quiz-edit ul.slots li.section li.activity.qtype_missingtype { + background-color: #f0c5c1; +} + +#page-mod-quiz-edit ul.slots li.section li.activity.qtype_missingtype .actions { + background-color: inherit; +} + #page-mod-quiz-edit ul.slots li.section li.activity.page { background: transparent; } diff --git a/mod/quiz/tests/behat/attempt_invalid.feature b/mod/quiz/tests/behat/attempt_invalid.feature new file mode 100644 index 00000000000..d41980d0db9 --- /dev/null +++ b/mod/quiz/tests/behat/attempt_invalid.feature @@ -0,0 +1,41 @@ +@mod_quiz +Feature: A quiz with invalid question types should not be able to be attempted + As a teacher + If my quiz has questions with invalid types + I want my students to be unable to attempt the quiz until it is fixed + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | student | Student | One | student@example.com | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | student | C1 | student | + And the following "activities" exist: + | activity | name | intro | course | idnumber | + | qbank | Qbank 1 | Question bank 1 | C1 | qbank1 | + And the following "question categories" exist: + | contextlevel | reference | name | + | Activity module | qbank1 | Test questions | + And the following "questions" exist: + | questioncategory | qtype | name | user | questiontext | + | Test questions | essay | Question 1 | admin | A text | + | Test questions | essay | Question 2 | admin | B text | + And the following "activities" exist: + | activity | name | intro | course | idnumber | grade | navmethod | + | quiz | Quiz 1 | Quiz 1 description | C1 | quiz1 | 100 | free | + And quiz "Quiz 1" contains the following questions: + | question | page | maxmark | + | Question 1 | 1 | | + | Question 2 | 1 | | + And question "Question 2" is changed to simulate being of an uninstalled type + + @javascript + Scenario: Quiz with invalid questions should disable attempts + Given I am logged in as "student" + When I am on the "Quiz 1" "mod_quiz > View" page + Then I should see "This quiz has questions with invalid types" + And I should not see "Attempt quiz" diff --git a/mod/quiz/view.php b/mod/quiz/view.php index 8bf1d0107b3..1a8799d1e04 100644 --- a/mod/quiz/view.php +++ b/mod/quiz/view.php @@ -23,6 +23,7 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core\output\notification; use mod_quiz\access_manager; use mod_quiz\output\list_of_attempts; use mod_quiz\output\renderer; @@ -261,6 +262,13 @@ if (!$viewobj->quizhasquestions) { } } } + + // If the quiz has any invalid questions, we cannot attempt it. + if (in_array('missingtype', $quizobj->get_all_question_types_used())) { + $viewobj->preventmessages[] = $OUTPUT->notification( + get_string('quizinvalidquestions', 'mod_quiz'), notification::NOTIFY_ERROR, false); + $viewobj->buttontext = ''; + } } $viewobj->showbacktocourse = ($viewobj->buttontext === '' && @@ -270,7 +278,7 @@ echo $OUTPUT->header(); if (!empty($gradinginfo->errors)) { foreach ($gradinginfo->errors as $error) { - $errortext = new \core\output\notification($error, \core\output\notification::NOTIFY_ERROR); + $errortext = new notification($error, notification::NOTIFY_ERROR); echo $OUTPUT->render($errortext); } } diff --git a/question/classes/local/bank/random_question_loader.php b/question/classes/local/bank/random_question_loader.php index 947b0df7f14..017d3cc2f13 100644 --- a/question/classes/local/bank/random_question_loader.php +++ b/question/classes/local/bank/random_question_loader.php @@ -46,8 +46,10 @@ class random_question_loader { /** @var \qubaid_condition which usages to consider previous attempts from. */ protected $qubaids; - /** @var array qtypes that cannot be used by random questions. */ - protected $excludedqtypes; + /** + * @var array Array of question types to include in random questions. + */ + protected $includedqtypes = []; /** @var array categoryid & include subcategories => num previous uses => questionid => 1. */ protected $availablequestionscache = []; @@ -69,9 +71,10 @@ class random_question_loader { $this->qubaids = $qubaids; $this->recentlyusedquestions = $usedquestions; + // Load the possible question types we can select from. foreach (\question_bank::get_all_qtypes() as $qtype) { - if (!$qtype->is_usable_by_random()) { - $this->excludedqtypes[] = $qtype->name(); + if ($qtype->is_usable_by_random()) { + $this->includedqtypes[] = $qtype->name(); } } } @@ -213,8 +216,8 @@ class random_question_loader { $filtercondition = $filterconditions ? 'AND ' . implode(' AND ', $filterconditions) : ''; // Prepare qtype check. - [$qtypecondition, $qtypeparams] = $DB->get_in_or_equal($this->excludedqtypes, - SQL_PARAMS_NAMED, 'excludedqtype', false); + [$qtypecondition, $qtypeparams] = $DB->get_in_or_equal($this->includedqtypes, + SQL_PARAMS_NAMED, 'includedqtype'); if ($qtypecondition) { $qtypecondition = 'AND q.qtype ' . $qtypecondition; } diff --git a/question/tests/random_question_loader_test.php b/question/tests/random_question_loader_test.php index c4775f7f2df..23eedeb9007 100644 --- a/question/tests/random_question_loader_test.php +++ b/question/tests/random_question_loader_test.php @@ -680,4 +680,31 @@ final class random_question_loader_test extends \advanced_testcase { return [$category, $questions]; } + + /** + * Test that the random question loader excludes questions with invalid types. + * @return void + * @throws \dml_exception + */ + public function test_invalid_questions_are_excluded(): void { + + global $DB; + + $this->resetAfterTest(); + + [$category, $questions] = $this->create_category_and_questions(4); + $loader = new random_question_loader(new qubaid_list([])); + $filters = question_filter_test_helper::create_filters([$category->id], false, []); + + // Update one of the questions to have an invalid type. + $invalid = $questions[0]; + $invalid->qtype = 'invalid'; + $DB->update_record('question', $invalid); + + // Assert that we get 1 less result back because the invalid type is excluded. + $result = $loader->get_filtered_questions($filters); + $this->assertEquals(count($questions) - 1, count($result)); + + } + }