diff --git a/lib/questionlib.php b/lib/questionlib.php index 61764ed59ea..22fa4f711e2 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -921,8 +921,6 @@ function question_load_questions($questionids, $extrafields = '', $join = '') { * @param stdClass[]|null $filtercourses The courses to filter the course tags by. */ function _tidy_question($question, $category, array $tagobjects = null, array $filtercourses = null) { - global $CFG; - // Load question-type specific fields. if (!question_bank::is_qtype_installed($question->qtype)) { $question->questiontext = html_writer::tag('p', get_string('warningmissingtype', @@ -1651,25 +1649,44 @@ class context_to_string_translator{ /** * Check capability on category * - * @param mixed $questionorid object or id. If an object is passed, it should include ->contextid and ->createdby. + * @param int|stdClass $questionorid object or id. If an object is passed, it should include ->contextid and ->createdby. * @param string $cap 'add', 'edit', 'view', 'use', 'move' or 'tag'. - * @param integer $notused no longer used. - * @return boolean this user has the capability $cap for this question $question? + * @param int $notused no longer used. + * @return bool this user has the capability $cap for this question $question? + * @throws coding_exception */ function question_has_capability_on($questionorid, $cap, $notused = -1) { - global $USER; + global $USER, $DB; if (is_numeric($questionorid)) { - $question = question_bank::load_question_data((int)$questionorid); + $questionid = (int)$questionorid; } else if (is_object($questionorid)) { + // All we really need in this function is the contextid and author of the question. + // We won't bother fetching other details of the question if these 2 fields are provided. if (isset($questionorid->contextid) && isset($questionorid->createdby)) { $question = $questionorid; + } else if (!empty($questionorid->id)) { + $questionid = $questionorid->id; } + } - if (!isset($question) && isset($questionorid->id) && $questionorid->id != 0) { - $question = question_bank::load_question_data($questionorid->id); + // At this point, either $question or $questionid is expected to be set. + if (isset($questionid)) { + try { + $question = question_bank::load_question_data($questionid); + } catch (Exception $e) { + // Let's log the exception for future debugging. + debugging($e->getMessage(), DEBUG_NORMAL, $e->getTrace()); + + // Well, at least we tried. Seems that we really have to read from DB. + $question = $DB->get_record_sql('SELECT q.id, q.createdby, qc.contextid + FROM {question} q + JOIN {question_categories} qc ON q.category = qc.id + WHERE q.id = :id', ['id' => $questionid]); } - } else { + } + + if (!isset($question)) { throw new coding_exception('$questionorid parameter needs to be an integer or an object.'); } diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php index ced468bbfb2..ae1cd80c23c 100644 --- a/lib/tests/questionlib_test.php +++ b/lib/tests/questionlib_test.php @@ -1643,6 +1643,37 @@ class core_questionlib_testcase extends advanced_testcase { ]; } + /** + * Tests that question_has_capability_on does not throw exception on broken questions. + */ + public function test_question_has_capability_on_broken_question() { + global $DB; + + // Create the test data. + $generator = $this->getDataGenerator(); + $questiongenerator = $generator->get_plugin_generator('core_question'); + + $category = $generator->create_category(); + $context = context_coursecat::instance($category->id); + $questioncat = $questiongenerator->create_question_category([ + 'contextid' => $context->id, + ]); + + // Create a cloze question. + $question = $questiongenerator->create_question('multianswer', null, [ + 'category' => $questioncat->id, + ]); + // Now, break the question. + $DB->delete_records('question_multianswer', ['question' => $question->id]); + + $this->setAdminUser(); + + $result = question_has_capability_on($question->id, 'tag'); + $this->assertTrue($result); + + $this->assertDebuggingCalled(); + } + /** * Tests for the deprecated question_has_capability_on function when passing a stdClass as parameter. * diff --git a/mod/quiz/classes/question/bank/add_action_column.php b/mod/quiz/classes/question/bank/add_action_column.php index 0e1d17cdf20..cf314b55f9b 100644 --- a/mod/quiz/classes/question/bank/add_action_column.php +++ b/mod/quiz/classes/question/bank/add_action_column.php @@ -52,8 +52,4 @@ class add_action_column extends \core_question\bank\action_column_base { } $this->print_icon('t/add', $this->stradd, $this->qbank->add_to_quiz_url($question->id)); } - - public function get_required_fields() { - return array('q.id'); - } } diff --git a/mod/quiz/tests/quiz_question_bank_view_test.php b/mod/quiz/tests/quiz_question_bank_view_test.php new file mode 100644 index 00000000000..a0c6ef8a911 --- /dev/null +++ b/mod/quiz/tests/quiz_question_bank_view_test.php @@ -0,0 +1,75 @@ +. + +/** + * Unit tests for the quiz's own question bank view class. + * + * @package mod_quiz + * @category test + * @copyright 2018 the Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/question/editlib.php'); + + +/** + * Unit tests for the quiz's own question bank view class. + * + * @copyright 2018 the Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class quiz_question_bank_view_testcase extends advanced_testcase { + + public function test_viewing_question_bank_should_not_load_individual_questions() { + $this->resetAfterTest(); + $this->setAdminUser(); + $generator = $this->getDataGenerator(); + /** @var core_question_generator $questiongenerator */ + $questiongenerator = $generator->get_plugin_generator('core_question'); + + // Create a course and a quiz. + $course = $generator->create_course(); + $quiz = $this->getDataGenerator()->create_module('quiz', array('course' => $course->id)); + $context = context_module::instance($quiz->cmid); + $cm = get_coursemodule_from_instance('quiz', $quiz->id); + + // Create a question in the default category. + $contexts = new question_edit_contexts($context); + $cat = question_make_default_categories($contexts->all()); + $questiondata = $questiongenerator->create_question('numerical', null, + ['name' => 'Example question', 'category' => $cat->id]); + + // Ensure the question is not in the cache. + $cache = cache::make('core', 'questiondata'); + $cache->delete($questiondata->id); + + // Generate the view. + $view = new mod_quiz\question\bank\custom_view($contexts, new moodle_url('/'), $course, $cm, $quiz); + ob_start(); + $view->display('editq', 0, 20, $cat->id . ',' . $cat->contextid, false, false, false); + $html = ob_get_clean(); + + // Verify the output includes the expected question. + $this->assertContains('Example question', $html); + + // Verify the question has not been loaded into the cache. + $this->assertFalse($cache->has($questiondata->id)); + } +} diff --git a/question/classes/bank/delete_action_column.php b/question/classes/bank/delete_action_column.php index 7ef065e7ae1..ad17bb3e55c 100644 --- a/question/classes/bank/delete_action_column.php +++ b/question/classes/bank/delete_action_column.php @@ -51,6 +51,8 @@ class delete_action_column extends action_column_base { } public function get_required_fields() { - return array('q.id', 'q.hidden'); + $required = parent::get_required_fields(); + $required[] = 'q.hidden'; + return $required; } } diff --git a/question/classes/bank/preview_action_column.php b/question/classes/bank/preview_action_column.php index f31d5cc359f..a81a83ca5bd 100644 --- a/question/classes/bank/preview_action_column.php +++ b/question/classes/bank/preview_action_column.php @@ -34,8 +34,4 @@ class preview_action_column extends action_column_base { $question->id, $this->qbank->get_most_specific_context(), false); } } - - public function get_required_fields() { - return array('q.id'); - } } diff --git a/question/engine/bank.php b/question/engine/bank.php index 6721cbfc05b..68c6aaf1357 100644 --- a/question/engine/bank.php +++ b/question/engine/bank.php @@ -267,7 +267,7 @@ abstract class question_bank { global $DB; if (self::$testmode) { - // Evil, test code in production, but now way round it. + // Evil, test code in production, but no way round it. return self::return_test_question_data($questionid); } diff --git a/question/tests/bank_view_test.php b/question/tests/bank_view_test.php index c9987ffb9cd..0f4ea44c781 100644 --- a/question/tests/bank_view_test.php +++ b/question/tests/bank_view_test.php @@ -44,7 +44,7 @@ class core_question_bank_view_testcase extends advanced_testcase { /** @var core_question_generator $questiongenerator */ $questiongenerator = $generator->get_plugin_generator('core_question'); - // Cerate a course. + // Create a course. $course = $generator->create_course(); $context = context_course::instance($course->id); @@ -54,7 +54,7 @@ class core_question_bank_view_testcase extends advanced_testcase { $questiondata = $questiongenerator->create_question('numerical', null, ['name' => 'Example question', 'category' => $cat->id]); - // Ensure the qusetion is not in the cache. + // Ensure the question is not in the cache. $cache = cache::make('core', 'questiondata'); $cache->delete($questiondata->id); @@ -67,7 +67,7 @@ class core_question_bank_view_testcase extends advanced_testcase { // Verify the output includes the expected question. $this->assertContains('Example question', $html); - // Verify the qusetion has not been loaded into the cache. + // Verify the question has not been loaded into the cache. $this->assertFalse($cache->has($questiondata->id)); } }